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

[Macpaul Lin]

On Wed, 2020-06-03 at 10:45 +0200, Takashi Iwai wrote:
> On Wed, 03 Jun 2020 08:54:51 +0200,
> Takashi Iwai wrote:
> > 
> > On Wed, 03 Jun 2020 08:28:09 +0200,
> > Takashi Iwai wrote:
> > > 
> > > And, the most suspicious case is the last one,
> > > chip->num_suspended-intf.  It means that the device has multiple
> > > USB interfaces and they went to suspend, while the resume isn't
> > > performed for the all suspended interfaces in return.
> > 
> > If this is the cause, a patch like below might help.
> > It gets/puts the all assigned interfaced instead of only the primary
> > one.
> 
> ... and considering of the problem again, rather the patch below might
> be the right answer.  Now the driver tries to remember at which state
> it entered into the system-suspend.  Upon resume, in return, when the
> state reaches back to that point, set the card state to D0.
> 
> The previous patch can be applied on the top, too, and it might be
> worth to apply both.
> 
> Let me know if any of those actually helps.
> 
> 
> Takashi

Thanks for your response so quickly.
I've just test this patch since it looks like enough for the issue.

This patch worked since the flag system_suspend will be set at the same
time when power state has been changed. I have 2 interface with the head
set. But actually the problem happened when primary one is suspended.
So I didn't test the earlier patch "suspend all interface instead of
only the primary one."

Will you resend this patch officially later? I think this solution is
required to send to stable, too. It's better to have it for other stable
kernel versions include android's.

> ---
> diff --git a/sound/usb/card.c b/sound/usb/card.c
> --- a/sound/usb/card.c
> +++ b/sound/usb/card.c
> @@ -843,9 +843,6 @@ static int usb_audio_suspend(struct usb_interface *intf, pm_message_t message)
>  	if (chip == (void *)-1L)
>  		return 0;
>  
> -	chip->autosuspended = !!PMSG_IS_AUTO(message);
> -	if (!chip->autosuspended)
> -		snd_power_change_state(chip->card, SNDRV_CTL_POWER_D3hot);
>  	if (!chip->num_suspended_intf++) {
>  		list_for_each_entry(as, &chip->pcm_list, list) {
>  			snd_usb_pcm_suspend(as);
> @@ -858,6 +855,11 @@ static int usb_audio_suspend(struct usb_interface *intf, pm_message_t message)
>  			snd_usb_mixer_suspend(mixer);
>  	}
>  
> +	if (!PMSG_IS_AUTO(message) && !chip->system_suspend) {
> +		snd_power_change_state(chip->card, SNDRV_CTL_POWER_D3hot);
> +		chip->system_suspend = chip->num_suspended_intf;
> +	}
> +
>  	return 0;
>  }
>  
> @@ -871,10 +873,10 @@ static int __usb_audio_resume(struct usb_interface *intf, bool reset_resume)
>  
>  	if (chip == (void *)-1L)
>  		return 0;
> -	if (--chip->num_suspended_intf)
> -		return 0;
>  
>  	atomic_inc(&chip->active); /* avoid autopm */
> +	if (chip->num_suspended_intf > 1)
> +		goto out;
>  
>  	list_for_each_entry(as, &chip->pcm_list, list) {
>  		err = snd_usb_pcm_resume(as);
> @@ -896,9 +898,12 @@ static int __usb_audio_resume(struct usb_interface *intf, bool reset_resume)
>  		snd_usbmidi_resume(p);
>  	}
>  
> -	if (!chip->autosuspended)
> + out:
> +	if (chip->num_suspended_intf == chip->system_suspend) {
>  		snd_power_change_state(chip->card, SNDRV_CTL_POWER_D0);
> -	chip->autosuspended = 0;
> +		chip->system_suspend = 0;
> +	}
> +	chip->num_suspended_intf--;
>  
>  err_out:
>  	atomic_dec(&chip->active); /* allow autopm after this point */
> diff --git a/sound/usb/usbaudio.h b/sound/usb/usbaudio.h
> index 1c892c7f14d7..e0ebfb25fbd5 100644
> --- a/sound/usb/usbaudio.h
> +++ b/sound/usb/usbaudio.h
> @@ -26,7 +26,7 @@ struct snd_usb_audio {
>  	struct usb_interface *pm_intf;
>  	u32 usb_id;
>  	struct mutex mutex;
> -	unsigned int autosuspended:1;	
> +	unsigned int system_suspend;
>  	atomic_t active;
>  	atomic_t shutdown;
>  	atomic_t usage_count;
> 
> _______________________________________________

Thank you very much!

Best regards,
Macpaul Lin