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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxx>
>> ---
>>  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.
> 

-- 
cheers,
-roger

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux