From: "Du, Changbin" <[email protected]>
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 <[email protected]>
---
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
Hi,
[email protected] writes:
> From: "Du, Changbin" <[email protected]>
>
> 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 <[email protected]>
> ---
> 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
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,
>
> [email protected] writes:
> > From: "Du, Changbin" <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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
Hello.
On 4/27/2016 11:29 AM, [email protected] wrote:
> From: "Du, Changbin" <[email protected]>
>
> 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 <[email protected]>
> ---
> 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
Hi,
(we don't top-post on this forum ;-)
"Du, Changbin" <[email protected]> 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
On 4/27/2016 11:48 PM, Felipe Balbi wrote:
>
> Hi,
>
> (we don't top-post on this forum ;-)
>
> "Du, Changbin" <[email protected]> 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
Hi,
John Youn <[email protected]> writes:
>> "Du, Changbin" <[email protected]> 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