[RFC,0/7] usb: gadget: add reset API at usb_gadget_driver

Message ID 20140902173149.GM16872@saruman.home
State New
Headers show

Commit Message

Felipe Balbi Sept. 2, 2014, 5:31 p.m.
Hi,

On Tue, Sep 02, 2014 at 01:15:18PM -0400, Alan Stern wrote:
> On Tue, 2 Sep 2014, Felipe Balbi wrote:
> 
> > > That needn't be a problem.  If Peter updates the four gadget drivers,
> > > adding reset callbacks, then we can remove the parts of our patches
> > > that invoke the disconnect callback if there is no reset callback.  In 
> > > other words, we can make reset callbacks mandatory for gadget drivers.
> > 
> > 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:


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.

> 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 ?

> 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().

> 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.

Comments

Alan Stern Sept. 2, 2014, 6:11 p.m. | #1
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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Felipe Balbi Sept. 2, 2014, 6:18 p.m. | #2
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.
Alan Stern Sept. 2, 2014, 7:25 p.m. | #3
On Tue, 2 Sep 2014, Felipe Balbi wrote:

> > 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.

Do we need a ->connect callback in the gadget drivers?  If the UDC core 
does all the work of tracking the connection status, maybe we don't.  
For now, I'll assume we don't need it.

Okay.  Let's start by adding the reset field to struct
usb_gadget_driver.  The initial implementation in the four gadget
drivers can be very simple: It calls the disconnect handler.  (At some
later time we can add a ->reset callback to struct
usb_composite_driver; then the reset handler in composite.c can invoke
that callback for all function drivers that define it.)

Next, add routines to udc-core to handle Vbus changes, function
activation changes, and resets.  The Vbus routine will invoke the
gadget driver's ->disconnect callback when Vbus goes off and then call
usb_gadget_disconnect.  The activation routine will call
usb_gadget_disconnect if any functions are deactivated, and
usb_gadget_connect if all the functions are activated (basically, do
whatever composite.c would do).  And the reset routine will invoke the
gadget driver's ->reset callback.  I suppose we'll also have to add
fields to struct usb_gadget, to store the current Vbus and activation
statuses.

Then modify all the UDC drivers; make them invoke the new udc-core
routines whenever there's a change in Vbus status or a reset, instead
of invoking the gadget driver's callbacks directly.  At the same time
we can remove the code that turns off the pullup when Vbus 
goes down, because now the udc-core routine will take care of it.

They don't all have to be updated at once.  Unchanged UDC drivers will 
continue to work exactly as before, because they will bypass the new 
code in udc-core.

How does that sound?

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Peter Chen Sept. 3, 2014, 3:51 a.m. | #4
On Tue, Sep 02, 2014 at 03:25:05PM -0400, Alan Stern wrote:
> On Tue, 2 Sep 2014, Felipe Balbi wrote:
> 
> > > 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.
> 
> Do we need a ->connect callback in the gadget drivers?  If the UDC core 
> does all the work of tracking the connection status, maybe we don't.  
> For now, I'll assume we don't need it.
> 
> Okay.  Let's start by adding the reset field to struct
> usb_gadget_driver.  The initial implementation in the four gadget
> drivers can be very simple: It calls the disconnect handler.  (At some
> later time we can add a ->reset callback to struct
> usb_composite_driver; then the reset handler in composite.c can invoke
> that callback for all function drivers that define it.)
> 
> Next, add routines to udc-core to handle Vbus changes, function
> activation changes, and resets.  The Vbus routine will invoke the
> gadget driver's ->disconnect callback when Vbus goes off and then call
> usb_gadget_disconnect.  The activation routine will call
> usb_gadget_disconnect if any functions are deactivated, and
> usb_gadget_connect if all the functions are activated (basically, do
> whatever composite.c would do).  And the reset routine will invoke the
> gadget driver's ->reset callback.  I suppose we'll also have to add
> fields to struct usb_gadget, to store the current Vbus and activation
> statuses.

If the usb_gadget_disconnect will be not called at gadget driver's
->disconnect, it has no much meaning we still have gadget driver's
->reset, since the content of its ->disconnect and ->reset are the
same if we don't consider to add function's reset handler.

