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@xxxxxxxxxxxx> writes:
>>> John Youn <John.Youn@xxxxxxxxxxxx> writes:
>>>>> "Du, Changbin" <changbin.du@xxxxxxxxx> 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