* Re: [PATCH] sound: usb: pcm: fix incorrect power state when playing sound after PM_AUTO suspend
@ 2020-06-03 6:28 ` Takashi Iwai
0 siblings, 0 replies; 39+ messages in thread
From: Takashi Iwai @ 2020-06-03 6:28 UTC (permalink / raw)
To: Macpaul Lin
Cc: alsa-devel, Mediatek WSD Upstream, linux-kernel, linux-usb,
Johan Hovold, Takashi Iwai, Hui Wang, Alexander Tsoy,
linux-mediatek, Greg Kroah-Hartman, Matthias Brugger,
Macpaul Lin, Jaroslav Kysela, Szabolcs Szőke,
linux-arm-kernel
On Wed, 03 Jun 2020 05:05:15 +0200,
Macpaul Lin wrote:
>
> On Tue, 2020-06-02 at 14:46 +0200, Takashi Iwai wrote:
> > On Tue, 02 Jun 2020 13:53:41 +0200,
> > 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.
> >
> > Hm, in exactly which situation does this happen? I still don't get
> > it. Could you elaborate how to trigger this?
>
> I'm not sure if this will happen on laptop or on PC.
> We've found this issue on Android phone (I'm not sure if each Android
> phone can reproduce this.).
>
> After booting the android phone, insert type-c headset without charging
> and play music at any duration, say, 1 second, then stop. Put phone away
> to idle about 17~18 minutes. Wait auto pm happened and the power state
> change to SNDRV_CTL_POWER_D3hot in sound/usb/card.c. Then wake up the
> phone, play music again. Then you'll probably found the music was not
> playing and the progress bar keep at the same position. It only happen
> when power state is SNDRV_CTL_POWER_D3hot. If not (the power state is
> SNDRV_CTL_POWER_D0), repeat the steps for several times, then it will be
> produced at some time.
>
> When it happened, sound_usb_pcm_open() will wake up the sound card by
> setup_hw_info()->__usb_audio_resume(). However, the card and the
> interface is function properly right now, the power state keeps remain
> SNDRV_CTL_POWER_D3hot.
And at this point it's already something wrong. We need to check why
SNDRV_CTL_POWER_D3hot is kept there, instead of working around the
rest behavior.
> The suggestive parameter settings from upper
> sound request will be pending since later snd_power_wait() call will
> still wait the card awaken. Ideally, auto PM should be recovered by
> sound card itself. But once the card is awaken at this circumstance, it
> looks like there are not more auto pm event. And the sound system of
> this interface will stuck here forever until user plug out the headset
> (reset the hardware).
>
> The root cause is that once the card has been resumed, it should inform
> auto pm change the state back into SNDRV_CTL_POWER_D0 and mark the
> device is using by some one.
>
> > > 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.
> >
> > This doesn't look like a right solution for the problem, sorry.
> > The card PM status must be recovered to D0 when the autoresume
> > succeeds. If not, something is broken there, and it must be fixed
> > instead of fiddling the status flag externally.
>
> Yes, I agreed, but after checking the code in sound drivers,
> it looks like there is only chance that auto pm triggered by low-level
> code in sound/usb/card.c. In kernel 4.14, auto pm suspend is triggered
> by snd_pcm_suspend_all(). In later kernel, it is triggered by
> snd_usb_pcm_suspend(). However, it looks like there are no any resume
> trigger to recover auto pm state when the card has been waken by
> sound_usb_pcm_open().
If a running PCM stream has been suspended, the stream needs to be
resumed manually by user-space. There is no automatic resume. You
can forget about it and skip scratching that surface.
Again, the point to be checked is why D3hot is kept after
snd_usb_autoresume() is called.
It's Android, and I wonder whether the system does the system-suspend
(S3), or it's all runtime PM? Basically D3hot is set only for the
former, the system suspend, where the driver's PM callback is called
with PMSG_SUSPEND. Please check this at first. That is,
usb_audio_suspend() receives PMSG_SUSPEND or such, which makes
chip->autosuspended=1. The D3hot flag is set only in this condition.
Then, check the resume patterns. The usb-audio suspend/resume has
multiple refcounts. One is the Linux device PM refcount, and
chip->active refcount, and chip->num_suspended_intf refcount.
The first one (PM refount) is the primary refcount to manage the whole
system, and this is incremented / decremented by the standard PM
calls. The second one, chip->active, is a temporary flag to avoid the
re-entrance of the PM callbacks, and incremented at the probe enter
and __usb_audio_resume(), and decremented at the probe exit and
__usb_audio_resume() exist. The last one, chip->num_suspended_intf is
a refcount for the multiple interfaces assigned to a single card.
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 that's the case, you need to check where the suspend gets called to
which USB-interface (and which pm_message_t) and whether the resume
gets called for those.
thanks,
Takashi
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH] sound: usb: pcm: fix incorrect power state when playing sound after PM_AUTO suspend
@ 2020-06-03 6:28 ` Takashi Iwai
0 siblings, 0 replies; 39+ messages in thread
From: Takashi Iwai @ 2020-06-03 6:28 UTC (permalink / raw)
To: Macpaul Lin
Cc: alsa-devel, Mediatek WSD Upstream, linux-kernel, linux-usb,
Johan Hovold, Takashi Iwai, Hui Wang, Alexander Tsoy,
linux-mediatek, Greg Kroah-Hartman, Matthias Brugger,
Macpaul Lin, Jaroslav Kysela, Szabolcs Szőke,
linux-arm-kernel
On Wed, 03 Jun 2020 05:05:15 +0200,
Macpaul Lin wrote:
>
> On Tue, 2020-06-02 at 14:46 +0200, Takashi Iwai wrote:
> > On Tue, 02 Jun 2020 13:53:41 +0200,
> > 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.
> >
> > Hm, in exactly which situation does this happen? I still don't get
> > it. Could you elaborate how to trigger this?
>
> I'm not sure if this will happen on laptop or on PC.
> We've found this issue on Android phone (I'm not sure if each Android
> phone can reproduce this.).
>
> After booting the android phone, insert type-c headset without charging
> and play music at any duration, say, 1 second, then stop. Put phone away
> to idle about 17~18 minutes. Wait auto pm happened and the power state
> change to SNDRV_CTL_POWER_D3hot in sound/usb/card.c. Then wake up the
> phone, play music again. Then you'll probably found the music was not
> playing and the progress bar keep at the same position. It only happen
> when power state is SNDRV_CTL_POWER_D3hot. If not (the power state is
> SNDRV_CTL_POWER_D0), repeat the steps for several times, then it will be
> produced at some time.
>
> When it happened, sound_usb_pcm_open() will wake up the sound card by
> setup_hw_info()->__usb_audio_resume(). However, the card and the
> interface is function properly right now, the power state keeps remain
> SNDRV_CTL_POWER_D3hot.
And at this point it's already something wrong. We need to check why
SNDRV_CTL_POWER_D3hot is kept there, instead of working around the
rest behavior.
> The suggestive parameter settings from upper
> sound request will be pending since later snd_power_wait() call will
> still wait the card awaken. Ideally, auto PM should be recovered by
> sound card itself. But once the card is awaken at this circumstance, it
> looks like there are not more auto pm event. And the sound system of
> this interface will stuck here forever until user plug out the headset
> (reset the hardware).
>
> The root cause is that once the card has been resumed, it should inform
> auto pm change the state back into SNDRV_CTL_POWER_D0 and mark the
> device is using by some one.
>
> > > 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.
> >
> > This doesn't look like a right solution for the problem, sorry.
> > The card PM status must be recovered to D0 when the autoresume
> > succeeds. If not, something is broken there, and it must be fixed
> > instead of fiddling the status flag externally.
>
> Yes, I agreed, but after checking the code in sound drivers,
> it looks like there is only chance that auto pm triggered by low-level
> code in sound/usb/card.c. In kernel 4.14, auto pm suspend is triggered
> by snd_pcm_suspend_all(). In later kernel, it is triggered by
> snd_usb_pcm_suspend(). However, it looks like there are no any resume
> trigger to recover auto pm state when the card has been waken by
> sound_usb_pcm_open().
If a running PCM stream has been suspended, the stream needs to be
resumed manually by user-space. There is no automatic resume. You
can forget about it and skip scratching that surface.
Again, the point to be checked is why D3hot is kept after
snd_usb_autoresume() is called.
It's Android, and I wonder whether the system does the system-suspend
(S3), or it's all runtime PM? Basically D3hot is set only for the
former, the system suspend, where the driver's PM callback is called
with PMSG_SUSPEND. Please check this at first. That is,
usb_audio_suspend() receives PMSG_SUSPEND or such, which makes
chip->autosuspended=1. The D3hot flag is set only in this condition.
Then, check the resume patterns. The usb-audio suspend/resume has
multiple refcounts. One is the Linux device PM refcount, and
chip->active refcount, and chip->num_suspended_intf refcount.
The first one (PM refount) is the primary refcount to manage the whole
system, and this is incremented / decremented by the standard PM
calls. The second one, chip->active, is a temporary flag to avoid the
re-entrance of the PM callbacks, and incremented at the probe enter
and __usb_audio_resume(), and decremented at the probe exit and
__usb_audio_resume() exist. The last one, chip->num_suspended_intf is
a refcount for the multiple interfaces assigned to a single card.
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 that's the case, you need to check where the suspend gets called to
which USB-interface (and which pm_message_t) and whether the resume
gets called for those.
thanks,
Takashi
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH] sound: usb: pcm: fix incorrect power state when playing sound after PM_AUTO suspend
@ 2020-06-03 6:28 ` Takashi Iwai
0 siblings, 0 replies; 39+ messages in thread
From: Takashi Iwai @ 2020-06-03 6:28 UTC (permalink / raw)
To: Macpaul Lin
Cc: alsa-devel, Mediatek WSD Upstream, linux-kernel, linux-usb,
Johan Hovold, Takashi Iwai, Hui Wang, Alexander Tsoy,
linux-mediatek, Greg Kroah-Hartman, Matthias Brugger,
Macpaul Lin, Szabolcs Szőke, linux-arm-kernel
On Wed, 03 Jun 2020 05:05:15 +0200,
Macpaul Lin wrote:
>
> On Tue, 2020-06-02 at 14:46 +0200, Takashi Iwai wrote:
> > On Tue, 02 Jun 2020 13:53:41 +0200,
> > 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.
> >
> > Hm, in exactly which situation does this happen? I still don't get
> > it. Could you elaborate how to trigger this?
>
> I'm not sure if this will happen on laptop or on PC.
> We've found this issue on Android phone (I'm not sure if each Android
> phone can reproduce this.).
>
> After booting the android phone, insert type-c headset without charging
> and play music at any duration, say, 1 second, then stop. Put phone away
> to idle about 17~18 minutes. Wait auto pm happened and the power state
> change to SNDRV_CTL_POWER_D3hot in sound/usb/card.c. Then wake up the
> phone, play music again. Then you'll probably found the music was not
> playing and the progress bar keep at the same position. It only happen
> when power state is SNDRV_CTL_POWER_D3hot. If not (the power state is
> SNDRV_CTL_POWER_D0), repeat the steps for several times, then it will be
> produced at some time.
>
> When it happened, sound_usb_pcm_open() will wake up the sound card by
> setup_hw_info()->__usb_audio_resume(). However, the card and the
> interface is function properly right now, the power state keeps remain
> SNDRV_CTL_POWER_D3hot.
And at this point it's already something wrong. We need to check why
SNDRV_CTL_POWER_D3hot is kept there, instead of working around the
rest behavior.
> The suggestive parameter settings from upper
> sound request will be pending since later snd_power_wait() call will
> still wait the card awaken. Ideally, auto PM should be recovered by
> sound card itself. But once the card is awaken at this circumstance, it
> looks like there are not more auto pm event. And the sound system of
> this interface will stuck here forever until user plug out the headset
> (reset the hardware).
>
> The root cause is that once the card has been resumed, it should inform
> auto pm change the state back into SNDRV_CTL_POWER_D0 and mark the
> device is using by some one.
>
> > > 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.
> >
> > This doesn't look like a right solution for the problem, sorry.
> > The card PM status must be recovered to D0 when the autoresume
> > succeeds. If not, something is broken there, and it must be fixed
> > instead of fiddling the status flag externally.
>
> Yes, I agreed, but after checking the code in sound drivers,
> it looks like there is only chance that auto pm triggered by low-level
> code in sound/usb/card.c. In kernel 4.14, auto pm suspend is triggered
> by snd_pcm_suspend_all(). In later kernel, it is triggered by
> snd_usb_pcm_suspend(). However, it looks like there are no any resume
> trigger to recover auto pm state when the card has been waken by
> sound_usb_pcm_open().
If a running PCM stream has been suspended, the stream needs to be
resumed manually by user-space. There is no automatic resume. You
can forget about it and skip scratching that surface.
Again, the point to be checked is why D3hot is kept after
snd_usb_autoresume() is called.
It's Android, and I wonder whether the system does the system-suspend
(S3), or it's all runtime PM? Basically D3hot is set only for the
former, the system suspend, where the driver's PM callback is called
with PMSG_SUSPEND. Please check this at first. That is,
usb_audio_suspend() receives PMSG_SUSPEND or such, which makes
chip->autosuspended=1. The D3hot flag is set only in this condition.
Then, check the resume patterns. The usb-audio suspend/resume has
multiple refcounts. One is the Linux device PM refcount, and
chip->active refcount, and chip->num_suspended_intf refcount.
The first one (PM refount) is the primary refcount to manage the whole
system, and this is incremented / decremented by the standard PM
calls. The second one, chip->active, is a temporary flag to avoid the
re-entrance of the PM callbacks, and incremented at the probe enter
and __usb_audio_resume(), and decremented at the probe exit and
__usb_audio_resume() exist. The last one, chip->num_suspended_intf is
a refcount for the multiple interfaces assigned to a single card.
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 that's the case, you need to check where the suspend gets called to
which USB-interface (and which pm_message_t) and whether the resume
gets called for those.
thanks,
Takashi
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH] sound: usb: pcm: fix incorrect power state when playing sound after PM_AUTO suspend
2020-06-03 6:28 ` Takashi Iwai
(?)
(?)
@ 2020-06-03 6:54 ` Takashi Iwai
-1 siblings, 0 replies; 39+ messages in thread
From: Takashi Iwai @ 2020-06-03 6:54 UTC (permalink / raw)
To: Macpaul Lin
Cc: Jaroslav Kysela, Takashi Iwai, Matthias Brugger, Alexander Tsoy,
Johan Hovold, Hui Wang, Szabolcs Szőke, alsa-devel,
linux-usb, Mediatek WSD Upstream, Macpaul Lin, linux-kernel,
linux-arm-kernel, linux-mediatek, Greg Kroah-Hartman
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.
Takashi
---
diff --git a/sound/usb/card.c b/sound/usb/card.c
--- 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,36 @@ 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]);
+ 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
--- 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 autosuspended:1;
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH] sound: usb: pcm: fix incorrect power state when playing sound after PM_AUTO suspend
@ 2020-06-03 6:54 ` Takashi Iwai
0 siblings, 0 replies; 39+ messages in thread
From: Takashi Iwai @ 2020-06-03 6:54 UTC (permalink / raw)
To: Macpaul Lin
Cc: alsa-devel, Mediatek WSD Upstream, linux-kernel, linux-usb,
Johan Hovold, Takashi Iwai, Hui Wang, Alexander Tsoy,
linux-mediatek, Greg Kroah-Hartman, Matthias Brugger,
Macpaul Lin, Jaroslav Kysela, Szabolcs Szőke,
linux-arm-kernel
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.
Takashi
---
diff --git a/sound/usb/card.c b/sound/usb/card.c
--- 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,36 @@ 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]);
+ 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
--- 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 autosuspended:1;
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH] sound: usb: pcm: fix incorrect power state when playing sound after PM_AUTO suspend
@ 2020-06-03 6:54 ` Takashi Iwai
0 siblings, 0 replies; 39+ messages in thread
From: Takashi Iwai @ 2020-06-03 6:54 UTC (permalink / raw)
To: Macpaul Lin
Cc: alsa-devel, Mediatek WSD Upstream, linux-kernel, linux-usb,
Johan Hovold, Takashi Iwai, Hui Wang, Alexander Tsoy,
linux-mediatek, Greg Kroah-Hartman, Matthias Brugger,
Macpaul Lin, Jaroslav Kysela, Szabolcs Szőke,
linux-arm-kernel
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.
Takashi
---
diff --git a/sound/usb/card.c b/sound/usb/card.c
--- 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,36 @@ 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]);
+ 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
--- 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 autosuspended:1;
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH] sound: usb: pcm: fix incorrect power state when playing sound after PM_AUTO suspend
@ 2020-06-03 6:54 ` Takashi Iwai
0 siblings, 0 replies; 39+ messages in thread
From: Takashi Iwai @ 2020-06-03 6:54 UTC (permalink / raw)
To: Macpaul Lin
Cc: alsa-devel, Mediatek WSD Upstream, linux-kernel, linux-usb,
Johan Hovold, Takashi Iwai, Hui Wang, Alexander Tsoy,
linux-mediatek, Greg Kroah-Hartman, Matthias Brugger,
Macpaul Lin, Szabolcs Szőke, linux-arm-kernel
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.
Takashi
---
diff --git a/sound/usb/card.c b/sound/usb/card.c
--- 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,36 @@ 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]);
+ 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
--- 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 autosuspended:1;
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH] sound: usb: pcm: fix incorrect power state when playing sound after PM_AUTO suspend
2020-06-03 6:54 ` Takashi Iwai
(?)
(?)
@ 2020-06-03 8:45 ` Takashi Iwai
-1 siblings, 0 replies; 39+ messages in thread
From: Takashi Iwai @ 2020-06-03 8:45 UTC (permalink / raw)
To: Macpaul Lin
Cc: Jaroslav Kysela, Takashi Iwai, Matthias Brugger, Alexander Tsoy,
Johan Hovold, Hui Wang, Szabolcs Szőke, alsa-devel,
linux-usb, Mediatek WSD Upstream, Macpaul Lin, linux-kernel,
linux-arm-kernel, linux-mediatek, Greg Kroah-Hartman
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
---
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;
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [PATCH] sound: usb: pcm: fix incorrect power state when playing sound after PM_AUTO suspend
@ 2020-06-03 8:45 ` Takashi Iwai
0 siblings, 0 replies; 39+ messages in thread
From: Takashi Iwai @ 2020-06-03 8:45 UTC (permalink / raw)
To: Macpaul Lin
Cc: alsa-devel, Mediatek WSD Upstream, linux-kernel, linux-usb,
Johan Hovold, Takashi Iwai, Hui Wang, Alexander Tsoy,
linux-mediatek, Greg Kroah-Hartman, Matthias Brugger,
Macpaul Lin, Jaroslav Kysela, Szabolcs Szőke,
linux-arm-kernel
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
---
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;
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [PATCH] sound: usb: pcm: fix incorrect power state when playing sound after PM_AUTO suspend
@ 2020-06-03 8:45 ` Takashi Iwai
0 siblings, 0 replies; 39+ messages in thread
From: Takashi Iwai @ 2020-06-03 8:45 UTC (permalink / raw)
To: Macpaul Lin
Cc: alsa-devel, Mediatek WSD Upstream, linux-kernel, linux-usb,
Johan Hovold, Takashi Iwai, Hui Wang, Alexander Tsoy,
linux-mediatek, Greg Kroah-Hartman, Matthias Brugger,
Macpaul Lin, Jaroslav Kysela, Szabolcs Szőke,
linux-arm-kernel
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
---
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;
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [PATCH] sound: usb: pcm: fix incorrect power state when playing sound after PM_AUTO suspend
@ 2020-06-03 8:45 ` Takashi Iwai
0 siblings, 0 replies; 39+ messages in thread
From: Takashi Iwai @ 2020-06-03 8:45 UTC (permalink / raw)
To: Macpaul Lin
Cc: alsa-devel, Mediatek WSD Upstream, linux-kernel, linux-usb,
Johan Hovold, Takashi Iwai, Hui Wang, Alexander Tsoy,
linux-mediatek, Greg Kroah-Hartman, Matthias Brugger,
Macpaul Lin, Szabolcs Szőke, linux-arm-kernel
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
---
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;
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [PATCH] sound: usb: pcm: fix incorrect power state when playing sound after PM_AUTO suspend
2020-06-03 8:45 ` Takashi Iwai
(?)
(?)
@ 2020-06-03 12:39 ` Macpaul Lin
-1 siblings, 0 replies; 39+ messages in thread
From: Macpaul Lin @ 2020-06-03 12:39 UTC (permalink / raw)
To: Takashi Iwai
Cc: alsa-devel, Mediatek WSD Upstream, linux-kernel, linux-usb,
Johan Hovold, Takashi Iwai, Hui Wang, Alexander Tsoy,
linux-mediatek, Greg Kroah-Hartman, Matthias Brugger,
Macpaul Lin, Jaroslav Kysela, Szabolcs Szőke,
linux-arm-kernel, stable
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
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH] sound: usb: pcm: fix incorrect power state when playing sound after PM_AUTO suspend
@ 2020-06-03 12:39 ` Macpaul Lin
0 siblings, 0 replies; 39+ messages in thread
From: Macpaul Lin @ 2020-06-03 12:39 UTC (permalink / raw)
To: Takashi Iwai
Cc: alsa-devel, Szabolcs Szőke, Mediatek WSD Upstream,
Greg Kroah-Hartman, linux-usb, Takashi Iwai, stable,
linux-kernel, Hui Wang, Alexander Tsoy, linux-mediatek,
Matthias Brugger, Johan Hovold, Jaroslav Kysela, Macpaul Lin,
linux-arm-kernel
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH] sound: usb: pcm: fix incorrect power state when playing sound after PM_AUTO suspend
@ 2020-06-03 12:39 ` Macpaul Lin
0 siblings, 0 replies; 39+ messages in thread
From: Macpaul Lin @ 2020-06-03 12:39 UTC (permalink / raw)
To: Takashi Iwai
Cc: alsa-devel, Szabolcs Szőke, Mediatek WSD Upstream,
Greg Kroah-Hartman, linux-usb, Takashi Iwai, stable,
linux-kernel, Hui Wang, Alexander Tsoy, linux-mediatek,
Matthias Brugger, Johan Hovold, Jaroslav Kysela, Macpaul Lin,
linux-arm-kernel
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
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH] sound: usb: pcm: fix incorrect power state when playing sound after PM_AUTO suspend
@ 2020-06-03 12:39 ` Macpaul Lin
0 siblings, 0 replies; 39+ messages in thread
From: Macpaul Lin @ 2020-06-03 12:39 UTC (permalink / raw)
To: Takashi Iwai
Cc: alsa-devel, Szabolcs Szőke, Mediatek WSD Upstream,
Greg Kroah-Hartman, linux-usb, Takashi Iwai, stable,
linux-kernel, Hui Wang, Alexander Tsoy, linux-mediatek,
Matthias Brugger, Johan Hovold, Macpaul Lin, linux-arm-kernel
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
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH] sound: usb: pcm: fix incorrect power state when playing sound after PM_AUTO suspend
2020-06-03 12:39 ` Macpaul Lin
(?)
(?)
@ 2020-06-03 12:47 ` Takashi Iwai
-1 siblings, 0 replies; 39+ messages in thread
From: Takashi Iwai @ 2020-06-03 12:47 UTC (permalink / raw)
To: Macpaul Lin
Cc: alsa-devel, Mediatek WSD Upstream, linux-kernel, linux-usb,
Johan Hovold, Takashi Iwai, Hui Wang, Alexander Tsoy,
linux-mediatek, Greg Kroah-Hartman, Matthias Brugger,
Macpaul Lin, Jaroslav Kysela, Szabolcs Szőke,
linux-arm-kernel, stable
On Wed, 03 Jun 2020 14:39:24 +0200,
Macpaul Lin wrote:
>
> 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.
Good to hear!
> 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.
Currently the autosuspend is set only to the primary interface; IOW,
the other interfaces will never get autosuspend, and the another
suspend-all-intf patch should improve that situation. But it won't
fix your actual bug, obviously :)
> So I didn't test the earlier patch "suspend all interface instead of
> only the primary one."
Could you try it one on top of the last patch? At least I'd like to
see whether it causes any regression.
> 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.
Yes, that's a general bug and worth to be merged quickly.
I'm going to submit a proper patch soon later.
thanks,
Takashi
>
> > ---
> > 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
>
>
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH] sound: usb: pcm: fix incorrect power state when playing sound after PM_AUTO suspend
@ 2020-06-03 12:47 ` Takashi Iwai
0 siblings, 0 replies; 39+ messages in thread
From: Takashi Iwai @ 2020-06-03 12:47 UTC (permalink / raw)
To: Macpaul Lin
Cc: alsa-devel, Szabolcs Szőke, Mediatek WSD Upstream,
Greg Kroah-Hartman, linux-usb, Takashi Iwai, stable,
linux-kernel, Hui Wang, Alexander Tsoy, linux-mediatek,
Matthias Brugger, Johan Hovold, Jaroslav Kysela, Macpaul Lin,
linux-arm-kernel
On Wed, 03 Jun 2020 14:39:24 +0200,
Macpaul Lin wrote:
>
> 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.
Good to hear!
> 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.
Currently the autosuspend is set only to the primary interface; IOW,
the other interfaces will never get autosuspend, and the another
suspend-all-intf patch should improve that situation. But it won't
fix your actual bug, obviously :)
> So I didn't test the earlier patch "suspend all interface instead of
> only the primary one."
Could you try it one on top of the last patch? At least I'd like to
see whether it causes any regression.
> 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.
Yes, that's a general bug and worth to be merged quickly.
I'm going to submit a proper patch soon later.
thanks,
Takashi
>
> > ---
> > 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
>
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH] sound: usb: pcm: fix incorrect power state when playing sound after PM_AUTO suspend
@ 2020-06-03 12:47 ` Takashi Iwai
0 siblings, 0 replies; 39+ messages in thread
From: Takashi Iwai @ 2020-06-03 12:47 UTC (permalink / raw)
To: Macpaul Lin
Cc: alsa-devel, Szabolcs Szőke, Mediatek WSD Upstream,
Greg Kroah-Hartman, linux-usb, Takashi Iwai, stable,
linux-kernel, Hui Wang, Alexander Tsoy, linux-mediatek,
Matthias Brugger, Johan Hovold, Jaroslav Kysela, Macpaul Lin,
linux-arm-kernel
On Wed, 03 Jun 2020 14:39:24 +0200,
Macpaul Lin wrote:
>
> 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.
Good to hear!
> 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.
Currently the autosuspend is set only to the primary interface; IOW,
the other interfaces will never get autosuspend, and the another
suspend-all-intf patch should improve that situation. But it won't
fix your actual bug, obviously :)
> So I didn't test the earlier patch "suspend all interface instead of
> only the primary one."
Could you try it one on top of the last patch? At least I'd like to
see whether it causes any regression.
> 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.
Yes, that's a general bug and worth to be merged quickly.
I'm going to submit a proper patch soon later.
thanks,
Takashi
>
> > ---
> > 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
>
>
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH] sound: usb: pcm: fix incorrect power state when playing sound after PM_AUTO suspend
@ 2020-06-03 12:47 ` Takashi Iwai
0 siblings, 0 replies; 39+ messages in thread
From: Takashi Iwai @ 2020-06-03 12:47 UTC (permalink / raw)
To: Macpaul Lin
Cc: alsa-devel, Szabolcs Szőke, Mediatek WSD Upstream,
Greg Kroah-Hartman, linux-usb, Takashi Iwai, stable,
linux-kernel, Hui Wang, Alexander Tsoy, linux-mediatek,
Matthias Brugger, Johan Hovold, Macpaul Lin, linux-arm-kernel
On Wed, 03 Jun 2020 14:39:24 +0200,
Macpaul Lin wrote:
>
> 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.
Good to hear!
> 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.
Currently the autosuspend is set only to the primary interface; IOW,
the other interfaces will never get autosuspend, and the another
suspend-all-intf patch should improve that situation. But it won't
fix your actual bug, obviously :)
> So I didn't test the earlier patch "suspend all interface instead of
> only the primary one."
Could you try it one on top of the last patch? At least I'd like to
see whether it causes any regression.
> 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.
Yes, that's a general bug and worth to be merged quickly.
I'm going to submit a proper patch soon later.
thanks,
Takashi
>
> > ---
> > 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
>
>
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH] sound: usb: pcm: fix incorrect power state when playing sound after PM_AUTO suspend
2020-06-03 12:47 ` Takashi Iwai
(?)
(?)
@ 2020-06-03 13:50 ` Macpaul Lin
-1 siblings, 0 replies; 39+ messages in thread
From: Macpaul Lin @ 2020-06-03 13:50 UTC (permalink / raw)
To: Takashi Iwai
Cc: alsa-devel, Mediatek WSD Upstream, linux-kernel, linux-usb,
Johan Hovold, Takashi Iwai, Hui Wang, Alexander Tsoy,
linux-mediatek, Greg Kroah-Hartman, Matthias Brugger,
Macpaul Lin, Jaroslav Kysela, Szabolcs Szőke,
linux-arm-kernel, stable
On Wed, 2020-06-03 at 14:47 +0200, Takashi Iwai wrote:
> On Wed, 03 Jun 2020 14:39:24 +0200,
> Macpaul Lin wrote:
> >
> > 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.
>
> Good to hear!
>
> > 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.
>
> Currently the autosuspend is set only to the primary interface; IOW,
> the other interfaces will never get autosuspend, and the another
> suspend-all-intf patch should improve that situation. But it won't
> fix your actual bug, obviously :)
>
> > So I didn't test the earlier patch "suspend all interface instead of
> > only the primary one."
>
> Could you try it one on top of the last patch? At least I'd like to
> see whether it causes any regression.
I've tried both of these 2 patches together, and it looks okay.
> > 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.
>
> Yes, that's a general bug and worth to be merged quickly.
> I'm going to submit a proper patch soon later.
>
>
> thanks,
>
> Takashi
>
Thanks!
Macpaul Lin
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH] sound: usb: pcm: fix incorrect power state when playing sound after PM_AUTO suspend
@ 2020-06-03 13:50 ` Macpaul Lin
0 siblings, 0 replies; 39+ messages in thread
From: Macpaul Lin @ 2020-06-03 13:50 UTC (permalink / raw)
To: Takashi Iwai
Cc: alsa-devel, Szabolcs Szőke, Mediatek WSD Upstream,
Greg Kroah-Hartman, linux-usb, Takashi Iwai, stable,
linux-kernel, Hui Wang, Alexander Tsoy, linux-mediatek,
Matthias Brugger, Johan Hovold, Jaroslav Kysela, Macpaul Lin,
linux-arm-kernel
On Wed, 2020-06-03 at 14:47 +0200, Takashi Iwai wrote:
> On Wed, 03 Jun 2020 14:39:24 +0200,
> Macpaul Lin wrote:
> >
> > 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.
>
> Good to hear!
>
> > 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.
>
> Currently the autosuspend is set only to the primary interface; IOW,
> the other interfaces will never get autosuspend, and the another
> suspend-all-intf patch should improve that situation. But it won't
> fix your actual bug, obviously :)
>
> > So I didn't test the earlier patch "suspend all interface instead of
> > only the primary one."
>
> Could you try it one on top of the last patch? At least I'd like to
> see whether it causes any regression.
I've tried both of these 2 patches together, and it looks okay.
> > 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.
>
> Yes, that's a general bug and worth to be merged quickly.
> I'm going to submit a proper patch soon later.
>
>
> thanks,
>
> Takashi
>
Thanks!
Macpaul Lin
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH] sound: usb: pcm: fix incorrect power state when playing sound after PM_AUTO suspend
@ 2020-06-03 13:50 ` Macpaul Lin
0 siblings, 0 replies; 39+ messages in thread
From: Macpaul Lin @ 2020-06-03 13:50 UTC (permalink / raw)
To: Takashi Iwai
Cc: alsa-devel, Szabolcs Szőke, Mediatek WSD Upstream,
Greg Kroah-Hartman, linux-usb, Takashi Iwai, stable,
linux-kernel, Hui Wang, Alexander Tsoy, linux-mediatek,
Matthias Brugger, Johan Hovold, Jaroslav Kysela, Macpaul Lin,
linux-arm-kernel
On Wed, 2020-06-03 at 14:47 +0200, Takashi Iwai wrote:
> On Wed, 03 Jun 2020 14:39:24 +0200,
> Macpaul Lin wrote:
> >
> > 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.
>
> Good to hear!
>
> > 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.
>
> Currently the autosuspend is set only to the primary interface; IOW,
> the other interfaces will never get autosuspend, and the another
> suspend-all-intf patch should improve that situation. But it won't
> fix your actual bug, obviously :)
>
> > So I didn't test the earlier patch "suspend all interface instead of
> > only the primary one."
>
> Could you try it one on top of the last patch? At least I'd like to
> see whether it causes any regression.
I've tried both of these 2 patches together, and it looks okay.
> > 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.
>
> Yes, that's a general bug and worth to be merged quickly.
> I'm going to submit a proper patch soon later.
>
>
> thanks,
>
> Takashi
>
Thanks!
Macpaul Lin
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH] sound: usb: pcm: fix incorrect power state when playing sound after PM_AUTO suspend
@ 2020-06-03 13:50 ` Macpaul Lin
0 siblings, 0 replies; 39+ messages in thread
From: Macpaul Lin @ 2020-06-03 13:50 UTC (permalink / raw)
To: Takashi Iwai
Cc: alsa-devel, Szabolcs Szőke, Mediatek WSD Upstream,
Greg Kroah-Hartman, linux-usb, Takashi Iwai, stable,
linux-kernel, Hui Wang, Alexander Tsoy, linux-mediatek,
Matthias Brugger, Johan Hovold, Macpaul Lin, linux-arm-kernel
On Wed, 2020-06-03 at 14:47 +0200, Takashi Iwai wrote:
> On Wed, 03 Jun 2020 14:39:24 +0200,
> Macpaul Lin wrote:
> >
> > 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.
>
> Good to hear!
>
> > 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.
>
> Currently the autosuspend is set only to the primary interface; IOW,
> the other interfaces will never get autosuspend, and the another
> suspend-all-intf patch should improve that situation. But it won't
> fix your actual bug, obviously :)
>
> > So I didn't test the earlier patch "suspend all interface instead of
> > only the primary one."
>
> Could you try it one on top of the last patch? At least I'd like to
> see whether it causes any regression.
I've tried both of these 2 patches together, and it looks okay.
> > 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.
>
> Yes, that's a general bug and worth to be merged quickly.
> I'm going to submit a proper patch soon later.
>
>
> thanks,
>
> Takashi
>
Thanks!
Macpaul Lin
^ permalink raw reply [flat|nested] 39+ messages in thread