* [2/4] usb: gadget: composite: add function to increment delayed_status
@ 2018-04-20 14:09 Alan Stern
0 siblings, 0 replies; 6+ messages in thread
From: Alan Stern @ 2018-04-20 14:09 UTC (permalink / raw)
To: Laurent Pinchart; +Cc: Paul Elder, linux-usb, Roger Quadros, Felipe Balbi
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [2/4] usb: gadget: composite: add function to increment delayed_status
@ 2018-04-21 15:38 Alan Stern
0 siblings, 0 replies; 6+ messages in thread
From: Alan Stern @ 2018-04-21 15:38 UTC (permalink / raw)
To: Laurent Pinchart; +Cc: Paul Elder, linux-usb, Roger Quadros, Felipe Balbi
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [2/4] usb: gadget: composite: add function to increment delayed_status
@ 2018-04-21 11:00 Laurent Pinchart
0 siblings, 0 replies; 6+ messages in thread
From: Laurent Pinchart @ 2018-04-21 11:00 UTC (permalink / raw)
To: Alan Stern; +Cc: Paul Elder, linux-usb, Roger Quadros, Felipe Balbi
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);
...
}
^ permalink raw reply [flat|nested] 6+ messages in thread
* [2/4] usb: gadget: composite: add function to increment delayed_status
@ 2018-04-20 14:51 Roger Quadros
0 siblings, 0 replies; 6+ messages in thread
From: Roger Quadros @ 2018-04-20 14:51 UTC (permalink / raw)
To: Laurent Pinchart, Paul Elder; +Cc: linux-usb, Felipe Balbi
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.
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [2/4] usb: gadget: composite: add function to increment delayed_status
@ 2018-04-19 19:42 Laurent Pinchart
0 siblings, 0 replies; 6+ messages in thread
From: Laurent Pinchart @ 2018-04-19 19:42 UTC (permalink / raw)
To: Paul Elder; +Cc: linux-usb, Roger Quadros, Felipe Balbi
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [2/4] usb: gadget: composite: add function to increment delayed_status
@ 2018-04-18 3:18 Paul Elder
0 siblings, 0 replies; 6+ messages in thread
From: Paul Elder @ 2018-04-18 3:18 UTC (permalink / raw)
To: linux-usb; +Cc: Paul Elder, laurent.pinchart
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(+)
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.
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-04-21 15:38 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-20 14:09 [2/4] usb: gadget: composite: add function to increment delayed_status Alan Stern
-- strict thread matches above, loose matches on Subject: below --
2018-04-21 15:38 Alan Stern
2018-04-21 11:00 Laurent Pinchart
2018-04-20 14:51 Roger Quadros
2018-04-19 19:42 Laurent Pinchart
2018-04-18 3:18 Paul Elder
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.