Skip to contentSkip to conversation

usb: dwc3: gadget: fix burst size corruption

PATCH In review Zhuang Jin Can 3 participants 6 messages Apr 30, 2014 – May 2, 2014

Subsystems (2)
USB DesignWare USB3 DRD IP driver
lore
Share & tools
lore

Zhuang Jin Can

author
#0 lore raw

PATCH 1/1 usb: dwc3: gadget: fix burst size corruption

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 <[email protected]>
---
 drivers/usb/dwc3/gadget.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/usb/dwc3/gadget.c +2 -1
index 70715ee..44eca95 100644
@@ -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);
 	}
Signature · 1 line
1.7.9.5
#1 lore raw
Quoted 20 lines · 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 <[email protected]>
---
 drivers/usb/dwc3/gadget.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/usb/dwc3/gadget.c +0 -0
index 70715ee..44eca95 100644

drivers/usb/dwc3/gadget.c static int dwc3_gadget_set_ep_config(struct dwc3 *dwc, struct dwc3_ep *dep, · line 444

Quoted 6 lines
drivers/usb/dwc3/gadget.c +2 -1
@@ -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.

Signature · 1 line
balbi
author
#2 lore raw
Quoted 22 lines · Felipe Balbi
Quoted 21 lines · 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 <[email protected]>
---
 drivers/usb/dwc3/gadget.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/usb/dwc3/gadget.c +0 -0
index 70715ee..44eca95 100644
@@ -440,7 +440,8 @@ static int dwc3_gadget_set_ep_config(struct dwc3 *dwc, struct dwc3_ep *dep,
Quoted 10 lines

drivers/usb/dwc3/gadget.c static int dwc3_gadget_set_ep_config(struct dwc3 *dwc, struct dwc3_ep *dep, · line 444

Quoted 5 lines
drivers/usb/dwc3/gadget.c +2 -1
 	/* 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

#3 lore raw
Quoted 4 lines · Zhuang Jin Can
Quoted 4 lines

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.

Quoted 8 lines

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

#4 lore raw

Hi,

Quoted 15 lines · Alan Stern
Quoted 4 lines · Zhuang Jin Can
Quoted 4 lines

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.

Quoted 8 lines

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.

Quoted 3 lines

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

Signature · 1 line
balbi
author
#5 lore raw

Hi,

Quoted 15 lines · Felipe Balbi
Quoted 14 lines · Alan Stern
Quoted 12 lines · Zhuang Jin Can
Quoted 4 lines

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.

Quoted 3 lines
Quoted 3 lines

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.

Quoted 3 lines

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