[PATCH] usb: dwc3: usb/dwc3: fake dissconnect event when turn off pullup

[PATCH] usb: dwc3: usb/dwc3: fake dissconnect event when turn off pullup

[changbin.du]

From: "Du, Changbin" <changbin.du@intel.com>

The dwc3 controller can't generate a disconnect event after it is
stopped. Thus gadget dissconnect callback is not invoked when do
soft dissconnect. Call dissconnect here to workaround this issue.

Note, most time we still see disconnect be called that because
it is invoked by dwc3_gadget_reset_interrupt(). But if we
disconnect cable once pullup disabled quickly, issue can be
observed.

Signed-off-by: Du, Changbin <changbin.du@intel.com>
---
 drivers/usb/dwc3/gadget.c | 33 ++++++++++++++++++++++++---------
 1 file changed, 24 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 8e4a1b1..cd73187 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1566,6 +1566,15 @@ static int dwc3_gadget_run_stop(struct dwc3 *dwc, int is_on, int suspend)
 	return 0;
 }
 
+static void dwc3_disconnect_gadget(struct dwc3 *dwc)
+{
+	if (dwc->gadget_driver && dwc->gadget_driver->disconnect) {
+		spin_unlock(&dwc->lock);
+		dwc->gadget_driver->disconnect(&dwc->gadget);
+		spin_lock(&dwc->lock);
+	}
+}
+
 static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
 {
 	struct dwc3		*dwc = gadget_to_dwc(g);
@@ -1575,6 +1584,21 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
 	is_on = !!is_on;
 
 	spin_lock_irqsave(&dwc->lock, flags);
+	/**
+	 * WORKAROUND: The dwc3 controller can't generate a disconnect
+	 * event after it is stopped. Thus gadget dissconnect callback
+	 * is not invoked when do soft dissconnect. Call dissconnect
+	 * here to workaround this issue.
+	 * Note, most time we still see disconnect be called that because
+	 * it is invoked by dwc3_gadget_reset_interrupt(). But if we
+	 * disconnect cable once pullup disabled quickly, issue can be
+	 * observed.
+	 */
+	if (!is_on && (dwc->gadget.speed != USB_SPEED_UNKNOWN)) {
+		dev_dbg(dwc->dev, "fake dissconnect event on pullup off\n");
+		dwc3_disconnect_gadget(dwc);
+		dwc->gadget.speed = USB_SPEED_UNKNOWN;
+	}
 	ret = dwc3_gadget_run_stop(dwc, is_on, false);
 	spin_unlock_irqrestore(&dwc->lock, flags);
 
@@ -2144,15 +2168,6 @@ static void dwc3_endpoint_interrupt(struct dwc3 *dwc,
 	}
 }
 
-static void dwc3_disconnect_gadget(struct dwc3 *dwc)
-{
-	if (dwc->gadget_driver && dwc->gadget_driver->disconnect) {
-		spin_unlock(&dwc->lock);
-		dwc->gadget_driver->disconnect(&dwc->gadget);
-		spin_lock(&dwc->lock);
-	}
-}
-
 static void dwc3_suspend_gadget(struct dwc3 *dwc)
 {
 	if (dwc->gadget_driver && dwc->gadget_driver->suspend) {
-- 
2.7.4

Re: [PATCH] usb: dwc3: usb/dwc3: fake dissconnect event when turn off pullup

[Felipe Balbi]

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


Hi,

changbin.du@intel.com writes:
> From: "Du, Changbin" <changbin.du@intel.com>
>
> The dwc3 controller can't generate a disconnect event after it is
> stopped. Thus gadget dissconnect callback is not invoked when do

not generating disconnect IRQ after you drop Run/Stop is expected.

> soft dissconnect. Call dissconnect here to workaround this issue.

I'm rather sure this is a bug elsewhere. How do you trigger this ?

> Note, most time we still see disconnect be called that because
> it is invoked by dwc3_gadget_reset_interrupt(). But if we
> disconnect cable once pullup disabled quickly, issue can be
> observed.

I can't understand what you're trying to say with this.

> Signed-off-by: Du, Changbin <changbin.du@intel.com>
> ---
>  drivers/usb/dwc3/gadget.c | 33 ++++++++++++++++++++++++---------
>  1 file changed, 24 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 8e4a1b1..cd73187 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -1566,6 +1566,15 @@ static int dwc3_gadget_run_stop(struct dwc3 *dwc, int is_on, int suspend)
>  	return 0;
>  }
>  
> +static void dwc3_disconnect_gadget(struct dwc3 *dwc)
> +{
> +	if (dwc->gadget_driver && dwc->gadget_driver->disconnect) {
> +		spin_unlock(&dwc->lock);
> +		dwc->gadget_driver->disconnect(&dwc->gadget);
> +		spin_lock(&dwc->lock);
> +	}
> +}
> +
>  static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
>  {
>  	struct dwc3		*dwc = gadget_to_dwc(g);
> @@ -1575,6 +1584,21 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
>  	is_on = !!is_on;
>  
>  	spin_lock_irqsave(&dwc->lock, flags);
> +	/**
> +	 * WORKAROUND: The dwc3 controller can't generate a disconnect
> +	 * event after it is stopped. Thus gadget dissconnect callback
> +	 * is not invoked when do soft dissconnect. Call dissconnect
> +	 * here to workaround this issue.
> +	 * Note, most time we still see disconnect be called that because
> +	 * it is invoked by dwc3_gadget_reset_interrupt(). But if we
> +	 * disconnect cable once pullup disabled quickly, issue can be
> +	 * observed.
> +	 */
> +	if (!is_on && (dwc->gadget.speed != USB_SPEED_UNKNOWN)) {
> +		dev_dbg(dwc->dev, "fake dissconnect event on pullup off\n");
> +		dwc3_disconnect_gadget(dwc);
> +		dwc->gadget.speed = USB_SPEED_UNKNOWN;
> +	}

this is *really* wrong. You shouldn't be calling pullup directly. This
patch looks wrong and you didn't even explain how to trigger this
problem; when does the problem happen ?

Gadget load/unload does the right thing here, so that can't be the
case. We also do the right thing on soft_connect sysfs file:

static ssize_t usb_udc_softconn_store(struct device *dev,
		struct device_attribute *attr, const char *buf, size_t n)
{
	struct usb_udc		*udc = container_of(dev, struct usb_udc, dev);

	if (!udc->driver) {
		dev_err(dev, "soft-connect without a gadget driver\n");
		return -EOPNOTSUPP;
	}

	if (sysfs_streq(buf, "connect")) {
		usb_gadget_udc_start(udc);
		usb_gadget_connect(udc->gadget);
	} else if (sysfs_streq(buf, "disconnect")) {
		usb_gadget_disconnect(udc->gadget);
		udc->driver->disconnect(udc->gadget);
		usb_gadget_udc_stop(udc);
	} else {
		dev_err(dev, "unsupported command '%s'\n", buf);
		return -EINVAL;
	}

	return n;
}
static DEVICE_ATTR(soft_connect, S_IWUSR, NULL, usb_udc_softconn_store);

So really, what _is_ the problem ?

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

RE: [PATCH] usb: dwc3: usb/dwc3: fake dissconnect event when turn off pullup

[Du, Changbin]

Hi, Balbi,

The step to reproduce this issue is:
1) connect device to a host and wait its enumeration.
2) trigger software disconnect by calling function
    usb_gadget_disconnect(), which finally call
   dwc3_gadget_pullup(false). Do not reconnect device
  (I mean no enumeration go on, keep bit Run/Stop 0.).

