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

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

Hi Dan,

On Sun, Jan 01, 2023 at 08:46:14PM +0000, Dan Scally wrote:
> On 28/12/2022 02:09, Laurent Pinchart wrote:
> > 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.

That's a good point. If it doesn't work, I don't mind switching the
default.

> >>   
> >>   	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;
> >>   

-- 
Regards,

Laurent Pinchart