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

[Geert Uytterhoeven]

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 ;-)

> --- 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@xxxxxxxxxxxxxx

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