| Message ID | 20190916210353.6318-1-yung-chuan.liao@linux.intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | a0a4bf57a977ed37bcbdfc8027a44485fe086a3d |
| Headers | show |
| Series | ASoC: core: delete component->card_list in soc_remove_component only | expand |
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); > } > >
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
> -----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
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
> -----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
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
> -----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
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.
> -----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? :)
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).
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
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); }