[2/4] usb: gadget: composite: add function to increment delayed_status

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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);
	...
}