Re: [PATCH] usb: dwc3: gadget: fix burst size corruption

Re: [PATCH] usb: dwc3: gadget: fix burst size corruption

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1465 bytes --]

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.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] usb: dwc3: gadget: fix burst size corruption

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1465 bytes --]

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.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH] usb: dwc3: gadget: fix burst size corruption

[Zhuang Jin Can]

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;
 
 		params.param0 |= DWC3_DEPCFG_BURST_SIZE(burst);
 	}
-- 
1.7.9.5

Re: [PATCH] usb: dwc3: gadget: fix burst size corruption

[Alan Stern]

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

Re: [PATCH] usb: dwc3: gadget: fix burst size corruption

[Alan Stern]

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

Re: [PATCH] usb: dwc3: gadget: fix burst size corruption

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1233 bytes --]

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

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] usb: dwc3: gadget: fix burst size corruption

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1233 bytes --]

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

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] usb: dwc3: gadget: fix burst size corruption

[Zhuang Jin Can]

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

Re: [PATCH] usb: dwc3: gadget: fix burst size corruption

[Zhuang Jin Can]

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