[PATCH] usb: dwc3: gadget: call gadget driver's ->suspend/->resume

[PATCH] usb: dwc3: gadget: call gadget driver's ->suspend/->resume

[David Cohen]

From: Felipe Balbi <balbi@ti.com>

When going into bus suspend/resume we _must_
call gadget driver's ->suspend/->resume callbacks
accordingly. This patch implements that very feature
which has been missing forever.

Cc: <stable@vger.kernel.org> # 3.14
Signed-off-by: Felipe Balbi <balbi@ti.com>
Signed-off-by: David Cohen <david.a.cohen@linux.intel.com>
---

Hi,

This patch was introduced on v3.15.
But the issue it fixes already existed on v3.14 and v3.14 is a long term
support version.
I propose to backport it over there as well.

BR, David
---

 drivers/usb/dwc3/gadget.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 8f6738d46b14..1bb752736c32 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2012,6 +2012,24 @@ static void dwc3_disconnect_gadget(struct dwc3 *dwc)
 	}
 }
 
+static void dwc3_suspend_gadget(struct dwc3 *dwc)
+{
+	if (dwc->gadget_driver && dwc->gadget_driver->disconnect) {
+		spin_unlock(&dwc->lock);
+		dwc->gadget_driver->suspend(&dwc->gadget);
+		spin_lock(&dwc->lock);
+	}
+}
+
+static void dwc3_resume_gadget(struct dwc3 *dwc)
+{
+	if (dwc->gadget_driver && dwc->gadget_driver->disconnect) {
+		spin_unlock(&dwc->lock);
+		dwc->gadget_driver->resume(&dwc->gadget);
+		spin_lock(&dwc->lock);
+	}
+}
+
 static void dwc3_stop_active_transfer(struct dwc3 *dwc, u32 epnum)
 {
 	struct dwc3_ep *dep;
@@ -2391,6 +2409,23 @@ static void dwc3_gadget_linksts_change_interrupt(struct dwc3 *dwc,
 
 	dwc->link_state = next;
 
+	switch (next) {
+	case DWC3_LINK_STATE_U1:
+		if (dwc->speed == USB_SPEED_SUPER)
+			dwc3_suspend_gadget(dwc);
+		break;
+	case DWC3_LINK_STATE_U2:
+	case DWC3_LINK_STATE_U3:
+		dwc3_suspend_gadget(dwc);
+		break;
+	case DWC3_LINK_STATE_RESUME:
+		dwc3_resume_gadget(dwc);
+		break;
+	default:
+		/* do nothing */
+		break;
+	}
+
 	dev_vdbg(dwc->dev, "%s link %d\n", __func__, dwc->link_state);
 }
 
-- 
2.1.4

Re: [PATCH] usb: dwc3: gadget: call gadget driver's ->suspend/->resume

[Greg KH]

On Fri, Apr 17, 2015 at 11:41:56AM -0700, David Cohen wrote:
> From: Felipe Balbi <balbi@ti.com>
> 
> When going into bus suspend/resume we _must_
> call gadget driver's ->suspend/->resume callbacks
> accordingly. This patch implements that very feature
> which has been missing forever.
> 
> Cc: <stable@vger.kernel.org> # 3.14
> Signed-off-by: Felipe Balbi <balbi@ti.com>
> Signed-off-by: David Cohen <david.a.cohen@linux.intel.com>
> ---
> 
> Hi,
> 
> This patch was introduced on v3.15.
> But the issue it fixes already existed on v3.14 and v3.14 is a long term
> support version.
> I propose to backport it over there as well.

What is the git commit id of the patch in Linus's tree?

thanks,

greg k-h

Re: [PATCH] usb: dwc3: gadget: call gadget driver's ->suspend/->resume

[David Cohen]

Hi Greg,

On Fri, Apr 17, 2015 at 09:42:57PM +0200, Greg KH wrote:
> On Fri, Apr 17, 2015 at 11:41:56AM -0700, David Cohen wrote:
> > From: Felipe Balbi <balbi@ti.com>
> > 
> > When going into bus suspend/resume we _must_
> > call gadget driver's ->suspend/->resume callbacks
> > accordingly. This patch implements that very feature
> > which has been missing forever.
> > 
> > Cc: <stable@vger.kernel.org> # 3.14
> > Signed-off-by: Felipe Balbi <balbi@ti.com>
> > Signed-off-by: David Cohen <david.a.cohen@linux.intel.com>
> > ---
> > 
> > Hi,
> > 
> > This patch was introduced on v3.15.
> > But the issue it fixes already existed on v3.14 and v3.14 is a long term
> > support version.
> > I propose to backport it over there as well.
> 
> What is the git commit id of the patch in Linus's tree?

Sorry for the missing info. As Felipe replied, this is the commit:
Upstream commit bc5ba2e0b829c9397f96df1191c7d2319ebc36d9

I'm resending it to fix the commit message and add a second patch as
well.

Br, David

> 
> thanks,
> 
> greg k-h

Re: [PATCH] usb: dwc3: gadget: call gadget driver's ->suspend/->resume

[Felipe Balbi]

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

On Fri, Apr 17, 2015 at 11:41:56AM -0700, David Cohen wrote:
> From: Felipe Balbi <balbi@ti.com>

missing the required:

[ Upstream commit bc5ba2e0b829c9397f96df1191c7d2319ebc36d9 ]

> 
> When going into bus suspend/resume we _must_
> call gadget driver's ->suspend/->resume callbacks
> accordingly. This patch implements that very feature
> which has been missing forever.
> 
> Cc: <stable@vger.kernel.org> # 3.14
> Signed-off-by: Felipe Balbi <balbi@ti.com>
> Signed-off-by: David Cohen <david.a.cohen@linux.intel.com>
> ---
> 
> Hi,
> 
> This patch was introduced on v3.15.
> But the issue it fixes already existed on v3.14 and v3.14 is a long term
> support version.

Can you show me a log of this breaking anywhere ? Why do you consider
this a bug fix ? What sort of drawbacks did you notice ?

> I propose to backport it over there as well.
> 
> BR, David
> ---
> 
>  drivers/usb/dwc3/gadget.c | 35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 8f6738d46b14..1bb752736c32 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2012,6 +2012,24 @@ static void dwc3_disconnect_gadget(struct dwc3 *dwc)
>  	}
>  }
>  
> +static void dwc3_suspend_gadget(struct dwc3 *dwc)
> +{
> +	if (dwc->gadget_driver && dwc->gadget_driver->disconnect) {

you also need Dan Carperter's commit which fixes this cut & paste error.
That's commit 73a30bfc0d526db899033165db6f95c427e70505

> +		spin_unlock(&dwc->lock);
> +		dwc->gadget_driver->suspend(&dwc->gadget);
> +		spin_lock(&dwc->lock);
> +	}
> +}
> +
> +static void dwc3_resume_gadget(struct dwc3 *dwc)
> +{
> +	if (dwc->gadget_driver && dwc->gadget_driver->disconnect) {
> +		spin_unlock(&dwc->lock);
> +		dwc->gadget_driver->resume(&dwc->gadget);
> +		spin_lock(&dwc->lock);
> +	}
> +}
> +
>  static void dwc3_stop_active_transfer(struct dwc3 *dwc, u32 epnum)
>  {
>  	struct dwc3_ep *dep;
> @@ -2391,6 +2409,23 @@ static void dwc3_gadget_linksts_change_interrupt(struct dwc3 *dwc,
>  
>  	dwc->link_state = next;
>  
> +	switch (next) {
> +	case DWC3_LINK_STATE_U1:
> +		if (dwc->speed == USB_SPEED_SUPER)
> +			dwc3_suspend_gadget(dwc);
> +		break;
> +	case DWC3_LINK_STATE_U2:
> +	case DWC3_LINK_STATE_U3:
> +		dwc3_suspend_gadget(dwc);
> +		break;
> +	case DWC3_LINK_STATE_RESUME:
> +		dwc3_resume_gadget(dwc);
> +		break;
> +	default:
> +		/* do nothing */
> +		break;
> +	}
> +
>  	dev_vdbg(dwc->dev, "%s link %d\n", __func__, dwc->link_state);
>  }

-- 
balbi

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

Re: [PATCH] usb: dwc3: gadget: call gadget driver's ->suspend/->resume

[Felipe Balbi]

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

On Fri, Apr 17, 2015 at 02:43:27PM -0500, Felipe Balbi wrote:
> On Fri, Apr 17, 2015 at 11:41:56AM -0700, David Cohen wrote:
> > From: Felipe Balbi <balbi@ti.com>
> 
> missing the required:
> 
> [ Upstream commit bc5ba2e0b829c9397f96df1191c7d2319ebc36d9 ]
> 
> > 
> > When going into bus suspend/resume we _must_
> > call gadget driver's ->suspend/->resume callbacks
> > accordingly. This patch implements that very feature
> > which has been missing forever.
> > 
> > Cc: <stable@vger.kernel.org> # 3.14
> > Signed-off-by: Felipe Balbi <balbi@ti.com>
> > Signed-off-by: David Cohen <david.a.cohen@linux.intel.com>
> > ---
> > 
> > Hi,
> > 
> > This patch was introduced on v3.15.
> > But the issue it fixes already existed on v3.14 and v3.14 is a long term
> > support version.
> 
> Can you show me a log of this breaking anywhere ? Why do you consider
> this a bug fix ? What sort of drawbacks did you notice ?
> 
> > I propose to backport it over there as well.
> > 
> > BR, David
> > ---
> > 
> >  drivers/usb/dwc3/gadget.c | 35 +++++++++++++++++++++++++++++++++++
> >  1 file changed, 35 insertions(+)
> > 
> > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> > index 8f6738d46b14..1bb752736c32 100644
> > --- a/drivers/usb/dwc3/gadget.c
> > +++ b/drivers/usb/dwc3/gadget.c
> > @@ -2012,6 +2012,24 @@ static void dwc3_disconnect_gadget(struct dwc3 *dwc)
> >  	}
> >  }
> >  
> > +static void dwc3_suspend_gadget(struct dwc3 *dwc)
> > +{
> > +	if (dwc->gadget_driver && dwc->gadget_driver->disconnect) {
> 
> you also need Dan Carperter's commit which fixes this cut & paste error.

That's Carpenter, sorry.

-- 
balbi

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

Re: [PATCH] usb: dwc3: gadget: call gadget driver's ->suspend/->resume

[David Cohen]

Hi Felipe,

On Fri, Apr 17, 2015 at 02:43:27PM -0500, Felipe Balbi wrote:
> On Fri, Apr 17, 2015 at 11:41:56AM -0700, David Cohen wrote:
> > From: Felipe Balbi <balbi@ti.com>
> 
> missing the required:
> 
> [ Upstream commit bc5ba2e0b829c9397f96df1191c7d2319ebc36d9 ]
> 
> > 
> > When going into bus suspend/resume we _must_
> > call gadget driver's ->suspend/->resume callbacks
> > accordingly. This patch implements that very feature
> > which has been missing forever.
> > 
> > Cc: <stable@vger.kernel.org> # 3.14
> > Signed-off-by: Felipe Balbi <balbi@ti.com>
> > Signed-off-by: David Cohen <david.a.cohen@linux.intel.com>
> > ---
> > 
> > Hi,
> > 
> > This patch was introduced on v3.15.
> > But the issue it fixes already existed on v3.14 and v3.14 is a long term
> > support version.
> 
> Can you show me a log of this breaking anywhere ? Why do you consider
> this a bug fix ? What sort of drawbacks did you notice ?

We're seeing BC1.2 compliance test failure. I borrowed this info from
the bug report :)

1. BC1.2 compliance testing - SDP2.0
-----------------------------------------------
1. On Connect to active Host (Expected result: 100mA to 500mA):
   Actual result 100mA to 500mA

2. On Host Suspend (ER: Fall back to 0mA):
   not falling back to 0mA, remains at 500mA

3. On Connect to Suspended Host (ER: 100mA to 0mA):
   cable-props shown as 100mA, which means drawing a current of 100mA from
   Suspended Host

4. On making Host active (ER: 500mA):
   500mA

> 
> > I propose to backport it over there as well.
> > 
> > BR, David
> > ---
> > 
> >  drivers/usb/dwc3/gadget.c | 35 +++++++++++++++++++++++++++++++++++
> >  1 file changed, 35 insertions(+)
> > 
> > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> > index 8f6738d46b14..1bb752736c32 100644
> > --- a/drivers/usb/dwc3/gadget.c
> > +++ b/drivers/usb/dwc3/gadget.c
> > @@ -2012,6 +2012,24 @@ static void dwc3_disconnect_gadget(struct dwc3 *dwc)
> >  	}
> >  }
> >  
> > +static void dwc3_suspend_gadget(struct dwc3 *dwc)
> > +{
> > +	if (dwc->gadget_driver && dwc->gadget_driver->disconnect) {
> 
> you also need Dan Carperter's commit which fixes this cut & paste error.
> That's commit 73a30bfc0d526db899033165db6f95c427e70505

Thanks. I'll add that to my next try.

Br, David

Re: [PATCH] usb: dwc3: gadget: call gadget driver's ->suspend/->resume

[Felipe Balbi]

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

Hi,

On Thu, Apr 23, 2015 at 03:37:48PM -0700, David Cohen wrote:
> On Fri, Apr 17, 2015 at 02:43:27PM -0500, Felipe Balbi wrote:
> > On Fri, Apr 17, 2015 at 11:41:56AM -0700, David Cohen wrote:
> > > From: Felipe Balbi <balbi@ti.com>
> > 
> > missing the required:
> > 
> > [ Upstream commit bc5ba2e0b829c9397f96df1191c7d2319ebc36d9 ]
> > 
> > > 
> > > When going into bus suspend/resume we _must_
> > > call gadget driver's ->suspend/->resume callbacks
> > > accordingly. This patch implements that very feature
> > > which has been missing forever.
> > > 
> > > Cc: <stable@vger.kernel.org> # 3.14
> > > Signed-off-by: Felipe Balbi <balbi@ti.com>
> > > Signed-off-by: David Cohen <david.a.cohen@linux.intel.com>
> > > ---
> > > 
> > > Hi,
> > > 
> > > This patch was introduced on v3.15.
> > > But the issue it fixes already existed on v3.14 and v3.14 is a long term
> > > support version.
> > 
> > Can you show me a log of this breaking anywhere ? Why do you consider
> > this a bug fix ? What sort of drawbacks did you notice ?
> 
> We're seeing BC1.2 compliance test failure. I borrowed this info from
> the bug report :)
> 
> 1. BC1.2 compliance testing - SDP2.0
> -----------------------------------------------
> 1. On Connect to active Host (Expected result: 100mA to 500mA):
>    Actual result 100mA to 500mA
> 
> 2. On Host Suspend (ER: Fall back to 0mA):
>    not falling back to 0mA, remains at 500mA
> 
> 3. On Connect to Suspended Host (ER: 100mA to 0mA):
>    cable-props shown as 100mA, which means drawing a current of 100mA from
>    Suspended Host
> 
> 4. On making Host active (ER: 500mA):
>    500mA

But we don't support Battery Charging with dwc3 as of now :-) In fact,
just note that none of the BC registers are even defined in the current
driver anywhere. Seems like you should cherry-pick these to your vendor
tree, but v3.14 vanilla, because it doesn't support BC1.2, can't be
claimed to be at fault, right ?

I'll leave the final decision to Greg and I don't really oppose having
both patches on v3.14-stable, but this is not a bug fix in my view.

-- 
balbi

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

Re: [PATCH] usb: dwc3: gadget: call gadget driver's ->suspend/->resume

[David Cohen]

On Fri, Apr 24, 2015 at 02:48:27PM -0500, Felipe Balbi wrote:
> Hi,

Hi Felipe,

> 
> On Thu, Apr 23, 2015 at 03:37:48PM -0700, David Cohen wrote:
> > On Fri, Apr 17, 2015 at 02:43:27PM -0500, Felipe Balbi wrote:
> > > On Fri, Apr 17, 2015 at 11:41:56AM -0700, David Cohen wrote:
> > > > From: Felipe Balbi <balbi@ti.com>
> > > 
> > > missing the required:
> > > 
> > > [ Upstream commit bc5ba2e0b829c9397f96df1191c7d2319ebc36d9 ]
> > > 
> > > > 
> > > > When going into bus suspend/resume we _must_
> > > > call gadget driver's ->suspend/->resume callbacks
> > > > accordingly. This patch implements that very feature
> > > > which has been missing forever.
> > > > 
> > > > Cc: <stable@vger.kernel.org> # 3.14
> > > > Signed-off-by: Felipe Balbi <balbi@ti.com>
> > > > Signed-off-by: David Cohen <david.a.cohen@linux.intel.com>
> > > > ---
> > > > 
> > > > Hi,
> > > > 
> > > > This patch was introduced on v3.15.
> > > > But the issue it fixes already existed on v3.14 and v3.14 is a long term
> > > > support version.
> > > 
> > > Can you show me a log of this breaking anywhere ? Why do you consider
> > > this a bug fix ? What sort of drawbacks did you notice ?
> > 
> > We're seeing BC1.2 compliance test failure. I borrowed this info from
> > the bug report :)
> > 
> > 1. BC1.2 compliance testing - SDP2.0
> > -----------------------------------------------
> > 1. On Connect to active Host (Expected result: 100mA to 500mA):
> >    Actual result 100mA to 500mA
> > 
> > 2. On Host Suspend (ER: Fall back to 0mA):
> >    not falling back to 0mA, remains at 500mA
> > 
> > 3. On Connect to Suspended Host (ER: 100mA to 0mA):
> >    cable-props shown as 100mA, which means drawing a current of 100mA from
> >    Suspended Host
> > 
> > 4. On making Host active (ER: 500mA):
> >    500mA
> 
> But we don't support Battery Charging with dwc3 as of now :-) In fact,
> just note that none of the BC registers are even defined in the current
> driver anywhere. Seems like you should cherry-pick these to your vendor
> tree, but v3.14 vanilla, because it doesn't support BC1.2, can't be
> claimed to be at fault, right ?

