diff mbox series

[v4] ALSA: core: Fix deadlock when shutdown a frozen userspace

Message ID 20221127-snd-freeze-v4-0-51ca64b7f2ab@chromium.org
State New
Headers
Series [v4] ALSA: core: Fix deadlock when shutdown a frozen userspace |

Commit Message

Ricardo Ribalda Nov. 28, 2022, 1:42 p.m. UTC 
  During kexec(), the userspace is frozen. Therefore we cannot wait for it
to complete.

Avoid running snd_sof_machine_unregister during shutdown.

This fixes:

[   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

And:

[  246.893222] INFO: task kexec-lite:4891 blocked for more than 122 seconds.
[  246.927709] Call Trace:
[  246.930461]  <TASK>
[  246.932819]  __schedule+0x5f9/0x1263
[  246.936855]  ? fsnotify_grab_connector+0x5c/0x70
[  246.942045]  schedule+0x87/0xc5
[  246.945567]  schedule_timeout+0x49/0xf3
[  246.949877]  wait_for_completion+0x86/0xe8
[  246.954463]  snd_card_free+0x68/0x89
...
[  247.001080]  platform_device_unregister+0x12/0x35

Cc: stable@vger.kernel.org
Fixes: 83bfc7e793b5 ("ASoC: SOF: core: unregister clients and machine drivers in .shutdown")
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
To: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
To: Liam Girdwood <lgirdwood@gmail.com>
To: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
To: Bard Liao <yung-chuan.liao@linux.intel.com>
To: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
To: Kai Vehmanen <kai.vehmanen@linux.intel.com>
To: Daniel Baluta <daniel.baluta@nxp.com>
To: Mark Brown <broonie@kernel.org>
To: Jaroslav Kysela <perex@perex.cz>
To: Takashi Iwai <tiwai@suse.com>
Cc: sound-open-firmware@alsa-project.org
Cc: alsa-devel@alsa-project.org
Cc: linux-kernel@vger.kernel.org
---
Changes in v4:
- Do not call snd_sof_machine_unregister from shutdown.
- Link to v3: https://lore.kernel.org/r/20221127-snd-freeze-v3-0-a2eda731ca14@chromium.org

Changes in v3:
- Wrap pm_freezing in a function
- Link to v2: https://lore.kernel.org/r/20221127-snd-freeze-v2-0-d8a425ea9663@chromium.org

Changes in v2:
- Only use pm_freezing if CONFIG_FREEZER 
- Link to v1: https://lore.kernel.org/r/20221127-snd-freeze-v1-0-57461a366ec2@chromium.org
---
 sound/soc/sof/core.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)


---
base-commit: 4312098baf37ee17a8350725e6e0d0e8590252d4
change-id: 20221127-snd-freeze-1ee143228326

Best regards,

Comments

