Re: [PATCH] sound: usb: pcm: fix incorrect power state when playing sound after PM_AUTO suspend

From: Macpaul Lin
Date: Tue Jun 02 2020 - 08:19:20 EST


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@xxxxxxxxxxxx>
> ---
> sound/usb/pcm.c | 11 +++++++++++linux-usb@xxxxxxxxxxxxxxx,
> 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