We could call it a missing feature that could lead to a potential bug :)
By your own comment, he "must" was stressed out:
'''
When going into bus suspend/resume we _must_
call gadget driver's ->suspend/->resume callbacks
accordingly. This patch implements that very feature
which has been missing forever.
'''

Since v3.14 is a LTS kernel and the changes are safe, it's worth to
consider.

> 
> I'll leave the final decision to Greg and I don't really oppose having
> both patches on v3.14-stable, but this is not a bug fix in my view.

Thanks. I appreciate your feedback.

BR, David

PS: FWIW implementing features or fixing bugs aren't much different tasks:
https://geekwhisperin.files.wordpress.com/2009/09/bug-vs-feature.jpg

Re: [PATCH] usb: dwc3: gadget: call gadget driver's ->suspend/->resume

[Felipe Balbi]

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

Hi,

On Fri, Apr 24, 2015 at 01:56:25PM -0700, David Cohen wrote:
> > > > > When going into bus suspend/resume we _must_
> > > > > call gadget driver's ->suspend/->resume callbacks
> > > > > accordingly. This patch implements that very feature
> > > > > which has been missing forever.
> > > > > 
> > > > > Cc: <stable@vger.kernel.org> # 3.14
> > > > > Signed-off-by: Felipe Balbi <balbi@ti.com>
> > > > > Signed-off-by: David Cohen <david.a.cohen@linux.intel.com>
> > > > > ---
> > > > > 
> > > > > Hi,
> > > > > 
> > > > > This patch was introduced on v3.15.
> > > > > But the issue it fixes already existed on v3.14 and v3.14 is a long term
> > > > > support version.
> > > > 
> > > > Can you show me a log of this breaking anywhere ? Why do you consider
> > > > this a bug fix ? What sort of drawbacks did you notice ?
> > > 
> > > We're seeing BC1.2 compliance test failure. I borrowed this info from
> > > the bug report :)
> > > 
> > > 1. BC1.2 compliance testing - SDP2.0
> > > -----------------------------------------------
> > > 1. On Connect to active Host (Expected result: 100mA to 500mA):
> > >    Actual result 100mA to 500mA
> > > 
> > > 2. On Host Suspend (ER: Fall back to 0mA):
> > >    not falling back to 0mA, remains at 500mA
> > > 
> > > 3. On Connect to Suspended Host (ER: 100mA to 0mA):
> > >    cable-props shown as 100mA, which means drawing a current of 100mA from
> > >    Suspended Host
> > > 
> > > 4. On making Host active (ER: 500mA):
> > >    500mA
> > 
> > But we don't support Battery Charging with dwc3 as of now :-) In fact,
> > just note that none of the BC registers are even defined in the current
> > driver anywhere. Seems like you should cherry-pick these to your vendor
> > tree, but v3.14 vanilla, because it doesn't support BC1.2, can't be
> > claimed to be at fault, right ?
> 
> We could call it a missing feature that could lead to a potential bug :)
> By your own comment, he "must" was stressed out:

