usb: gadget: composite: add function to increment delayed_status
PATCH 2/4 usb: gadget: composite: add function to increment delayed_status
Hi Alan,
Quoted 16 lines · On Friday, 20 April 2018 17:09:57 EEST Alan Stern wrote:
Quoted 14 lines · Laurent Pinchart
According to
include/linux/usb/composite.h, thedelayed_statusfield should be protected bycdev->lock, which you should use here.I've read through the code and found out that, while all callers of
reset_config(), as well asusb_composite_setup_continue(), correctly take the lock, it isn't taken aroundf->set_alt()incomposite_setup(). This causes the race condition. I wonder if a simpler fix wouldn't be to take the lock before callingf->set_alt()and releasing it after incrementingdelayed_status. I am however worried that this could lead to deadlocks if one of the existingset_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.
Quoted 2 lines
set_althandlers 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 ?
intcomposite_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);
...
}