At here, gadget driver's disconnect callback should be
Called, right? We has been disconnected. But no, as
You said " not generating disconnect IRQ after you
drop Run/Stop is expected".

And I am testing on an Android device, Android only
use dwc3_gadget_pullup(false) to issue a soft disconnection.
This confused user that the UI still show usb as connected
State, caused by missing a disconnect event.

> Hi,
> 
> changbin.du@intel.com writes:
> > From: "Du, Changbin" <changbin.du@intel.com>
> >
> > The dwc3 controller can't generate a disconnect event after it is
> > stopped. Thus gadget dissconnect callback is not invoked when do
> 
> not generating disconnect IRQ after you drop Run/Stop is expected.
> 
> > soft dissconnect. Call dissconnect here to workaround this issue.
> 
> I'm rather sure this is a bug elsewhere. How do you trigger this ?
> 
> > Note, most time we still see disconnect be called that because
> > it is invoked by dwc3_gadget_reset_interrupt(). But if we
> > disconnect cable once pullup disabled quickly, issue can be
> > observed.
> 
> I can't understand what you're trying to say with this.
> 
> > Signed-off-by: Du, Changbin <changbin.du@intel.com>
> > ---
> >  drivers/usb/dwc3/gadget.c | 33 ++++++++++++++++++++++++---------
> >  1 file changed, 24 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> > index 8e4a1b1..cd73187 100644
> > --- a/drivers/usb/dwc3/gadget.c
> > +++ b/drivers/usb/dwc3/gadget.c
> > @@ -1566,6 +1566,15 @@ static int dwc3_gadget_run_stop(struct dwc3
> *dwc, int is_on, int suspend)
> >  	return 0;
> >  }
> >
> > +static void dwc3_disconnect_gadget(struct dwc3 *dwc)
> > +{
> > +	if (dwc->gadget_driver && dwc->gadget_driver->disconnect) {
> > +		spin_unlock(&dwc->lock);
> > +		dwc->gadget_driver->disconnect(&dwc->gadget);
> > +		spin_lock(&dwc->lock);
> > +	}
> > +}
> > +
> >  static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
> >  {
> >  	struct dwc3		*dwc = gadget_to_dwc(g);
> > @@ -1575,6 +1584,21 @@ static int dwc3_gadget_pullup(struct usb_gadget
> *g, int is_on)
> >  	is_on = !!is_on;
> >
> >  	spin_lock_irqsave(&dwc->lock, flags);
> > +	/**
> > +	 * WORKAROUND: The dwc3 controller can't generate a disconnect
> > +	 * event after it is stopped. Thus gadget dissconnect callback
> > +	 * is not invoked when do soft dissconnect. Call dissconnect
> > +	 * here to workaround this issue.
> > +	 * Note, most time we still see disconnect be called that because
> > +	 * it is invoked by dwc3_gadget_reset_interrupt(). But if we
> > +	 * disconnect cable once pullup disabled quickly, issue can be
> > +	 * observed.
> > +	 */
> > +	if (!is_on && (dwc->gadget.speed != USB_SPEED_UNKNOWN)) {
> > +		dev_dbg(dwc->dev, "fake dissconnect event on pullup
> off\n");
> > +		dwc3_disconnect_gadget(dwc);
> > +		dwc->gadget.speed = USB_SPEED_UNKNOWN;
> > +	}
> 
> this is *really* wrong. You shouldn't be calling pullup directly. This
> patch looks wrong and you didn't even explain how to trigger this
> problem; when does the problem happen ?
> 
> Gadget load/unload does the right thing here, so that can't be the
> case. We also do the right thing on soft_connect sysfs file:
> 
> static ssize_t usb_udc_softconn_store(struct device *dev,
> 		struct device_attribute *attr, const char *buf, size_t n)
> {
> 	struct usb_udc		*udc = container_of(dev, struct usb_udc,
> dev);
> 
> 	if (!udc->driver) {
> 		dev_err(dev, "soft-connect without a gadget driver\n");
> 		return -EOPNOTSUPP;
> 	}
> 
> 	if (sysfs_streq(buf, "connect")) {
> 		usb_gadget_udc_start(udc);
> 		usb_gadget_connect(udc->gadget);
> 	} else if (sysfs_streq(buf, "disconnect")) {
> 		usb_gadget_disconnect(udc->gadget);
> 		udc->driver->disconnect(udc->gadget);
> 		usb_gadget_udc_stop(udc);
> 	} else {
> 		dev_err(dev, "unsupported command '%s'\n", buf);
> 		return -EINVAL;
> 	}
> 
> 	return n;
> }
> static DEVICE_ATTR(soft_connect, S_IWUSR, NULL,
> usb_udc_softconn_store);
> 
> So really, what _is_ the problem ?
> 
> --
> balbi

Re: [PATCH] usb: dwc3: usb/dwc3: fake dissconnect event when turn off pullup

[Sergei Shtylyov]

Hello.

On 4/27/2016 11:29 AM, changbin.du@intel.com wrote:

> From: "Du, Changbin" <changbin.du@intel.com>
>
> The dwc3 controller can't generate a disconnect event after it is
> stopped. Thus gadget dissconnect callback is not invoked when do
> soft dissconnect. Call dissconnect here to workaround this issue.

    "Disconnect" everywhere.

> Note, most time we still see disconnect be called that because
> it is invoked by dwc3_gadget_reset_interrupt(). But if we
> disconnect cable once pullup disabled quickly, issue can be
> observed.
>
> Signed-off-by: Du, Changbin <changbin.du@intel.com>
> ---
>  drivers/usb/dwc3/gadget.c | 33 ++++++++++++++++++++++++---------
>  1 file changed, 24 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 8e4a1b1..cd73187 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
[...]
> @@ -1575,6 +1584,21 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
>  	is_on = !!is_on;
>
>  	spin_lock_irqsave(&dwc->lock, flags);
> +	/**
> +	 * WORKAROUND: The dwc3 controller can't generate a disconnect
> +	 * event after it is stopped. Thus gadget dissconnect callback
> +	 * is not invoked when do soft dissconnect. Call dissconnect
> +	 * here to workaround this issue.

    "Disconnect" everywhere.

> +	 * Note, most time we still see disconnect be called that because

    I couldn't parse that.

> +	 * it is invoked by dwc3_gadget_reset_interrupt(). But if we
> +	 * disconnect cable once pullup disabled quickly, issue can be
> +	 * observed.
> +	 */
> +	if (!is_on && (dwc->gadget.speed != USB_SPEED_UNKNOWN)) {
> +		dev_dbg(dwc->dev, "fake dissconnect event on pullup off\n");

    Disconnect.

> +		dwc3_disconnect_gadget(dwc);
> +		dwc->gadget.speed = USB_SPEED_UNKNOWN;
> +	}
>  	ret = dwc3_gadget_run_stop(dwc, is_on, false);
>  	spin_unlock_irqrestore(&dwc->lock, flags);
>
[...]

MBR, Sergei

RE: [PATCH] usb: dwc3: usb/dwc3: fake dissconnect event when turn off pullup

[Felipe Balbi]

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


Hi,

(we don't top-post on this forum ;-)

"Du, Changbin" <changbin.du@intel.com> writes:
> Hi, Balbi,
>
> The step to reproduce this issue is:
> 1) connect device to a host and wait its enumeration.
> 2) trigger software disconnect by calling function
>     usb_gadget_disconnect(), which finally call
>    dwc3_gadget_pullup(false). Do not reconnect device
>   (I mean no enumeration go on, keep bit Run/Stop 0.).
>
> At here, gadget driver's disconnect callback should be
> Called, right? We has been disconnected. But no, as
> You said " not generating disconnect IRQ after you
> drop Run/Stop is expected".
>
> And I am testing on an Android device, Android only
> use dwc3_gadget_pullup(false) to issue a soft disconnection.
> This confused user that the UI still show usb as connected
> State, caused by missing a disconnect event.

okay, so I know what this is. This is caused by Android gadget itself
not notifying the gadget that a disconnect has happened. Just look at
udc-core's soft_connect implementation for the sysfs interface, and
you'll see what I mean.

This should be fixed at Android gadget itself. The only thing we could
do is introduce a new usb_gadget_soft_connect()/disconnect() to wrap the
logic so it's easier for Android gadget to use; but even that I'm a
little bit reluctant to do because Android should be using our
soft_connect interface instead of reimplementing it (wrongly) by its
own.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH] usb: dwc3: usb/dwc3: fake dissconnect event when turn off pullup

