* Re: [PATCH] sound: usb: pcm: fix incorrect power state when playing sound after PM_AUTO suspend
[not found] ` <s5h367cfsga.wl-tiwai@suse.de>
@ 2020-06-03 12:39 ` Macpaul Lin
2020-06-03 12:47 ` Takashi Iwai
0 siblings, 1 reply; 3+ 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] 3+ messages in thread
* Re: [PATCH] sound: usb: pcm: fix incorrect power state when playing sound after PM_AUTO suspend
2020-06-03 12:39 ` [PATCH] sound: usb: pcm: fix incorrect power state when playing sound after PM_AUTO suspend Macpaul Lin
@ 2020-06-03 12:47 ` Takashi Iwai
2020-06-03 13:50 ` Macpaul Lin
0 siblings, 1 reply; 3+ 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] 3+ 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
0 siblings, 0 replies; 3+ 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] 3+ messages in thread
end of thread, other threads:[~2020-06-03 13:50 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <s5hpnahhbz8.wl-tiwai@suse.de>
[not found] ` <1591153515.23525.50.camel@mtkswgap22>
[not found] ` <s5heeqwfyti.wl-tiwai@suse.de>
[not found] ` <s5hblm0fxl0.wl-tiwai@suse.de>
[not found] ` <s5h367cfsga.wl-tiwai@suse.de>
2020-06-03 12:39 ` [PATCH] sound: usb: pcm: fix incorrect power state when playing sound after PM_AUTO suspend Macpaul Lin
2020-06-03 12:47 ` Takashi Iwai
2020-06-03 13:50 ` Macpaul Lin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).