On Thu, Aug 20, 2015 at 10:51:46PM +0200, Robert Baldyga wrote:
> On 08/20/2015 10:23 PM, Alan Stern wrote:
> > [CC: list drastically trimmed]
> >
> > On Thu, 20 Aug 2015, Robert Baldyga wrote:
> >
> >> On 08/20/2015 06:48 PM, Felipe Balbi wrote:
> >>> On Thu, Aug 20, 2015 at 06:28:14PM +0200, Robert Baldyga wrote:
> >>>> Hi Felipe,
> >>>>
> >>>> On 08/20/2015 05:35 PM, Felipe Balbi wrote:
> >>>> [...]
> >>>>> just letting you know that this regresses all gadget drivers making them
> >>>>> try to disable previously disabled endpoints and enable previously
> >>>>> enabled endpoints.
> >>>>>
> >>>>> I have a possible fix (see below) but then it shows a problem on the
> >>>>> host side when using with g_zero (see further below):
> >>>>>
> >>>>> commit 3b8932100aacb6cfbffe288ca93025d8b8430c00
> >>>>> Author: Felipe Balbi <balbi@xxxxxx>
> >>>>> Date: Wed Aug 19 18:05:27 2015 -0500
> >>>>>
> >>>>> usb: gadget: fix ep->claimed lifetime
> >>>>>
> >>>>> In order to fix a regression introduced by commit
> >>>>> cc476b42a39d ("usb: gadget: encapsulate endpoint
> >>>>> claiming mechanism") we have to introduce a simple
> >>>>> helper to check if a particular is enabled or not.
> >>>>>
> >>>>> After that, we need to move ep->claimed lifetime to
> >>>>> usb_ep_enable() and usb_ep_disable() since those
> >>>>> are the only functions which actually enable and
> >>>>> disable endpoints.
> >>>>>
> >>>>> A follow-up patch will come to drop all driver_data
> >>>>> checks from function drivers, since those are, now,
> >>>>> pointless.
> >>>>>
> >>>>> Fixes: cc476b42a39d ("usb: gadget: encapsulate endpoint
> >>>>> claiming mechanism")
> >>>>> Cc: Robert Baldyga <r.baldyga@xxxxxxxxxxx>
> >>>>> Signed-off-by: Felipe Balbi <balbi@xxxxxx>
> >>>>>
> >>>>> diff --git a/drivers/usb/gadget/epautoconf.c b/drivers/usb/gadget/epautoconf.c
> >>>>> index 978435a51038..ad45070cd76f 100644
> >>>>> --- a/drivers/usb/gadget/epautoconf.c
> >>>>> +++ b/drivers/usb/gadget/epautoconf.c
> >>>>> @@ -126,7 +126,6 @@ found_ep:
> >>>>> ep->address = desc->bEndpointAddress;
> >>>>> ep->desc = NULL;
> >>>>> ep->comp_desc = NULL;
> >>>>> - ep->claimed = true;
> >>>>
> >>>> Removing this line causes autoconfig can return the same endpoint many
> >>>> times. This probably causes problems with g_zero.
> >>>>
> >>>> I will try to fix it ASAP.
> >>>
> >>> I was considering the same thing, but the lifetime of ->claimed doesn't
> >>> look correct to me either way. Note that once the flag is enabled, it
> >>> won't get disabled by most gadget drivers.
> >>
> >> And it should not be. This flag is indicator, that endpoint is used by
> >> some function. It should be set once by usb_ep_autoconfig() and cleared
> >> by usb_ep_autoconfig_reset().
> >>
> >> I wonder what is reason of this enable/disable regression. Maybe the
> >> problem is that we don't set ep->driver_data to NULL in
> >> usb_ep_autoconfig_reset() (so far it was done). Does this problem occur
> >> while gadget is binded to UDC for the first time, or at any next time?
> >> Unfortunately at this moment I don't have access to my hardware, so it
> >> will take a moment before I will setup some testing environment.
> >
> > To understand this, you have to think back over the history.
> > Originally there was no composite gadget driver; each function was its
> > own gadget. The driver would allocate endpoints only once, when
> > starting up. ep->claimed was used for telling whether or not ep had ,
> > already been allocated, not for whether ep was enabled or anything like
> > that (none of the endpoints were enabled at this stage). The only
> > thing the driver had to be careful about was clearing all the ->claimed
> > flags initially, in case some other gadget driver had been loaded
> > earlier and left the flags set.
> >
> > Things are different now with the composite driver. I don't know the
> > details of how it works, but some things are clear. One function
> > driver must not be allowed to interfere with the endpoints allocated by
> > another. A function driver can't allocate all the endpoints it needs
> > for all configs in one go; instead the configs have to be handled
> > independently and each function belonging to the config must allocate
> > all the endpoints it needs before another config is handled. The only
> > time usb_ep_autoconfig_reset() should be called is when the composite
> > core is ready to start allocating endpoints for a new config.
> >
> > At any rate, I don't see how ep->claimed is related in any way to
> > whether or not the endpoint is enabled. Claiming endpoints takes place
> > when the physical endpoints are allocated for use as the functions'
> > logical endpoints. This has to happen before the host reads the config
> > descriptors -- before any endpoints get enabled.
>
> Thats right. My intention was to introduce flag which can be used only
> for marking endpoints as claimed during functions bind. It should
> definitely not have anything to do with endpoint enable/disable.
>
> It looks like before ep->claimed flag was added, ep->driver_data was
> used for both, marking endpoint as claimed during bind and marking
> endpoints as enabled/disabled after gadget enumeration. My patch
> slightly modified ep->driver_data handling during bind/unbind process,
> and that is probably cause of problems with enable/disable.
>
> However using ep->driver_data field to marking endpoint as
> enabled/disabled is not the most fortunate solution. I agree with John
> Youn that we should have separate 'claimed' and 'enabled' flags.
right, however, for the -rc cycle we can have the minimal fix to make
sure driver_data semantics don't change for v4.3-final. Then we add the
new flag for v4.4 merge window.
I'll make sure to add patches to WARN() about bad use of the API. That
code is currently only on dwc3 but should be dead simnple to make it
generic. That way, such small mistakes can be found with any UDC.
--
balbi
Attachment:
signature.asc
Description: Digital signature
