Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1254151 > unrolled thread

[PATCH] usb: dwc2: host: Fix remote wakeup when not in DWC2_L2

Back to article view | Back to linux.kernel

Contents

  [PATCH] usb: dwc2: host: Fix remote wakeup when not in DWC2_L2 Douglas Anderson <dianders@chromium.org> - 2015-10-22 22:10 +0200
    Re: [PATCH] usb: dwc2: host: Fix remote wakeup when not in DWC2_L2 Doug Anderson <dianders@chromium.org> - 2015-10-29 17:50 +0100
      Re: [PATCH] usb: dwc2: host: Fix remote wakeup when not in DWC2_L2 John Youn <John.Youn@synopsys.com> - 2015-10-30 04:30 +0100
        Re: [PATCH] usb: dwc2: host: Fix remote wakeup when not in DWC2_L2 "Herrero, Gregory" <gregory.herrero@intel.com> - 2015-10-30 09:40 +0100
    Re: [PATCH] usb: dwc2: host: Fix remote wakeup when not in DWC2_L2 Heiko Stuebner <heiko@sntech.de> - 2015-10-30 00:30 +0100
      Re: [PATCH] usb: dwc2: host: Fix remote wakeup when not in DWC2_L2 Doug Anderson <dianders@chromium.org> - 2015-10-30 00:50 +0100

#1254151 — [PATCH] usb: dwc2: host: Fix remote wakeup when not in DWC2_L2

