diff mbox series

ASoc: SOF: Fix sof-audio-pci-intel-tgl shutdown timeout during hibernation

Message ID 20221207102229.25962-1-nizhen@uniontech.com (mailing list archive)
State New, archived
Headers show
Series ASoc: SOF: Fix sof-audio-pci-intel-tgl shutdown timeout during hibernation | expand

Commit Message

Zhen Ni Dec. 7, 2022, 10:22 a.m. UTC
On Dell Latitude 3420 Notebook, sof-audio-pci-intel-tgl may fail to shutdown
sporadically during hibernation as following log:

[   43.281110] PM: Image saving done
[   43.281699] PM: hibernation: Wrote 2828852 kbytes in 2.78 seconds(1017.57 MB/s)
[   43.282359] PM: SI
[   43.345156] kvm: exiting hardware virtualization
[   43.345865] auxiliary snd_sof.hda-probes.0: shutdown
[   43.346359] skl_hda_dsp_generic skl_hda_dsp_generic: shutdown
[   43.346849] skl_hda_codec hdmi ehdaudio0D2: shutdown
[   43.398204] snd_hda_codec_realtek ehdaudio0DO: shutdown
[   43.419621] dmic-codec dmic-codec: shutdown
[   43.420194] sof-audio-pci-intel-tgl 0000:00:1f.3: shutdown

Call wait_xxx_timeout() to process the timeout.

Signed-off-by: Zhen Ni <nizhen@uniontech.com>
---
 sound/core/init.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Comments

Takashi Iwai Dec. 7, 2022, 10:32 a.m. UTC | #1
On Wed, 07 Dec 2022 11:22:29 +0100,
Zhen Ni wrote:
> 
> On Dell Latitude 3420 Notebook, sof-audio-pci-intel-tgl may fail to shutdown
> sporadically during hibernation as following log:
> 
> [   43.281110] PM: Image saving done
> [   43.281699] PM: hibernation: Wrote 2828852 kbytes in 2.78 seconds(1017.57 MB/s)
> [   43.282359] PM: SI
> [   43.345156] kvm: exiting hardware virtualization
> [   43.345865] auxiliary snd_sof.hda-probes.0: shutdown
> [   43.346359] skl_hda_dsp_generic skl_hda_dsp_generic: shutdown
> [   43.346849] skl_hda_codec hdmi ehdaudio0D2: shutdown
> [   43.398204] snd_hda_codec_realtek ehdaudio0DO: shutdown
> [   43.419621] dmic-codec dmic-codec: shutdown
> [   43.420194] sof-audio-pci-intel-tgl 0000:00:1f.3: shutdown
> 
> Call wait_xxx_timeout() to process the timeout.
> 
> Signed-off-by: Zhen Ni <nizhen@uniontech.com>

It's a known problem and being discussed (although it's a slightly
different code path):
  https://lore.kernel.org/r/20221127-snd-freeze-v4-0-51ca64b7f2ab@chromium.org

We need the proper fix for ASoC shutdown.
Adding relevant people to Cc.

And, the unconditional exit from the sync is dangerous.  It may lead
to use-after-free or such.


thanks,

Takashi

> ---
>  sound/core/init.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/sound/core/init.c b/sound/core/init.c
> index 5377f94eb211..9bd674d7a0fd 100644
> --- a/sound/core/init.c
> +++ b/sound/core/init.c
> @@ -574,9 +574,10 @@ void snd_card_disconnect_sync(struct snd_card *card)
>  	}
>  
>  	spin_lock_irq(&card->files_lock);
> -	wait_event_lock_irq(card->remove_sleep,
> +	wait_event_lock_irq_timeout(card->remove_sleep,
>  			    list_empty(&card->files_list),
> -			    card->files_lock);
> +			    card->files_lock,
> +			    msecs_to_jiffies(2000));
>  	spin_unlock_irq(&card->files_lock);
>  }
>  EXPORT_SYMBOL_GPL(snd_card_disconnect_sync);
> @@ -659,7 +660,7 @@ int snd_card_free(struct snd_card *card)
>  	if (ret)
>  		return ret;
>  	/* wait, until all devices are ready for the free operation */
> -	wait_for_completion(&released);
> +	wait_for_completion_timeout(&released, msecs_to_jiffies(2000))
>  
>  	return 0;
>  }
> -- 
> 2.20.1
> 
>
Kai Vehmanen Dec. 7, 2022, 12:55 p.m. UTC | #2
Hi,