[John Youn]

On 4/27/2016 11:48 PM, Felipe Balbi wrote:
> 
> Hi,
> 
> (we don't top-post on this forum ;-)
> 
> "Du, Changbin" <changbin.du@intel.com> writes:
>> Hi, Balbi,
>>
>> The step to reproduce this issue is:
>> 1) connect device to a host and wait its enumeration.
>> 2) trigger software disconnect by calling function
>>     usb_gadget_disconnect(), which finally call
>>    dwc3_gadget_pullup(false). Do not reconnect device
>>   (I mean no enumeration go on, keep bit Run/Stop 0.).
>>
>> At here, gadget driver's disconnect callback should be
>> Called, right? We has been disconnected. But no, as
>> You said " not generating disconnect IRQ after you
>> drop Run/Stop is expected".
>>
>> And I am testing on an Android device, Android only
>> use dwc3_gadget_pullup(false) to issue a soft disconnection.
>> This confused user that the UI still show usb as connected
>> State, caused by missing a disconnect event.
> 
> okay, so I know what this is. This is caused by Android gadget itself
> not notifying the gadget that a disconnect has happened. Just look at
> udc-core's soft_connect implementation for the sysfs interface, and
> you'll see what I mean.
> 
> This should be fixed at Android gadget itself. The only thing we could
> do is introduce a new usb_gadget_soft_connect()/disconnect() to wrap the
> logic so it's easier for Android gadget to use; but even that I'm a
> little bit reluctant to do because Android should be using our
> soft_connect interface instead of reimplementing it (wrongly) by its
> own.
> 