In commit 734643dfbdde ("usb: dwc2: host: add flag to reflect bus
state") we changed dwc2_port_suspend() not to set the lx_state
anymore (instead it sets the new bus_suspended variable).  This
introduced a bug where we would fail to detect device insertions if:

1. Plug empty hub into dwc2
2. Plug USB flash drive into the empty hub.
3. Wait a few seconds
4. Unplug USB flash drive
5. Less than 2 seconds after step 4, plug the USB flash drive in again.

The dwc2_hcd_rem_wakeup() function should have been changed to look at
the new bus_suspended variable.

Let's fix it.  Since commit b46146d59fda ("usb: dwc2: host: resume root
hub on remote wakeup") talks about needing the root hub resumed if the
bus was suspended, we'll include it in our test.

It appears that the "port_l1_change" should only be set to 1 if we were
in DWC2_L1 (the driver currently never sets this), so we'll update the
former "else" case based on this test.

Fixes: 734643dfbdde ("usb: dwc2: host: add flag to reflect bus state")
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
 drivers/usb/dwc2/hcd.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index e79baf73c234..571c21727ff9 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -324,12 +324,13 @@ void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg)
  */
 static void dwc2_hcd_rem_wakeup(struct dwc2_hsotg *hsotg)
 {
-	if (hsotg->lx_state == DWC2_L2) {
+	if (hsotg->bus_suspended) {
 		hsotg->flags.b.port_suspend_change = 1;
 		usb_hcd_resume_root_hub(hsotg->priv);
-	} else {
-		hsotg->flags.b.port_l1_change = 1;
 	}
+
+	if (hsotg->lx_state == DWC2_L1)
+		hsotg->flags.b.port_l1_change = 1;
 }
 
 /**
@@ -1428,8 +1429,8 @@ static void dwc2_wakeup_detected(unsigned long data)
 	dev_dbg(hsotg->dev, "Clear Resume: HPRT0=%0x\n",
 		dwc2_readl(hsotg->regs + HPRT0));
 
-	hsotg->bus_suspended = 0;
 	dwc2_hcd_rem_wakeup(hsotg);
+	hsotg->bus_suspended = 0;
 
 	/* Change to L0 state */
 	hsotg->lx_state = DWC2_L0;
-- 
2.6.0.rc2.230.g3dd15c0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1258915

John,

On Thu, Oct 22, 2015 at 1:05 PM, Douglas Anderson <dianders@chromium.org> wrote:
> In commit 734643dfbdde ("usb: dwc2: host: add flag to reflect bus
> state") we changed dwc2_port_suspend() not to set the lx_state
> anymore (instead it sets the new bus_suspended variable).  This
> introduced a bug where we would fail to detect device insertions if:
>
> 1. Plug empty hub into dwc2
> 2. Plug USB flash drive into the empty hub.
> 3. Wait a few seconds
> 4. Unplug USB flash drive
> 5. Less than 2 seconds after step 4, plug the USB flash drive in again.
>
> The dwc2_hcd_rem_wakeup() function should have been changed to look at
> the new bus_suspended variable.
>
> Let's fix it.  Since commit b46146d59fda ("usb: dwc2: host: resume root
> hub on remote wakeup") talks about needing the root hub resumed if the
> bus was suspended, we'll include it in our test.
>
> It appears that the "port_l1_change" should only be set to 1 if we were
> in DWC2_L1 (the driver currently never sets this), so we'll update the
> former "else" case based on this test.
>
> Fixes: 734643dfbdde ("usb: dwc2: host: add flag to reflect bus state")
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
>  drivers/usb/dwc2/hcd.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)

I know I've sent up a lot of patches recently, but this one in
particular would be good to get tested, reviewed, and landed sooner
rather than later.  I believe it fixes a recent regression that is
probably experienced across all dwc2 users.

Please let me know if you have any questions.

Thanks!

-Doug
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1259186

On 10/29/2015 9:43 AM, Doug Anderson wrote:
> John,
> 
> On Thu, Oct 22, 2015 at 1:05 PM, Douglas Anderson <dianders@chromium.org> wrote:
>> In commit 734643dfbdde ("usb: dwc2: host: add flag to reflect bus
>> state") we changed dwc2_port_suspend() not to set the lx_state
>> anymore (instead it sets the new bus_suspended variable).  This
>> introduced a bug where we would fail to detect device insertions if:
>>
>> 1. Plug empty hub into dwc2
>> 2. Plug USB flash drive into the empty hub.
>> 3. Wait a few seconds
>> 4. Unplug USB flash drive
>> 5. Less than 2 seconds after step 4, plug the USB flash drive in again.
>>
>> The dwc2_hcd_rem_wakeup() function should have been changed to look at
>> the new bus_suspended variable.
>>
>> Let's fix it.  Since commit b46146d59fda ("usb: dwc2: host: resume root
>> hub on remote wakeup") talks about needing the root hub resumed if the
>> bus was suspended, we'll include it in our test.
>>
>> It appears that the "port_l1_change" should only be set to 1 if we were
>> in DWC2_L1 (the driver currently never sets this), so we'll update the
>> former "else" case based on this test.
>>
>> Fixes: 734643dfbdde ("usb: dwc2: host: add flag to reflect bus state")
>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
>> ---
>>  drivers/usb/dwc2/hcd.c | 9 +++++----
>>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> I know I've sent up a lot of patches recently, but this one in
> particular would be good to get tested, reviewed, and landed sooner
> rather than later.  I believe it fixes a recent regression that is
> probably experienced across all dwc2 users.
> 
> Please let me know if you have any questions.


Acked-by: John Youn <johnyoun@synopsys.com>

Sorry for the delay. I'll get to the others soon.


Yousaf, Gregory,

Care to provide reviewed or tested-bys?

Regards,
John

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1259317

On Fri, Oct 30, 2015 at 03:21:29AM +0000, John Youn wrote:
> On 10/29/2015 9:43 AM, Doug Anderson wrote:
> > John,
> > 
> > On Thu, Oct 22, 2015 at 1:05 PM, Douglas Anderson <dianders@chromium.org> wrote:
> >> In commit 734643dfbdde ("usb: dwc2: host: add flag to reflect bus
> >> state") we changed dwc2_port_suspend() not to set the lx_state
> >> anymore (instead it sets the new bus_suspended variable).  This
> >> introduced a bug where we would fail to detect device insertions if:
> >>
> >> 1. Plug empty hub into dwc2
> >> 2. Plug USB flash drive into the empty hub.
> >> 3. Wait a few seconds
> >> 4. Unplug USB flash drive
> >> 5. Less than 2 seconds after step 4, plug the USB flash drive in again.
> >>
> >> The dwc2_hcd_rem_wakeup() function should have been changed to look at
> >> the new bus_suspended variable.
> >>
> >> Let's fix it.  Since commit b46146d59fda ("usb: dwc2: host: resume root
> >> hub on remote wakeup") talks about needing the root hub resumed if the
> >> bus was suspended, we'll include it in our test.
> >>
> >> It appears that the "port_l1_change" should only be set to 1 if we were
> >> in DWC2_L1 (the driver currently never sets this), so we'll update the
> >> former "else" case based on this test.
> >>
> >> Fixes: 734643dfbdde ("usb: dwc2: host: add flag to reflect bus state")
> >> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> >> ---
> >>  drivers/usb/dwc2/hcd.c | 9 +++++----
> >>  1 file changed, 5 insertions(+), 4 deletions(-)
> > 
> > I know I've sent up a lot of patches recently, but this one in
> > particular would be good to get tested, reviewed, and landed sooner
> > rather than later.  I believe it fixes a recent regression that is
> > probably experienced across all dwc2 users.
> > 
> > Please let me know if you have any questions.
> 
> 
> Acked-by: John Youn <johnyoun@synopsys.com>
> 
> Sorry for the delay. I'll get to the others soon.
> 
> 
> Yousaf, Gregory,
> 
> Care to provide reviewed or tested-bys?
> 

Hi John,

Patch looks good to me.
I tested multiple suspend/resume and connect/disconnect with it.

Tested-by: Gregory Herrero <gregory.herrero@intel.com>

Regards,
Gregory
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1259093

Am Donnerstag, 22. Oktober 2015, 13:05:03 schrieb Douglas Anderson:
> In commit 734643dfbdde ("usb: dwc2: host: add flag to reflect bus
> state") we changed dwc2_port_suspend() not to set the lx_state
> anymore (instead it sets the new bus_suspended variable).  This
> introduced a bug where we would fail to detect device insertions if:
> 
> 1. Plug empty hub into dwc2
> 2. Plug USB flash drive into the empty hub.
> 3. Wait a few seconds
> 4. Unplug USB flash drive
> 5. Less than 2 seconds after step 4, plug the USB flash drive in again.
> 
> The dwc2_hcd_rem_wakeup() function should have been changed to look at
> the new bus_suspended variable.
> 
> Let's fix it.  Since commit b46146d59fda ("usb: dwc2: host: resume root
> hub on remote wakeup") talks about needing the root hub resumed if the
> bus was suspended, we'll include it in our test.
> 
> It appears that the "port_l1_change" should only be set to 1 if we were
> in DWC2_L1 (the driver currently never sets this), so we'll update the
> former "else" case based on this test.
> 
> Fixes: 734643dfbdde ("usb: dwc2: host: add flag to reflect bus state")
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

I've talked with Doug a lot about that problem today and from reading
this change and the referenced causing change, it looks correct
and good to me, so

Reviewed-by: Heiko Stuebner <heiko@sntech..de>


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1259124

Hi,

On Thu, Oct 29, 2015 at 3:49 PM, Heiko Stuebner <heiko@sntech.de> wrote:
> Am Donnerstag, 22. Oktober 2015, 13:05:03 schrieb Douglas Anderson:
>> In commit 734643dfbdde ("usb: dwc2: host: add flag to reflect bus
>> state") we changed dwc2_port_suspend() not to set the lx_state
>> anymore (instead it sets the new bus_suspended variable).  This
>> introduced a bug where we would fail to detect device insertions if:
>>
>> 1. Plug empty hub into dwc2
>> 2. Plug USB flash drive into the empty hub.
>> 3. Wait a few seconds
>> 4. Unplug USB flash drive
>> 5. Less than 2 seconds after step 4, plug the USB flash drive in again.
>>
>> The dwc2_hcd_rem_wakeup() function should have been changed to look at
>> the new bus_suspended variable.
>>
>> Let's fix it.  Since commit b46146d59fda ("usb: dwc2: host: resume root
>> hub on remote wakeup") talks about needing the root hub resumed if the
>> bus was suspended, we'll include it in our test.
>>
>> It appears that the "port_l1_change" should only be set to 1 if we were
>> in DWC2_L1 (the driver currently never sets this), so we'll update the
>> former "else" case based on this test.
>>
>> Fixes: 734643dfbdde ("usb: dwc2: host: add flag to reflect bus state")
>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
>
> I've talked with Doug a lot about that problem today and from reading
> this change and the referenced causing change, it looks correct
> and good to me, so

It does also appear that the steps in the commit message are not
sufficient to reproduce the problem.  Apparently something in the way
my system is configured adds a delay before the bus actually gets
suspended.  Presumably you could simulate the setup of my current
system with an msleep() at the start of _dwc2_hcd_suspend() (before
grabbing the spinlock).  I've stepped away from my test machine for
the day though, so I can't confirm that.

-Doug
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web