[PATCH 2/4] USB: UDC: Implement udc_async_callbacks in dummy-hcd

[PATCH 2/4] USB: UDC: Implement udc_async_callbacks in dummy-hcd

[Alan Stern]

This patch adds a udc_async_callbacks handler to the dummy-hcd UDC
driver, which will prevent a theoretical race during gadget unbinding.

The implementation is simple, since dummy-hcd already has a flag to
keep track of whether emulated IRQs are enabled.  All the handler has
to do is store the enable value in the flag, and avoid setting the
flag prematurely.

Signed-off-by: Alan Stern <stern@rowland.harvard.edu>

---


[as1957]


 drivers/usb/gadget/udc/dummy_hcd.c |   11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

Index: usb-devel/drivers/usb/gadget/udc/dummy_hcd.c
===================================================================
--- usb-devel.orig/drivers/usb/gadget/udc/dummy_hcd.c
+++ usb-devel/drivers/usb/gadget/udc/dummy_hcd.c
@@ -919,6 +919,15 @@ static void dummy_udc_set_speed(struct u
 	dummy_udc_update_ep0(dum);
 }
 
+static void dummy_udc_async_callbacks(struct usb_gadget *_gadget, bool enable)
+{
+	struct dummy	*dum = gadget_dev_to_dummy(&_gadget->dev);
+
+	spin_lock_irq(&dum->lock);
+	dum->ints_enabled = enable;
+	spin_unlock_irq(&dum->lock);
+}
+
 static int dummy_udc_start(struct usb_gadget *g,
 		struct usb_gadget_driver *driver);
 static int dummy_udc_stop(struct usb_gadget *g);
@@ -931,6 +940,7 @@ static const struct usb_gadget_ops dummy
 	.udc_start	= dummy_udc_start,
 	.udc_stop	= dummy_udc_stop,
 	.udc_set_speed	= dummy_udc_set_speed,
+	.udc_async_callbacks = dummy_udc_async_callbacks,
 };
 
 /*-------------------------------------------------------------------------*/
@@ -990,7 +1000,6 @@ static int dummy_udc_start(struct usb_ga
 	spin_lock_irq(&dum->lock);
 	dum->devstatus = 0;
 	dum->driver = driver;
-	dum->ints_enabled = 1;
 	spin_unlock_irq(&dum->lock);
 
 	return 0;

Re: [PATCH 2/4] USB: UDC: Implement udc_async_callbacks in dummy-hcd

[Felipe Balbi]

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


Hi,

Alan Stern <stern@rowland.harvard.edu> writes:
> @@ -990,7 +1000,6 @@ static int dummy_udc_start(struct usb_ga
>  	spin_lock_irq(&dum->lock);
>  	dum->devstatus = 0;
>  	dum->driver = driver;
> -	dum->ints_enabled = 1;

should the matching write of 0 be removed from dummy_udc_stop()?

Other than that:

Acked-by: Felipe Balbi <balbi@kernel.org>

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 511 bytes --]

Re: [PATCH 2/4] USB: UDC: Implement udc_async_callbacks in dummy-hcd

[Alan Stern]

On Fri, Jun 04, 2021 at 08:21:11AM +0300, Felipe Balbi wrote:
> 
> Hi,
> 
> Alan Stern <stern@rowland.harvard.edu> writes:
> > @@ -990,7 +1000,6 @@ static int dummy_udc_start(struct usb_ga
> >  	spin_lock_irq(&dum->lock);
> >  	dum->devstatus = 0;
> >  	dum->driver = driver;
> > -	dum->ints_enabled = 1;
> 
> should the matching write of 0 be removed from dummy_udc_stop()?

No, it's okay to leave that one.  In practice it won't make any 
difference because now the core will always turn off async callbacks 
before doing udc_stop.  It's there for the sake of thoroughness, and it 
lets the reader know that emulated interrupts are supposed to be turned 
off whenever the UDC stops running (just like a driver for a real UDC).

Whereas this line here in dummy_udc_start would be actively wrong if it 
were to remain.

Alan Stern

> Other than that:
> 
> Acked-by: Felipe Balbi <balbi@kernel.org>
> 
> -- 
> balbi

Re: [PATCH 2/4] USB: UDC: Implement udc_async_callbacks in dummy-hcd

[Felipe Balbi]

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

Alan Stern <stern@rowland.harvard.edu> writes:

> On Fri, Jun 04, 2021 at 08:21:11AM +0300, Felipe Balbi wrote:
>> 
>> Hi,
>> 
>> Alan Stern <stern@rowland.harvard.edu> writes:
>> > @@ -990,7 +1000,6 @@ static int dummy_udc_start(struct usb_ga
>> >  	spin_lock_irq(&dum->lock);
>> >  	dum->devstatus = 0;
>> >  	dum->driver = driver;
>> > -	dum->ints_enabled = 1;
>> 
>> should the matching write of 0 be removed from dummy_udc_stop()?
>
> No, it's okay to leave that one.  In practice it won't make any 
> difference because now the core will always turn off async callbacks 
> before doing udc_stop.  It's there for the sake of thoroughness, and it 
> lets the reader know that emulated interrupts are supposed to be turned 
> off whenever the UDC stops running (just like a driver for a real UDC).
>
> Whereas this line here in dummy_udc_start would be actively wrong if it 
> were to remain.

fair enough :-) I see Greg took the series already ;-) Thanks for
working on this, again.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 511 bytes --]