[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 <ba...@ti.com>
> >>> 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.bald...@samsung.com>
> >>> Signed-off-by: Felipe Balbi <ba...@ti.com>
> >>>
> >>> 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.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
