Re: [PATCH 1/2] ALSA: usb-audio: Fix inconsistent card PM state after resume

[Macpaul Lin <macpaul.lin@mediatek.com>]

On Wed, 2020-06-03 at 17:37 +0200, Takashi Iwai wrote:
> When a USB-audio interface gets runtime-suspended via auto-pm feature,
> the driver suspends all functionality and increment
> chip->num_suspended_intf.  Later on, when the system gets suspended to
> S3, the driver increments chip->num_suspended_intf again, skips the
> device changes, and sets the card power state to
> SNDRV_CTL_POWER_D3hot.  In return, when the system gets resumed from
> S3, the resume callback decrements chip->num_suspended_intf.  Since
> this refcount is still not zero (it's been runtime-suspended), the
> whole resume is skipped.  But there is a small pitfall here.
> 
> The problem is that the driver doesn't restore the card power state
> after this resume call, leaving it as SNDRV_CTL_POWER_D3hot.  So,
> even after the system resume finishes, the card instance still appears
> as if it were system-suspended, and this confuses many ioctl accesses
> that are blocked unexpectedly.
> 
> In details, we have two issues behind the scene: one is that the card
> power state is changed only when the refcount becomes zero, and
> another is that the prior auto-suspend check is kept in a boolean
> flag.  Although the latter problem is almost negligible since the
> auto-pm feature is imposed only on the primary interface, but this can
> be a potential problem on the devices with multiple interfaces.
> 
> This patch addresses those issues by the following:
> 
> - Replace chip->autosuspended boolean flag with chip->system_suspend
>   counter
> 
> - At the first system-suspend, chip->num_suspended_intf is recorded to
>   chip->system_suspend
> 
> - At system-resume, the card power state is restored when the
>   chip->num_suspended_intf refcount reaches to chip->system_suspend,
>   i.e. the state returns to the auto-suspended
> 
> Also, the patch fixes yet another hidden problem by the code
> refactoring along with the fixes above: namely, when some resume
> procedure failed, the driver left chip->num_suspended_intf that was
> already decreased, and it might lead to the refcount unbalance.
> In the new code, the refcount decrement is done after the whole resume
> procedure, and the problem is avoided as well.
> 
> Fixes: 0662292aec05 ("ALSA: usb-audio: Handle normal and auto-suspend equally")
> Reported-and-tested-by: Macpaul Lin <macpaul.lin@mediatek.com>
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>  sound/usb/card.c     | 19 ++++++++++++-------
>  sound/usb/usbaudio.h |  2 +-
>  2 files changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/sound/usb/card.c b/sound/usb/card.c
> index fd6fd1726ea0..359f7a04be1c 100644
> --- 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;

Tested-by: Macpaul Lin <macpaul.lin@mediatek.com>