On Tue, Sep 02, 2014 at 02:11:52PM -0400, Alan Stern wrote:
> 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
heh, good point :-)
> 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.
Sure thing, that can be done :-)
> > > 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.
I'll agree with this, 100%.
> That might be more of a change than you want to make, however...
I wouldn't say that. As long as regressions can be avoided, I have no
issues modifying the framework and every single driver :-) I guess we
won't be able to do everything in one go, however, so we might need to
split this accross two merge windows to give people time to test all the
changes.
--
balbi
Attachment:
signature.asc
Description: Digital signature
