ASoC: core: delete component->card_list in soc_remove_component only
diff mbox series

Message ID 20190916210353.6318-1-yung-chuan.liao@linux.intel.com
State Accepted
Commit a0a4bf57a977ed37bcbdfc8027a44485fe086a3d
Headers show
Series
  • ASoC: core: delete component->card_list in soc_remove_component only
Related show

Commit Message

Bard liao Sept. 16, 2019, 9:03 p.m. UTC
From: Bard Liao <yung-chuan.liao@linux.intel.com>

We add component->card_list in the end of soc_probe_component(). In
other words, component->card_list will not be added if there is an
error in the soc_probe_component() function. So we can't delete
component->card_list in the error handling of soc_probe_component().

Fixes: 22d1423187e5 ("ASoC: soc-core: add soc_cleanup_component()")
Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
---
 sound/soc/soc-core.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Comments

Pierre-Louis Bossart Sept. 17, 2019, 2:33 p.m. UTC | #1
On 9/16/19 4:03 PM, Bard liao wrote:
> From: Bard Liao <yung-chuan.liao@linux.intel.com>
> 
> We add component->card_list in the end of soc_probe_component(). In
> other words, component->card_list will not be added if there is an
> error in the soc_probe_component() function. So we can't delete
> component->card_list in the error handling of soc_probe_component().
> 
> Fixes: 22d1423187e5 ("ASoC: soc-core: add soc_cleanup_component()")
> Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

I wish we had a way to do this on the SOF GitHub, it's painful that 
prior reviews and approvals are not tracked automagically.


> ---
>   sound/soc/soc-core.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
> index 35f48e9c5ead..aff4b4bf4d07 100644
> --- a/sound/soc/soc-core.c
> +++ b/sound/soc/soc-core.c
> @@ -978,7 +978,6 @@ static void soc_cleanup_component(struct snd_soc_component *component)
>   	/* For framework level robustness */
>   	snd_soc_component_set_jack(component, NULL, NULL);
>   
> -	list_del(&component->card_list);
>   	snd_soc_dapm_free(snd_soc_component_get_dapm(component));
>   	soc_cleanup_component_debugfs(component);
>   	component->card = NULL;
> @@ -991,7 +990,7 @@ static void soc_remove_component(struct snd_soc_component *component)
>   		return;
>   
>   	snd_soc_component_remove(component);
> -
> +	list_del(&component->card_list);
>   	soc_cleanup_component(component);
>   }
>   
>
Kuninori Morimoto Sept. 18, 2019, 12:51 a.m. UTC | #2
Hi Bard

Thank you for your patch

> From: Bard Liao <yung-chuan.liao@linux.intel.com>
> 
> We add component->card_list in the end of soc_probe_component(). In
> other words, component->card_list will not be added if there is an
> error in the soc_probe_component() function. So we can't delete
> component->card_list in the error handling of soc_probe_component().
> 
> Fixes: 22d1423187e5 ("ASoC: soc-core: add soc_cleanup_component()")
> Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
> ---

Hmm... what happen if you get error at soc_probe_component() ?
What does your "can't delete component->card_list" mean ?
Kernel Oops or warning ?

I tried to create an error on purpose at soc_probe_component(),
but, there was no kernel oops, no warning, etc.
It just can't create sound card. It is very normal for me.
Or, which kernel are you using ?

