| Message ID | 20210520202152.GD1216852@rowland.harvard.edu |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
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
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
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
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;
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(-)