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@xxxxxxxxxxxx>
> Cc: <stable@xxxxxxxxxxxxxxx>
> Signed-off-by: Takashi Iwai <tiwai@xxxxxxx>
> ---
> 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@xxxxxxxxxxxx>
