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
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: stable@vger.kernel.org Fixes: b0d7ffd44ba9 ("usb: dwc3: gadget: don't request IRQs in atomic") Signed-off-by: Thinh Nguyen Thinh.Nguyen@synopsys.com --- drivers/usb/dwc3/gadget.c | 24 ++++++++---------------- 1 file changed, 8 insertions(+), 16 deletions(-)
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 25f654b79e48..51d81a32ce78 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -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)
Hi,
Thinh Nguyen Thinh.Nguyen@synopsys.com writes:
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.
Hi,
Felipe Balbi wrote:
Hi,
Thinh Nguyen Thinh.Nguyen@synopsys.com writes:
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.
Sure. We can do that.
Thanks, Thinh
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: stable@vger.kernel.org Fixes: 8698e2acf3a5 ("usb: dwc3: gadget: introduce and use enable/disable irq methods") Signed-off-by: Thinh Nguyen Thinh.Nguyen@synopsys.com --- drivers/usb/dwc3/gadget.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 51d81a32ce78..9ec70282e610 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -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);
Hi,
Thinh Nguyen Thinh.Nguyen@synopsys.com writes:
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: stable@vger.kernel.org Fixes: 8698e2acf3a5 ("usb: dwc3: gadget: introduce and use enable/disable irq methods") Signed-off-by: Thinh Nguyen Thinh.Nguyen@synopsys.com
drivers/usb/dwc3/gadget.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 51d81a32ce78..9ec70282e610 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -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
On 21-01-05 08:56:28, Thinh Nguyen wrote:
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
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
2.28.0
Hi Peter,
Peter Chen wrote:
On 21-01-05 08:56:28, Thinh Nguyen wrote:
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
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
2.28.0
On 21-01-08 02:40:55, Thinh Nguyen wrote:
Hi Peter,
Peter Chen wrote:
On 21-01-05 08:56:28, Thinh Nguyen wrote:
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
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.
linux-stable-mirror@lists.linaro.org
-
Felipe Balbi -
Peter Chen -
Thinh Nguyen