Thank you for your help !!
Best regards
---
Kuninori Morimoto
Liao, Bard Sept. 18, 2019, 2:47 a.m. UTC | #3
> -----Original Message-----
> From: Kuninori Morimoto [mailto:kuninori.morimoto.gx@renesas.com]
> Sent: Wednesday, September 18, 2019 8:52 AM
> To: Bard liao <yung-chuan.liao@linux.intel.com>
> Cc: broonie@kernel.org; tiwai@suse.de; alsa-devel@alsa-project.org; pierre-
> louis.bossart@linux.intel.com; Liao, Bard <bard.liao@intel.com>
> Subject: Re: [PATCH] ASoC: core: delete component->card_list in
> soc_remove_component only
> 
> 
> Hi Bard
> 
> Thank you for your patch
> 
> > From: Bard Liao <yung-chuan.liao@linux.intel.com>
> >
> > We add component->card_list in the end of soc_probe_component(). In
> > other words, component->card_list will not be added if there is an
> > error in the soc_probe_component() function. So we can't delete
> > component->card_list in the error handling of soc_probe_component().
> >
> > Fixes: 22d1423187e5 ("ASoC: soc-core: add soc_cleanup_component()")
> > Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
> > ---
> 
> Hmm... what happen if you get error at soc_probe_component() ?
> What does your "can't delete component->card_list" mean ?
> Kernel Oops or warning ?

That is Kernel Oops, see the dmesg below.
[   84.180608] rt711 sdw:0:25d:711:0:1: ASoC: failed to probe component -517
[   84.180653] sdw_rt711_rt1308_rt715 sdw_rt711_rt1308_rt715: ASoC: failed to instantiate card -517
[   84.180701] sdw_rt711_rt1308_rt715 sdw_rt711_rt1308_rt715: snd_soc_register_card failed -517
...
[   94.419962] rt711 sdw:0:25d:711:0:1: ASoC: failed to probe component -517
[   94.419983] general protection fault: 0000 [#1] SMP PTI
[   94.419986] CPU: 6 PID: 119 Comm: kworker/6:1 Not tainted 5.3.0-rc7+ #821
[   94.419988] Hardware name: Intel Corporation Ice Lake Client Platform/IceLake U DDR4 SODIMM PD RVP TLC, BIOS ICLSFWR1.R00.3042.A02.1901222005 01/22/2019
[   94.419994] Workqueue: events deferred_probe_work_func
[   94.420012] RIP: 0010:soc_cleanup_component+0x1c/0x70 [snd_soc_core]
[   94.420015] Code: c3 0f 1f 00 66 2e 0f 1f 84 00 00 00 00 00 53 31 d2 48 89 fb 31 f6 e8 03 bc 00 00 48 8b 43 58 48 8b 53 50 48 8d bb c0 00 00 00 <48> 89 42 08 48 89 10 48 b8 00 01 00 00 00 00 ad de 48 89 43 50 48
[   94.420017] RSP: 0000:ffffa40340317c38 EFLAGS: 00010246
[   94.420019] RAX: dead000000000122 RBX: ffff8fe22576ac28 RCX: 0000000000000006
[   94.420023] RDX: dead000000000100 RSI: 0000000000000000 RDI: ffff8fe22576ace8
[   94.420024] RBP: ffffffffc07430c0 R08: 0000000000000586 R09: 000000000000004c
[   94.420026] R10: ffffa40340317a68 R11: ffffa403403179a0 R12: ffff8fe22576ac20
[   94.420027] R13: ffff8fe22576ace8 R14: ffff8fe22576ac90 R15: 0000000000000000
[   94.420028] FS:  0000000000000000(0000) GS:ffff8fe227f80000(0000) knlGS:0000000000000000
[   94.420030] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   94.420034] CR2: 00007ffdf5555c40 CR3: 000000028cd5c002 CR4: 0000000000760ee0
[   94.420036] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[   94.420037] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[   94.420038] PKRU: 55555554
[   94.420039] Call Trace:
[   94.420050]  soc_probe_component+0x218/0x370 [snd_soc_core]
[   94.420059]  snd_soc_instantiate_card+0x4da/0xd10 [snd_soc_core]
[   94.420064]  ? devm_snd_soc_register_card+0x2e/0x80 [snd_soc_core]
[   94.420071]  snd_soc_register_card+0x169/0x190 [snd_soc_core]
[   94.420076]  devm_snd_soc_register_card+0x3e/0x80 [snd_soc_core]
[   94.420082]  mc_probe+0x152/0x1ac [snd_soc_sdw_rt711_rt1308_rt715]
[   94.420087]  ? acpi_dev_pm_attach+0x1b/0xa0
[   94.420090]  platform_drv_probe+0x32/0x90
[   94.420095]  really_probe+0xea/0x3d0
[   94.420097]  ? driver_allows_async_probing+0x50/0x50
[   94.420100]  driver_probe_device+0x10b/0x120
[   94.420102]  ? driver_allows_async_probing+0x50/0x50
[   94.420107]  bus_for_each_drv+0x61/0xa0
[   94.420110]  __device_attach+0xcf/0x150
[   94.420113]  bus_probe_device+0x82/0x90
[   94.420115]  deferred_probe_work_func+0x6f/0xc0
[   94.420121]  process_one_work+0x1e3/0x3d0
[   94.420124]  worker_thread+0x28/0x3c0
[   94.420126]  ? cancel_delayed_work+0x80/0x80
[   94.420128]  kthread+0x10e/0x130
[   94.420132]  ? kthread_park+0xa0/0xa0
[   94.420135]  ret_from_fork+0x35/0x40
[   94.420138] Modules linked in: snd_soc_rt715 snd_soc_rt1308_sdw snd_soc_rt711 regmap_sdw soundwire_intel snd_soc_sdw_rt711_rt1308_rt715 soundwire_cadence snd_soc_hdac_hdmi snd_soc_dmic snd_sof_pci snd_sof_intel_hda_common snd_soc_hdac_hda snd_hda_codec soundwire_intel_init snd_intel_nhlt snd_sof_intel_hda snd_sof_intel_byt snd_soc_acpi_intel_match snd_soc_acpi snd_sof_intel_ipc snd_sof snd_sof_xtensa_dsp snd_hda_ext_core snd_hda_core snd_soc_core snd_pcm snd_seq_midi snd_seq_midi_event snd_rawmidi snd_seq snd_seq_device snd_timer snd soundcore ax88179_178a usbnet x86_pkg_temp_thermal intel_powerclamp intel_lpss_pci mei_me intel_lpss mfd_core mei efivarfs i915 i2c_algo_bit drm_kms_helper syscopyarea xhci_pci sysfillrect sysimgblt sdhci_pci fb_sys_fops cqhci sdhci xhci_hcd drm
[   94.420169] ---[ end trace 32434ffe9a1d6bab ]---