We've run in to the same issue with our usb_gadget_driver.

If the usb_gadget_disconnect() API function, which seems like it is
intended to be called by the gadget_driver, does cause the gadget to
disconnect, it seems reasonable to expect the gadget or the UDC core
to notify the gadget_driver via the callback.

As you mentioned this is handled in the soft_disconnect sysfs. Why
shouldn't usb_gadget_disconnect() do the same thing, if not the gadget
itself? Exposing the sysfs as an API function would work too. Though
both functions are "soft" disconnects and both are called
"disconnect".

In our gadget_driver we do the workaround where we notify ourself that
we called the usb_gadget_disconnect() and that we should now be
disconnected. It just seems more correct that we shouldn't have to
handle that.

By the way, I'm not completely sure of the correct terminology, but
I'm referring to the struct usb_gadget (dwc3, dwc2, etc) as the
"gadget" and the struct usb_gadget_driver as the "gadget_driver"
(normally this would be the composite gadget framework, but we are
using our own driver in this case). Is there a less confusing way to
refer to these :)

John

Re: [PATCH] usb: dwc3: usb/dwc3: fake dissconnect event when turn off pullup

[Felipe Balbi]

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


Hi,

John Youn <John.Youn@synopsys.com> writes:
>> "Du, Changbin" <changbin.du@intel.com> writes:
>>> Hi, Balbi,
>>>
>>> The step to reproduce this issue is:
>>> 1) connect device to a host and wait its enumeration.
>>> 2) trigger software disconnect by calling function
>>>     usb_gadget_disconnect(), which finally call
>>>    dwc3_gadget_pullup(false). Do not reconnect device
>>>   (I mean no enumeration go on, keep bit Run/Stop 0.).
>>>
>>> At here, gadget driver's disconnect callback should be
>>> Called, right? We has been disconnected. But no, as
>>> You said " not generating disconnect IRQ after you
>>> drop Run/Stop is expected".
>>>
>>> And I am testing on an Android device, Android only
>>> use dwc3_gadget_pullup(false) to issue a soft disconnection.
>>> This confused user that the UI still show usb as connected
>>> State, caused by missing a disconnect event.
>> 
>> okay, so I know what this is. This is caused by Android gadget itself
>> not notifying the gadget that a disconnect has happened. Just look at
>> udc-core's soft_connect implementation for the sysfs interface, and
>> you'll see what I mean.
>> 
>> This should be fixed at Android gadget itself. The only thing we could
>> do is introduce a new usb_gadget_soft_connect()/disconnect() to wrap the
>> logic so it's easier for Android gadget to use; but even that I'm a
>> little bit reluctant to do because Android should be using our
>> soft_connect interface instead of reimplementing it (wrongly) by its
>> own.
>> 
>
> We've run in to the same issue with our usb_gadget_driver.
>
> If the usb_gadget_disconnect() API function, which seems like it is
> intended to be called by the gadget_driver, does cause the gadget to
> disconnect, it seems reasonable to expect the gadget or the UDC core
> to notify the gadget_driver via the callback.

Well, the API is supposed to disconnect D+ pullup and that's about it.

> As you mentioned this is handled in the soft_disconnect sysfs. Why
> shouldn't usb_gadget_disconnect() do the same thing, if not the gadget

because there might be cases where we don't need/want the gadget to know
about this disconnect.

> itself? Exposing the sysfs as an API function would work too. Though

it already _is_ exported. I just don't know why people are re-inventing
the same solution :-)

> both functions are "soft" disconnects and both are called
> "disconnect".
>
> In our gadget_driver we do the workaround where we notify ourself that
> we called the usb_gadget_disconnect() and that we should now be

you could just rely on the sysfs interface, right ? :-)

> disconnected. It just seems more correct that we shouldn't have to
> handle that.
>
> By the way, I'm not completely sure of the correct terminology, but
> I'm referring to the struct usb_gadget (dwc3, dwc2, etc) as the
> "gadget" and the struct usb_gadget_driver as the "gadget_driver"
> (normally this would be the composite gadget framework, but we are
> using our own driver in this case). Is there a less confusing way to
> refer to these :)

what I've been doing is that I refer to dwc3, dwc3, etc as UDC (as in
USB Device Controller) and g_mass_storage, g_ether, g_zero, etc as
gadget driver.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH] usb: dwc3: usb/dwc3: fake dissconnect event when turn off pullup

[John Youn]

On 4/28/2016 11:12 PM, Felipe Balbi wrote:
> 
> Hi,
> 
> John Youn <John.Youn@synopsys.com> writes:
>>> "Du, Changbin" <changbin.du@intel.com> writes:
>>>> Hi, Balbi,
>>>>
>>>> The step to reproduce this issue is:
>>>> 1) connect device to a host and wait its enumeration.
>>>> 2) trigger software disconnect by calling function
>>>>     usb_gadget_disconnect(), which finally call
>>>>    dwc3_gadget_pullup(false). Do not reconnect device
>>>>   (I mean no enumeration go on, keep bit Run/Stop 0.).
>>>>
>>>> At here, gadget driver's disconnect callback should be
>>>> Called, right? We has been disconnected. But no, as
>>>> You said " not generating disconnect IRQ after you
>>>> drop Run/Stop is expected".
>>>>
>>>> And I am testing on an Android device, Android only
>>>> use dwc3_gadget_pullup(false) to issue a soft disconnection.
>>>> This confused user that the UI still show usb as connected
>>>> State, caused by missing a disconnect event.
>>>
>>> okay, so I know what this is. This is caused by Android gadget itself
>>> not notifying the gadget that a disconnect has happened. Just look at
>>> udc-core's soft_connect implementation for the sysfs interface, and
>>> you'll see what I mean.
>>>
>>> This should be fixed at Android gadget itself. The only thing we could
>>> do is introduce a new usb_gadget_soft_connect()/disconnect() to wrap the
>>> logic so it's easier for Android gadget to use; but even that I'm a
>>> little bit reluctant to do because Android should be using our
>>> soft_connect interface instead of reimplementing it (wrongly) by its
>>> own.
>>>
>>
>> We've run in to the same issue with our usb_gadget_driver.
>>
>> If the usb_gadget_disconnect() API function, which seems like it is
>> intended to be called by the gadget_driver, does cause the gadget to
>> disconnect, it seems reasonable to expect the gadget or the UDC core
>> to notify the gadget_driver via the callback.
> 
> Well, the API is supposed to disconnect D+ pullup and that's about it.
> 
>> As you mentioned this is handled in the soft_disconnect sysfs. Why
>> shouldn't usb_gadget_disconnect() do the same thing, if not the gadget
> 
> because there might be cases where we don't need/want the gadget to know
> about this disconnect.
> 

