| Message ID | 20150820153553.GD1639@saruman.tx.rr.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@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.baldyga@samsung.com> > Signed-off-by: Felipe Balbi <balbi@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. Thanks, Robert
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@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.baldyga@samsung.com> > > Signed-off-by: Felipe Balbi <balbi@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.
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@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.baldyga@samsung.com> >>> Signed-off-by: Felipe Balbi <balbi@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. Thanks, Robert
Hi, On Thu, Aug 20, 2015 at 07:16:48PM +0200, 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@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.baldyga@samsung.com> > >>> Signed-off-by: Felipe Balbi <balbi@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(). have you considered switching interfaces and/or alternate settings ? > 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. yeah, that's okay. We've got time.
On 8/20/2015 10:45 AM, Felipe Balbi wrote: > Hi, > > On Thu, Aug 20, 2015 at 07:16:48PM +0200, 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@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.baldyga@samsung.com> >>>>> Signed-off-by: Felipe Balbi <balbi@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(). And the 'claimed' flag should be used for the ep autoconfig mechanism alone. We may want to reset it during the autoconfig phase for multiple configs and alt-interfaces. So there should be separate 'claimed' and 'enabled' flags. > > have you considered switching interfaces and/or alternate settings ? We ran into similar issues before with this very scenario. Handling of set_config(0 or N), in both addressed and configured states, and set_interface requests. John
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; return ep; } EXPORT_SYMBOL_GPL(usb_ep_autoconfig_ss); @@ -182,11 +181,6 @@ EXPORT_SYMBOL_GPL(usb_ep_autoconfig); */ void usb_ep_autoconfig_reset (struct usb_gadget *gadget) { - struct usb_ep *ep; - - list_for_each_entry (ep, &gadget->ep_list, ep_list) { - ep->claimed = false; - } gadget->in_epnum = 0; gadget->out_epnum = 0; } diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h index c14a69b36d27..9b3d60c1cf9f 100644 --- a/include/linux/usb/gadget.h +++ b/include/linux/usb/gadget.h @@ -243,6 +243,22 @@ static inline void usb_ep_set_maxpacket_limit(struct usb_ep *ep, } /** + * usb_ep_enabled - is endpoint enabled ? + * @ep: the endpoint being checked. may not be the endpoint named "ep0". + * + * Whenever a function driver wants to check if a particular endpoint is + * enabled or not, it must check using this helper function. This will + * encapsulate details about how the endpoint is checked, saving the function + * driver from using private methods for doing so. + * + * return true if endpoint is enabled, false otherwise. + */ +static inline bool usb_ep_enabled(struct usb_ep *ep) +{ + return ep->claimed; +} + +/** * usb_ep_enable - configure endpoint, making it usable * @ep:the endpoint being configured. may not be the endpoint named "ep0". * drivers discover endpoints through the ep_list of a usb_gadget. @@ -264,7 +280,18 @@ static inline void usb_ep_set_maxpacket_limit(struct usb_ep *ep, */ static inline int usb_ep_enable(struct usb_ep *ep) { - return ep->ops->enable(ep, ep->desc); + int ret; + + if (usb_ep_enabled(ep)) + return 0; + + ret = ep->ops->enable(ep, ep->desc); + if (ret) + return ret; + + ep->claimed = true; + + return 0; } /** @@ -281,7 +308,18 @@ static inline int usb_ep_enable(struct usb_ep *ep) */ static inline int usb_ep_disable(struct usb_ep *ep) { - return ep->ops->disable(ep); + int ret; + + if (!usb_ep_enabled(ep)) + return 0; + + ret = ep->ops->disable(ep); + if (ret) + return ret; + + ep->claimed = false; + + return 0; } /**