> 
> I tried to create an error on purpose at soc_probe_component(), but, there was
> no kernel oops, no warning, etc.
> It just can't create sound card. It is very normal for me.
> Or, which kernel are you using ?

Somehow I can only see the second attempt of component probe when it return
-517 (EPROBE_DEFER) in the first attempt by using below kernel.
https://github.com/plbossart/sound/commits/fix/soundwire-split-lookup-init

To me, the easiest way to see the issue is force return - EPROBE_DEFER on
codec driver's probe function and call list_del(&component->card_list);
before calling soc_cleanup_component(component);
So list_del() will be called twice and you will see the issue.

> 
> Thank you for your help !!
> Best regards
> ---
> Kuninori Morimoto
Kuninori Morimoto Sept. 18, 2019, 4:06 a.m. UTC | #4
Hi Liao

Thank you for your feedback

> > I tried to create an error on purpose at soc_probe_component(), but, there was
> > no kernel oops, no warning, etc.
> > It just can't create sound card. It is very normal for me.
> > Or, which kernel are you using ?
> 
> Somehow I can only see the second attempt of component probe when it return
> -517 (EPROBE_DEFER) in the first attempt by using below kernel.
> https://github.com/plbossart/sound/commits/fix/soundwire-split-lookup-init
> 
> To me, the easiest way to see the issue is force return - EPROBE_DEFER on
> codec driver's probe function and call list_del(&component->card_list);
> before calling soc_cleanup_component(component);
> So list_del() will be called twice and you will see the issue.

