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

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

[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>
---
 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

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

[Doug Anderson]

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

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

[John Youn]

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

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

[Herrero, Gregory]

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

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

[Heiko Stuebner]

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>

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

[Doug Anderson]

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