On Wed, 7 Dec 2022, Takashi Iwai wrote:

> On Wed, 07 Dec 2022 11:22:29 +0100, Zhen Ni wrote:
> > On Dell Latitude 3420 Notebook, sof-audio-pci-intel-tgl may fail to shutdown
> > sporadically during hibernation as following log:
> > 
> > [   43.281110] PM: Image saving done
> > [   43.281699] PM: hibernation: Wrote 2828852 kbytes in 2.78 seconds(1017.57 MB/s)
> > [   43.282359] PM: SI
> > [   43.345156] kvm: exiting hardware virtualization
> > [   43.345865] auxiliary snd_sof.hda-probes.0: shutdown
>
> 
> It's a known problem and being discussed (although it's a slightly
> different code path):
>   https://lore.kernel.org/r/20221127-snd-freeze-v4-0-51ca64b7f2ab@chromium.org

thanks Zhen Ni for the report and patch. We have a fix candidate
in review+testing at:
https://github.com/thesofproject/linux/pull/4072

... that should help in your case as well. Once testing is complete,
we'll be sending the patch series to alsa-devel.

Br, Kai
kernel test robot Dec. 7, 2022, 4:21 p.m. UTC | #3
Hi Zhen,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on tiwai-sound/for-next]
[also build test ERROR on tiwai-sound/for-linus linus/master v6.1-rc8 next-20221207]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Zhen-Ni/ASoc-SOF-Fix-sof-audio-pci-intel-tgl-shutdown-timeout-during-hibernation/20221207-182430
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git for-next
patch link:    https://lore.kernel.org/r/20221207102229.25962-1-nizhen%40uniontech.com
patch subject: [PATCH] ASoc: SOF: Fix sof-audio-pci-intel-tgl shutdown timeout during hibernation
config: arm-randconfig-r046-20221207
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 6e4cea55f0d1104408b26ac574566a0e4de48036)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install arm cross compiling tool for clang build
        # apt-get install binutils-arm-linux-gnueabi
        # https://github.com/intel-lab-lkp/linux/commit/38abc03ca81d8486e02a58fa505b25777b03bcdd
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Zhen-Ni/ASoc-SOF-Fix-sof-audio-pci-intel-tgl-shutdown-timeout-during-hibernation/20221207-182430
        git checkout 38abc03ca81d8486e02a58fa505b25777b03bcdd
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash sound/core/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> sound/core/init.c:663:64: error: expected ';' after expression
           wait_for_completion_timeout(&released, msecs_to_jiffies(2000))
                                                                         ^
                                                                         ;
   1 error generated.


vim +663 sound/core/init.c

   630	
   631	/**
   632	 * snd_card_free - frees given soundcard structure
   633	 * @card: soundcard structure
   634	 *
   635	 * This function releases the soundcard structure and the all assigned
   636	 * devices automatically.  That is, you don't have to release the devices
   637	 * by yourself.
   638	 *
   639	 * This function waits until the all resources are properly released.
   640	 *
   641	 * Return: Zero. Frees all associated devices and frees the control
   642	 * interface associated to given soundcard.
   643	 */
   644	int snd_card_free(struct snd_card *card)
   645	{
   646		DECLARE_COMPLETION_ONSTACK(released);
   647		int ret;
   648	
   649		/* The call of snd_card_free() is allowed from various code paths;
   650		 * a manual call from the driver and the call via devres_free, and
   651		 * we need to avoid double-free. Moreover, the release via devres
   652		 * may call snd_card_free() twice due to its nature, we need to have
   653		 * the check here at the beginning.
   654		 */
   655		if (card->releasing)
   656			return 0;
   657	
   658		card->release_completion = &released;
   659		ret = snd_card_free_when_closed(card);
   660		if (ret)
   661			return ret;
   662		/* wait, until all devices are ready for the free operation */
 > 663		wait_for_completion_timeout(&released, msecs_to_jiffies(2000))
   664	
   665		return 0;
   666	}
   667	EXPORT_SYMBOL(snd_card_free);
   668
