| Message ID | 20200624160300.21703-1-tiwai@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ALSA: core: Warn on empty module | expand |
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.
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. Jaroslav
On 6/24/20 11:33 AM, 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. ok, will submit fixes for the Intel cards where this is missing. Thanks Jaroslav for reporting this and Takashi for identifying the issue.
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
Hi On 24.06.2020 18:03, 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> I know that this is intended, but I would like to note that this patch reveals the following issue on Raspberry Pi 3B with ARM 32bit kernel compiled from multi_v7_defconfig: ------------[ cut here ]------------ WARNING: CPU: 1 PID: 210 at sound/core/init.c:207 snd_card_new+0x378/0x398 [snd] Modules linked in: vc4(+) snd_soc_core ac97_bus snd_pcm_dmaengine bluetooth snd_pcm snd_timer crc32_arm_ce raspberrypi_hwmon snd soundcore ecdh_generic ecc bcm2835_thermal phy_generic CPU: 1 PID: 210 Comm: systemd-udevd Not tainted 5.8.0-rc1-00027-g81033c6b584b #1087 Hardware name: BCM2835 [<c03113c0>] (unwind_backtrace) from [<c030bcb4>] (show_stack+0x10/0x14) [<c030bcb4>] (show_stack) from [<c071cef8>] (dump_stack+0xd4/0xe8) [<c071cef8>] (dump_stack) from [<c0345bfc>] (__warn+0xdc/0xf4) [<c0345bfc>] (__warn) from [<c0345cc4>] (warn_slowpath_fmt+0xb0/0xb8) [<c0345cc4>] (warn_slowpath_fmt) from [<bf02ff74>] (snd_card_new+0x378/0x398 [snd]) [<bf02ff74>] (snd_card_new [snd]) from [<bf11f0b4>] (snd_soc_bind_card+0x280/0x99c [snd_soc_core]) [<bf11f0b4>] (snd_soc_bind_card [snd_soc_core]) from [<bf12f000>] (devm_snd_soc_register_card+0x34/0x6c [snd_soc_core]) [<bf12f000>] (devm_snd_soc_register_card [snd_soc_core]) from [<bf165654>] (vc4_hdmi_bind+0x43c/0x5f4 [vc4]) [<bf165654>] (vc4_hdmi_bind [vc4]) from [<c09d660c>] (component_bind_all+0xec/0x24c) [<c09d660c>] (component_bind_all) from [<bf15c44c>] (vc4_drm_bind+0xd4/0x174 [vc4]) [<bf15c44c>] (vc4_drm_bind [vc4]) from [<c09d6ac0>] (try_to_bring_up_master+0x160/0x1b0) [<c09d6ac0>] (try_to_bring_up_master) from [<c09d6f38>] (component_master_add_with_match+0xd0/0x104) [<c09d6f38>] (component_master_add_with_match) from [<bf15c588>] (vc4_platform_drm_probe+0x9c/0xbc [vc4]) [<bf15c588>] (vc4_platform_drm_probe [vc4]) from [<c09df740>] (platform_drv_probe+0x6c/0xa4) [<c09df740>] (platform_drv_probe) from [<c09dd6f0>] (really_probe+0x210/0x350) [<c09dd6f0>] (really_probe) from [<c09dd940>] (driver_probe_device+0x5c/0xb4) [<c09dd940>] (driver_probe_device) from [<c09ddb38>] (device_driver_attach+0x58/0x60) [<c09ddb38>] (device_driver_attach) from [<c09ddbc0>] (__driver_attach+0x80/0xbc) [<c09ddbc0>] (__driver_attach) from [<c09db820>] (bus_for_each_dev+0x68/0xb4) [<c09db820>] (bus_for_each_dev) from [<c09dc9f8>] (bus_add_driver+0x130/0x1e8) [<c09dc9f8>] (bus_add_driver) from [<c09de648>] (driver_register+0x78/0x110) [<c09de648>] (driver_register) from [<c0302038>] (do_one_initcall+0x50/0x220) [<c0302038>] (do_one_initcall) from [<c03db544>] (do_init_module+0x60/0x210) [<c03db544>] (do_init_module) from [<c03da4f8>] (load_module+0x1e34/0x2338) [<c03da4f8>] (load_module) from [<c03dac00>] (sys_finit_module+0xac/0xbc) [<c03dac00>] (sys_finit_module) from [<c03000c0>] (ret_fast_syscall+0x0/0x54) Exception stack(0xeded9fa8 to 0xeded9ff0) ... ---[ end trace 6414689569c2bc08 ]--- This warning is not present when booting ARM 64bit kernel, but I suspect that this is due to the differences in the kernel configuration. > --- > 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); Best regards
Hi Marek, Thanks for pointing this out! On Tue, 2020-06-30 at 14:30 +0200, Marek Szyprowski wrote: > Hi > > On 24.06.2020 18:03, 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> > > I know that this is intended, but I would like to note that this patch > reveals the following issue on Raspberry Pi 3B with ARM 32bit kernel > compiled from multi_v7_defconfig: > > ------------[ cut here ]------------ > WARNING: CPU: 1 PID: 210 at sound/core/init.c:207 > snd_card_new+0x378/0x398 [snd] > Modules linked in: vc4(+) snd_soc_core ac97_bus snd_pcm_dmaengine > bluetooth snd_pcm snd_timer crc32_arm_ce raspberrypi_hwmon snd soundcore > ecdh_generic ecc bcm2835_thermal phy_generic > CPU: 1 PID: 210 Comm: systemd-udevd Not tainted > 5.8.0-rc1-00027-g81033c6b584b #1087 > Hardware name: BCM2835 > [<c03113c0>] (unwind_backtrace) from [<c030bcb4>] (show_stack+0x10/0x14) > [<c030bcb4>] (show_stack) from [<c071cef8>] (dump_stack+0xd4/0xe8) > [<c071cef8>] (dump_stack) from [<c0345bfc>] (__warn+0xdc/0xf4) > [<c0345bfc>] (__warn) from [<c0345cc4>] (warn_slowpath_fmt+0xb0/0xb8) > [<c0345cc4>] (warn_slowpath_fmt) from [<bf02ff74>] > (snd_card_new+0x378/0x398 [snd]) > [<bf02ff74>] (snd_card_new [snd]) from [<bf11f0b4>] > (snd_soc_bind_card+0x280/0x99c [snd_soc_core]) > [<bf11f0b4>] (snd_soc_bind_card [snd_soc_core]) from [<bf12f000>] > (devm_snd_soc_register_card+0x34/0x6c [snd_soc_core]) > [<bf12f000>] (devm_snd_soc_register_card [snd_soc_core]) from > [<bf165654>] (vc4_hdmi_bind+0x43c/0x5f4 [vc4]) > [<bf165654>] (vc4_hdmi_bind [vc4]) from [<c09d660c>] > (component_bind_all+0xec/0x24c) > [<c09d660c>] (component_bind_all) from [<bf15c44c>] > (vc4_drm_bind+0xd4/0x174 [vc4]) > [<bf15c44c>] (vc4_drm_bind [vc4]) from [<c09d6ac0>] > (try_to_bring_up_master+0x160/0x1b0) > [<c09d6ac0>] (try_to_bring_up_master) from [<c09d6f38>] > (component_master_add_with_match+0xd0/0x104) > [<c09d6f38>] (component_master_add_with_match) from [<bf15c588>] > (vc4_platform_drm_probe+0x9c/0xbc [vc4]) > [<bf15c588>] (vc4_platform_drm_probe [vc4]) from [<c09df740>] > (platform_drv_probe+0x6c/0xa4) > [<c09df740>] (platform_drv_probe) from [<c09dd6f0>] > (really_probe+0x210/0x350) > [<c09dd6f0>] (really_probe) from [<c09dd940>] > (driver_probe_device+0x5c/0xb4) > [<c09dd940>] (driver_probe_device) from [<c09ddb38>] > (device_driver_attach+0x58/0x60) > [<c09ddb38>] (device_driver_attach) from [<c09ddbc0>] > (__driver_attach+0x80/0xbc) > [<c09ddbc0>] (__driver_attach) from [<c09db820>] > (bus_for_each_dev+0x68/0xb4) > [<c09db820>] (bus_for_each_dev) from [<c09dc9f8>] > (bus_add_driver+0x130/0x1e8) > [<c09dc9f8>] (bus_add_driver) from [<c09de648>] (driver_register+0x78/0x110) > [<c09de648>] (driver_register) from [<c0302038>] > (do_one_initcall+0x50/0x220) > [<c0302038>] (do_one_initcall) from [<c03db544>] (do_init_module+0x60/0x210) > [<c03db544>] (do_init_module) from [<c03da4f8>] (load_module+0x1e34/0x2338) > [<c03da4f8>] (load_module) from [<c03dac00>] (sys_finit_module+0xac/0xbc) > [<c03dac00>] (sys_finit_module) from [<c03000c0>] > (ret_fast_syscall+0x0/0x54) > Exception stack(0xeded9fa8 to 0xeded9ff0) > ... > ---[ end trace 6414689569c2bc08 ]--- > > This warning is not present when booting ARM 64bit kernel, but I suspect > that this is due to the differences in the kernel configuration. It's because vc4 is not yet supported on RPi4. Maxime Rippard is working on it: https://lkml.kernel.org/lkml/20200629142145.aa2vdfkgeugrze4c@gilmour.lan/T/. Regards, Nicolas
On 30.06.2020 14:34, Nicolas Saenz Julienne wrote: > Hi Marek, > Thanks for pointing this out! > > On Tue, 2020-06-30 at 14:30 +0200, Marek Szyprowski wrote: >> Hi >> >> On 24.06.2020 18:03, 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> >> I know that this is intended, but I would like to note that this patch >> reveals the following issue on Raspberry Pi 3B with ARM 32bit kernel >> compiled from multi_v7_defconfig: >> >> ------------[ cut here ]------------ >> WARNING: CPU: 1 PID: 210 at sound/core/init.c:207 >> snd_card_new+0x378/0x398 [snd] >> Modules linked in: vc4(+) snd_soc_core ac97_bus snd_pcm_dmaengine >> bluetooth snd_pcm snd_timer crc32_arm_ce raspberrypi_hwmon snd soundcore >> ecdh_generic ecc bcm2835_thermal phy_generic >> CPU: 1 PID: 210 Comm: systemd-udevd Not tainted >> 5.8.0-rc1-00027-g81033c6b584b #1087 >> Hardware name: BCM2835 >> [<c03113c0>] (unwind_backtrace) from [<c030bcb4>] (show_stack+0x10/0x14) >> [<c030bcb4>] (show_stack) from [<c071cef8>] (dump_stack+0xd4/0xe8) >> [<c071cef8>] (dump_stack) from [<c0345bfc>] (__warn+0xdc/0xf4) >> [<c0345bfc>] (__warn) from [<c0345cc4>] (warn_slowpath_fmt+0xb0/0xb8) >> [<c0345cc4>] (warn_slowpath_fmt) from [<bf02ff74>] >> (snd_card_new+0x378/0x398 [snd]) >> [<bf02ff74>] (snd_card_new [snd]) from [<bf11f0b4>] >> (snd_soc_bind_card+0x280/0x99c [snd_soc_core]) >> [<bf11f0b4>] (snd_soc_bind_card [snd_soc_core]) from [<bf12f000>] >> (devm_snd_soc_register_card+0x34/0x6c [snd_soc_core]) >> [<bf12f000>] (devm_snd_soc_register_card [snd_soc_core]) from >> [<bf165654>] (vc4_hdmi_bind+0x43c/0x5f4 [vc4]) >> [<bf165654>] (vc4_hdmi_bind [vc4]) from [<c09d660c>] >> (component_bind_all+0xec/0x24c) >> [<c09d660c>] (component_bind_all) from [<bf15c44c>] >> (vc4_drm_bind+0xd4/0x174 [vc4]) >> [<bf15c44c>] (vc4_drm_bind [vc4]) from [<c09d6ac0>] >> (try_to_bring_up_master+0x160/0x1b0) >> [<c09d6ac0>] (try_to_bring_up_master) from [<c09d6f38>] >> (component_master_add_with_match+0xd0/0x104) >> [<c09d6f38>] (component_master_add_with_match) from [<bf15c588>] >> (vc4_platform_drm_probe+0x9c/0xbc [vc4]) >> [<bf15c588>] (vc4_platform_drm_probe [vc4]) from [<c09df740>] >> (platform_drv_probe+0x6c/0xa4) >> [<c09df740>] (platform_drv_probe) from [<c09dd6f0>] >> (really_probe+0x210/0x350) >> [<c09dd6f0>] (really_probe) from [<c09dd940>] >> (driver_probe_device+0x5c/0xb4) >> [<c09dd940>] (driver_probe_device) from [<c09ddb38>] >> (device_driver_attach+0x58/0x60) >> [<c09ddb38>] (device_driver_attach) from [<c09ddbc0>] >> (__driver_attach+0x80/0xbc) >> [<c09ddbc0>] (__driver_attach) from [<c09db820>] >> (bus_for_each_dev+0x68/0xb4) >> [<c09db820>] (bus_for_each_dev) from [<c09dc9f8>] >> (bus_add_driver+0x130/0x1e8) >> [<c09dc9f8>] (bus_add_driver) from [<c09de648>] (driver_register+0x78/0x110) >> [<c09de648>] (driver_register) from [<c0302038>] >> (do_one_initcall+0x50/0x220) >> [<c0302038>] (do_one_initcall) from [<c03db544>] (do_init_module+0x60/0x210) >> [<c03db544>] (do_init_module) from [<c03da4f8>] (load_module+0x1e34/0x2338) >> [<c03da4f8>] (load_module) from [<c03dac00>] (sys_finit_module+0xac/0xbc) >> [<c03dac00>] (sys_finit_module) from [<c03000c0>] >> (ret_fast_syscall+0x0/0x54) >> Exception stack(0xeded9fa8 to 0xeded9ff0) >> ... >> ---[ end trace 6414689569c2bc08 ]--- >> >> This warning is not present when booting ARM 64bit kernel, but I suspect >> that this is due to the differences in the kernel configuration. > It's because vc4 is not yet supported on RPi4. This happens on RPi *3B* :) Best regards
On Tue, 2020-06-30 at 14:36 +0200, Marek Szyprowski wrote: > On 30.06.2020 14:34, Nicolas Saenz Julienne wrote: > > Hi Marek, > > Thanks for pointing this out! > > > > On Tue, 2020-06-30 at 14:30 +0200, Marek Szyprowski wrote: > > > Hi > > > > > > On 24.06.2020 18:03, 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> > > > I know that this is intended, but I would like to note that this patch > > > reveals the following issue on Raspberry Pi 3B with ARM 32bit kernel > > > compiled from multi_v7_defconfig: > > > > > > ------------[ cut here ]------------ > > > WARNING: CPU: 1 PID: 210 at sound/core/init.c:207 > > > snd_card_new+0x378/0x398 [snd] > > > Modules linked in: vc4(+) snd_soc_core ac97_bus snd_pcm_dmaengine > > > bluetooth snd_pcm snd_timer crc32_arm_ce raspberrypi_hwmon snd soundcore > > > ecdh_generic ecc bcm2835_thermal phy_generic > > > CPU: 1 PID: 210 Comm: systemd-udevd Not tainted > > > 5.8.0-rc1-00027-g81033c6b584b #1087 > > > Hardware name: BCM2835 > > > [<c03113c0>] (unwind_backtrace) from [<c030bcb4>] (show_stack+0x10/0x14) > > > [<c030bcb4>] (show_stack) from [<c071cef8>] (dump_stack+0xd4/0xe8) > > > [<c071cef8>] (dump_stack) from [<c0345bfc>] (__warn+0xdc/0xf4) > > > [<c0345bfc>] (__warn) from [<c0345cc4>] (warn_slowpath_fmt+0xb0/0xb8) > > > [<c0345cc4>] (warn_slowpath_fmt) from [<bf02ff74>] > > > (snd_card_new+0x378/0x398 [snd]) > > > [<bf02ff74>] (snd_card_new [snd]) from [<bf11f0b4>] > > > (snd_soc_bind_card+0x280/0x99c [snd_soc_core]) > > > [<bf11f0b4>] (snd_soc_bind_card [snd_soc_core]) from [<bf12f000>] > > > (devm_snd_soc_register_card+0x34/0x6c [snd_soc_core]) > > > [<bf12f000>] (devm_snd_soc_register_card [snd_soc_core]) from > > > [<bf165654>] (vc4_hdmi_bind+0x43c/0x5f4 [vc4]) > > > [<bf165654>] (vc4_hdmi_bind [vc4]) from [<c09d660c>] > > > (component_bind_all+0xec/0x24c) > > > [<c09d660c>] (component_bind_all) from [<bf15c44c>] > > > (vc4_drm_bind+0xd4/0x174 [vc4]) > > > [<bf15c44c>] (vc4_drm_bind [vc4]) from [<c09d6ac0>] > > > (try_to_bring_up_master+0x160/0x1b0) > > > [<c09d6ac0>] (try_to_bring_up_master) from [<c09d6f38>] > > > (component_master_add_with_match+0xd0/0x104) > > > [<c09d6f38>] (component_master_add_with_match) from [<bf15c588>] > > > (vc4_platform_drm_probe+0x9c/0xbc [vc4]) > > > [<bf15c588>] (vc4_platform_drm_probe [vc4]) from [<c09df740>] > > > (platform_drv_probe+0x6c/0xa4) > > > [<c09df740>] (platform_drv_probe) from [<c09dd6f0>] > > > (really_probe+0x210/0x350) > > > [<c09dd6f0>] (really_probe) from [<c09dd940>] > > > (driver_probe_device+0x5c/0xb4) > > > [<c09dd940>] (driver_probe_device) from [<c09ddb38>] > > > (device_driver_attach+0x58/0x60) > > > [<c09ddb38>] (device_driver_attach) from [<c09ddbc0>] > > > (__driver_attach+0x80/0xbc) > > > [<c09ddbc0>] (__driver_attach) from [<c09db820>] > > > (bus_for_each_dev+0x68/0xb4) > > > [<c09db820>] (bus_for_each_dev) from [<c09dc9f8>] > > > (bus_add_driver+0x130/0x1e8) > > > [<c09dc9f8>] (bus_add_driver) from [<c09de648>] > > > (driver_register+0x78/0x110) > > > [<c09de648>] (driver_register) from [<c0302038>] > > > (do_one_initcall+0x50/0x220) > > > [<c0302038>] (do_one_initcall) from [<c03db544>] > > > (do_init_module+0x60/0x210) > > > [<c03db544>] (do_init_module) from [<c03da4f8>] > > > (load_module+0x1e34/0x2338) > > > [<c03da4f8>] (load_module) from [<c03dac00>] (sys_finit_module+0xac/0xbc) > > > [<c03dac00>] (sys_finit_module) from [<c03000c0>] > > > (ret_fast_syscall+0x0/0x54) > > > Exception stack(0xeded9fa8 to 0xeded9ff0) > > > ... > > > ---[ end trace 6414689569c2bc08 ]--- > > > > > > This warning is not present when booting ARM 64bit kernel, but I suspect > > > that this is due to the differences in the kernel configuration. > > It's because vc4 is not yet supported on RPi4. > > This happens on RPi *3B* :) Ok, read too fast. Thanks! Regards, Nicolas
On Tue, 30 Jun 2020 14:38:14 +0200, Nicolas Saenz Julienne wrote: > > On Tue, 2020-06-30 at 14:36 +0200, Marek Szyprowski wrote: > > On 30.06.2020 14:34, Nicolas Saenz Julienne wrote: > > > Hi Marek, > > > Thanks for pointing this out! > > > > > > On Tue, 2020-06-30 at 14:30 +0200, Marek Szyprowski wrote: > > > > Hi > > > > > > > > On 24.06.2020 18:03, 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> > > > > I know that this is intended, but I would like to note that this patch > > > > reveals the following issue on Raspberry Pi 3B with ARM 32bit kernel > > > > compiled from multi_v7_defconfig: > > > > > > > > ------------[ cut here ]------------ > > > > WARNING: CPU: 1 PID: 210 at sound/core/init.c:207 > > > > snd_card_new+0x378/0x398 [snd] > > > > Modules linked in: vc4(+) snd_soc_core ac97_bus snd_pcm_dmaengine > > > > bluetooth snd_pcm snd_timer crc32_arm_ce raspberrypi_hwmon snd soundcore > > > > ecdh_generic ecc bcm2835_thermal phy_generic > > > > CPU: 1 PID: 210 Comm: systemd-udevd Not tainted > > > > 5.8.0-rc1-00027-g81033c6b584b #1087 > > > > Hardware name: BCM2835 > > > > [<c03113c0>] (unwind_backtrace) from [<c030bcb4>] (show_stack+0x10/0x14) > > > > [<c030bcb4>] (show_stack) from [<c071cef8>] (dump_stack+0xd4/0xe8) > > > > [<c071cef8>] (dump_stack) from [<c0345bfc>] (__warn+0xdc/0xf4) > > > > [<c0345bfc>] (__warn) from [<c0345cc4>] (warn_slowpath_fmt+0xb0/0xb8) > > > > [<c0345cc4>] (warn_slowpath_fmt) from [<bf02ff74>] > > > > (snd_card_new+0x378/0x398 [snd]) > > > > [<bf02ff74>] (snd_card_new [snd]) from [<bf11f0b4>] > > > > (snd_soc_bind_card+0x280/0x99c [snd_soc_core]) > > > > [<bf11f0b4>] (snd_soc_bind_card [snd_soc_core]) from [<bf12f000>] > > > > (devm_snd_soc_register_card+0x34/0x6c [snd_soc_core]) > > > > [<bf12f000>] (devm_snd_soc_register_card [snd_soc_core]) from > > > > [<bf165654>] (vc4_hdmi_bind+0x43c/0x5f4 [vc4]) > > > > [<bf165654>] (vc4_hdmi_bind [vc4]) from [<c09d660c>] > > > > (component_bind_all+0xec/0x24c) > > > > [<c09d660c>] (component_bind_all) from [<bf15c44c>] > > > > (vc4_drm_bind+0xd4/0x174 [vc4]) > > > > [<bf15c44c>] (vc4_drm_bind [vc4]) from [<c09d6ac0>] > > > > (try_to_bring_up_master+0x160/0x1b0) > > > > [<c09d6ac0>] (try_to_bring_up_master) from [<c09d6f38>] > > > > (component_master_add_with_match+0xd0/0x104) > > > > [<c09d6f38>] (component_master_add_with_match) from [<bf15c588>] > > > > (vc4_platform_drm_probe+0x9c/0xbc [vc4]) > > > > [<bf15c588>] (vc4_platform_drm_probe [vc4]) from [<c09df740>] > > > > (platform_drv_probe+0x6c/0xa4) > > > > [<c09df740>] (platform_drv_probe) from [<c09dd6f0>] > > > > (really_probe+0x210/0x350) > > > > [<c09dd6f0>] (really_probe) from [<c09dd940>] > > > > (driver_probe_device+0x5c/0xb4) > > > > [<c09dd940>] (driver_probe_device) from [<c09ddb38>] > > > > (device_driver_attach+0x58/0x60) > > > > [<c09ddb38>] (device_driver_attach) from [<c09ddbc0>] > > > > (__driver_attach+0x80/0xbc) > > > > [<c09ddbc0>] (__driver_attach) from [<c09db820>] > > > > (bus_for_each_dev+0x68/0xb4) > > > > [<c09db820>] (bus_for_each_dev) from [<c09dc9f8>] > > > > (bus_add_driver+0x130/0x1e8) > > > > [<c09dc9f8>] (bus_add_driver) from [<c09de648>] > > > > (driver_register+0x78/0x110) > > > > [<c09de648>] (driver_register) from [<c0302038>] > > > > (do_one_initcall+0x50/0x220) > > > > [<c0302038>] (do_one_initcall) from [<c03db544>] > > > > (do_init_module+0x60/0x210) > > > > [<c03db544>] (do_init_module) from [<c03da4f8>] > > > > (load_module+0x1e34/0x2338) > > > > [<c03da4f8>] (load_module) from [<c03dac00>] (sys_finit_module+0xac/0xbc) > > > > [<c03dac00>] (sys_finit_module) from [<c03000c0>] > > > > (ret_fast_syscall+0x0/0x54) > > > > Exception stack(0xeded9fa8 to 0xeded9ff0) > > > > ... > > > > ---[ end trace 6414689569c2bc08 ]--- > > > > > > > > This warning is not present when booting ARM 64bit kernel, but I suspect > > > > that this is due to the differences in the kernel configuration. > > > It's because vc4 is not yet supported on RPi4. > > > > This happens on RPi *3B* :) > > Ok, read too fast. Thanks! FYI, you just need to set card->owner field properly in vc4_hdmi_audio_init(). thanks, Takashi
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);
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(+)