> 
> Then modify all the UDC drivers; make them invoke the new udc-core
> routines whenever there's a change in Vbus status or a reset, instead
> of invoking the gadget driver's callbacks directly.  At the same time
> we can remove the code that turns off the pullup when Vbus 
> goes down, because now the udc-core routine will take care of it.

Why we need to record the reset at udc-core?

> 
> They don't all have to be updated at once.  Unchanged UDC drivers will 
> continue to work exactly as before, because they will bypass the new 
> code in udc-core.
> 
> How does that sound?
> 
> Alan Stern
>
Alan Stern Sept. 3, 2014, 5:53 p.m. | #5
On Wed, 3 Sep 2014, Peter Chen wrote:

> > Okay.  Let's start by adding the reset field to struct
> > usb_gadget_driver.  The initial implementation in the four gadget
> > drivers can be very simple: It calls the disconnect handler.  (At some
> > later time we can add a ->reset callback to struct
> > usb_composite_driver; then the reset handler in composite.c can invoke
> > that callback for all function drivers that define it.)
> > 
> > Next, add routines to udc-core to handle Vbus changes, function
> > activation changes, and resets.  The Vbus routine will invoke the
> > gadget driver's ->disconnect callback when Vbus goes off and then call
> > usb_gadget_disconnect.  The activation routine will call
> > usb_gadget_disconnect if any functions are deactivated, and
> > usb_gadget_connect if all the functions are activated (basically, do
> > whatever composite.c would do).  And the reset routine will invoke the
> > gadget driver's ->reset callback.  I suppose we'll also have to add
> > fields to struct usb_gadget, to store the current Vbus and activation
> > statuses.
> 
> If the usb_gadget_disconnect will be not called at gadget driver's
> ->disconnect, it has no much meaning we still have gadget driver's
> ->reset, since the content of its ->disconnect and ->reset are the
> same if we don't consider to add function's reset handler.

No, they aren't always the same.  For example, g_mass_storage ought to 
flush its dirty buffers when a disconnect occurs, but it doesn't have 
to flush them when a reset occurs.

> > Then modify all the UDC drivers; make them invoke the new udc-core
> > routines whenever there's a change in Vbus status or a reset, instead
> > of invoking the gadget driver's callbacks directly.  At the same time
> > we can remove the code that turns off the pullup when Vbus 
> > goes down, because now the udc-core routine will take care of it.
> 
> Why we need to record the reset at udc-core?

We don't really need to.  But it seems like a good idea to have events
that affect the entire device (including connect, disconnect, reset,
and probably also bus suspend and bus resume) all pass through
udc-core.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Peter Chen Sept. 4, 2014, 1:09 a.m. | #6
On Wed, Sep 03, 2014 at 01:53:23PM -0400, Alan Stern wrote:
> On Wed, 3 Sep 2014, Peter Chen wrote:
> 
> > > Okay.  Let's start by adding the reset field to struct
> > > usb_gadget_driver.  The initial implementation in the four gadget
> > > drivers can be very simple: It calls the disconnect handler.  (At some
> > > later time we can add a ->reset callback to struct
> > > usb_composite_driver; then the reset handler in composite.c can invoke
> > > that callback for all function drivers that define it.)
> > > 
> > > Next, add routines to udc-core to handle Vbus changes, function
> > > activation changes, and resets.  The Vbus routine will invoke the
> > > gadget driver's ->disconnect callback when Vbus goes off and then call
> > > usb_gadget_disconnect.  The activation routine will call
> > > usb_gadget_disconnect if any functions are deactivated, and
> > > usb_gadget_connect if all the functions are activated (basically, do
> > > whatever composite.c would do).  And the reset routine will invoke the
> > > gadget driver's ->reset callback.  I suppose we'll also have to add
> > > fields to struct usb_gadget, to store the current Vbus and activation
> > > statuses.
> > 
> > If the usb_gadget_disconnect will be not called at gadget driver's
> > ->disconnect, it has no much meaning we still have gadget driver's
> > ->reset, since the content of its ->disconnect and ->reset are the
> > same if we don't consider to add function's reset handler.
> 
> No, they aren't always the same.  For example, g_mass_storage ought to 
> flush its dirty buffers when a disconnect occurs, but it doesn't have 
> to flush them when a reset occurs.
> 
> > > Then modify all the UDC drivers; make them invoke the new udc-core
> > > routines whenever there's a change in Vbus status or a reset, instead
> > > of invoking the gadget driver's callbacks directly.  At the same time
> > > we can remove the code that turns off the pullup when Vbus 
> > > goes down, because now the udc-core routine will take care of it.
> > 
> > Why we need to record the reset at udc-core?
> 
> We don't really need to.  But it seems like a good idea to have events
> that affect the entire device (including connect, disconnect, reset,
> and probably also bus suspend and bus resume) all pass through
> udc-core.
> 

