| Message ID | bdaa01e16c9a71ad16ea7657e2c6ad546306f2df.1524011428.git.paul.elder@pitt.edu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
On Wednesday, 18 April 2018 06:18:14 EEST Paul Elder wrote:
> The completion of the usb status phase from uvc_function_set_alt needs
> to be delayed until uvc_v4l2_streamon/off. This is currently done by
> uvc_function_set_alt returning USB_GADGET_DELAYED_STATUS and
> composite_setup detecting this to increment cdev->delayed_status.
> However, if uvc_v4l2_streamon/off is called in between this return and
> increment, uvc_function_setup_continue within uvc_v4l2_streamon/off will
> WARN that cdev->delayed_status is 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.
> To fix situations like this, add a function to increment
> cdev->delayed_status.
>
> Signed-off-by: Paul Elder <paul.elder@pitt.edu>
> ---
> drivers/usb/gadget/composite.c | 6 ++++++
> include/linux/usb/composite.h | 2 ++
> 2 files changed, 8 insertions(+)
>
> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
> index 77c7ecca816a..c02ab640a7ae 100644
> --- a/drivers/usb/gadget/composite.c
> +++ b/drivers/usb/gadget/composite.c
> @@ -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.
> +}
> +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
> diff --git a/include/linux/usb/composite.h b/include/linux/usb/composite.h
> index cef0e44601f8..049f77a4d42b 100644
> --- a/include/linux/usb/composite.h
> +++ b/include/linux/usb/composite.h
> @@ -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.
On Thu, 19 Apr 2018, Laurent Pinchart wrote: > 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. set_alt handlers generally have to disable and enable endpoints, which requires a process context. They cannot run with interrupts disabled. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 19/04/18 22:42, Laurent Pinchart wrote: > 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. > > On Wednesday, 18 April 2018 06:18:14 EEST Paul Elder wrote: >> The completion of the usb status phase from uvc_function_set_alt needs >> to be delayed until uvc_v4l2_streamon/off. This is currently done by >> uvc_function_set_alt returning USB_GADGET_DELAYED_STATUS and >> composite_setup detecting this to increment cdev->delayed_status. >> However, if uvc_v4l2_streamon/off is called in between this return and >> increment, uvc_function_setup_continue within uvc_v4l2_streamon/off will >> WARN that cdev->delayed_status is 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. > >> To fix situations like this, add a function to increment >> cdev->delayed_status. >> >> Signed-off-by: Paul Elder <paul.elder@pitt.edu> >> --- >> drivers/usb/gadget/composite.c | 6 ++++++ >> include/linux/usb/composite.h | 2 ++ >> 2 files changed, 8 insertions(+) >> >> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c >> index 77c7ecca816a..c02ab640a7ae 100644 >> --- a/drivers/usb/gadget/composite.c >> +++ b/drivers/usb/gadget/composite.c >> @@ -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. Yes, I too think it is necessary to serialize .set_alt() and usb_composite_setup_continue(). > > 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. Might be worth a try :) > >> +} >> +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 >> diff --git a/include/linux/usb/composite.h b/include/linux/usb/composite.h >> index cef0e44601f8..049f77a4d42b 100644 >> --- a/include/linux/usb/composite.h >> +++ b/include/linux/usb/composite.h >> @@ -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. >
Hi Alan, On Friday, 20 April 2018 17:09:57 EEST Alan Stern wrote: > On Thu, 19 Apr 2018, Laurent Pinchart wrote: > > 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. > > set_alt handlers generally have to disable and enable endpoints, which > requires a process context. They cannot run with interrupts disabled. Thank you for the information. Isn't the following code path problematic then ? int composite_setup(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl) { ... case USB_REQ_SET_CONFIGURATION: ... spin_lock(&cdev->lock); value = set_config(cdev, ctrl, w_value); spin_unlock(&cdev->lock); ... } static int set_config(struct usb_composite_dev *cdev, const struct usb_ctrlrequest *ctrl, unsigned number) { ... /* Initialize all interfaces by setting them to altsetting zero. */ for (tmp = 0; tmp < MAX_CONFIG_INTERFACES; tmp++) { ... result = f->set_alt(f, tmp, 0); ... }
On Sat, 21 Apr 2018, Laurent Pinchart wrote: > Hi Alan, > > On Friday, 20 April 2018 17:09:57 EEST Alan Stern wrote: > > On Thu, 19 Apr 2018, Laurent Pinchart wrote: > > > 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. > > > > set_alt handlers generally have to disable and enable endpoints, which > > requires a process context. They cannot run with interrupts disabled. > > Thank you for the information. Isn't the following code path problematic then > ? > > int > composite_setup(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl) > { > ... > case USB_REQ_SET_CONFIGURATION: > ... > spin_lock(&cdev->lock); > value = set_config(cdev, ctrl, w_value); > spin_unlock(&cdev->lock); > ... > } > > static int set_config(struct usb_composite_dev *cdev, > const struct usb_ctrlrequest *ctrl, unsigned number) > { > ... > /* Initialize all interfaces by setting them to altsetting zero. */ > for (tmp = 0; tmp < MAX_CONFIG_INTERFACES; tmp++) { > ... > result = f->set_alt(f, tmp, 0); > ... > } Ah, no, this was a mistake on my part. I should have said that set_alt handlers _can_ be called in an atomic context, but they generally require a process context to carry out their work. That is why they return DELAYED_STATUS; they need to defer a large part of their activities to a work queue or something similar. Sorry for the confusion. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c index 77c7ecca816a..c02ab640a7ae 100644 --- a/drivers/usb/gadget/composite.c +++ b/drivers/usb/gadget/composite.c @@ -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++; +} +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 diff --git a/include/linux/usb/composite.h b/include/linux/usb/composite.h index cef0e44601f8..049f77a4d42b 100644 --- a/include/linux/usb/composite.h +++ b/include/linux/usb/composite.h @@ -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.
The completion of the usb status phase from uvc_function_set_alt needs to be delayed until uvc_v4l2_streamon/off. This is currently done by uvc_function_set_alt returning USB_GADGET_DELAYED_STATUS and composite_setup detecting this to increment cdev->delayed_status. However, if uvc_v4l2_streamon/off is called in between this return and increment, uvc_function_setup_continue within uvc_v4l2_streamon/off will WARN that cdev->delayed_status is zero. To fix situations like this, add a function to increment cdev->delayed_status. Signed-off-by: Paul Elder <paul.elder@pitt.edu> --- drivers/usb/gadget/composite.c | 6 ++++++ include/linux/usb/composite.h | 2 ++ 2 files changed, 8 insertions(+)