usb: dwc3: gadget: Check for multiple start/stop

PATCH In review Thinh Nguyen 3 participants 9 messages Jan 5, 2021 – Jan 8, 2021

Subsystems (3)

Stable branch USB DesignWare USB3 DRD IP driver

lore

Share & tools

lore

Thinh Nguyen

author maintainer

2021-01-05 16:57 UTC #0 lore raw

cover usb: dwc3: gadget: Check for multiple start/stop

Add some checks to avoid going through the start/stop sequence if the gadget had already started/stopped. This series base-commit is Greg's usb-linus branch.

Thinh Nguyen (2):
usb: dwc3: gadget: Check if the gadget had started
usb: dwc3: gadget: Check if the gadget had stopped

 drivers/usb/dwc3/gadget.c | 28 ++++++++++++----------------
 1 file changed, 12 insertions(+), 16 deletions(-)

base-commit: 96ebc9c871d8a28fb22aa758dd9188a4732df482

Signature · 1 line

2.28.0

Thinh Nguyen

author maintainer

2021-01-05 16:57 UTC #1 lore raw

PATCH 1/2 usb: dwc3: gadget: Check if the gadget had started

If the gadget had already started, don't try to start again. Otherwise, we may request the same threaded irq with the same dev_id, it will mess up the interrupt freeing logic. This can happen if a user tries to trigger a soft-connect from soft_connect sysfs multiple times. Check to make sure that the gadget had started before proceeding to request threaded irq. Fix this by checking if there's bounded gadget driver.

Cc [email protected]

Fixes b0d7ffd44ba9 ("usb: dwc3: gadget: don't request IRQs in atomic")

Signed-off-by Thinh Nguyen <[email protected]>

---
 drivers/usb/dwc3/gadget.c | 24 ++++++++----------------
 1 file changed, 8 insertions(+), 16 deletions(-)

drivers/usb/dwc3/gadget.c +8 ~~-16~~

index 25f654b79e48..51d81a32ce78 100644
@@ -2303,35 +2303,27 @@ static int dwc3_gadget_start(struct usb_gadget *g,
 	int			ret = 0;
 	int			irq;
 
+	if (dwc->gadget_driver) {
+		dev_err(dwc->dev, "%s is already bound to %s\n",
+				dwc->gadget->name,
+				dwc->gadget_driver->driver.name);
+		return -EBUSY;
+	}
+
 	irq = dwc->irq_gadget;
 	ret = request_threaded_irq(irq, dwc3_interrupt, dwc3_thread_interrupt,
 			IRQF_SHARED, "dwc3", dwc->ev_buf);
 	if (ret) {
 		dev_err(dwc->dev, "failed to request irq #%d --> %d\n",
 				irq, ret);
-		goto err0;
+		return ret;
 	}
 
 	spin_lock_irqsave(&dwc->lock, flags);
-	if (dwc->gadget_driver) {
-		dev_err(dwc->dev, "%s is already bound to %s\n",
-				dwc->gadget->name,
-				dwc->gadget_driver->driver.name);
-		ret = -EBUSY;
-		goto err1;
-	}
-
 	dwc->gadget_driver	= driver;
 	spin_unlock_irqrestore(&dwc->lock, flags);
 
 	return 0;
-
-err1:
-	spin_unlock_irqrestore(&dwc->lock, flags);
-	free_irq(irq, dwc);
-
-err0:
-	return ret;
 }
 
 static void __dwc3_gadget_stop(struct dwc3 *dwc)

Signature · 1 line

2.28.0

Felipe Balbi

2021-01-06 07:54 UTC #2 lore raw

Re: [PATCH 1/2] usb: dwc3: gadget: Check if the gadget had started

Hi,

Quoted 6 lines · Thinh Nguyen

If the gadget had already started, don't try to start again. Otherwise, we may request the same threaded irq with the same dev_id, it will mess up the interrupt freeing logic. This can happen if a user tries to trigger a soft-connect from soft_connect sysfs multiple times. Check to make sure that the gadget had started before proceeding to request threaded irq. Fix this by checking if there's bounded gadget driver.

Looks like this should be fixed at the framework level, otherwise we will have to patch every single UDC. After that is done, we can remove the dwc->gadget_driver check from here.

Signature · 1 line

balbi

Thinh Nguyen

author maintainer

2021-01-06 09:36 UTC #3 lore raw

Re: [PATCH 1/2] usb: dwc3: gadget: Check if the gadget had started

Hi,

Quoted 9 lines · Felipe Balbi

Hi,
Quoted 6 lines · Thinh Nguyen
If the gadget had already started, don't try to start again. Otherwise, we may request the same threaded irq with the same dev_id, it will mess up the interrupt freeing logic. This can happen if a user tries to trigger a soft-connect from soft_connect sysfs multiple times. Check to make sure that the gadget had started before proceeding to request threaded irq. Fix this by checking if there's bounded gadget driver.

Quoted 4 lines

Looks like this should be fixed at the framework level, otherwise we will have to patch every single UDC. After that is done, we can remove the dwc->gadget_driver check from here.

Sure. We can do that.

Thanks,
Thinh

Thinh Nguyen

author maintainer

2021-01-05 16:57 UTC #4 lore raw

PATCH 2/2 usb: dwc3: gadget: Check if the gadget had stopped

In reply to Thinh Nguyen

If the gadget had already stopped, don't try to stop again. Otherwise we'd get a warning for trying to free an already freed irq. This can happen if a user tries to trigger a soft-disconnect from soft_connect sysfs multiple times. The fix is to check if there's a bounded gadget driver to determined if the gadget had stopped.