Hi Felipe & Alan, how about using below steps for this reset/vbus/pullup
changes? It mainly uses Alan's suggestion.

Step 1:
	The initial implementation in the four gadget
	drivers can be very simple: It calls the disconnect handler.
	the ->reset is mandatory for all gadget drivers (currently,
	only four, they are composite, configfs, gadgetfs , dbgp.
Step 2:
	Add routines to udc-core to handle Vbus changes, function
	activation changes, and resets. It will include three sub-steps,
	from easy to hard: reset handler, vbus handler, and activation
	handler.
Step 3:
	Modify all UDCs to call udc-core's reset and vbus handler, delete
	usb_gadget_connect/usb_gadget_disconnect operation at all UDC drivers,
     	and delete invoking usb_gadget_connect unconditional at udc_bind_to_driver
Step 4: 
	Modify the composite.c to disable pullup for adding every function, and
	enable pullup when necessary.
Alan Stern Sept. 4, 2014, 3:04 p.m. | #7
On Thu, 4 Sep 2014, Peter Chen wrote:

> Hi Felipe & Alan, how about using below steps for this reset/vbus/pullup
> changes? It mainly uses Alan's suggestion.
> 
> Step 1:
> 	The initial implementation in the four gadget
> 	drivers can be very simple: It calls the disconnect handler.
> 	the ->reset is mandatory for all gadget drivers (currently,
> 	only four, they are composite, configfs, gadgetfs , dbgp.
> Step 2:
> 	Add routines to udc-core to handle Vbus changes, function
> 	activation changes, and resets. It will include three sub-steps,
> 	from easy to hard: reset handler, vbus handler, and activation
> 	handler.

Perhaps the activation handler can be delayed until step 4.  It won't 
get used until composite.c is modified to call it.

> Step 3:
> 	Modify all UDCs to call udc-core's reset and vbus handler, delete
> 	usb_gadget_connect/usb_gadget_disconnect operation at all UDC drivers,
>      	and delete invoking usb_gadget_connect unconditional at udc_bind_to_driver
> Step 4: 
> 	Modify the composite.c to disable pullup for adding every function, and
> 	enable pullup when necessary.

Actually, composite.c should be modified to call the activation handler 
in udc-core, and then _that_ routine would adjust the pullup as 
necessary.

This plan is okay with me.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Peter Chen Sept. 5, 2014, 12:44 a.m. | #8
On Thu, Sep 04, 2014 at 11:04:38AM -0400, Alan Stern wrote:
> On Thu, 4 Sep 2014, Peter Chen wrote:
> 
> > Hi Felipe & Alan, how about using below steps for this reset/vbus/pullup
> > changes? It mainly uses Alan's suggestion.
> > 
> > Step 1:
> > 	The initial implementation in the four gadget
> > 	drivers can be very simple: It calls the disconnect handler.
> > 	the ->reset is mandatory for all gadget drivers (currently,
> > 	only four, they are composite, configfs, gadgetfs , dbgp.
> > Step 2:
> > 	Add routines to udc-core to handle Vbus changes, function
> > 	activation changes, and resets. It will include three sub-steps,
> > 	from easy to hard: reset handler, vbus handler, and activation
> > 	handler.
> 
> Perhaps the activation handler can be delayed until step 4.  It won't 
> get used until composite.c is modified to call it.
> 
> > Step 3:
> > 	Modify all UDCs to call udc-core's reset and vbus handler, delete
> > 	usb_gadget_connect/usb_gadget_disconnect operation at all UDC drivers,
> >      	and delete invoking usb_gadget_connect unconditional at udc_bind_to_driver
> > Step 4: 
> > 	Modify the composite.c to disable pullup for adding every function, and
> > 	enable pullup when necessary.
> 
> Actually, composite.c should be modified to call the activation handler 
> in udc-core, and then _that_ routine would adjust the pullup as 
> necessary.
> 
> This plan is okay with me.
> 

OK, let's put the function activation change to the last. If the Felipe has
no other comments, I will send the patch for step one next Monday.
Felipe Balbi Sept. 5, 2014, 4:01 p.m. | #9
On Fri, Sep 05, 2014 at 08:44:08AM +0800, Peter Chen wrote:
> On Thu, Sep 04, 2014 at 11:04:38AM -0400, Alan Stern wrote:
> > On Thu, 4 Sep 2014, Peter Chen wrote:
> > 
> > > Hi Felipe & Alan, how about using below steps for this reset/vbus/pullup
> > > changes? It mainly uses Alan's suggestion.
> > > 
> > > Step 1:
> > > 	The initial implementation in the four gadget
> > > 	drivers can be very simple: It calls the disconnect handler.
> > > 	the ->reset is mandatory for all gadget drivers (currently,
> > > 	only four, they are composite, configfs, gadgetfs , dbgp.
> > > Step 2:
> > > 	Add routines to udc-core to handle Vbus changes, function
> > > 	activation changes, and resets. It will include three sub-steps,
> > > 	from easy to hard: reset handler, vbus handler, and activation
> > > 	handler.
> > 
> > Perhaps the activation handler can be delayed until step 4.  It won't 
> > get used until composite.c is modified to call it.
> > 
> > > Step 3:
> > > 	Modify all UDCs to call udc-core's reset and vbus handler, delete
> > > 	usb_gadget_connect/usb_gadget_disconnect operation at all UDC drivers,
> > >      	and delete invoking usb_gadget_connect unconditional at udc_bind_to_driver
> > > Step 4: 
> > > 	Modify the composite.c to disable pullup for adding every function, and
> > > 	enable pullup when necessary.
> > 
> > Actually, composite.c should be modified to call the activation handler 
> > in udc-core, and then _that_ routine would adjust the pullup as 
> > necessary.
> > 
> > This plan is okay with me.
> > 
> 
> OK, let's put the function activation change to the last. If the Felipe has
> no other comments, I will send the patch for step one next Monday. 

Please do, the thread already has too much information and we better put
all that in code. Keep in mind that me and Alan have resurected an old
patchset adding ->reset() callback to the gadget framework. If you want
to take a look it's at [1]

https://git.kernel.org/cgit/linux/kernel/git/balbi/usb.git/log/?h=gadget-add-reset-method
Peter Chen Sept. 6, 2014, 12:09 a.m. | #10
> On Fri, Sep 05, 2014 at 08:44:08AM +0800, Peter Chen wrote:
> > On Thu, Sep 04, 2014 at 11:04:38AM -0400, Alan Stern wrote:
> > > On Thu, 4 Sep 2014, Peter Chen wrote:
> > >
> > > > Hi Felipe & Alan, how about using below steps for this
> > > > reset/vbus/pullup changes? It mainly uses Alan's suggestion.
> > > >
> > > > Step 1:
> > > > 	The initial implementation in the four gadget
> > > > 	drivers can be very simple: It calls the disconnect handler.
> > > > 	the ->reset is mandatory for all gadget drivers (currently,
> > > > 	only four, they are composite, configfs, gadgetfs , dbgp.
> > > > Step 2:
> > > > 	Add routines to udc-core to handle Vbus changes, function
> > > > 	activation changes, and resets. It will include three sub-steps,
> > > > 	from easy to hard: reset handler, vbus handler, and activation
> > > > 	handler.
> > >
> > > Perhaps the activation handler can be delayed until step 4.  It
> > > won't get used until composite.c is modified to call it.
> > >
> > > > Step 3:
> > > > 	Modify all UDCs to call udc-core's reset and vbus handler, delete
> > > > 	usb_gadget_connect/usb_gadget_disconnect operation at all UDC
> drivers,
> > > >      	and delete invoking usb_gadget_connect unconditional at
> > > > udc_bind_to_driver Step 4:
> > > > 	Modify the composite.c to disable pullup for adding every function, and
> > > > 	enable pullup when necessary.
> > >
> > > Actually, composite.c should be modified to call the activation
> > > handler in udc-core, and then _that_ routine would adjust the pullup
> > > as necessary.
> > >
> > > This plan is okay with me.
> > >
> >
> > OK, let's put the function activation change to the last. If the
> > Felipe has no other comments, I will send the patch for step one next
> Monday.
> 
> Please do, the thread already has too much information and we better put all
> that in code. Keep in mind that me and Alan have resurected an old patchset
> adding ->reset() callback to the gadget framework. If you want to take a look
> it's at [1]
> 
> https://git.kernel.org/cgit/linux/kernel/git/balbi/usb.git/log/?h=gadget-add-
> reset-method
> 

Thanks, Felipe.

It still takes gadget driver's ->reset as optional, but we want to take it as mandatory
per our discussion.

Peter

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Felipe Balbi Sept. 8, 2014, 1:30 p.m. | #11
On Sat, Sep 06, 2014 at 12:09:22AM +0000, Peter Chen wrote:
>  
> > On Fri, Sep 05, 2014 at 08:44:08AM +0800, Peter Chen wrote:
> > > On Thu, Sep 04, 2014 at 11:04:38AM -0400, Alan Stern wrote:
> > > > On Thu, 4 Sep 2014, Peter Chen wrote:
> > > >
> > > > > Hi Felipe & Alan, how about using below steps for this
> > > > > reset/vbus/pullup changes? It mainly uses Alan's suggestion.
> > > > >
> > > > > Step 1:
> > > > > 	The initial implementation in the four gadget
> > > > > 	drivers can be very simple: It calls the disconnect handler.
> > > > > 	the ->reset is mandatory for all gadget drivers (currently,
> > > > > 	only four, they are composite, configfs, gadgetfs , dbgp.
> > > > > Step 2:
> > > > > 	Add routines to udc-core to handle Vbus changes, function
> > > > > 	activation changes, and resets. It will include three sub-steps,
> > > > > 	from easy to hard: reset handler, vbus handler, and activation
> > > > > 	handler.
> > > >
> > > > Perhaps the activation handler can be delayed until step 4.  It
> > > > won't get used until composite.c is modified to call it.
> > > >
> > > > > Step 3:
> > > > > 	Modify all UDCs to call udc-core's reset and vbus handler, delete
> > > > > 	usb_gadget_connect/usb_gadget_disconnect operation at all UDC
> > drivers,
> > > > >      	and delete invoking usb_gadget_connect unconditional at
> > > > > udc_bind_to_driver Step 4:
> > > > > 	Modify the composite.c to disable pullup for adding every function, and
> > > > > 	enable pullup when necessary.
> > > >
> > > > Actually, composite.c should be modified to call the activation
> > > > handler in udc-core, and then _that_ routine would adjust the pullup
> > > > as necessary.
> > > >
> > > > This plan is okay with me.
> > > >
> > >
> > > OK, let's put the function activation change to the last. If the
> > > Felipe has no other comments, I will send the patch for step one next
> > Monday.
> > 
> > Please do, the thread already has too much information and we better put all
> > that in code. Keep in mind that me and Alan have resurected an old patchset
> > adding ->reset() callback to the gadget framework. If you want to take a look
> > it's at [1]
> > 
> > https://git.kernel.org/cgit/linux/kernel/git/balbi/usb.git/log/?h=gadget-add-
> > reset-method
> > 
> 
> Thanks, Felipe.
> 
> It still takes gadget driver's ->reset as optional, but we want to
> take it as mandatory per our discussion.

right, I was going to change that but since you're already working on it
I figured you might prefer doing it all yourself :-)

cheers

Patch

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"