But what if we do?

>> itself? Exposing the sysfs as an API function would work too. Though
> 
> it already _is_ exported. I just don't know why people are re-inventing
> the same solution :-)
> 
>> both functions are "soft" disconnects and both are called
>> "disconnect".
>>
>> In our gadget_driver we do the workaround where we notify ourself that
>> we called the usb_gadget_disconnect() and that we should now be
> 
> you could just rely on the sysfs interface, right ? :-)

Not from the gadget driver, at least I don't think so. The gadget
driver itself is the one that wants to initiate the soft disconnect.

> 
>> disconnected. It just seems more correct that we shouldn't have to
>> handle that.
>>
>> By the way, I'm not completely sure of the correct terminology, but
>> I'm referring to the struct usb_gadget (dwc3, dwc2, etc) as the
>> "gadget" and the struct usb_gadget_driver as the "gadget_driver"
>> (normally this would be the composite gadget framework, but we are
>> using our own driver in this case). Is there a less confusing way to
>> refer to these :)
> 
> what I've been doing is that I refer to dwc3, dwc3, etc as UDC (as in
> USB Device Controller) and g_mass_storage, g_ether, g_zero, etc as
> gadget driver.
> 

Ok thanks, that's less confusing than calling them gadgets :)

John

Re: [PATCH] usb: dwc3: usb/dwc3: fake dissconnect event when turn off pullup

[Felipe Balbi]

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


Hi,

John Youn <John.Youn@synopsys.com> writes:
>> John Youn <John.Youn@synopsys.com> writes:
>>>> "Du, Changbin" <changbin.du@intel.com> writes:
>>>>> Hi, Balbi,
>>>>>
>>>>> The step to reproduce this issue is:
>>>>> 1) connect device to a host and wait its enumeration.
>>>>> 2) trigger software disconnect by calling function
>>>>>     usb_gadget_disconnect(), which finally call
>>>>>    dwc3_gadget_pullup(false). Do not reconnect device
>>>>>   (I mean no enumeration go on, keep bit Run/Stop 0.).
>>>>>
>>>>> At here, gadget driver's disconnect callback should be
>>>>> Called, right? We has been disconnected. But no, as
>>>>> You said " not generating disconnect IRQ after you
>>>>> drop Run/Stop is expected".
>>>>>
>>>>> And I am testing on an Android device, Android only
>>>>> use dwc3_gadget_pullup(false) to issue a soft disconnection.
>>>>> This confused user that the UI still show usb as connected
>>>>> State, caused by missing a disconnect event.
>>>>
>>>> okay, so I know what this is. This is caused by Android gadget itself
>>>> not notifying the gadget that a disconnect has happened. Just look at
>>>> udc-core's soft_connect implementation for the sysfs interface, and
>>>> you'll see what I mean.
>>>>
>>>> This should be fixed at Android gadget itself. The only thing we could
>>>> do is introduce a new usb_gadget_soft_connect()/disconnect() to wrap the
>>>> logic so it's easier for Android gadget to use; but even that I'm a
>>>> little bit reluctant to do because Android should be using our
>>>> soft_connect interface instead of reimplementing it (wrongly) by its
>>>> own.
>>>>
>>>
>>> We've run in to the same issue with our usb_gadget_driver.
>>>
>>> If the usb_gadget_disconnect() API function, which seems like it is
>>> intended to be called by the gadget_driver, does cause the gadget to
>>> disconnect, it seems reasonable to expect the gadget or the UDC core
>>> to notify the gadget_driver via the callback.
>> 
>> Well, the API is supposed to disconnect D+ pullup and that's about it.
>> 
>>> As you mentioned this is handled in the soft_disconnect sysfs. Why
>>> shouldn't usb_gadget_disconnect() do the same thing, if not the gadget
>> 
>> because there might be cases where we don't need/want the gadget to know
>> about this disconnect.
>> 
>
> But what if we do?

well, if the gadget is the one faking a disconnect, then it ought to
cancel requests and do all the other necessary steps, right ? :-)

>>> itself? Exposing the sysfs as an API function would work too. Though
>> 
>> it already _is_ exported. I just don't know why people are re-inventing
>> the same solution :-)
>> 
>>> both functions are "soft" disconnects and both are called
>>> "disconnect".
>>>
>>> In our gadget_driver we do the workaround where we notify ourself that
>>> we called the usb_gadget_disconnect() and that we should now be
>> 
>> you could just rely on the sysfs interface, right ? :-)
>
> Not from the gadget driver, at least I don't think so. The gadget
> driver itself is the one that wants to initiate the soft disconnect.

I need to understand this requirement of yours a little better. Can you
describe exactly what your gadget is doing ? Also, any chance of showing
the code for that gadget ? I don't mind carrying an extra function
driver if it helps you validate your IP :-)

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH] usb: dwc3: usb/dwc3: fake dissconnect event when turn off pullup

[John Youn]

