Re: [PATCH] ALSA: core: Warn on empty module

[Takashi Iwai <tiwai@suse.de>]

On Wed, 24 Jun 2020 18:33:00 +0200,
Jaroslav Kysela wrote:
> 
> Dne 24. 06. 20 v 18:13 Pierre-Louis Bossart napsal(a):
> >
> >
> > On 6/24/20 11:03 AM, Takashi Iwai wrote:
> >> The module argument passed to snd_card_new() must be a valid non-NULL
> >> pointer when the module support is enabled.  Since ASoC driver passes
> >> the argument from each snd_soc_card definition, one may forget to set
> >> the owner field and lead to a NULL module easily.
> >>
> >> For catching such an overlook, add a WARN_ON() in snd_card_new().
> >> Also, put the card->module assignment in the ifdef block for a very
> >> minor optimization.
> >>
> >> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> >> ---
> >>    sound/core/init.c | 3 +++
> >>    1 file changed, 3 insertions(+)
> >>
> >> diff --git a/sound/core/init.c b/sound/core/init.c
> >> index b02a99766351..0478847ba2b8 100644
> >> --- a/sound/core/init.c
> >> +++ b/sound/core/init.c
> >> @@ -203,7 +203,10 @@ int snd_card_new(struct device *parent, int idx, const char *xid,
> >>    	mutex_unlock(&snd_card_mutex);
> >>    	card->dev = parent;
> >>    	card->number = idx;
> >> +#ifdef MODULE
> >> +	WARN_ON(!module);
> >>    	card->module = module;
> >> +#endif
> >>    	INIT_LIST_HEAD(&card->devices);
> >>    	init_rwsem(&card->controls_rwsem);
> >>    	rwlock_init(&card->ctl_files_rwlock);
> >
> > Would it make sense to also change the ASoC code to use THIS_MODULE
> > instead of card->owner?
> >
> > /* card bind complete so register a sound card */
> > ret = snd_card_new(card->dev, SNDRV_DEFAULT_IDX1, SNDRV_DEFAULT_STR1,
> > 		card->owner, 0, &card->snd_card);
> >
> > A quick grep shows we are setting .owner = THIS_MODULE pretty much all
> > the time for machine drivers.
> >
> 
> THIS_MODULE is defined when the object file is created (compile
> time). We want to assign the real module which creates the card here,
> not "snd_soc_core" which is misleading.

Right.  We could make a helper macro to pass THIS_MODULE automagically
but I don't think it worth.  A simple check should suffice.
Of course, we can put a similar check in ASoC side, too, so that it
points more clearly what's wrong.


thanks,

Takashi