OK, I could reproduce your issue.
And I think it will be solved if you can use
list_del_init() instead of list_del() at soc_cleanup_component() ?
(= without your patch)

	- list_del()
	+ list_del_init()

If possible, I want to cleanup all component related resource at
soc_cleanup_component(). Because it is easy to read / understand.

Thank you for your help !!
Best regards
---
Kuninori Morimoto
Liao, Bard Sept. 18, 2019, 5:27 a.m. UTC | #5
> -----Original Message-----
> From: Kuninori Morimoto [mailto:kuninori.morimoto.gx@renesas.com]
> Sent: Wednesday, September 18, 2019 12:06 PM
> To: Liao, Bard <bard.liao@intel.com>
> Cc: Bard liao <yung-chuan.liao@linux.intel.com>; broonie@kernel.org;
> tiwai@suse.de; alsa-devel@alsa-project.org; pierre-
> louis.bossart@linux.intel.com
> Subject: Re: [PATCH] ASoC: core: delete component->card_list in
> soc_remove_component only
> 
> 
> Hi Liao
> 
> Thank you for your feedback
> 
> > > I tried to create an error on purpose at soc_probe_component(), but,
> > > there was no kernel oops, no warning, etc.
> > > It just can't create sound card. It is very normal for me.
> > > Or, which kernel are you using ?
> >
> > Somehow I can only see the second attempt of component probe when it
> > return
> > -517 (EPROBE_DEFER) in the first attempt by using below kernel.
> > https://github.com/plbossart/sound/commits/fix/soundwire-split-lookup-
> > init
> >
> > To me, the easiest way to see the issue is force return - EPROBE_DEFER
> > on codec driver's probe function and call
> > list_del(&component->card_list); before calling
> > soc_cleanup_component(component); So list_del() will be called twice and you
> will see the issue.
> 
> OK, I could reproduce your issue.
> And I think it will be solved if you can use
> list_del_init() instead of list_del() at soc_cleanup_component() ?
> (= without your patch)
> 
> 	- list_del()
> 	+ list_del_init()
> 
> If possible, I want to cleanup all component related resource at
> soc_cleanup_component(). Because it is easy to read / understand.

Thanks Morimoto san, I verified the solution works and agree with
you. Will you send a patch to upstream?

> 
> Thank you for your help !!
> Best regards
> ---
> Kuninori Morimoto
Kuninori Morimoto Sept. 18, 2019, 5:46 a.m. UTC | #6
Hi Liao

Thank you for your feedback

> > > To me, the easiest way to see the issue is force return - EPROBE_DEFER
> > > on codec driver's probe function and call
> > > list_del(&component->card_list); before calling
> > > soc_cleanup_component(component); So list_del() will be called twice and you
> > will see the issue.
> > 
> > OK, I could reproduce your issue.
> > And I think it will be solved if you can use
> > list_del_init() instead of list_del() at soc_cleanup_component() ?
> > (= without your patch)
> > 
> > 	- list_del()
> > 	+ list_del_init()
> > 
> > If possible, I want to cleanup all component related resource at
> > soc_cleanup_component(). Because it is easy to read / understand.
> 
> Thanks Morimoto san, I verified the solution works and agree with
> you. Will you send a patch to upstream?

Good to know !!
I'm happy if you can update it :)

