[PATCH] ALSA: core: Warn on empty module

[PATCH] ALSA: core: Warn on empty module

[Takashi Iwai]

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

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

[Pierre-Louis Bossart]

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.

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

[Jaroslav Kysela]

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

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

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

[Pierre-Louis Bossart]

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.

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

[Takashi Iwai]

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

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

[Marek Szyprowski]

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
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

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

[Nicolas Saenz Julienne]

[-- Attachment #1: Type: text/plain, Size: 4064 bytes --]

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


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Marek Szyprowski]

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
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

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

[Nicolas Saenz Julienne]

[-- Attachment #1: Type: text/plain, Size: 4443 bytes --]

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


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Takashi Iwai]

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