sure it was because they really must be called :-) However, the only
actual problem that arises from not calling them is with something
that's not supported :-)

> When going into bus suspend/resume we _must_
> call gadget driver's ->suspend/->resume callbacks
> accordingly. This patch implements that very feature
> which has been missing forever.
> '''
> 
> Since v3.14 is a LTS kernel and the changes are safe, it's worth to
> consider.

definitely worth to consider, but not as something that fixes BC1.2
because that's, as said, not supported in any public tree :-)

> > I'll leave the final decision to Greg and I don't really oppose having
> > both patches on v3.14-stable, but this is not a bug fix in my view.
> 
> Thanks. I appreciate your feedback.
> 
> BR, David
> 
> PS: FWIW implementing features or fixing bugs aren't much different tasks:
> https://geekwhisperin.files.wordpress.com/2009/09/bug-vs-feature.jpg

Yes, I used to have that on my office door ;-)

-- 
balbi

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

Re: [PATCH] usb: dwc3: gadget: call gadget driver's ->suspend/->resume

[David Cohen]

On Sat, Apr 25, 2015 at 10:47:42AM -0500, Felipe Balbi wrote:
> Hi,

Hi Felipe,

> 
> On Fri, Apr 24, 2015 at 01:56:25PM -0700, David Cohen wrote:
> > > > > > When going into bus suspend/resume we _must_
> > > > > > call gadget driver's ->suspend/->resume callbacks
> > > > > > accordingly. This patch implements that very feature
> > > > > > which has been missing forever.
> > > > > > 
> > > > > > Cc: <stable@vger.kernel.org> # 3.14
> > > > > > Signed-off-by: Felipe Balbi <balbi@ti.com>
> > > > > > Signed-off-by: David Cohen <david.a.cohen@linux.intel.com>
> > > > > > ---
> > > > > > 
> > > > > > Hi,
> > > > > > 
> > > > > > This patch was introduced on v3.15.
> > > > > > But the issue it fixes already existed on v3.14 and v3.14 is a long term
> > > > > > support version.
> > > > > 
> > > > > Can you show me a log of this breaking anywhere ? Why do you consider
> > > > > this a bug fix ? What sort of drawbacks did you notice ?
> > > > 
> > > > We're seeing BC1.2 compliance test failure. I borrowed this info from
> > > > the bug report :)
> > > > 
> > > > 1. BC1.2 compliance testing - SDP2.0
> > > > -----------------------------------------------
> > > > 1. On Connect to active Host (Expected result: 100mA to 500mA):
> > > >    Actual result 100mA to 500mA
> > > > 
> > > > 2. On Host Suspend (ER: Fall back to 0mA):
> > > >    not falling back to 0mA, remains at 500mA
> > > > 
> > > > 3. On Connect to Suspended Host (ER: 100mA to 0mA):
> > > >    cable-props shown as 100mA, which means drawing a current of 100mA from
> > > >    Suspended Host
> > > > 
> > > > 4. On making Host active (ER: 500mA):
> > > >    500mA
> > > 
> > > But we don't support Battery Charging with dwc3 as of now :-) In fact,
> > > just note that none of the BC registers are even defined in the current
> > > driver anywhere. Seems like you should cherry-pick these to your vendor
> > > tree, but v3.14 vanilla, because it doesn't support BC1.2, can't be
> > > claimed to be at fault, right ?
> > 
> > We could call it a missing feature that could lead to a potential bug :)
> > By your own comment, he "must" was stressed out:
> 
> sure it was because they really must be called :-) However, the only
> actual problem that arises from not calling them is with something
> that's not supported :-)
> 
> > When going into bus suspend/resume we _must_
> > call gadget driver's ->suspend/->resume callbacks
> > accordingly. This patch implements that very feature
> > which has been missing forever.
> > '''
> > 
> > Since v3.14 is a LTS kernel and the changes are safe, it's worth to
> > consider.
> 
> definitely worth to consider, but not as something that fixes BC1.2
> because that's, as said, not supported in any public tree :-)