kernel test robot Dec. 7, 2022, 4:31 p.m. UTC | #4
Hi Zhen,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on tiwai-sound/for-next]
[also build test ERROR on tiwai-sound/for-linus linus/master v6.1-rc8 next-20221207]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Zhen-Ni/ASoc-SOF-Fix-sof-audio-pci-intel-tgl-shutdown-timeout-during-hibernation/20221207-182430
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git for-next
patch link:    https://lore.kernel.org/r/20221207102229.25962-1-nizhen%40uniontech.com
patch subject: [PATCH] ASoc: SOF: Fix sof-audio-pci-intel-tgl shutdown timeout during hibernation
config: i386-randconfig-a001
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/38abc03ca81d8486e02a58fa505b25777b03bcdd
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Zhen-Ni/ASoc-SOF-Fix-sof-audio-pci-intel-tgl-shutdown-timeout-during-hibernation/20221207-182430
        git checkout 38abc03ca81d8486e02a58fa505b25777b03bcdd
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   sound/core/init.c: In function 'snd_card_free':
>> sound/core/init.c:663:71: error: expected ';' before 'return'
     663 |         wait_for_completion_timeout(&released, msecs_to_jiffies(2000))
         |                                                                       ^
         |                                                                       ;
     664 | 
     665 |         return 0;
         |         ~~~~~~                                                         
   sound/core/init.c:666:1: error: control reaches end of non-void function [-Werror=return-type]
     666 | }
         | ^
   cc1: some warnings being treated as errors


vim +663 sound/core/init.c

   630	
   631	/**
   632	 * snd_card_free - frees given soundcard structure
   633	 * @card: soundcard structure
   634	 *
   635	 * This function releases the soundcard structure and the all assigned
   636	 * devices automatically.  That is, you don't have to release the devices
   637	 * by yourself.
   638	 *
   639	 * This function waits until the all resources are properly released.
   640	 *
   641	 * Return: Zero. Frees all associated devices and frees the control
   642	 * interface associated to given soundcard.
   643	 */
   644	int snd_card_free(struct snd_card *card)
   645	{
   646		DECLARE_COMPLETION_ONSTACK(released);
   647		int ret;
   648	
   649		/* The call of snd_card_free() is allowed from various code paths;
   650		 * a manual call from the driver and the call via devres_free, and
   651		 * we need to avoid double-free. Moreover, the release via devres
   652		 * may call snd_card_free() twice due to its nature, we need to have
   653		 * the check here at the beginning.
   654		 */
   655		if (card->releasing)
   656			return 0;
   657	
   658		card->release_completion = &released;
   659		ret = snd_card_free_when_closed(card);
   660		if (ret)
   661			return ret;
   662		/* wait, until all devices are ready for the free operation */
 > 663		wait_for_completion_timeout(&released, msecs_to_jiffies(2000))
   664	
   665		return 0;
   666	}
   667	EXPORT_SYMBOL(snd_card_free);
   668
diff mbox series

Patch

diff --git a/sound/core/init.c b/sound/core/init.c
index 5377f94eb211..9bd674d7a0fd 100644
--- a/sound/core/init.c
+++ b/sound/core/init.c
@@ -574,9 +574,10 @@  void snd_card_disconnect_sync(struct snd_card *card)
 	}
 
 	spin_lock_irq(&card->files_lock);
-	wait_event_lock_irq(card->remove_sleep,
+	wait_event_lock_irq_timeout(card->remove_sleep,
 			    list_empty(&card->files_list),
-			    card->files_lock);
+			    card->files_lock,
+			    msecs_to_jiffies(2000));
 	spin_unlock_irq(&card->files_lock);
 }
 EXPORT_SYMBOL_GPL(snd_card_disconnect_sync);
@@ -659,7 +660,7 @@  int snd_card_free(struct snd_card *card)
 	if (ret)
 		return ret;
 	/* wait, until all devices are ready for the free operation */
-	wait_for_completion(&released);
+	wait_for_completion_timeout(&released, msecs_to_jiffies(2000))
 
 	return 0;
 }