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

Message ID 20210520202152.GD1216852@rowland.harvard.edu
State New
Headers show
Series
  • Untitled series #129677
Related show

Commit Message

Alan Stern May 20, 2021, 8:21 p.m.
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(-)

Comments

Felipe Balbi June 4, 2021, 5:21 a.m. | #1
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
Alan Stern June 4, 2021, 2:15 p.m. | #2
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
Felipe Balbi June 4, 2021, 2:20 p.m. | #3
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

Patch

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;