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

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

[Takashi Iwai]

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;
-- 
2.25.0

[PATCH 2/2] ALSA: usb-audio: Manage auto-pm of all bundled interfaces

[Takashi Iwai]

Currently USB-audio driver manages the auto-pm of the primary
interface although a card may consist of multiple interfaces.
This may leave the secondary and other interfaces left running
unnecessarily after the auto-suspend.

This patch allows the driver managing the auto-pm of all bundled
interfaces per card.  The chip->pm_intf field is extended as
chip->intf[] to contain the array of assigned interfaces, and the
runtime-PM is performed to all those interfaces.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/usb/card.c     | 35 ++++++++++++++++++++++++++++++-----
 sound/usb/usbaudio.h |  4 +++-
 2 files changed, 33 insertions(+), 6 deletions(-)

diff --git a/sound/usb/card.c b/sound/usb/card.c
index 359f7a04be1c..f648587d2342 100644
--- a/sound/usb/card.c
+++ b/sound/usb/card.c
@@ -634,7 +634,6 @@ static int usb_audio_probe(struct usb_interface *intf,
 								   id, &chip);
 					if (err < 0)
 						goto __error;
-					chip->pm_intf = intf;
 					break;
 				} else if (vid[i] != -1 || pid[i] != -1) {
 					dev_info(&dev->dev,
@@ -651,6 +650,13 @@ static int usb_audio_probe(struct usb_interface *intf,
 			goto __error;
 		}
 	}
+
+	if (chip->num_interfaces >= MAX_CARD_INTERFACES) {
+		dev_info(&dev->dev, "Too many interfaces assigned to the single USB-audio card\n");
+		err = -EINVAL;
+		goto __error;
+	}
+
 	dev_set_drvdata(&dev->dev, chip);
 
 	/*
@@ -703,6 +709,7 @@ static int usb_audio_probe(struct usb_interface *intf,
 	}
 
 	usb_chip[chip->index] = chip;
+	chip->intf[chip->num_interfaces] = intf;
 	chip->num_interfaces++;
 	usb_set_intfdata(intf, chip);
 	atomic_dec(&chip->active);
@@ -818,19 +825,37 @@ void snd_usb_unlock_shutdown(struct snd_usb_audio *chip)
 
 int snd_usb_autoresume(struct snd_usb_audio *chip)
 {
+	int i, err;
+
 	if (atomic_read(&chip->shutdown))
 		return -EIO;
-	if (atomic_inc_return(&chip->active) == 1)
-		return usb_autopm_get_interface(chip->pm_intf);
+	if (atomic_inc_return(&chip->active) != 1)
+		return 0;
+
+	for (i = 0; i < chip->num_interfaces; i++) {
+		err = usb_autopm_get_interface(chip->intf[i]);
+		if (err < 0) {
+			/* rollback */
+			while (--i >= 0)
+				usb_autopm_put_interface(chip->intf[i]);
+			atomic_dec(&chip->active))
+			return err;
+		}
+	}
 	return 0;
 }
 
 void snd_usb_autosuspend(struct snd_usb_audio *chip)
 {
+	int i;
+
 	if (atomic_read(&chip->shutdown))
 		return;
-	if (atomic_dec_and_test(&chip->active))
-		usb_autopm_put_interface(chip->pm_intf);
+	if (!atomic_dec_and_test(&chip->active))
+		return;
+
+	for (i = 0; i < chip->num_interfaces; i++)
+		usb_autopm_put_interface(chip->intf[i]);
 }
 
 static int usb_audio_suspend(struct usb_interface *intf, pm_message_t message)
diff --git a/sound/usb/usbaudio.h b/sound/usb/usbaudio.h
index e0ebfb25fbd5..b91c4c0807ec 100644
--- a/sound/usb/usbaudio.h
+++ b/sound/usb/usbaudio.h
@@ -19,11 +19,13 @@
 struct media_device;
 struct media_intf_devnode;
 
