diff mbox series

[v2] sound/oss/dmasound: fix build when drivers are mixed =y/=m

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

Commit Message

Randy Dunlap April 3, 2022, 10:25 p.m. UTC
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(-)

Comments

Geert Uytterhoeven April 4, 2022, 1:57 p.m. UTC | #1
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
Randy Dunlap April 4, 2022, 9:53 p.m. UTC | #2
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.
diff mbox series

Patch

--- 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 */