| Message ID | 20221209114529.3909192-3-kai.vehmanen@linux.intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 44fda61d2bcfb74a942df93959e083a4e8eff75f |
| Headers | show |
| Series | ASoC: SOF: remove unregister calls from shutdown | expand |
On Fri, Dec 09, 2022 at 01:45:29PM +0200, Kai Vehmanen wrote: > This reverts commit 83bfc7e793b555291785136c3ae86abcdc046887. Please include human readable descriptions of things like commits and issues being discussed in e-mail in your mails, this makes them much easier for humans to read especially when they have no internet access. I do frequently catch up on my mail on flights or while otherwise travelling so this is even more pressing for me than just being about making things a bit easier to read. > - /* > - * make sure clients and machine driver(s) are unregistered to force > - * all userspace devices to be closed prior to the DSP shutdown sequence > - */ > - sof_unregister_clients(sdev); > - > - snd_sof_machine_unregister(sdev, pdata); > - I am somewhat surprised that these block on the kernel side rather than just disconnecting the kernel side of whatever userspace sees - I'd thought they were more like hotplug operations than something blocking.
Hi, On Fri, 9 Dec 2022, Mark Brown wrote: > On Fri, Dec 09, 2022 at 01:45:29PM +0200, Kai Vehmanen wrote: > > > This reverts commit 83bfc7e793b555291785136c3ae86abcdc046887. > > Please include human readable descriptions of things like commits and > issues being discussed in e-mail in your mails, this makes them much ack, will fix in V2. > easier for humans to read especially when they have no internet access. > I do frequently catch up on my mail on flights or while otherwise > travelling so this is even more pressing for me than just being about > making things a bit easier to read. > > > - /* > > - * make sure clients and machine driver(s) are unregistered to force > > - * all userspace devices to be closed prior to the DSP shutdown sequence > > - */ > > - sof_unregister_clients(sdev); > > - > > - snd_sof_machine_unregister(sdev, pdata); > > - > > I am somewhat surprised that these block on the kernel side rather than > just disconnecting the kernel side of whatever userspace sees - I'd > thought they were more like hotplug operations than something blocking. Yes, this is not so obvious. The machine unregister ends up in soc-core.c:soc_cleanup_card_resources() --cut-- static void soc_cleanup_card_resources(struct snd_soc_card *card) { » struct snd_soc_pcm_runtime *rtd, *n; » if (card->snd_card) » » snd_card_disconnect_sync(card->snd_card); --cut-- ... and there we have the "sync" variant call that gets us in trouble with e.g. kexec(). This seems to be only user of snd_card_disconnect_sync(), but it was specifically added to fix a bug in commit 0ced7b050224 ("ASoC: soc-pcm: remove soc_pcm_private_free()"). Br, Kai
Hi Kai Thanks for the patch. Just tested it on Alderlake-P On Fri, 9 Dec 2022 at 12:46, Kai Vehmanen <kai.vehmanen@linux.intel.com> wrote: > > The unregister machine drivers call is not safe to do when > kexec is used. Kexec-lite gets blocked with following backtrace: > > [ 84.943749] Freezing user space processes ... (elapsed 0.111 seconds) done. > [ 246.784446] INFO: task kexec-lite:5123 blocked for more than 122 seconds. > [ 246.819035] Call Trace: > [ 246.821782] <TASK> > [ 246.824186] __schedule+0x5f9/0x1263 > [ 246.828231] schedule+0x87/0xc5 > [ 246.831779] snd_card_disconnect_sync+0xb5/0x127 > ... > [ 246.889249] snd_sof_device_shutdown+0xb4/0x150 > [ 246.899317] pci_device_shutdown+0x37/0x61 > [ 246.903990] device_shutdown+0x14c/0x1d6 > [ 246.908391] kernel_kexec+0x45/0xb9 > > This reverts commit 83bfc7e793b555291785136c3ae86abcdc046887. > > Reported-by: Ricardo Ribalda <ribalda@chromium.org> > Cc: Ricardo Ribalda <ribalda@chromium.org> > Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com> Tested-by: Ricardo Ribalda <ribalda@chromium.org> > Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > Reviewed-by: Péter Ujfalusi <peter.ujfalusi@linux.intel.com> > Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com> > --- > sound/soc/sof/core.c | 9 --------- > 1 file changed, 9 deletions(-) > > diff --git a/sound/soc/sof/core.c b/sound/soc/sof/core.c > index 3e6141d03770..625977a29d8a 100644 > --- a/sound/soc/sof/core.c > +++ b/sound/soc/sof/core.c > @@ -475,19 +475,10 @@ EXPORT_SYMBOL(snd_sof_device_remove); > int snd_sof_device_shutdown(struct device *dev) > { > struct snd_sof_dev *sdev = dev_get_drvdata(dev); > - struct snd_sof_pdata *pdata = sdev->pdata; > > if (IS_ENABLED(CONFIG_SND_SOC_SOF_PROBE_WORK_QUEUE)) > cancel_work_sync(&sdev->probe_work); > > - /* > - * make sure clients and machine driver(s) are unregistered to force > - * all userspace devices to be closed prior to the DSP shutdown sequence > - */ > - sof_unregister_clients(sdev); > - > - snd_sof_machine_unregister(sdev, pdata); > - > if (sdev->fw_state == SOF_FW_BOOT_COMPLETE) > return snd_sof_shutdown(sdev); > > -- > 2.38.1 >
diff --git a/sound/soc/sof/core.c b/sound/soc/sof/core.c index 3e6141d03770..625977a29d8a 100644 --- a/sound/soc/sof/core.c +++ b/sound/soc/sof/core.c @@ -475,19 +475,10 @@ EXPORT_SYMBOL(snd_sof_device_remove); int snd_sof_device_shutdown(struct device *dev) { struct snd_sof_dev *sdev = dev_get_drvdata(dev); - struct snd_sof_pdata *pdata = sdev->pdata; if (IS_ENABLED(CONFIG_SND_SOC_SOF_PROBE_WORK_QUEUE)) cancel_work_sync(&sdev->probe_work); - /* - * make sure clients and machine driver(s) are unregistered to force - * all userspace devices to be closed prior to the DSP shutdown sequence - */ - sof_unregister_clients(sdev); - - snd_sof_machine_unregister(sdev, pdata); - if (sdev->fw_state == SOF_FW_BOOT_COMPLETE) return snd_sof_shutdown(sdev);