Thank you for your help !!
Best regards
---
Kuninori Morimoto
Liao, Bard Sept. 18, 2019, 6:13 a.m. UTC | #7
> -----Original Message-----
> From: Kuninori Morimoto [mailto:kuninori.morimoto.gx@renesas.com]
> Sent: Wednesday, September 18, 2019 1:46 PM
> To: Liao, Bard <bard.liao@intel.com>
> Cc: Bard liao <yung-chuan.liao@linux.intel.com>; broonie@kernel.org;
> tiwai@suse.de; alsa-devel@alsa-project.org; pierre-
> louis.bossart@linux.intel.com
> Subject: Re: [PATCH] ASoC: core: delete component->card_list in
> soc_remove_component only
> 
> 
> Hi Liao
> 
> Thank you for your feedback
> 
> > > > To me, the easiest way to see the issue is force return -
> > > > EPROBE_DEFER on codec driver's probe function and call
> > > > list_del(&component->card_list); before calling
> > > > soc_cleanup_component(component); So list_del() will be called
> > > > twice and you
> > > will see the issue.
> > >
> > > OK, I could reproduce your issue.
> > > And I think it will be solved if you can use
> > > list_del_init() instead of list_del() at soc_cleanup_component() ?
> > > (= without your patch)
> > >
> > > 	- list_del()
> > > 	+ list_del_init()
> > >
> > > If possible, I want to cleanup all component related resource at
> > > soc_cleanup_component(). Because it is easy to read / understand.
> >
> > Thanks Morimoto san, I verified the solution works and agree with you.
> > Will you send a patch to upstream?
> 
> Good to know !!
> I'm happy if you can update it :)

The original patch has been applied by Mark. Should I send a patch on top
of the original patch or overwrite the original one?


> 
> Thank you for your help !!
> Best regards
> ---
> Kuninori Morimoto
Mark Brown Sept. 18, 2019, 12:07 p.m. UTC | #8
On Wed, Sep 18, 2019 at 06:13:54AM +0000, Liao, Bard wrote:

> The original patch has been applied by Mark. Should I send a patch on top
> of the original patch or overwrite the original one?

Send an incremental patch on top of the one that's already applied
please.
Liao, Bard Sept. 18, 2019, 12:31 p.m. UTC | #9
> -----Original Message-----
> From: Mark Brown [mailto:broonie@kernel.org]
> Sent: Wednesday, September 18, 2019 8:08 PM
> To: Liao, Bard <bard.liao@intel.com>
> Cc: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>; Bard liao 
> <yung- chuan.liao@linux.intel.com>; tiwai@suse.de; 
> alsa-devel@alsa-project.org; pierre-louis.bossart@linux.intel.com
> Subject: Re: [PATCH] ASoC: core: delete component->card_list in 
> soc_remove_component only
> 
> On Wed, Sep 18, 2019 at 06:13:54AM +0000, Liao, Bard wrote:
> 
> > The original patch has been applied by Mark. Should I send a patch 
> > on top of the original patch or overwrite the original one?
> 
> Send an incremental patch on top of the one that's already applied please.

Thanks Mark for the instruction. 

Hi Morimoto-san,

May I use your signed-off as first author since that is your idea? :)
Mark Brown Sept. 18, 2019, 12:53 p.m. UTC | #10
On Wed, Sep 18, 2019 at 12:31:10PM +0000, Liao, Bard wrote:

> May I use your signed-off as first author since that is your idea? :)

Suggested-by also works for situations like this (and is a bit better if
you didn't get sent an actual patch).
Kuninori Morimoto Sept. 18, 2019, 11:50 p.m. UTC | #11
Hi Liao, Mark

> > May I use your signed-off as first author since that is your idea? :)
> 
> Suggested-by also works for situations like this (and is a bit better if
> you didn't get sent an actual patch).

Everything is OK for me.

Thank you for your help !!
Best regards
---
Kuninori Morimoto

Patch
diff mbox series

diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index 35f48e9c5ead..aff4b4bf4d07 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -978,7 +978,6 @@  static void soc_cleanup_component(struct snd_soc_component *component)
 	/* For framework level robustness */
 	snd_soc_component_set_jack(component, NULL, NULL);
 
-	list_del(&component->card_list);
 	snd_soc_dapm_free(snd_soc_component_get_dapm(component));
 	soc_cleanup_component_debugfs(component);
 	component->card = NULL;
@@ -991,7 +990,7 @@  static void soc_remove_component(struct snd_soc_component *component)
 		return;
 
 	snd_soc_component_remove(component);
-
+	list_del(&component->card_list);
 	soc_cleanup_component(component);
 }