| Message ID | 20140501071604.GB30575@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, May 01, 2014 at 03:16:04AM -0400, Zhuang Jin Can wrote: > endpoint.maxburst may be 0 if a gadget doesn't call config_ep_by_speed() > to update it from the companion descriptor. > And endpoint.maxburst - 1 returns 11111b which wrongly sets bit > 26 of endpoint parameter 0. > This sets a wrong endpoint state and will cause "Get Endpoint State" > command can't get the corret endpoint state and "Set Endpoint Config" > command can't restore the correct endpoint state during hibernation > resume flow. > Thus, when endpoint.maxburst is 0, we should set burst as 0 directly. > > Signed-off-by: Zhuang Jin Can <jin.can.zhuang@intel.com> > --- > drivers/usb/dwc3/gadget.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > index 70715ee..44eca95 100644 > --- a/drivers/usb/dwc3/gadget.c > +++ b/drivers/usb/dwc3/gadget.c > @@ -440,7 +440,8 @@ static int dwc3_gadget_set_ep_config(struct dwc3 *dwc, struct dwc3_ep *dep, > > /* Burst size is only needed in SuperSpeed mode */ > if (dwc->gadget.speed == USB_SPEED_SUPER) { > - u32 burst = dep->endpoint.maxburst - 1; > + u32 burst = dep->endpoint.maxburst ? > + dep->endpoint.maxburst - 1 : 0; again, you found a bug on the gadget driver. Fix that. composite.c guarantees that for those functions which don't pass bMaxBurst, gadget->maxburst will be set to *at least* 1.
On Thu, 1 May 2014, Zhuang Jin Can wrote: > > again, you found a bug on the gadget driver. Fix that. composite.c > > guarantees that for those functions which don't pass bMaxBurst, > > gadget->maxburst will be set to *at least* 1. > > > I agree the real fix should be in the gadget driver. The patch intents > to prevent hibernatition from being corrupted by a bad gadget driver. > If OEMs develop their own gadget driver forgetting to call > config_ep_by_speed(), it'll turn out to be everything works except > dwc3 hibernation, and they'll complain to dwc3. f_ffs is an > example has SuperSpeed support but doesn't call config_ep_by_speed(). > It's just for robustness, and dwc3 is not doing anything wrong. > It did cause me a long time to figure out why the hibernation was broken. You could include the check, for the sake of robustness, in dwc3 -- but if it fails, you should write a message to the kernel log saying that the gadget driver needs to be fixed. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Thu, May 01, 2014 at 09:45:17AM -0400, Alan Stern wrote: > On Thu, 1 May 2014, Zhuang Jin Can wrote: > > > > again, you found a bug on the gadget driver. Fix that. composite.c > > > guarantees that for those functions which don't pass bMaxBurst, > > > gadget->maxburst will be set to *at least* 1. > > > > > I agree the real fix should be in the gadget driver. The patch intents > > to prevent hibernatition from being corrupted by a bad gadget driver. > > If OEMs develop their own gadget driver forgetting to call > > config_ep_by_speed(), it'll turn out to be everything works except > > dwc3 hibernation, and they'll complain to dwc3. f_ffs is an > > example has SuperSpeed support but doesn't call config_ep_by_speed(). > > It's just for robustness, and dwc3 is not doing anything wrong. > > It did cause me a long time to figure out why the hibernation was broken. > > You could include the check, for the sake of robustness, in dwc3 -- but > if it fails, you should write a message to the kernel log saying that > the gadget driver needs to be fixed. Also, if we're adding something to dwc3, we need to add to other USB3-capable UDCs too. Namely dummy and marvel's. cheers
On Wed, Apr 30, 2014 at 03:03:53PM -0500, Felipe Balbi wrote: > On Thu, May 01, 2014 at 03:16:04AM -0400, Zhuang Jin Can wrote: > > endpoint.maxburst may be 0 if a gadget doesn't call config_ep_by_speed() > > to update it from the companion descriptor. > > And endpoint.maxburst - 1 returns 11111b which wrongly sets bit > > 26 of endpoint parameter 0. > > This sets a wrong endpoint state and will cause "Get Endpoint State" > > command can't get the corret endpoint state and "Set Endpoint Config" > > command can't restore the correct endpoint state during hibernation > > resume flow. > > Thus, when endpoint.maxburst is 0, we should set burst as 0 directly. > > > > Signed-off-by: Zhuang Jin Can <jin.can.zhuang@intel.com> > > --- > > drivers/usb/dwc3/gadget.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > > index 70715ee..44eca95 100644 > > --- a/drivers/usb/dwc3/gadget.c > > +++ b/drivers/usb/dwc3/gadget.c > > @@ -440,7 +440,8 @@ static int dwc3_gadget_set_ep_config(struct dwc3 *dwc, struct dwc3_ep *dep, > > > > /* Burst size is only needed in SuperSpeed mode */ > > if (dwc->gadget.speed == USB_SPEED_SUPER) { > > - u32 burst = dep->endpoint.maxburst - 1; > > + u32 burst = dep->endpoint.maxburst ? > > + dep->endpoint.maxburst - 1 : 0; > > again, you found a bug on the gadget driver. Fix that. composite.c > guarantees that for those functions which don't pass bMaxBurst, > gadget->maxburst will be set to *at least* 1. > I agree the real fix should be in the gadget driver. The patch intents to prevent hibernatition from being corrupted by a bad gadget driver. If OEMs develop their own gadget driver forgetting to call config_ep_by_speed(), it'll turn out to be everything works except dwc3 hibernation, and they'll complain to dwc3. f_ffs is an example has SuperSpeed support but doesn't call config_ep_by_speed(). It's just for robustness, and dwc3 is not doing anything wrong. It did cause me a long time to figure out why the hibernation was broken. Thanks Jincan -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Thu, May 01, 2014 at 10:15:00AM -0500, Felipe Balbi wrote: > On Thu, May 01, 2014 at 09:45:17AM -0400, Alan Stern wrote: > > On Thu, 1 May 2014, Zhuang Jin Can wrote: > > > > again, you found a bug on the gadget driver. Fix that. composite.c > > > > guarantees that for those functions which don't pass bMaxBurst, > > > > gadget->maxburst will be set to *at least* 1. > > > > > > > I agree the real fix should be in the gadget driver. The patch intents > > > to prevent hibernatition from being corrupted by a bad gadget driver. > > > If OEMs develop their own gadget driver forgetting to call > > > config_ep_by_speed(), it'll turn out to be everything works except > > > dwc3 hibernation, and they'll complain to dwc3. f_ffs is an > > > example has SuperSpeed support but doesn't call config_ep_by_speed(). > > > It's just for robustness, and dwc3 is not doing anything wrong. > > > It did cause me a long time to figure out why the hibernation was broken. > > > > You could include the check, for the sake of robustness, in dwc3 -- but > > if it fails, you should write a message to the kernel log saying that > > the gadget driver needs to be fixed. I admit the fix is too paranoid. Thanks your comment. > > Also, if we're adding something to dwc3, we need to add to other > USB3-capable UDCs too. Namely dummy and marvel's. So I think the fix is not valuable to you. Thanks for your comment. And I'm new to communitiy, hope you can bear with me:) Jincan -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 70715ee..44eca95 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -440,7 +440,8 @@ static int dwc3_gadget_set_ep_config(struct dwc3 *dwc, struct dwc3_ep *dep, /* Burst size is only needed in SuperSpeed mode */ if (dwc->gadget.speed == USB_SPEED_SUPER) { - u32 burst = dep->endpoint.maxburst - 1; + u32 burst = dep->endpoint.maxburst ? + dep->endpoint.maxburst - 1 : 0; params.param0 |= DWC3_DEPCFG_BURST_SIZE(burst); }
endpoint.maxburst may be 0 if a gadget doesn't call config_ep_by_speed() to update it from the companion descriptor. And endpoint.maxburst - 1 returns 11111b which wrongly sets bit 26 of endpoint parameter 0. This sets a wrong endpoint state and will cause "Get Endpoint State" command can't get the corret endpoint state and "Set Endpoint Config" command can't restore the correct endpoint state during hibernation resume flow. Thus, when endpoint.maxburst is 0, we should set burst as 0 directly. Signed-off-by: Zhuang Jin Can <jin.can.zhuang@intel.com> --- drivers/usb/dwc3/gadget.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)