| Message ID | 20220403222510.12670-1-rdunlap@infradead.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] sound/oss/dmasound: fix build when drivers are mixed =y/=m | expand |
Hi Randy, On Mon, Apr 4, 2022 at 12:25 AM Randy Dunlap <rdunlap@infradead.org> wrote: > When CONFIG_DMASOUND_ATARI=m and CONFIG_DMASOUND_Q40=y (or vice versa), > dmasound_core.o can be built without dmasound_deinit() being defined, > causing a build error: > > ERROR: modpost: "dmasound_deinit" [sound/oss/dmasound/dmasound_atari.ko] undefined! > > Modify dmasound_core.c so that dmasound_deinit() is always available. > > Suggested-by: Arnd Bergmann <arnd@arndb.de> > Signed-off-by: Randy Dunlap <rdunlap@infradead.org> Thanks for spending more time on this ;-) > --- linux-next-20220401.orig/sound/oss/dmasound/dmasound_core.c > +++ linux-next-20220401/sound/oss/dmasound/dmasound_core.c > @@ -1424,27 +1424,29 @@ int dmasound_init(void) > return 0; > } > > -#ifdef MODULE > - > void dmasound_deinit(void) > { > +#ifdef MODULE I think this #ifdef must not be added: if the modular subdriver calls dmasound_deinit(), the resources should be freed, else a subsequent reload of the subdriver will not work. This does mean all variables protected by "#ifdef MODULE" must exist unconditionally. Alternatively, the test can be replaced by "#ifdef CONFIG_MODULES". One big caveat below... > if (irq_installed) { > sound_silence(); > dmasound.mach.irqcleanup(); > irq_installed = 0; > } > +#endif > > write_sq_release_buffers(); > > +#ifdef MODULE Likewise. > if (mixer_unit >= 0) > unregister_sound_mixer(mixer_unit); > if (state_unit >= 0) > unregister_sound_special(state_unit); > if (sq_unit >= 0) > unregister_sound_dsp(sq_unit); > +#endif > } > > -#else /* !MODULE */ > +#ifndef MODULE > > static int dmasound_setup(char *str) > { > --- linux-next-20220401.orig/sound/oss/dmasound/dmasound.h > +++ linux-next-20220401/sound/oss/dmasound/dmasound.h > @@ -88,11 +88,7 @@ static inline int ioctl_return(int __use > */ > > extern int dmasound_init(void); > -#ifdef MODULE > extern void dmasound_deinit(void); > -#else > -#define dmasound_deinit() do { } while (0) > -#endif > > /* description of the set-up applies to either hard or soft settings */ ... Below, there is: typedef struct { [...] #ifdef MODULE void (*irqcleanup)(void); #endif [...] } MACHINE; This means the MACHINE struct is not compatible between builtin and modular code :-( Hence the "#ifdef MODULE" should be removed, or replaced by "#ifdef CONFIG_MODULES", too. P.S. I think the younger myself is responsible for this mess. Please accept my apologies, after +25 years... Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi Geert, On 4/4/22 06:57, Geert Uytterhoeven wrote: > Hi Randy, > > On Mon, Apr 4, 2022 at 12:25 AM Randy Dunlap <rdunlap@infradead.org> wrote: >> When CONFIG_DMASOUND_ATARI=m and CONFIG_DMASOUND_Q40=y (or vice versa), >> dmasound_core.o can be built without dmasound_deinit() being defined, >> causing a build error: >> >> ERROR: modpost: "dmasound_deinit" [sound/oss/dmasound/dmasound_atari.ko] undefined! >> >> Modify dmasound_core.c so that dmasound_deinit() is always available. >> >> Suggested-by: Arnd Bergmann <arnd@arndb.de> >> Signed-off-by: Randy Dunlap <rdunlap@infradead.org> > > Thanks for spending more time on this ;-) Well that bot keeps reporting this problem, although I suppose that we could ask for it to be ignored... >> --- linux-next-20220401.orig/sound/oss/dmasound/dmasound_core.c >> +++ linux-next-20220401/sound/oss/dmasound/dmasound_core.c >> @@ -1424,27 +1424,29 @@ int dmasound_init(void) >> return 0; >> } >> >> -#ifdef MODULE >> - >> void dmasound_deinit(void) >> { >> +#ifdef MODULE > > I think this #ifdef must not be added: if the modular subdriver > calls dmasound_deinit(), the resources should be freed, else a subsequent > reload of the subdriver will not work. This does mean all variables > protected by "#ifdef MODULE" must exist unconditionally. OK, I like that simplification. > Alternatively, the test can be replaced by "#ifdef CONFIG_MODULES". > > One big caveat below... > >> if (irq_installed) { >> sound_silence(); >> dmasound.mach.irqcleanup(); >> irq_installed = 0; >> } >> +#endif >> >> write_sq_release_buffers(); >> >> +#ifdef MODULE > > Likewise. > >> if (mixer_unit >= 0) >> unregister_sound_mixer(mixer_unit); >> if (state_unit >= 0) >> unregister_sound_special(state_unit); >> if (sq_unit >= 0) >> unregister_sound_dsp(sq_unit); >> +#endif >> } >> >> -#else /* !MODULE */ >> +#ifndef MODULE >> >> static int dmasound_setup(char *str) >> { > >> --- linux-next-20220401.orig/sound/oss/dmasound/dmasound.h >> +++ linux-next-20220401/sound/oss/dmasound/dmasound.h >> @@ -88,11 +88,7 @@ static inline int ioctl_return(int __use >> */ >> >> extern int dmasound_init(void); >> -#ifdef MODULE >> extern void dmasound_deinit(void); >> -#else >> -#define dmasound_deinit() do { } while (0) >> -#endif >> >> /* description of the set-up applies to either hard or soft settings */ > > ... Below, there is: > > typedef struct { > [...] > #ifdef MODULE > void (*irqcleanup)(void); > #endif > [...] > } MACHINE; > > This means the MACHINE struct is not compatible between builtin > and modular code :-( Hence the "#ifdef MODULE" should be removed, > or replaced by "#ifdef CONFIG_MODULES", too. ditto > P.S. I think the younger myself is responsible for this mess. > Please accept my apologies, after +25 years... :) I'll see how it goes. Thanks.
--- linux-next-20220401.orig/sound/oss/dmasound/dmasound_core.c +++ linux-next-20220401/sound/oss/dmasound/dmasound_core.c @@ -1424,27 +1424,29 @@ int dmasound_init(void) return 0; } -#ifdef MODULE - void dmasound_deinit(void) { +#ifdef MODULE if (irq_installed) { sound_silence(); dmasound.mach.irqcleanup(); irq_installed = 0; } +#endif write_sq_release_buffers(); +#ifdef MODULE if (mixer_unit >= 0) unregister_sound_mixer(mixer_unit); if (state_unit >= 0) unregister_sound_special(state_unit); if (sq_unit >= 0) unregister_sound_dsp(sq_unit); +#endif } -#else /* !MODULE */ +#ifndef MODULE static int dmasound_setup(char *str) { @@ -1577,9 +1579,7 @@ char dmasound_alaw2dma8[] = { EXPORT_SYMBOL(dmasound); EXPORT_SYMBOL(dmasound_init); -#ifdef MODULE EXPORT_SYMBOL(dmasound_deinit); -#endif EXPORT_SYMBOL(dmasound_write_sq); EXPORT_SYMBOL(dmasound_catchRadius); #ifdef HAS_8BIT_TABLES --- linux-next-20220401.orig/sound/oss/dmasound/dmasound.h +++ linux-next-20220401/sound/oss/dmasound/dmasound.h @@ -88,11 +88,7 @@ static inline int ioctl_return(int __use */ extern int dmasound_init(void); -#ifdef MODULE extern void dmasound_deinit(void); -#else -#define dmasound_deinit() do { } while (0) -#endif /* description of the set-up applies to either hard or soft settings */
When CONFIG_DMASOUND_ATARI=m and CONFIG_DMASOUND_Q40=y (or vice versa), dmasound_core.o can be built without dmasound_deinit() being defined, causing a build error: ERROR: modpost: "dmasound_deinit" [sound/oss/dmasound/dmasound_atari.ko] undefined! Modify dmasound_core.c so that dmasound_deinit() is always available. Suggested-by: Arnd Bergmann <arnd@arndb.de> Signed-off-by: Randy Dunlap <rdunlap@infradead.org> Reported-by: kernel test robot <lkp@intel.com> Link: lore.kernel.org/r/202204032138.EFT9qGEd-lkp@intel.com Cc: Geert Uytterhoeven <geert@linux-m68k.org> Cc: Jaroslav Kysela <perex@perex.cz> Cc: Takashi Iwai <tiwai@suse.com> Cc: alsa-devel@alsa-project.org --- #Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") # "forever, but backport not important" sound/oss/dmasound/dmasound.h | 4 ---- sound/oss/dmasound/dmasound_core.c | 10 +++++----- 2 files changed, 5 insertions(+), 9 deletions(-)