| Message ID | 1416273684-11300-1-git-send-email-kever.yang@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Kever, On Mon, Nov 17, 2014 at 5:21 PM, Kever Yang <kever.yang@rock-chips.com> wrote: > After we implement the bus_suspend/resume, auto suspend id enabled. > The root hub will be auto suspend if there is no device connected, > we need to resume the root hub when a device connect detect. > > This patch tested on rk3288. > > Signed-off-by: Roy Li <roy.li@rock-chips.com> > Signed-off-by: Kever Yang <kever.yang@rock-chips.com> > --- > > Changes in v2: > - add definition for hcd structure > - remove check for bus->root_hub > > drivers/usb/dwc2/hcd_intr.c | 5 +++++ > 1 file changed, 5 insertions(+) I can confirm that in my tests this fixes the problems introduced by your v3 patch at <https://patchwork.kernel.org/patch/5266281/>. On rk3288-pinky: Tested-by: Doug Anderson <dianders@chromium.org>
On Tue, Nov 18, 2014 at 09:21:24AM +0800, Kever Yang wrote: > After we implement the bus_suspend/resume, auto suspend id enabled. > The root hub will be auto suspend if there is no device connected, > we need to resume the root hub when a device connect detect. > > This patch tested on rk3288. > > Signed-off-by: Roy Li <roy.li@rock-chips.com> > Signed-off-by: Kever Yang <kever.yang@rock-chips.com> looks correct to my eyes. It's the same thing XHCI does. Reviewed-by: Felipe Balbi <balbi@ti.com>
On Tue, 18 Nov 2014, Kever Yang wrote: > After we implement the bus_suspend/resume, auto suspend id enabled. > The root hub will be auto suspend if there is no device connected, > we need to resume the root hub when a device connect detect. > > This patch tested on rk3288. > > Signed-off-by: Roy Li <roy.li@rock-chips.com> > Signed-off-by: Kever Yang <kever.yang@rock-chips.com> > --- > > Changes in v2: > - add definition for hcd structure > - remove check for bus->root_hub > > drivers/usb/dwc2/hcd_intr.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/usb/dwc2/hcd_intr.c b/drivers/usb/dwc2/hcd_intr.c > index 551ba87..680206f 100644 > --- a/drivers/usb/dwc2/hcd_intr.c > +++ b/drivers/usb/dwc2/hcd_intr.c > @@ -329,6 +329,7 @@ static void dwc2_port_intr(struct dwc2_hsotg *hsotg) > { > u32 hprt0; > u32 hprt0_modify; > + struct usb_hcd *hcd = (struct usb_hcd *)hsotg->priv; > > dev_vdbg(hsotg->dev, "--Port Interrupt--\n"); > > @@ -354,6 +355,10 @@ static void dwc2_port_intr(struct dwc2_hsotg *hsotg) > hsotg->flags.b.port_connect_status = 1; > hprt0_modify |= HPRT0_CONNDET; > > + /* resume root hub? */ > + if (hcd->state == HC_STATE_SUSPENDED) > + usb_hcd_resume_root_hub(hcd); You should be aware that it's not safe to use hcd->state for anything in a host controller driver. That field is owned by usbcore, not by the HCD, and it is not protected by any locks. Thus, for example, hcd->state does not get set to HC_STATE_SUSPENDED until some time after the bus_suspend routine has returned. A port-change interrupt might occur during that time interval. Alan Stern
On Tue, Nov 18, 2014 at 11:07:34AM -0500, Alan Stern wrote: > On Tue, 18 Nov 2014, Kever Yang wrote: > > > After we implement the bus_suspend/resume, auto suspend id enabled. > > The root hub will be auto suspend if there is no device connected, > > we need to resume the root hub when a device connect detect. > > > > This patch tested on rk3288. > > > > Signed-off-by: Roy Li <roy.li@rock-chips.com> > > Signed-off-by: Kever Yang <kever.yang@rock-chips.com> > > --- > > > > Changes in v2: > > - add definition for hcd structure > > - remove check for bus->root_hub > > > > drivers/usb/dwc2/hcd_intr.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/drivers/usb/dwc2/hcd_intr.c b/drivers/usb/dwc2/hcd_intr.c > > index 551ba87..680206f 100644 > > --- a/drivers/usb/dwc2/hcd_intr.c > > +++ b/drivers/usb/dwc2/hcd_intr.c > > @@ -329,6 +329,7 @@ static void dwc2_port_intr(struct dwc2_hsotg *hsotg) > > { > > u32 hprt0; > > u32 hprt0_modify; > > + struct usb_hcd *hcd = (struct usb_hcd *)hsotg->priv; > > > > dev_vdbg(hsotg->dev, "--Port Interrupt--\n"); > > > > @@ -354,6 +355,10 @@ static void dwc2_port_intr(struct dwc2_hsotg *hsotg) > > hsotg->flags.b.port_connect_status = 1; > > hprt0_modify |= HPRT0_CONNDET; > > > > + /* resume root hub? */ > > + if (hcd->state == HC_STATE_SUSPENDED) > > + usb_hcd_resume_root_hub(hcd); > > You should be aware that it's not safe to use hcd->state for anything > in a host controller driver. That field is owned by usbcore, not by > the HCD, and it is not protected by any locks. > > Thus, for example, hcd->state does not get set to HC_STATE_SUSPENDED > until some time after the bus_suspend routine has returned. A > port-change interrupt might occur during that time interval. In that case, XHCI has a bug :-) Mathias, care to add it to your TODO list ?
>> You should be aware that it's not safe to use hcd->state for anything >> in a host controller driver. That field is owned by usbcore, not by >> the HCD, and it is not protected by any locks. >> >> Thus, for example, hcd->state does not get set to HC_STATE_SUSPENDED >> until some time after the bus_suspend routine has returned. A >> port-change interrupt might occur during that time interval. Looks like there is explicit code in hcd_bus_suspend() to check for that race condition right after setting hcd->state, or do I misinterpret that (the "Did we race with a root-hub wakeup event?" part)? Also, it seems xhci_bus_suspend() explicitly sets 'hcd->state = HC_STATE_SUSPENDED' before giving up the spinlock for some undocumented reason, maybe to avoid exactly this problem. We could just copy that trick if the hcd.c solution isn't enough (although the DWC2 bus_suspend/bus_resume in the other patch don't grab that spinlock right now, where I'm also not so sure if that's a good idea...).
On Tue, 18 Nov 2014, Julius Werner wrote: > >> You should be aware that it's not safe to use hcd->state for anything > >> in a host controller driver. That field is owned by usbcore, not by > >> the HCD, and it is not protected by any locks. > >> > >> Thus, for example, hcd->state does not get set to HC_STATE_SUSPENDED > >> until some time after the bus_suspend routine has returned. A > >> port-change interrupt might occur during that time interval. > > Looks like there is explicit code in hcd_bus_suspend() to check for > that race condition right after setting hcd->state, or do I > misinterpret that (the "Did we race with a root-hub wakeup event?" > part)? That code doesn't quite do what you think. For example: CPU 1 CPU 2 ----- ----- hcd_bus_suspend(): call hcd->bus_suspend(): root hub gets suspended Wakeup IRQ arrives and is ignored because hcd->state is still HC_STATE_QUIESCING set hcd->state to HC_STATE_SUSPENDED Did we race with a wakeup event? No because usb_hcd_resume_root_hub() wasn't called. Result: the wakeup event is lost. > Also, it seems xhci_bus_suspend() explicitly sets 'hcd->state = > HC_STATE_SUSPENDED' before giving up the spinlock for some > undocumented reason, maybe to avoid exactly this problem. We could > just copy that trick if the hcd.c solution isn't enough (although the > DWC2 bus_suspend/bus_resume in the other patch don't grab that > spinlock right now, where I'm also not so sure if that's a good > idea...). It's better for HCDs to avoid testing hcd->state at all. They should set it to appropriate values at the right times, because usbcore checks it, but they should not test it. This is why ehci-hcd, ohci-hcd, and uhci-hcd all have a private rh_state variable, and they use it while holding their respective private spinlocks. Alan Stern
On 18.11.2014 18:41, Felipe Balbi wrote: > On Tue, Nov 18, 2014 at 11:07:34AM -0500, Alan Stern wrote: >> On Tue, 18 Nov 2014, Kever Yang wrote: >> >>> After we implement the bus_suspend/resume, auto suspend id enabled. >>> The root hub will be auto suspend if there is no device connected, >>> we need to resume the root hub when a device connect detect. >>> >>> This patch tested on rk3288. >>> >>> Signed-off-by: Roy Li <roy.li@rock-chips.com> >>> Signed-off-by: Kever Yang <kever.yang@rock-chips.com> >>> --- >>> >>> Changes in v2: >>> - add definition for hcd structure >>> - remove check for bus->root_hub >>> >>> drivers/usb/dwc2/hcd_intr.c | 5 +++++ >>> 1 file changed, 5 insertions(+) >>> >>> diff --git a/drivers/usb/dwc2/hcd_intr.c b/drivers/usb/dwc2/hcd_intr.c >>> index 551ba87..680206f 100644 >>> --- a/drivers/usb/dwc2/hcd_intr.c >>> +++ b/drivers/usb/dwc2/hcd_intr.c >>> @@ -329,6 +329,7 @@ static void dwc2_port_intr(struct dwc2_hsotg *hsotg) >>> { >>> u32 hprt0; >>> u32 hprt0_modify; >>> + struct usb_hcd *hcd = (struct usb_hcd *)hsotg->priv; >>> >>> dev_vdbg(hsotg->dev, "--Port Interrupt--\n"); >>> >>> @@ -354,6 +355,10 @@ static void dwc2_port_intr(struct dwc2_hsotg *hsotg) >>> hsotg->flags.b.port_connect_status = 1; >>> hprt0_modify |= HPRT0_CONNDET; >>> >>> + /* resume root hub? */ >>> + if (hcd->state == HC_STATE_SUSPENDED) >>> + usb_hcd_resume_root_hub(hcd); >> >> You should be aware that it's not safe to use hcd->state for anything >> in a host controller driver. That field is owned by usbcore, not by >> the HCD, and it is not protected by any locks. >> >> Thus, for example, hcd->state does not get set to HC_STATE_SUSPENDED >> until some time after the bus_suspend routine has returned. A >> port-change interrupt might occur during that time interval. > > In that case, XHCI has a bug :-) Mathias, care to add it to your TODO > list ? > I guess I'll need to check it out, thanks for pointing it out. -Mathias
On Wed, Nov 19, 2014 at 1:47 AM, ??? <lyz@rock-chips.com> wrote: > hi Julius & Alan > > Shall we use dwc2's private status "hsotg->lx_state" here instesd of > "hcd->state" for checking root hub is in suspend state ? > I see the EHCI driver do something like this(ehci->rh_state == > EHCI_RH_SUSPENDED) before resume the root hub. It's not this simple, because lx_state only relates to the status of the one port on the DWC2. That may be suspended even though the root hub is not. The USB core differentiates between suspending individual ports, and suspending the whole root hub (which should automatically suspend all ports in a host-controller-specific manner). This distinction may seem silly on DWC2 because there is only one port to suspend, but you still need to make it so that the USB core doesn't get confused. So the different things you need to keep track of are: * is the one port individually suspended (through the hub_control(SetPortFeature(PORT_SUSPEND)) method) * is the root hub suspended (through calling bus_suspend()) * if the root hub gets suspended (through bus_suspend()), had the port already been suspended before that (through a earlier hub_control())... this is the thing I mentioned in your other patch You can decide whether you want to bake that all into one variable somehow or make it three, but we need to be able to tell all of these things apart. The third bullet point will also require you to prevent races of hub_control() with bus_suspend() (not sure if the kernel already guarantees that or not), so I think we may have to rethink the way the spinlock works (because it currently doesn't cover that).
diff --git a/drivers/usb/dwc2/hcd_intr.c b/drivers/usb/dwc2/hcd_intr.c index 551ba87..680206f 100644 --- a/drivers/usb/dwc2/hcd_intr.c +++ b/drivers/usb/dwc2/hcd_intr.c @@ -329,6 +329,7 @@ static void dwc2_port_intr(struct dwc2_hsotg *hsotg) { u32 hprt0; u32 hprt0_modify; + struct usb_hcd *hcd = (struct usb_hcd *)hsotg->priv; dev_vdbg(hsotg->dev, "--Port Interrupt--\n"); @@ -354,6 +355,10 @@ static void dwc2_port_intr(struct dwc2_hsotg *hsotg) hsotg->flags.b.port_connect_status = 1; hprt0_modify |= HPRT0_CONNDET; + /* resume root hub? */ + if (hcd->state == HC_STATE_SUSPENDED) + usb_hcd_resume_root_hub(hcd); + /* * The Hub driver asserts a reset when it sees port connect * status change flag