On 5/4/2016 3:42 AM, Felipe Balbi wrote:
> 
> Hi,
> 
> John Youn <John.Youn@synopsys.com> writes:
>>> John Youn <John.Youn@synopsys.com> writes:
>>>>> "Du, Changbin" <changbin.du@intel.com> writes:
>>>>>> Hi, Balbi,
>>>>>>
>>>>>> The step to reproduce this issue is:
>>>>>> 1) connect device to a host and wait its enumeration.
>>>>>> 2) trigger software disconnect by calling function
>>>>>>     usb_gadget_disconnect(), which finally call
>>>>>>    dwc3_gadget_pullup(false). Do not reconnect device
>>>>>>   (I mean no enumeration go on, keep bit Run/Stop 0.).
>>>>>>
>>>>>> At here, gadget driver's disconnect callback should be
>>>>>> Called, right? We has been disconnected. But no, as
>>>>>> You said " not generating disconnect IRQ after you
>>>>>> drop Run/Stop is expected".
>>>>>>
>>>>>> And I am testing on an Android device, Android only
>>>>>> use dwc3_gadget_pullup(false) to issue a soft disconnection.
>>>>>> This confused user that the UI still show usb as connected
>>>>>> State, caused by missing a disconnect event.
>>>>>
>>>>> okay, so I know what this is. This is caused by Android gadget itself
>>>>> not notifying the gadget that a disconnect has happened. Just look at
>>>>> udc-core's soft_connect implementation for the sysfs interface, and
>>>>> you'll see what I mean.
>>>>>
>>>>> This should be fixed at Android gadget itself. The only thing we could
>>>>> do is introduce a new usb_gadget_soft_connect()/disconnect() to wrap the
>>>>> logic so it's easier for Android gadget to use; but even that I'm a
>>>>> little bit reluctant to do because Android should be using our
>>>>> soft_connect interface instead of reimplementing it (wrongly) by its
>>>>> own.
>>>>>
>>>>
>>>> We've run in to the same issue with our usb_gadget_driver.
>>>>
>>>> If the usb_gadget_disconnect() API function, which seems like it is
>>>> intended to be called by the gadget_driver, does cause the gadget to
>>>> disconnect, it seems reasonable to expect the gadget or the UDC core
>>>> to notify the gadget_driver via the callback.
>>>
>>> Well, the API is supposed to disconnect D+ pullup and that's about it.
>>>
>>>> As you mentioned this is handled in the soft_disconnect sysfs. Why
>>>> shouldn't usb_gadget_disconnect() do the same thing, if not the gadget
>>>
>>> because there might be cases where we don't need/want the gadget to know
>>> about this disconnect.
>>>
>>
>> But what if we do?
> 
> well, if the gadget is the one faking a disconnect, then it ought to

It's not a really "faking" since the dwc3 controller supports soft
disconnect :)

> cancel requests and do all the other necessary steps, right ? :-)
> 

It does take those steps whenever it's notified of disconnect via its
disconnect callback. But since it doesn't get the callback when the
disconnect happens, it has to call it explicitly. What I'm saying is
that the API or UDC is the one that should call it since it knows that
the usb_gadget_disconnect() API was called and/or the UDC
disconnected.

>>>> itself? Exposing the sysfs as an API function would work too. Though
>>>
>>> it already _is_ exported. I just don't know why people are re-inventing
>>> the same solution :-)
>>>
>>>> both functions are "soft" disconnects and both are called
>>>> "disconnect".
>>>>
>>>> In our gadget_driver we do the workaround where we notify ourself that
>>>> we called the usb_gadget_disconnect() and that we should now be
>>>
>>> you could just rely on the sysfs interface, right ? :-)
>>
>> Not from the gadget driver, at least I don't think so. The gadget
>> driver itself is the one that wants to initiate the soft disconnect.
> 
> I need to understand this requirement of yours a little better. Can you
> describe exactly what your gadget is doing ? Also, any chance of showing
> the code for that gadget ? I don't mind carrying an extra function
> driver if it helps you validate your IP :-)
> 

This gadget driver does a programmable disconnect during testing. I
don't think it will be released anytime soon. Which is why I never
bothered to submit a fix. Also note that this isn't a function but a
gadget driver (same place in the stack as libcomposite framework). I'm
not sure if we have those anymore in the kernel.

John

Re: [PATCH] usb: dwc3: usb/dwc3: fake dissconnect event when turn off pullup

[Peter Chen]

On Thu, Apr 28, 2016 at 09:46:15AM +0300, Felipe Balbi wrote:
> 
> Hi,
> 
> (we don't top-post on this forum ;-)
> 
> "Du, Changbin" <changbin.du@intel.com> writes:
> > Hi, Balbi,
> >
> > The step to reproduce this issue is:
> > 1) connect device to a host and wait its enumeration.
> > 2) trigger software disconnect by calling function
> >     usb_gadget_disconnect(), which finally call
> >    dwc3_gadget_pullup(false). Do not reconnect device
> >   (I mean no enumeration go on, keep bit Run/Stop 0.).
> >
> > At here, gadget driver's disconnect callback should be
> > Called, right? We has been disconnected. But no, as
> > You said " not generating disconnect IRQ after you
> > drop Run/Stop is expected".
> >
> > And I am testing on an Android device, Android only
> > use dwc3_gadget_pullup(false) to issue a soft disconnection.
> > This confused user that the UI still show usb as connected
> > State, caused by missing a disconnect event.
> 
> okay, so I know what this is. This is caused by Android gadget itself
> not notifying the gadget that a disconnect has happened. Just look at
> udc-core's soft_connect implementation for the sysfs interface, and
> you'll see what I mean.
> 
> This should be fixed at Android gadget itself. The only thing we could
> do is introduce a new usb_gadget_soft_connect()/disconnect() to wrap the
> logic so it's easier for Android gadget to use; but even that I'm a
> little bit reluctant to do because Android should be using our
> soft_connect interface instead of reimplementing it (wrongly) by its
> own.
> 

