Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1390479 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] usb: dwc3: usb/dwc3: fake dissconnect event when turn off  pullup John Youn <John.Youn@synopsys.com> - 2016-04-28 21:40 +0200
    Re: [PATCH] usb: dwc3: usb/dwc3: fake dissconnect event when turn off pullup Felipe Balbi <balbi@kernel.org> - 2016-04-29 08:20 +0200
      Re: [PATCH] usb: dwc3: usb/dwc3: fake dissconnect event when turn off  pullup John Youn <John.Youn@synopsys.com> - 2016-05-02 03:50 +0200
        Re: [PATCH] usb: dwc3: usb/dwc3: fake dissconnect event when turn off pullup Felipe Balbi <balbi@kernel.org> - 2016-05-04 12:50 +0200
          Re: [PATCH] usb: dwc3: usb/dwc3: fake dissconnect event when turn off  pullup John Youn <John.Youn@synopsys.com> - 2016-05-05 05:20 +0200
            Re: [PATCH] usb: dwc3: usb/dwc3: fake dissconnect event when turn off pullup Felipe Balbi <balbi@kernel.org> - 2016-05-06 10:10 +0200

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

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

[toc] | [next] | [standalone]

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

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

#1391946

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

[toc] | [prev] | [next] | [standalone]

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

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

#1394845

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

[toc] | [prev] | [next] | [standalone]

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

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web