Takashi Iwai Nov. 28, 2022, 1:47 p.m. UTC | #1 
On Mon, 28 Nov 2022 14:42:49 +0100,
Ricardo Ribalda wrote:
> 
> During kexec(), the userspace is frozen. Therefore we cannot wait for it
> to complete.
> 
> Avoid running snd_sof_machine_unregister during shutdown.
> 
> This fixes:
> 
> [   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
> 
> And:
> 
> [  246.893222] INFO: task kexec-lite:4891 blocked for more than 122 seconds.
> [  246.927709] Call Trace:
> [  246.930461]  <TASK>
> [  246.932819]  __schedule+0x5f9/0x1263
> [  246.936855]  ? fsnotify_grab_connector+0x5c/0x70
> [  246.942045]  schedule+0x87/0xc5
> [  246.945567]  schedule_timeout+0x49/0xf3
> [  246.949877]  wait_for_completion+0x86/0xe8
> [  246.954463]  snd_card_free+0x68/0x89
> ...
> [  247.001080]  platform_device_unregister+0x12/0x35
> 
> Cc: stable@vger.kernel.org
> Fixes: 83bfc7e793b5 ("ASoC: SOF: core: unregister clients and machine drivers in .shutdown")
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
> To: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> To: Liam Girdwood <lgirdwood@gmail.com>
> To: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
> To: Bard Liao <yung-chuan.liao@linux.intel.com>
> To: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
> To: Kai Vehmanen <kai.vehmanen@linux.intel.com>
> To: Daniel Baluta <daniel.baluta@nxp.com>
> To: Mark Brown <broonie@kernel.org>
> To: Jaroslav Kysela <perex@perex.cz>
> To: Takashi Iwai <tiwai@suse.com>
> Cc: sound-open-firmware@alsa-project.org
> Cc: alsa-devel@alsa-project.org
> Cc: linux-kernel@vger.kernel.org
> ---
> Changes in v4:
> - Do not call snd_sof_machine_unregister from shutdown.
> - Link to v3: https://lore.kernel.org/r/20221127-snd-freeze-v3-0-a2eda731ca14@chromium.org

The subject prefix should be adjusted -- now it's no longer about ALSA
core but specific to ASoC SOF.


thanks,

Takashi
Pierre-Louis Bossart Nov. 28, 2022, 4:49 p.m. UTC | #2 
On 11/28/22 07:42, Ricardo Ribalda wrote:
> During kexec(), the userspace is frozen. Therefore we cannot wait for it
> to complete.
> 
> Avoid running snd_sof_machine_unregister during shutdown.
> 
> This fixes:
> 
> [   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
> 
> And:
> 
> [  246.893222] INFO: task kexec-lite:4891 blocked for more than 122 seconds.
> [  246.927709] Call Trace:
> [  246.930461]  <TASK>
> [  246.932819]  __schedule+0x5f9/0x1263
> [  246.936855]  ? fsnotify_grab_connector+0x5c/0x70
> [  246.942045]  schedule+0x87/0xc5
> [  246.945567]  schedule_timeout+0x49/0xf3
> [  246.949877]  wait_for_completion+0x86/0xe8
> [  246.954463]  snd_card_free+0x68/0x89
> ...
> [  247.001080]  platform_device_unregister+0x12/0x35
> 
> Cc: stable@vger.kernel.org
> Fixes: 83bfc7e793b5 ("ASoC: SOF: core: unregister clients and machine drivers in .shutdown")
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
> To: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> To: Liam Girdwood <lgirdwood@gmail.com>
> To: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
> To: Bard Liao <yung-chuan.liao@linux.intel.com>
> To: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
> To: Kai Vehmanen <kai.vehmanen@linux.intel.com>
> To: Daniel Baluta <daniel.baluta@nxp.com>
> To: Mark Brown <broonie@kernel.org>
> To: Jaroslav Kysela <perex@perex.cz>
> To: Takashi Iwai <tiwai@suse.com>
> Cc: sound-open-firmware@alsa-project.org
> Cc: alsa-devel@alsa-project.org
> Cc: linux-kernel@vger.kernel.org
> ---
> Changes in v4:
> - Do not call snd_sof_machine_unregister from shutdown.
> - Link to v3: https://lore.kernel.org/r/20221127-snd-freeze-v3-0-a2eda731ca14@chromium.org
> 
> Changes in v3:
> - Wrap pm_freezing in a function
> - Link to v2: https://lore.kernel.org/r/20221127-snd-freeze-v2-0-d8a425ea9663@chromium.org
> 
> Changes in v2:
> - Only use pm_freezing if CONFIG_FREEZER 
> - Link to v1: https://lore.kernel.org/r/20221127-snd-freeze-v1-0-57461a366ec2@chromium.org
> ---
>  sound/soc/sof/core.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/sound/soc/sof/core.c b/sound/soc/sof/core.c
> index 3e6141d03770..9616ba607ded 100644
> --- a/sound/soc/sof/core.c
> +++ b/sound/soc/sof/core.c
> @@ -475,19 +475,16 @@ 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
> +	 * make sure clients are unregistered prior to the DSP shutdown
> +	 * sequence.
>  	 */
>  	sof_unregister_clients(sdev);
>  
> -	snd_sof_machine_unregister(sdev, pdata);
> -

The comment clearly says that we do want all userspace devices to be
closed. This was added in 83bfc7e793b5 ("ASoC: SOF: core: unregister
clients and machine drivers in .shutdown") precisely to avoid a platform
hang if the devices are used after the shutdown completes.

So you are not fixing 83bfc7e793b5, just re-adding a problem to fix
another one...
Takashi Iwai Nov. 28, 2022, 5:04 p.m. UTC | #3 
On Mon, 28 Nov 2022 17:49:20 +0100,
Pierre-Louis Bossart wrote:
> 
> 
> 
> On 11/28/22 07:42, Ricardo Ribalda wrote:
> > During kexec(), the userspace is frozen. Therefore we cannot wait for it
> > to complete.
> > 
> > Avoid running snd_sof_machine_unregister during shutdown.
> > 
> > This fixes:
> > 
> > [   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
> > 
> > And:
> > 
> > [  246.893222] INFO: task kexec-lite:4891 blocked for more than 122 seconds.
> > [  246.927709] Call Trace:
> > [  246.930461]  <TASK>
> > [  246.932819]  __schedule+0x5f9/0x1263
> > [  246.936855]  ? fsnotify_grab_connector+0x5c/0x70
> > [  246.942045]  schedule+0x87/0xc5
> > [  246.945567]  schedule_timeout+0x49/0xf3
> > [  246.949877]  wait_for_completion+0x86/0xe8
> > [  246.954463]  snd_card_free+0x68/0x89
> > ...
> > [  247.001080]  platform_device_unregister+0x12/0x35
> > 
> > Cc: stable@vger.kernel.org
> > Fixes: 83bfc7e793b5 ("ASoC: SOF: core: unregister clients and machine drivers in .shutdown")
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> > To: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> > To: Liam Girdwood <lgirdwood@gmail.com>
> > To: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
> > To: Bard Liao <yung-chuan.liao@linux.intel.com>
> > To: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
> > To: Kai Vehmanen <kai.vehmanen@linux.intel.com>
> > To: Daniel Baluta <daniel.baluta@nxp.com>
> > To: Mark Brown <broonie@kernel.org>
> > To: Jaroslav Kysela <perex@perex.cz>
> > To: Takashi Iwai <tiwai@suse.com>
> > Cc: sound-open-firmware@alsa-project.org
> > Cc: alsa-devel@alsa-project.org
> > Cc: linux-kernel@vger.kernel.org
> > ---
> > Changes in v4:
> > - Do not call snd_sof_machine_unregister from shutdown.
> > - Link to v3: https://lore.kernel.org/r/20221127-snd-freeze-v3-0-a2eda731ca14@chromium.org
> > 
> > Changes in v3:
> > - Wrap pm_freezing in a function
> > - Link to v2: https://lore.kernel.org/r/20221127-snd-freeze-v2-0-d8a425ea9663@chromium.org
> > 
> > Changes in v2:
> > - Only use pm_freezing if CONFIG_FREEZER 
> > - Link to v1: https://lore.kernel.org/r/20221127-snd-freeze-v1-0-57461a366ec2@chromium.org
> > ---
> >  sound/soc/sof/core.c | 7 ++-----
> >  1 file changed, 2 insertions(+), 5 deletions(-)
> > 
> > diff --git a/sound/soc/sof/core.c b/sound/soc/sof/core.c
> > index 3e6141d03770..9616ba607ded 100644
> > --- a/sound/soc/sof/core.c
> > +++ b/sound/soc/sof/core.c
> > @@ -475,19 +475,16 @@ 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
> > +	 * make sure clients are unregistered prior to the DSP shutdown
> > +	 * sequence.
> >  	 */
> >  	sof_unregister_clients(sdev);
> >  
> > -	snd_sof_machine_unregister(sdev, pdata);
> > -
> 
> The comment clearly says that we do want all userspace devices to be
> closed. This was added in 83bfc7e793b5 ("ASoC: SOF: core: unregister
> clients and machine drivers in .shutdown") precisely to avoid a platform
> hang if the devices are used after the shutdown completes.

The problem is that it wants the *close* of the user-space programs
unnecessarily.  Basically the shutdown can be seen as a sort of device
hot unplug; i.e. the disconnection of the device files and the cleanup
of device state are the main task.  The difference is that the hot
unplug (unbind) usually follows the sync for the all processes being
closed (so that you can release all resources gracefully), while this
step is skipped for the shutdown (no need for resource-free).


Takashi
Pierre-Louis Bossart Nov. 28, 2022, 5:26 p.m. UTC | #4 
On 11/28/22 11:04, Takashi Iwai wrote:
> On Mon, 28 Nov 2022 17:49:20 +0100,
> Pierre-Louis Bossart wrote:
>>
>>
>>
>> On 11/28/22 07:42, Ricardo Ribalda wrote:
>>> During kexec(), the userspace is frozen. Therefore we cannot wait for it
>>> to complete.
>>>
>>> Avoid running snd_sof_machine_unregister during shutdown.
>>>
>>> This fixes:
>>>
>>> [   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
>>>
>>> And:
>>>
>>> [  246.893222] INFO: task kexec-lite:4891 blocked for more than 122 seconds.
>>> [  246.927709] Call Trace:
>>> [  246.930461]  <TASK>
>>> [  246.932819]  __schedule+0x5f9/0x1263
>>> [  246.936855]  ? fsnotify_grab_connector+0x5c/0x70
>>> [  246.942045]  schedule+0x87/0xc5
>>> [  246.945567]  schedule_timeout+0x49/0xf3
>>> [  246.949877]  wait_for_completion+0x86/0xe8
>>> [  246.954463]  snd_card_free+0x68/0x89
>>> ...
>>> [  247.001080]  platform_device_unregister+0x12/0x35
>>>
>>> Cc: stable@vger.kernel.org
>>> Fixes: 83bfc7e793b5 ("ASoC: SOF: core: unregister clients and machine drivers in .shutdown")
>>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
>>> ---
>>> To: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
>>> To: Liam Girdwood <lgirdwood@gmail.com>
>>> To: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
>>> To: Bard Liao <yung-chuan.liao@linux.intel.com>
>>> To: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
>>> To: Kai Vehmanen <kai.vehmanen@linux.intel.com>
>>> To: Daniel Baluta <daniel.baluta@nxp.com>
>>> To: Mark Brown <broonie@kernel.org>
>>> To: Jaroslav Kysela <perex@perex.cz>
>>> To: Takashi Iwai <tiwai@suse.com>
>>> Cc: sound-open-firmware@alsa-project.org
>>> Cc: alsa-devel@alsa-project.org
>>> Cc: linux-kernel@vger.kernel.org
>>> ---
>>> Changes in v4:
>>> - Do not call snd_sof_machine_unregister from shutdown.
>>> - Link to v3: https://lore.kernel.org/r/20221127-snd-freeze-v3-0-a2eda731ca14@chromium.org
>>>
>>> Changes in v3:
>>> - Wrap pm_freezing in a function
>>> - Link to v2: https://lore.kernel.org/r/20221127-snd-freeze-v2-0-d8a425ea9663@chromium.org
>>>
>>> Changes in v2:
>>> - Only use pm_freezing if CONFIG_FREEZER 
>>> - Link to v1: https://lore.kernel.org/r/20221127-snd-freeze-v1-0-57461a366ec2@chromium.org
>>> ---
>>>  sound/soc/sof/core.c | 7 ++-----
>>>  1 file changed, 2 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/sound/soc/sof/core.c b/sound/soc/sof/core.c
>>> index 3e6141d03770..9616ba607ded 100644
>>> --- a/sound/soc/sof/core.c
>>> +++ b/sound/soc/sof/core.c
>>> @@ -475,19 +475,16 @@ 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
>>> +	 * make sure clients are unregistered prior to the DSP shutdown
>>> +	 * sequence.
>>>  	 */
>>>  	sof_unregister_clients(sdev);
>>>  
>>> -	snd_sof_machine_unregister(sdev, pdata);
>>> -
>>
>> The comment clearly says that we do want all userspace devices to be
>> closed. This was added in 83bfc7e793b5 ("ASoC: SOF: core: unregister
>> clients and machine drivers in .shutdown") precisely to avoid a platform
>> hang if the devices are used after the shutdown completes.
> 
> The problem is that it wants the *close* of the user-space programs
> unnecessarily.  Basically the shutdown can be seen as a sort of device
> hot unplug; i.e. the disconnection of the device files and the cleanup
> of device state are the main task.  The difference is that the hot
> unplug (unbind) usually follows the sync for the all processes being
> closed (so that you can release all resources gracefully), while this
> step is skipped for the shutdown (no need for resource-free).

Sorry Takashi, I don't have enough background to follow your explanations.

As Kai mentioned it, this step helped with a S5 issue earlier in 2022.
Removing this will mechanically bring the issue back and break other
Chromebooks.
Takashi Iwai Nov. 29, 2022, 7:52 a.m. UTC | #5 
On Mon, 28 Nov 2022 18:26:03 +0100,
Pierre-Louis Bossart wrote:
> 
> 
> 
> On 11/28/22 11:04, Takashi Iwai wrote:
> > On Mon, 28 Nov 2022 17:49:20 +0100,
> > Pierre-Louis Bossart wrote:
> >>
> >>
> >>
> >> On 11/28/22 07:42, Ricardo Ribalda wrote:
> >>> During kexec(), the userspace is frozen. Therefore we cannot wait for it
> >>> to complete.
> >>>
> >>> Avoid running snd_sof_machine_unregister during shutdown.
> >>>
> >>> This fixes:
> >>>
> >>> [   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
> >>>
> >>> And:
> >>>
> >>> [  246.893222] INFO: task kexec-lite:4891 blocked for more than 122 seconds.
> >>> [  246.927709] Call Trace:
> >>> [  246.930461]  <TASK>
> >>> [  246.932819]  __schedule+0x5f9/0x1263
> >>> [  246.936855]  ? fsnotify_grab_connector+0x5c/0x70
> >>> [  246.942045]  schedule+0x87/0xc5
> >>> [  246.945567]  schedule_timeout+0x49/0xf3
> >>> [  246.949877]  wait_for_completion+0x86/0xe8
> >>> [  246.954463]  snd_card_free+0x68/0x89
> >>> ...
> >>> [  247.001080]  platform_device_unregister+0x12/0x35
> >>>
> >>> Cc: stable@vger.kernel.org
> >>> Fixes: 83bfc7e793b5 ("ASoC: SOF: core: unregister clients and machine drivers in .shutdown")
> >>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> >>> ---
> >>> To: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> >>> To: Liam Girdwood <lgirdwood@gmail.com>
> >>> To: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
> >>> To: Bard Liao <yung-chuan.liao@linux.intel.com>
> >>> To: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
> >>> To: Kai Vehmanen <kai.vehmanen@linux.intel.com>
> >>> To: Daniel Baluta <daniel.baluta@nxp.com>
> >>> To: Mark Brown <broonie@kernel.org>
> >>> To: Jaroslav Kysela <perex@perex.cz>
> >>> To: Takashi Iwai <tiwai@suse.com>
> >>> Cc: sound-open-firmware@alsa-project.org
> >>> Cc: alsa-devel@alsa-project.org
> >>> Cc: linux-kernel@vger.kernel.org
> >>> ---
> >>> Changes in v4:
> >>> - Do not call snd_sof_machine_unregister from shutdown.
> >>> - Link to v3: https://lore.kernel.org/r/20221127-snd-freeze-v3-0-a2eda731ca14@chromium.org
> >>>
> >>> Changes in v3:
> >>> - Wrap pm_freezing in a function
> >>> - Link to v2: https://lore.kernel.org/r/20221127-snd-freeze-v2-0-d8a425ea9663@chromium.org
> >>>
> >>> Changes in v2:
> >>> - Only use pm_freezing if CONFIG_FREEZER 
> >>> - Link to v1: https://lore.kernel.org/r/20221127-snd-freeze-v1-0-57461a366ec2@chromium.org
> >>> ---
> >>>  sound/soc/sof/core.c | 7 ++-----
> >>>  1 file changed, 2 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/sound/soc/sof/core.c b/sound/soc/sof/core.c
> >>> index 3e6141d03770..9616ba607ded 100644
> >>> --- a/sound/soc/sof/core.c
> >>> +++ b/sound/soc/sof/core.c
> >>> @@ -475,19 +475,16 @@ 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
> >>> +	 * make sure clients are unregistered prior to the DSP shutdown
> >>> +	 * sequence.
> >>>  	 */
> >>>  	sof_unregister_clients(sdev);
> >>>  
> >>> -	snd_sof_machine_unregister(sdev, pdata);
> >>> -
> >>
> >> The comment clearly says that we do want all userspace devices to be
> >> closed. This was added in 83bfc7e793b5 ("ASoC: SOF: core: unregister
> >> clients and machine drivers in .shutdown") precisely to avoid a platform
> >> hang if the devices are used after the shutdown completes.
> > 
> > The problem is that it wants the *close* of the user-space programs
> > unnecessarily.  Basically the shutdown can be seen as a sort of device
> > hot unplug; i.e. the disconnection of the device files and the cleanup
> > of device state are the main task.  The difference is that the hot
> > unplug (unbind) usually follows the sync for the all processes being
> > closed (so that you can release all resources gracefully), while this
> > step is skipped for the shutdown (no need for resource-free).
> 
> Sorry Takashi, I don't have enough background to follow your explanations.
> 
> As Kai mentioned it, this step helped with a S5 issue earlier in 2022.
> Removing this will mechanically bring the issue back and break other
> Chromebooks.

Yeah I don't mean that this fix is right, either.  But the earlier fix
has apparently a problem and needs another fix.

Though, it's not clear why the full unregister of clients is needed at
the first place; judging only from the patch description of commit
83bfc7e793b5, what we want is only to shut up the further user space
action?  If so, just call snd_card_disconnect() would suffice?


Takashi
Kai Vehmanen Nov. 29, 2022, 12:11 p.m. UTC | #6 
Hi

On Tue, 29 Nov 2022, Takashi Iwai wrote:

> On Mon, 28 Nov 2022 18:26:03 +0100, Pierre-Louis Bossart wrote:
> > As Kai mentioned it, this step helped with a S5 issue earlier in 2022.
> > Removing this will mechanically bring the issue back and break other
> > Chromebooks.
> 
> Yeah I don't mean that this fix is right, either.  But the earlier fix
> has apparently a problem and needs another fix.
> 
> Though, it's not clear why the full unregister of clients is needed at
> the first place; judging only from the patch description of commit
> 83bfc7e793b5, what we want is only to shut up the further user space
> action?  If so, just call snd_card_disconnect() would suffice?

I think the snd_card_disconnect() is what we are looking after here, but 
it's just easiest to do via unregister in SOF as that will trigger will 
look up the platform device, unregister it, and it eventually the driver 
owning the card will do the disconnect. Possibility for sure to do a more
direct implementation and not run the full unregister.

On the other end of the solution spectrum, we had this alternative to let 
user-space stay connected and just have the DSP implementations handle 
any pending work in their respective shutdown handlers. I.e. we had
"ASoC: SOF: Intel: pci-tgl: unblock S5 entry if DMA stop has failed"
https://github.com/thesofproject/linux/pull/3388

This was eventually dropped (and never sent upstream) as 83bfc7e793b5 got 
the same result, and covered all SOF platforms with a single code path.
Bringing this back is of course one option, but then this might suprise 
other platforms (which might have got used to user-space getting 
disconnected at shutdown via 83bfc7e793b5).

Br, Kai
Ricardo Ribalda Nov. 30, 2022, 3:49 p.m. UTC | #7 
Hi

I just sent a v6 that only avoids unregistering the clients during
kexec... let me know if that works for you

Thanks!

On Tue, 29 Nov 2022 at 13:12, Kai Vehmanen <kai.vehmanen@linux.intel.com> wrote:
>
> Hi
>
> On Tue, 29 Nov 2022, Takashi Iwai wrote:
>
> > On Mon, 28 Nov 2022 18:26:03 +0100, Pierre-Louis Bossart wrote:
> > > As Kai mentioned it, this step helped with a S5 issue earlier in 2022.
> > > Removing this will mechanically bring the issue back and break other
> > > Chromebooks.
> >
> > Yeah I don't mean that this fix is right, either.  But the earlier fix
> > has apparently a problem and needs another fix.
> >
> > Though, it's not clear why the full unregister of clients is needed at
> > the first place; judging only from the patch description of commit
> > 83bfc7e793b5, what we want is only to shut up the further user space
> > action?  If so, just call snd_card_disconnect() would suffice?
>
> I think the snd_card_disconnect() is what we are looking after here, but
> it's just easiest to do via unregister in SOF as that will trigger will
> look up the platform device, unregister it, and it eventually the driver
> owning the card will do the disconnect. Possibility for sure to do a more
> direct implementation and not run the full unregister.
>
> On the other end of the solution spectrum, we had this alternative to let
> user-space stay connected and just have the DSP implementations handle
> any pending work in their respective shutdown handlers. I.e. we had
> "ASoC: SOF: Intel: pci-tgl: unblock S5 entry if DMA stop has failed"
> https://github.com/thesofproject/linux/pull/3388
>
> This was eventually dropped (and never sent upstream) as 83bfc7e793b5 got
> the same result, and covered all SOF platforms with a single code path.
> Bringing this back is of course one option, but then this might suprise
> other platforms (which might have got used to user-space getting
> disconnected at shutdown via 83bfc7e793b5).
>
> Br, Kai
diff mbox series

Patch

diff --git a/sound/soc/sof/core.c b/sound/soc/sof/core.c
index 3e6141d03770..9616ba607ded 100644
--- a/sound/soc/sof/core.c
+++ b/sound/soc/sof/core.c
@@ -475,19 +475,16 @@  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
+	 * make sure clients are unregistered 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);