If it is a gadget driver, it can call its disconnect explicitly.
Another thing is the gadget driver should not call usb_gadget_disconnect
directly, it should call usb_gadget_deactivate or usb_function_deactivate.

Since currently, calling usb_gadget_disconnect may not do real pull down
dp, Felipe, will you consider adding gadget_driver->disconnect into
usb_gadget_disconnect after pull down dp?

-- 

Best Regards,
Peter Chen

Re: [PATCH] usb: dwc3: usb/dwc3: fake dissconnect event when turn off pullup

[Felipe Balbi]

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


Hi,

Peter Chen <hzpeterchen@gmail.com> writes:
>> "Du, Changbin" <changbin.du@intel.com> writes:
>> > Hi, Balbi,
>> >
>> > The step to reproduce this issue is:
>> > 1) connect device to a host and wait its enumeration.
>> > 2) trigger software disconnect by calling function
>> >     usb_gadget_disconnect(), which finally call
>> >    dwc3_gadget_pullup(false). Do not reconnect device
>> >   (I mean no enumeration go on, keep bit Run/Stop 0.).
>> >
>> > At here, gadget driver's disconnect callback should be
>> > Called, right? We has been disconnected. But no, as
>> > You said " not generating disconnect IRQ after you
>> > drop Run/Stop is expected".
>> >
>> > And I am testing on an Android device, Android only
>> > use dwc3_gadget_pullup(false) to issue a soft disconnection.
>> > This confused user that the UI still show usb as connected
>> > State, caused by missing a disconnect event.
>> 
>> okay, so I know what this is. This is caused by Android gadget itself
>> not notifying the gadget that a disconnect has happened. Just look at
>> udc-core's soft_connect implementation for the sysfs interface, and
>> you'll see what I mean.
>> 
>> This should be fixed at Android gadget itself. The only thing we could
>> do is introduce a new usb_gadget_soft_connect()/disconnect() to wrap the
>> logic so it's easier for Android gadget to use; but even that I'm a
>> little bit reluctant to do because Android should be using our
>> soft_connect interface instead of reimplementing it (wrongly) by its
>> own.
>> 
>
> If it is a gadget driver, it can call its disconnect explicitly.
> Another thing is the gadget driver should not call usb_gadget_disconnect
> directly, it should call usb_gadget_deactivate or usb_function_deactivate.
>
> Since currently, calling usb_gadget_disconnect may not do real pull down
> dp, Felipe, will you consider adding gadget_driver->disconnect into
> usb_gadget_disconnect after pull down dp?

this is the detail that I'm not yet entirely sure is always valid. Would
there ever be a situation where we want to drop pull-ups but not tell
the gadget about it ? I'm not sure. Anybody wants to investigate ?
Otherwise I'll add it to my TODO list.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH] usb: dwc3: usb/dwc3: fake dissconnect event when turn off pullup

[Peter Chen]

On Fri, May 06, 2016 at 10:01:20AM +0300, Felipe Balbi wrote:
> 
> Hi,
> 
> Peter Chen <hzpeterchen@gmail.com> writes:
> >> "Du, Changbin" <changbin.du@intel.com> writes:
> >> > Hi, Balbi,
> >> >
> >> > The step to reproduce this issue is:
> >> > 1) connect device to a host and wait its enumeration.
> >> > 2) trigger software disconnect by calling function
> >> >     usb_gadget_disconnect(), which finally call
> >> >    dwc3_gadget_pullup(false). Do not reconnect device
> >> >   (I mean no enumeration go on, keep bit Run/Stop 0.).
> >> >
> >> > At here, gadget driver's disconnect callback should be
> >> > Called, right? We has been disconnected. But no, as
> >> > You said " not generating disconnect IRQ after you
> >> > drop Run/Stop is expected".
> >> >
> >> > And I am testing on an Android device, Android only
> >> > use dwc3_gadget_pullup(false) to issue a soft disconnection.
> >> > This confused user that the UI still show usb as connected
> >> > State, caused by missing a disconnect event.
> >> 
> >> okay, so I know what this is. This is caused by Android gadget itself
> >> not notifying the gadget that a disconnect has happened. Just look at
> >> udc-core's soft_connect implementation for the sysfs interface, and
> >> you'll see what I mean.
> >> 
> >> This should be fixed at Android gadget itself. The only thing we could
> >> do is introduce a new usb_gadget_soft_connect()/disconnect() to wrap the
> >> logic so it's easier for Android gadget to use; but even that I'm a
> >> little bit reluctant to do because Android should be using our
> >> soft_connect interface instead of reimplementing it (wrongly) by its
> >> own.
> >> 
> >
> > If it is a gadget driver, it can call its disconnect explicitly.
> > Another thing is the gadget driver should not call usb_gadget_disconnect
> > directly, it should call usb_gadget_deactivate or usb_function_deactivate.
> >
> > Since currently, calling usb_gadget_disconnect may not do real pull down
> > dp, Felipe, will you consider adding gadget_driver->disconnect into
> > usb_gadget_disconnect after pull down dp?
> 
> this is the detail that I'm not yet entirely sure is always valid. Would
> there ever be a situation where we want to drop pull-ups but not tell
> the gadget about it ?

Yes, we have.

- We have enabled pullup dp default for most of platforms, and for
gadget obex and uvc will pull down it since it wants app to
control it

- Some USB charger detection design may need it for the secondary
detection.

After checking again for usb_gadget_disconnect and
usb_gadget_disconnect, I think there is no possible that the dp is
still pulled after we call usb_gadget_disconnect, so we don't need
to change anything:)

-- 

Best Regards,
Peter Chen

Re: [PATCH] usb: dwc3: usb/dwc3: fake dissconnect event when turn off pullup

[Felipe Balbi]

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


Hi,

