On Tue, 2 Sep 2014, Felipe Balbi wrote:
> > > alright, but we need to do this in steps to avoid regressions or a
> > > non-bisectable tree. So maybe we add ->reset as an optional method,
> > > implement support for it to all UDC drivers, patch all gadget drivers to
> > > implement reset, make reset mandatory. Does that work ?
> >
> > It would be okay, but doing things in a different order would be a
> > little better: Add the reset callback to the usb_gadget_driver
> > structure, implement it in all four gadget drivers, and then implement
> > it in the UDC drivers. That way we don't have to make a second round
>
> it can't be only in these four gadget drivers. It needs to be
> implemented in all of them even if:
There _are_ only four gadget drivers: dbgp, gadgetfs, configfs, and
composite. (Unless some are hiding outside of drivers/usb/gadget -- I
didn't bother to check the entire kernel tree.)
> diff --git a/drivers/usb/gadget/legacy/dbgp.c b/drivers/usb/gadget/legacy/dbgp.c
> index 986fc51..2210289 100644
> --- a/drivers/usb/gadget/legacy/dbgp.c
> +++ b/drivers/usb/gadget/legacy/dbgp.c
> @@ -409,6 +409,7 @@ static __refdata struct usb_gadget_driver dbgp_driver = {
> .unbind = dbgp_unbind,
> .setup = dbgp_setup,
> .disconnect = dbgp_disconnect,
> + .reset = dbgp_disconnect,
> .driver = {
> .owner = THIS_MODULE,
> .name = "dbgp"
>
> otherwise ->reset() will be optional and that means UDC will have to:
>
> if (gadget_driver->reset)
> gadget_driver->reset(g);
> else if (gadget_driver->disconnect && g.speed != UNKNOWN)
> gadget_driver->disconnect(g);
>
> and then we end up with a possibility of disconnecting from the host
> when we don't want to. Bottom line, we _must_ tell the gadget driver
> about a reset IRQ, so it can reset its internal state.
So make reset mandatory from the start.
> > of modifications to the UDC drivers, removing the fallback calls to
> > ->disconnect for when ->reset is missing.
>
> ->reset() can't be missing if it were to be mandatory, right ?
Right.
> > The kerneldoc could state that some UDC drivers may call ->disconnect
> > when a reset occurs, instead of calling ->reset. Then we won't have to
> > fix up all the UDC drivers at once.
>
> we won't be able to do that because the discussion on this thread is
> that ->disconnect() should call usb_gadget_disconnect().
I had forgotten that little detail. :-(
Well, strictly speaking, the disconnect handler doesn't have to call
usb_gadget_disconnect if the UDC driver takes care of it. And
currently all of the UDC drivers do.
Still, in the end it would be cleaner to allow disconnect handlers to
call usb_gadget_disconnect. And that means every UDC driver has to
support ->reset callbacks. I don't know how practical that is -- in
some cases there may not be anybody capable of making the necessary
changes.
> > If you want, you could add a remark to the kerneldoc saying that a
> > disconnect handler generally should do everything that a reset handler
> > does plus perhaps a few additional things.
>
> yeah, that additional thing is usb_gadget_disconnect() and we don't want
> to call that from a simple USB reset.
That's not the only additional thing. The gadget driver also should
remember that the host isn't connected, so that it won't call
usb_gadget_connect when all the userspace components become ready.
You know, more and more it seems like this ought to be handled by the
UDC core. There are two conditions for turning on the pullup: Vbus
must be active, and the gadget's functions must all be activated. The
UDC driver knows about the first and the gadget driver knows about the
second -- so the UDC core is where those two pieces of knowledge should
be brought together.
That might be more of a change than you want to make, however...
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
