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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