Thanks for the reply.
The gadget having this issue is really out-of-tree (android gadget).
I could do 2 next steps:
1) I could drop android gadget and try to reproduce this issue using
legacy g_ffs, which supports adbd as well.
2) I could propose this change directly on google's android 3.14 tree
instead.

Would you prefer one of those?

Thanks,

David

Re: [PATCH] usb: dwc3: gadget: call gadget driver's ->suspend/->resume

[Felipe Balbi]

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

On Mon, Apr 27, 2015 at 07:55:58AM -0700, David Cohen wrote:
> On Sat, Apr 25, 2015 at 10:47:42AM -0500, Felipe Balbi wrote:
> > Hi,
> 
> Hi Felipe,
> 
> > 
> > On Fri, Apr 24, 2015 at 01:56:25PM -0700, David Cohen wrote:
> > > > > > > When going into bus suspend/resume we _must_
> > > > > > > call gadget driver's ->suspend/->resume callbacks
> > > > > > > accordingly. This patch implements that very feature
> > > > > > > which has been missing forever.
> > > > > > > 
> > > > > > > Cc: <stable@vger.kernel.org> # 3.14
> > > > > > > Signed-off-by: Felipe Balbi <balbi@ti.com>
> > > > > > > Signed-off-by: David Cohen <david.a.cohen@linux.intel.com>
> > > > > > > ---
> > > > > > > 
> > > > > > > Hi,
> > > > > > > 
> > > > > > > This patch was introduced on v3.15.
> > > > > > > But the issue it fixes already existed on v3.14 and v3.14 is a long term
> > > > > > > support version.
> > > > > > 
> > > > > > Can you show me a log of this breaking anywhere ? Why do you consider
> > > > > > this a bug fix ? What sort of drawbacks did you notice ?
> > > > > 
> > > > > We're seeing BC1.2 compliance test failure. I borrowed this info from
> > > > > the bug report :)
> > > > > 
> > > > > 1. BC1.2 compliance testing - SDP2.0
> > > > > -----------------------------------------------
> > > > > 1. On Connect to active Host (Expected result: 100mA to 500mA):
> > > > >    Actual result 100mA to 500mA
> > > > > 
> > > > > 2. On Host Suspend (ER: Fall back to 0mA):
> > > > >    not falling back to 0mA, remains at 500mA
> > > > > 
> > > > > 3. On Connect to Suspended Host (ER: 100mA to 0mA):
> > > > >    cable-props shown as 100mA, which means drawing a current of 100mA from
> > > > >    Suspended Host
> > > > > 
> > > > > 4. On making Host active (ER: 500mA):
> > > > >    500mA
> > > > 
> > > > But we don't support Battery Charging with dwc3 as of now :-) In fact,
> > > > just note that none of the BC registers are even defined in the current
> > > > driver anywhere. Seems like you should cherry-pick these to your vendor
> > > > tree, but v3.14 vanilla, because it doesn't support BC1.2, can't be
> > > > claimed to be at fault, right ?
> > > 
> > > We could call it a missing feature that could lead to a potential bug :)
> > > By your own comment, he "must" was stressed out:
> > 
> > sure it was because they really must be called :-) However, the only
> > actual problem that arises from not calling them is with something
> > that's not supported :-)
> > 
> > > When going into bus suspend/resume we _must_
> > > call gadget driver's ->suspend/->resume callbacks
> > > accordingly. This patch implements that very feature
> > > which has been missing forever.
> > > '''
> > > 
> > > Since v3.14 is a LTS kernel and the changes are safe, it's worth to
> > > consider.
> > 
> > definitely worth to consider, but not as something that fixes BC1.2
> > because that's, as said, not supported in any public tree :-)
> 
> Thanks for the reply.
> The gadget having this issue is really out-of-tree (android gadget).
> I could do 2 next steps:
> 1) I could drop android gadget and try to reproduce this issue using
> legacy g_ffs, which supports adbd as well.
> 2) I could propose this change directly on google's android 3.14 tree
> instead.
> 
> Would you prefer one of those?

hehe, I'll let Greg decide, I'm not against the patch, as I said before,
but I'm also not entirely sure it fits within stable rules ;-)

cheers

-- 
balbi

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