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

[Randy Dunlap]

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@xxxxxxxxxxxxx> 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@xxxxxxxx>
>> Signed-off-by: Randy Dunlap <rdunlap@xxxxxxxxxxxxx>
> 
> 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.

-- 
~Randy