linux-kernel - Re: [PATCH] sound: usb: pcm: fix incorrect power state when playing sound after PM

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1591100342.23525.8.camel@mtkswgap22>
Date:   Tue, 2 Jun 2020 20:19:02 +0800
From:   Macpaul Lin <macpaul.lin@...iatek.com>
To:     Jaroslav Kysela <perex@...ex.cz>,
        Alexander Tsoy <alexander@...y.me>,
        Johan Hovold <johan@...nel.org>,
        Hui Wang <hui.wang@...onical.com>,
        Szabolcs Szőke <szszoke.code@...il.com>,
        <alsa-devel@...a-project.org>, <linux-usb@...r.kernel.org>,
        "Mediatek WSD Upstream" <wsd_upstream@...iatek.com>,
        Macpaul Lin <macpaul.lin@...il.com>,
        <linux-kernel@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-mediatek@...ts.infradead.org>,
        Matthias Brugger <matthias.bgg@...il.com>,
        Takashi Iwai <tiwai@...e.com>
Subject: Re: [PATCH] sound: usb: pcm: fix incorrect power state when playing
 sound after PM_AUTO suspend

On Tue, 2020-06-02 at 19:53 +0800, Macpaul Lin wrote:
> This patch fix incorrect power state changed by usb_audio_suspend()
> when CONFIG_PM is enabled.
> 
> After receiving suspend PM message with auto flag, usb_audio_suspend()
> change card's power state to SNDRV_CTL_POWER_D3hot. Only when the other
> resume PM message with auto flag can change power state to
> SNDRV_CTL_POWER_D0 in __usb_audio_resume().
> 
> However, when system is not under auto suspend, resume PM message with
> auto flag might not be able to receive on time which cause the power
> state was incorrect. At this time, if a player starts to play sound,
> will cause snd_usb_pcm_open() to access the card and setup_hw_info() will
> resume the card.
> 
> But even the card is back to work and all function normal, the power
> state is still in SNDRV_CTL_POWER_D3hot. Which cause the infinite loop
> happened in snd_power_wait() to check the power state. Thus the
> successive setting ioctl cannot be passed to card.
> 
> Hence we suggest to change power state to SNDRV_CTL_POWER_D0 when card
> has been resumed successfully.
> 
> Signed-off-by: Macpaul Lin <macpaul.lin@...iatek.com>
> ---
>  sound/usb/pcm.c |   11 +++++++++++linux-usb@...r.kernel.org,
>  1 file changed, 11 insertions(+)
> 
> diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
> index a4e4064..d667ecb 100644
> --- a/sound/usb/pcm.c
> +++ b/sound/usb/pcm.c
> @@ -1322,6 +1322,17 @@ static int setup_hw_info(struct snd_pcm_runtime *runtime, struct snd_usb_substre
>  	if (err < 0)
>  		return err;
>  
> +	/* fix incorrect power state when resuming by open and later ioctls */
> +	if (IS_ENABLED(CONFIG_PM) &&
> +		snd_power_get_state(subs->stream->chip->card)
> +			== SNDRV_CTL_POWER_D3hot) {
> +		/* set these variables for power state correction */
> +		subs->stream->chip->autosuspended = 0;
> +		subs->stream->chip->num_suspended_intf = 1;
> +		dev_info(&subs->dev->dev,
> +			"change power state from D3hot to D0\n");
> +	}
> +
>  	return snd_usb_autoresume(subs->stream->chip);
>  }
>  

The issue was found on kernel 4.14 (android tree). The test is to add
debug log in sound/core/init.c to check if the power state is
SNDRV_CTL_POWER_D3hot.

diff --git a/sound/core/init.c b/sound/core/init.c
index b02a997..a0bee76 100644
--- a/sound/core/init.c
+++ b/sound/core/init.c
@@ -1011,6 +1011,8 @@ int snd_power_wait(struct snd_card *card, unsigned
int power_state)
 		if (snd_power_get_state(card) == power_state)
 			break;
 		set_current_state(TASK_UNINTERRUPTIBLE);
+		pr_info("%s snd_power_get_state[%x]\n", __func__,
+			snd_power_get_state(card));
 		schedule_timeout(30 * HZ);
 	}
 	remove_wait_queue(&card->power_sleep, &wait);

After applied a work around by forcing the power state, pcm related
ioctl and parameter settings can be set to usb sound card correctly.
Otherwise a infinite loop will happened in snd_power_wait().

Here is the origin work around for verifying this power state issue on
kernel 4.14.

diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
index 933adcd7af81..9acd50dd7155 100644
--- a/sound/usb/pcm.c
+++ b/sound/usb/pcm.c
@@ -1274,6 +1274,16 @@ static int setup_hw_info(struct snd_pcm_runtime
*runtime, struct snd_usb_substre
 	if (err < 0)
 		return err;
 
+	/* avoid incorrect power state when executing IOCTL */
+	if (IS_ENABLED(CONFIG_PM) &&
+		snd_power_get_state(subs->stream->chip->card)
+			== SNDRV_CTL_POWER_D3hot) {
+		dev_info(&subs->dev->dev,
+			"change power state from D3hot to D0\n");
+		snd_power_change_state(subs->stream->chip->card,
+					SNDRV_CTL_POWER_D0);
+	}
+
 	param_period_time_if_needed = SNDRV_PCM_HW_PARAM_PERIOD_TIME;
 	if (subs->speed == USB_SPEED_FULL)
 		/* full speed devices have fixed data packet interval */

However, the patch I've send is meant to make sure the power state will
be corrected before snd_usb_autoresume(), It should be adapt to kernel
4.14 and later.

Thanks.
Macpaul Lin