Peter Chen <hzpeterchen@gmail.com> writes:
>> Peter Chen <hzpeterchen@gmail.com> writes:
>> >> "Du, Changbin" <changbin.du@intel.com> writes:
>> >> > Hi, Balbi,
>> >> >
>> >> > The step to reproduce this issue is:
>> >> > 1) connect device to a host and wait its enumeration.
>> >> > 2) trigger software disconnect by calling function
>> >> >     usb_gadget_disconnect(), which finally call
>> >> >    dwc3_gadget_pullup(false). Do not reconnect device
>> >> >   (I mean no enumeration go on, keep bit Run/Stop 0.).
>> >> >
>> >> > At here, gadget driver's disconnect callback should be
>> >> > Called, right? We has been disconnected. But no, as
>> >> > You said " not generating disconnect IRQ after you
>> >> > drop Run/Stop is expected".
>> >> >
>> >> > And I am testing on an Android device, Android only
>> >> > use dwc3_gadget_pullup(false) to issue a soft disconnection.
>> >> > This confused user that the UI still show usb as connected
>> >> > State, caused by missing a disconnect event.
>> >> 
>> >> okay, so I know what this is. This is caused by Android gadget itself
>> >> not notifying the gadget that a disconnect has happened. Just look at
>> >> udc-core's soft_connect implementation for the sysfs interface, and
>> >> you'll see what I mean.
>> >> 
>> >> This should be fixed at Android gadget itself. The only thing we could
>> >> do is introduce a new usb_gadget_soft_connect()/disconnect() to wrap the
>> >> logic so it's easier for Android gadget to use; but even that I'm a
>> >> little bit reluctant to do because Android should be using our
>> >> soft_connect interface instead of reimplementing it (wrongly) by its
>> >> own.
>> >> 
>> >
>> > If it is a gadget driver, it can call its disconnect explicitly.
>> > Another thing is the gadget driver should not call usb_gadget_disconnect
>> > directly, it should call usb_gadget_deactivate or usb_function_deactivate.
>> >
>> > Since currently, calling usb_gadget_disconnect may not do real pull down
>> > dp, Felipe, will you consider adding gadget_driver->disconnect into
>> > usb_gadget_disconnect after pull down dp?
>> 
>> this is the detail that I'm not yet entirely sure is always valid. Would
>> there ever be a situation where we want to drop pull-ups but not tell
>> the gadget about it ?
>
> Yes, we have.
>
> - We have enabled pullup dp default for most of platforms, and for
> gadget obex and uvc will pull down it since it wants app to
> control it
>
> - Some USB charger detection design may need it for the secondary
> detection.
>
> After checking again for usb_gadget_disconnect and
> usb_gadget_disconnect, I think there is no possible that the dp is
> still pulled after we call usb_gadget_disconnect, so we don't need
> to change anything:)

alright, cool. So we'll keep the requirement to call
gadget_driver->disconnect() for anybody who calls
usb_gadget_disconnect(). Caller should know if we need to notify
gadget_driver or not about that disconnect.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH] usb: dwc3: usb/dwc3: fake dissconnect event when turn off pullup

[Felipe Balbi]

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


Hi,

John Youn <John.Youn@synopsys.com> writes:
>>>>> As you mentioned this is handled in the soft_disconnect sysfs. Why
>>>>> shouldn't usb_gadget_disconnect() do the same thing, if not the gadget
>>>>
>>>> because there might be cases where we don't need/want the gadget to know
>>>> about this disconnect.
>>>>
>>>
>>> But what if we do?
>> 
>> well, if the gadget is the one faking a disconnect, then it ought to
>
> It's not a really "faking" since the dwc3 controller supports soft
> disconnect :)

It's "fake" in the sense that pins are still mated in the connector,
that's what I mean by fake ;)

>> cancel requests and do all the other necessary steps, right ? :-)
>> 
>
> It does take those steps whenever it's notified of disconnect via its
> disconnect callback. But since it doesn't get the callback when the
> disconnect happens, it has to call it explicitly. What I'm saying is

right, and that's the requirement. You're implementing it correctly.

> that the API or UDC is the one that should call it since it knows that
> the usb_gadget_disconnect() API was called and/or the UDC
> disconnected.

but udc-core doesn't know why it was called :) see
usb_function_activate()/deactivate(), we don't want to cancel requests
in that case, just delay the moment when host notices a port change.

>>>>> itself? Exposing the sysfs as an API function would work too. Though
>>>>
>>>> it already _is_ exported. I just don't know why people are re-inventing
>>>> the same solution :-)
>>>>
>>>>> both functions are "soft" disconnects and both are called
>>>>> "disconnect".
>>>>>
>>>>> In our gadget_driver we do the workaround where we notify ourself that
>>>>> we called the usb_gadget_disconnect() and that we should now be
>>>>
>>>> you could just rely on the sysfs interface, right ? :-)
>>>
>>> Not from the gadget driver, at least I don't think so. The gadget
>>> driver itself is the one that wants to initiate the soft disconnect.
>> 
>> I need to understand this requirement of yours a little better. Can you
>> describe exactly what your gadget is doing ? Also, any chance of showing
>> the code for that gadget ? I don't mind carrying an extra function
>> driver if it helps you validate your IP :-)
>> 
>
> This gadget driver does a programmable disconnect during testing. I
> don't think it will be released anytime soon. Which is why I never
> bothered to submit a fix. Also note that this isn't a function but a
> gadget driver (same place in the stack as libcomposite framework). I'm
> not sure if we have those anymore in the kernel.

We still have those, yes. But it seems like your stuff should be
integrated into composite.c itself under a #ifdef USB_GADGET_TESTING or
something like that.

If it helps IP providers using a real OS (linux, of course heh) for
silicon validation, I'd be very happy to integrate these
changes. There's a lot which we can still do to make Linux more
interesting for IP providers and SoC integrators (from validation point
of view), if this minimal change helps, we should do it :)

In fact, if you want to list out some extra requirements for dwc3's
trace functionality, I'd be happy to implement those. Are you missing
any visibility into some internal controller state ? Any sort of tooling
which you'd like to have ?

Let me know ;)

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]