usb: gadget: composite: add function to increment delayed_status
PATCH 2/4 usb: gadget: composite: add function to increment delayed_status
Hi Paul,
(CC'ing Felipe Balbi and Roger Quadros)
Thank you for the patch.
Have you used scripts/get_maintainer.pl ? It should point you to Felipe Balbi, the maintainer of the USB gadget subsystem, who I recommend you CC, at least on patch 2/4. The script also points to Greg, who I don't think needs to be CC'ed as he doesn't deal with USB gadget as much as with USB in general, and who is fairly busy as usual.
While the get_maintainer script doesn't point to Roger, his name can be found as the author of the USB_GADGET_DELAYED_STATUS mechanism. He authored commit 1b9ba000177e ("usb: gadget: composite: Allow function drivers to pause control transfers") with his nokia.com address back then, but git log --author 'Roger Quadros' shows that he's still active on USB and working for TI now. I've thus CC'ed him on this reply.
Quoted 7 lines · On Wednesday, 18 April 2018 06:18:14 EEST Paul Elder wrote:
The completion of the usb status phase from
uvc_function_set_altneeds to be delayed until uvc_v4l2_streamon/off. This is currently done byuvc_function_set_altreturningUSB_GADGET_DELAYED_STATUSandcomposite_setupdetecting this to incrementcdev->delayed_status. However, if uvc_v4l2_streamon/off is called in between this return and increment,uvc_function_setup_continuewithin uvc_v4l2_streamon/off will WARN thatcdev->delayed_statusis zero.
While this is correct, I wouldn't mention UVC here as the patch is for the USB composite gadget framework and isn't specific to UVC. You should write a more generic explanation of the problem to explain why the race between returning USB_GADGET_DELAYED_STATUS (and processing it in the caller) and calling usb_composite_setup_continue() can't be properly solved in gadget drivers, thus requiring a new function.
Quoted 14 lines
To fix situations like this, add a function to increment
cdev->delayed_status.Signed-off-by Paul Elder <[email protected]>--- drivers/usb/gadget/composite.c | 6 ++++++ include/linux/usb/composite.h | 2 ++ 2 files changed, 8 insertions(+)drivers/usb/gadget/composite.c +0
-0
drivers/usb/gadget/composite.c static int fill_ext_prop(struct usb_configuration *c, · line 1553
Quoted 6 lines
drivers/usb/gadget/composite.c +3
-0@@ -1548,6 +1548,12 @@ static int fill_ext_prop(struct usb_configuration *c, int interface, u8 *buf) return 0; } +void usb_composite_setup_delay (struct usb_composite_dev *cdev) +{ + cdev->delayed_status++;
According to include/linux/usb/composite.h, the delayed_status field should be protected by cdev->lock, which you should use here.
I've read through the code and found out that, while all callers of reset_config(), as well as usb_composite_setup_continue(), correctly take the lock, it isn't taken around f->set_alt() in composite_setup(). This causes the race condition. I wonder if a simpler fix wouldn't be to take the lock before calling f->set_alt() and releasing it after incrementing delayed_status. I am however worried that this could lead to deadlocks if one of the existing set_alt() handlers calls a function that takes the same lock. Another worry is that some of the .set_alt() handlers might not expect to be called with interrupts disabled. This should be analyzed, and I hope that Roger and/or Felipe will have some insight on this.
If usb_composite_setup_delay() turns out to be the preferred solution, it would be nice to document the function with kerneldoc.
Finally, I just came to wonder whether the UVC gadget driver really needs to defer the status phase of the SET_INTERFACE request, or if it couldn't just proceed normally.
Quoted 19 lines
drivers/usb/gadget/composite.c +3
-0+} +EXPORT_SYMBOL (usb_composite_setup_delay); +/* * The setup() callback implements all the ep0 functionality that's * not handled lower down, in hardware or the hardware driver(like include/linux/usb/composite.h +2
-0@@ -524,6 +524,8 @@ extern int composite_setup(struct usb_gadget *gadget,extern void composite_suspend (struct usb_gadget *gadget);extern void composite_resume (struct usb_gadget *gadget); +extern void usb_composite_setup_delay (struct usb_composite_dev *c); +/* * Some systems will need runtime overrides for the product identifiers * published in the device descriptor, either numbers or strings or both.