+#define MAX_CARD_INTERFACES	16
+
 struct snd_usb_audio {
 	int index;
 	struct usb_device *dev;
 	struct snd_card *card;
-	struct usb_interface *pm_intf;
+	struct usb_interface *intf[MAX_CARD_INTERFACES];
 	u32 usb_id;
 	struct mutex mutex;
 	unsigned int system_suspend;
-- 
2.25.0

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

[Macpaul Lin]

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>

Re: [PATCH 2/2] ALSA: usb-audio: Manage auto-pm of all bundled interfaces

[Macpaul Lin]

On Wed, 2020-06-03 at 17:37 +0200, Takashi Iwai wrote:
> Currently USB-audio driver manages the auto-pm of the primary
> interface although a card may consist of multiple interfaces.
> This may leave the secondary and other interfaces left running
> unnecessarily after the auto-suspend.
> 
> This patch allows the driver managing the auto-pm of all bundled
> interfaces per card.  The chip->pm_intf field is extended as
> chip->intf[] to contain the array of assigned interfaces, and the
> runtime-PM is performed to all those interfaces.
> 
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>  sound/usb/card.c     | 35 ++++++++++++++++++++++++++++++-----
>  sound/usb/usbaudio.h |  4 +++-
>  2 files changed, 33 insertions(+), 6 deletions(-)
> 
> diff --git a/sound/usb/card.c b/sound/usb/card.c
> index 359f7a04be1c..f648587d2342 100644
> --- a/sound/usb/card.c
> +++ b/sound/usb/card.c
> @@ -634,7 +634,6 @@ static int usb_audio_probe(struct usb_interface *intf,
>  								   id, &chip);
>  					if (err < 0)
>  						goto __error;
> -					chip->pm_intf = intf;
>  					break;
>  				} else if (vid[i] != -1 || pid[i] != -1) {
>  					dev_info(&dev->dev,
> @@ -651,6 +650,13 @@ static int usb_audio_probe(struct usb_interface *intf,
>  			goto __error;
>  		}
>  	}
> +
> +	if (chip->num_interfaces >= MAX_CARD_INTERFACES) {
> +		dev_info(&dev->dev, "Too many interfaces assigned to the single USB-audio card\n");
> +		err = -EINVAL;
> +		goto __error;
> +	}
> +
>  	dev_set_drvdata(&dev->dev, chip);
>  
>  	/*
> @@ -703,6 +709,7 @@ static int usb_audio_probe(struct usb_interface *intf,
>  	}
>  
>  	usb_chip[chip->index] = chip;
> +	chip->intf[chip->num_interfaces] = intf;
>  	chip->num_interfaces++;
>  	usb_set_intfdata(intf, chip);
>  	atomic_dec(&chip->active);
> @@ -818,19 +825,37 @@ void snd_usb_unlock_shutdown(struct snd_usb_audio *chip)
>  
>  int snd_usb_autoresume(struct snd_usb_audio *chip)
>  {
> +	int i, err;
> +
>  	if (atomic_read(&chip->shutdown))
>  		return -EIO;
> -	if (atomic_inc_return(&chip->active) == 1)
> -		return usb_autopm_get_interface(chip->pm_intf);
> +	if (atomic_inc_return(&chip->active) != 1)
> +		return 0;
> +
> +	for (i = 0; i < chip->num_interfaces; i++) {
> +		err = usb_autopm_get_interface(chip->intf[i]);
> +		if (err < 0) {
> +			/* rollback */
> +			while (--i >= 0)
> +				usb_autopm_put_interface(chip->intf[i]);
> +			atomic_dec(&chip->active))
> +			return err;
> +		}
> +	}
>  	return 0;
>  }
>  
>  void snd_usb_autosuspend(struct snd_usb_audio *chip)
>  {
> +	int i;
> +
>  	if (atomic_read(&chip->shutdown))
>  		return;
> -	if (atomic_dec_and_test(&chip->active))
> -		usb_autopm_put_interface(chip->pm_intf);
> +	if (!atomic_dec_and_test(&chip->active))
> +		return;
> +
> +	for (i = 0; i < chip->num_interfaces; i++)
> +		usb_autopm_put_interface(chip->intf[i]);
>  }
>  
>  static int usb_audio_suspend(struct usb_interface *intf, pm_message_t message)
> diff --git a/sound/usb/usbaudio.h b/sound/usb/usbaudio.h
> index e0ebfb25fbd5..b91c4c0807ec 100644
> --- a/sound/usb/usbaudio.h
> +++ b/sound/usb/usbaudio.h
> @@ -19,11 +19,13 @@
>  struct media_device;
>  struct media_intf_devnode;
>  
> +#define MAX_CARD_INTERFACES	16
> +
>  struct snd_usb_audio {
>  	int index;
>  	struct usb_device *dev;
>  	struct snd_card *card;
> -	struct usb_interface *pm_intf;
> +	struct usb_interface *intf[MAX_CARD_INTERFACES];
>  	u32 usb_id;
>  	struct mutex mutex;
>  	unsigned int system_suspend;

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

--
Thanks!
Macpaul Lin