Re: [PATCH 3/3] usb: gadget: uvc: Allow disabling of interrupt endpoint

Dan Scally
2023-01-01T20:46:22Z

Hi Laurent

On 28/12/2022 02:09, Laurent Pinchart wrote:
> Hi Dan,
>
> Thank you for the patch.
>
> On Mon, Dec 05, 2022 at 02:37:58PM +0000, Daniel Scally wrote:
>> The f_uvc code includes an interrupt endpoint against the VideoControl
>> interface. According to section 2.4.2 of the UVC specification however
>> this endpoint is optional in at least some cases:
>>
>> "This endpoint is optional, but may be mandatory under certain
>> conditions"
>>
>> The conditions enumerated are whether...
>>
>> 1. The device supports hardware triggers
>> 2. The device implements any AutoUpdate controls
>> 3. The device implements any Asynchronous controls
>>
>> As all of those things are implementation dependent, this endpoint
>> might be unnecessary for some users. Check whether the user has
>> requested it be disable via configfs and don't proceed with its
> s/disable/disabled/
>
>> instantiation if so.
>>
>> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
>> ---
>> drivers/usb/gadget/function/f_uvc.c | 42 ++++++++++++++++++-----------
>> drivers/usb/gadget/function/uvc.h | 1 +
>> 2 files changed, 27 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
>> index 49b7231742d6..76ec84d3a5fd 100644
>> --- a/drivers/usb/gadget/function/f_uvc.c
>> +++ b/drivers/usb/gadget/function/f_uvc.c
>> @@ -79,7 +79,7 @@ static struct usb_interface_descriptor uvc_control_intf = {
>> .bDescriptorType = USB_DT_INTERFACE,
>> .bInterfaceNumber = UVC_INTF_VIDEO_CONTROL,
>> .bAlternateSetting = 0,
>> - .bNumEndpoints = 1,
>> + .bNumEndpoints = 0,
>> .bInterfaceClass = USB_CLASS_VIDEO,
>> .bInterfaceSubClass = UVC_SC_VIDEOCONTROL,
>> .bInterfaceProtocol = 0x00,
>> @@ -290,14 +290,16 @@ uvc_function_set_alt(struct usb_function *f, unsigned interface, unsigned alt)
>> if (alt)
>> return -EINVAL;
>>
>> - uvcg_info(f, "reset UVC interrupt endpoint\n");
>> - usb_ep_disable(uvc->interrupt_ep);
>> + if (!uvc->disable_interrupt_ep) {
>> + uvcg_info(f, "reset UVC interrupt endpoint\n");
>> + usb_ep_disable(uvc->interrupt_ep);
>>
>> - if (!uvc->interrupt_ep->desc)
>> - if (config_ep_by_speed(cdev->gadget, f, uvc->interrupt_ep))
>> - return -EINVAL;
>> + if (!uvc->interrupt_ep->desc)
>> + if (config_ep_by_speed(cdev->gadget, f, uvc->interrupt_ep))
> Line wrap would be nice.
>
> if (config_ep_by_speed(cdev->gadget, f,
> uvc->interrupt_ep))
>
>> + return -EINVAL;
>>
>> - usb_ep_enable(uvc->interrupt_ep);
>> + usb_ep_enable(uvc->interrupt_ep);
>> + }
>>
>> if (uvc->state == UVC_STATE_DISCONNECTED) {
>> memset(&v4l2_event, 0, sizeof(v4l2_event));
>> @@ -375,7 +377,8 @@ uvc_function_disable(struct usb_function *f)
>> uvc->state = UVC_STATE_DISCONNECTED;
>>
>> usb_ep_disable(uvc->video.ep);
>> - usb_ep_disable(uvc->interrupt_ep);
>> + if (!uvc->disable_interrupt_ep)
>> + usb_ep_disable(uvc->interrupt_ep);
>> }
>>
>> /* --------------------------------------------------------------------------
>> @@ -523,8 +526,10 @@ uvc_copy_descriptors(struct uvc_device *uvc, enum usb_device_speed speed)
>> control_size = 0;
>> streaming_size = 0;
>> bytes = uvc_iad.bLength + uvc_control_intf.bLength
>> - + uvc_interrupt_ep.bLength + uvc_control_cs_ep.bLength
>> - + uvc_streaming_intf_alt0.bLength;
>> + + uvc_control_cs_ep.bLength + uvc_streaming_intf_alt0.bLength;
> Wrong indentation.
>
>> +
>> + if (!uvc->disable_interrupt_ep)
>> + bytes += uvc_interrupt_ep.bLength;
>>
>> if (speed == USB_SPEED_SUPER) {
>> bytes += uvc_ss_control_comp.bLength;
>> @@ -569,7 +574,8 @@ uvc_copy_descriptors(struct uvc_device *uvc, enum usb_device_speed speed)
>> uvc_control_header->bInCollection = 1;
>> uvc_control_header->baInterfaceNr[0] = uvc->streaming_intf;
>>
>> - UVC_COPY_DESCRIPTOR(mem, dst, &uvc_interrupt_ep);
>> + if (!uvc->disable_interrupt_ep)
>> + UVC_COPY_DESCRIPTOR(mem, dst, &uvc_interrupt_ep);
>> if (speed == USB_SPEED_SUPER)
>> UVC_COPY_DESCRIPTOR(mem, dst, &uvc_ss_control_comp);
> This is the companion descriptor for the interrupt endpoint, it should
> only be included if the interrupt endpoint is enabled. Same for
> uvc_control_cs_ep.
>
>>
>> @@ -656,12 +662,16 @@ uvc_function_bind(struct usb_configuration *c, struct usb_function *f)
>> (opts->streaming_maxburst + 1));
>>
>> /* Allocate endpoints. */
>> - ep = usb_ep_autoconfig(cdev->gadget, &uvc_interrupt_ep);
>> - if (!ep) {
>> - uvcg_info(f, "Unable to allocate control EP\n");
>> - goto error;
>> + if (!opts->disable_interrupt_ep) {
>> + ep = usb_ep_autoconfig(cdev->gadget, &uvc_interrupt_ep);
>> + if (!ep) {
>> + uvcg_info(f, "Unable to allocate interrupt EP\n");
>> + goto error;
>> + }
>> + uvc->interrupt_ep = ep;
>> + uvc_control_intf.bNumEndpoints = 1;
>> }
>> - uvc->interrupt_ep = ep;
>> + uvc->disable_interrupt_ep = opts->disable_interrupt_ep;
> Given that you always test for !uvc->disable_interrupt_ep, how about
> naming this enable_interrupt_ep and switching the tests ? I think
> positive logic is more readable that double negation. I would then move
> this line up, in order to test uvc->enable_interrupt_ep instead of
> !opts->disable_interrupt_ep above.

Sure, that's fine. Should we still keep the default functionality (I.E.
set enable_interrupt_ep=true by default)? The thing is...that endpoint
doesn't actually work. There's no mechanism in the f_uvc module that
allows userspace to send a packet through it. Adding one is in the
pipeline now, but given it's unuseable anyway retaining backwards
compatability doesn't seem valuable.

>>
>> if (gadget_is_superspeed(c->cdev->gadget))
>> ep = usb_ep_autoconfig_ss(cdev->gadget, &uvc_ss_streaming_ep,
>> diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h
>> index 48b71e04c2b1..0d0ef9b90b1a 100644
>> --- a/drivers/usb/gadget/function/uvc.h
>> +++ b/drivers/usb/gadget/function/uvc.h
>> @@ -149,6 +149,7 @@ struct uvc_device {
>> struct usb_ep *interrupt_ep;
>> struct usb_request *control_req;
>> void *control_buf;
>> + bool disable_interrupt_ep;
>>
>> unsigned int streaming_intf;
>>