[PATCH 0/2] usb: dwc3: gadget: Check for multiple start/stop

[PATCH 0/2] usb: dwc3: gadget: Check for multiple start/stop

[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.



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

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

[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.

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)
-- 
2.28.0

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

[Felipe Balbi]

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.

-- 
balbi

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

[Thinh Nguyen]

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

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

[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: 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);
-- 
2.28.0

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

[Felipe Balbi]

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

-- 
balbi

Re: [PATCH 0/2] usb: dwc3: gadget: Check for multiple start/stop

[Peter Chen]

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
> 

-- 

Thanks,
Peter Chen

Re: [PATCH 0/2] usb: dwc3: gadget: Check for multiple start/stop

[Thinh Nguyen]

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
>>

Re: [PATCH 0/2] usb: dwc3: gadget: Check for multiple start/stop

[Peter Chen]

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.

-- 

Thanks,
Peter Chen