Cc [email protected]

Fixes 8698e2acf3a5 ("usb: dwc3: gadget: introduce and use enable/disable irq methods")

Signed-off-by Thinh Nguyen <[email protected]>

---
 drivers/usb/dwc3/gadget.c | 4 ++++
 1 file changed, 4 insertions(+)

drivers/usb/dwc3/gadget.c +4 -0

index 51d81a32ce78..9ec70282e610 100644
@@ -2338,6 +2338,10 @@ static int dwc3_gadget_stop(struct usb_gadget *g)
 	struct dwc3		*dwc = gadget_to_dwc(g);
 	unsigned long		flags;
 
+	/* The controller is already stopped if there's no gadget driver */
+	if (!dwc->gadget_driver)
+		return 0;
+
 	spin_lock_irqsave(&dwc->lock, flags);
 	dwc->gadget_driver	= NULL;
 	spin_unlock_irqrestore(&dwc->lock, flags);

Signature · 1 line

2.28.0

Felipe Balbi

2021-01-06 07:54 UTC #5 lore raw

Re: [PATCH 2/2] usb: dwc3: gadget: Check if the gadget had stopped

Hi,

Quoted 18 lines · Thinh Nguyen

If the gadget had already stopped, don't try to stop again. Otherwise we'd get a warning for trying to free an already freed irq. This can happen if a user tries to trigger a soft-disconnect from soft_connect sysfs multiple times. The fix is to check if there's a bounded gadget driver to determined if the gadget had stopped.
Cc [email protected]
Fixes 8698e2acf3a5 ("usb: dwc3: gadget: introduce and use enable/disable irq methods")
Signed-off-by Thinh Nguyen <[email protected]>
---
 drivers/usb/dwc3/gadget.c | 4 ++++
 1 file changed, 4 insertions(+)
drivers/usb/dwc3/gadget.c +0 -0
index 51d81a32ce78..9ec70282e610 100644

drivers/usb/dwc3/gadget.c static int dwc3_gadget_stop(struct usb_gadget *g) · line 2343

Quoted 6 lines

drivers/usb/dwc3/gadget.c +3 -0

@@ -2338,6 +2338,10 @@ static int dwc3_gadget_stop(struct usb_gadget *g)
 	struct dwc3		*dwc = gadget_to_dwc(g);
 	unsigned long		flags;
 
+	/* The controller is already stopped if there's no gadget driver */
+	if (!dwc->gadget_driver)
+		return 0;

same here. Better done at the framework

Signature · 1 line

balbi

Peter Chen

2021-01-08 02:37 UTC #6 lore raw

In reply to Thinh Nguyen

Quoted 4 lines · Thinh Nguyen

Add some checks to avoid going through the start/stop sequence if the gadget had already started/stopped. This series base-commit is Greg's usb-linus branch.

Hi Thinh,

What's the sequence your could reproduce it?

Peter

Quoted 14 lines

Thinh Nguyen (2):
usb: dwc3: gadget: Check if the gadget had started
usb: dwc3: gadget: Check if the gadget had stopped
 drivers/usb/dwc3/gadget.c | 28 ++++++++++++----------------
 1 file changed, 12 insertions(+), 16 deletions(-)
base-commit: 96ebc9c871d8a28fb22aa758dd9188a4732df482
Signature · 1 line
2.28.0

Signature · 2 lines

Thanks,
Peter Chen

Thinh Nguyen

author maintainer

2021-01-08 02:42 UTC #7 lore raw

Hi Peter,

Quoted 10 lines · Peter Chen

Quoted 4 lines · Thinh Nguyen
Add some checks to avoid going through the start/stop sequence if the gadget had already started/stopped. This series base-commit is Greg's usb-linus branch.
Hi Thinh,
What's the sequence your could reproduce it?
Peter

You can test as follow:

# echo connect > /sys/class/udc/<UDC>/soft_connect
# echo connect > /sys/class/udc/<UDC>/soft_connect

and

# echo disconnect > /sys/class/udc/<UDC>/soft_connect
# echo disconnect > /sys/class/udc/<UDC>/soft_connect

Thinh

Quoted 13 lines

Quoted 13 lines
Thinh Nguyen (2):
usb: dwc3: gadget: Check if the gadget had started
usb: dwc3: gadget: Check if the gadget had stopped
 drivers/usb/dwc3/gadget.c | 28 ++++++++++++----------------
 1 file changed, 12 insertions(+), 16 deletions(-)
base-commit: 96ebc9c871d8a28fb22aa758dd9188a4732df482
Signature · 1 line
2.28.0

Peter Chen

2021-01-08 09:41 UTC #8 lore raw

Quoted 19 lines · Thinh Nguyen

Hi Peter,
Quoted 10 lines · Peter Chen
Quoted 4 lines · Thinh Nguyen
Add some checks to avoid going through the start/stop sequence if the gadget had already started/stopped. This series base-commit is Greg's usb-linus branch.
Hi Thinh,
What's the sequence your could reproduce it?
Peter
You can test as follow:
# echo connect > /sys/class/udc/<UDC>/soft_connect
# echo connect > /sys/class/udc/<UDC>/soft_connect

Quoted 7 lines

and

# echo disconnect > /sys/class/udc/<UDC>/soft_connect
# echo disconnect > /sys/class/udc/<UDC>/soft_connect

Thinh

Thanks, now I reproduce the issue. Another improvement you might consider adding is checking return value for usb_gadget_udc_start at soft_connect_store.

Signature · 2 lines

Thanks,
Peter Chen