| Message ID | 1445544303-24202-1-git-send-email-dianders@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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>
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
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
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
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;
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(-)