[PATCH v2] usb: dwc2: resume root hub when device detect with suspend state

[PATCH v2] usb: dwc2: resume root hub when device detect with suspend state

[Kever Yang]

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);
+
 		/*
 		 * The Hub driver asserts a reset when it sees port connect
 		 * status change flag
-- 
1.9.1

Re: [PATCH v2] usb: dwc2: resume root hub when device detect with suspend state

[Doug Anderson]

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>

Re: [PATCH v2] usb: dwc2: resume root hub when device detect with suspend state

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 530 bytes --]

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>

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2] usb: dwc2: resume root hub when device detect with suspend state

[Alan Stern]

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

Re: [PATCH v2] usb: dwc2: resume root hub when device detect with suspend state

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1917 bytes --]

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 ?

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2] usb: dwc2: resume root hub when device detect with suspend state

[Julius Werner]

>> 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...).

Re: [PATCH v2] usb: dwc2: resume root hub when device detect with suspend state

[Alan Stern]

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

Re: [PATCH v2] usb: dwc2: resume root hub when device detect with suspend state

[Mathias Nyman]

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

Re: Re: [PATCH v2] usb: dwc2: resume root hub when device detect with suspend state

[Julius Werner]

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).