* [PATCH 0/3] Add ability to disable UVC Gadget's interrupt endpoint
@ 2022-12-05 14:37 Daniel Scally
2022-12-05 14:37 ` [PATCH 1/3] usb: gadget: uvc: Rename uvc_control_ep Daniel Scally
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: Daniel Scally @ 2022-12-05 14:37 UTC (permalink / raw
To: linux-usb; +Cc: laurent.pinchart, gregkh, mgr, kieran.bingham, Daniel Scally
The UVC Gadget includes a hardcoded interrupt endpoint against the VideoControl
interface, though it is misnamed as a control endpoint (the default endpoint 0
is actually used for that role). The UVC specification says that this is an
optional feature of a UVC compliant camera provided certain scenarios don't hold
true, specifically...
1. The device supports hardware triggers for still image capture
2. The device implements any AutoUpdate controls
3. The device implements any Asynchronous controls
Those are all scenarios that will be determined by userspace, meaning that in some
implementations the interrupt endpoint is unnecessary. This series adds the means
to disable it, though retains the current behaviour as the default.
Daniel Scally (3):
usb: gadget: uvc: Rename uvc_control_ep
usb: gadget: uvc: Add new disable_interrupt_ep attribute
usb: gadget: uvc: Allow disabling of interrupt endpoint
drivers/usb/gadget/function/f_uvc.c | 46 +++++++++++--------
drivers/usb/gadget/function/u_uvc.h | 2 +
drivers/usb/gadget/function/uvc.h | 3 +-
drivers/usb/gadget/function/uvc_configfs.c | 53 ++++++++++++++++++++++
4 files changed, 85 insertions(+), 19 deletions(-)
--
2.34.1
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/3] usb: gadget: uvc: Rename uvc_control_ep
2022-12-05 14:37 [PATCH 0/3] Add ability to disable UVC Gadget's interrupt endpoint Daniel Scally
@ 2022-12-05 14:37 ` Daniel Scally
2022-12-08 15:33 ` Greg KH
2022-12-28 1:59 ` Laurent Pinchart
2022-12-05 14:37 ` [PATCH 2/3] usb: gadget: uvc: Add new disable_interrupt_ep attribute Daniel Scally
2022-12-05 14:37 ` [PATCH 3/3] usb: gadget: uvc: Allow disabling of interrupt endpoint Daniel Scally
2 siblings, 2 replies; 13+ messages in thread
From: Daniel Scally @ 2022-12-05 14:37 UTC (permalink / raw
To: linux-usb; +Cc: laurent.pinchart, gregkh, mgr, kieran.bingham, Daniel Scally
The f_uvc code defines an endpoint named "uvc_control_ep" but it
is configured with a non-zero endpoint address and has its
bmAttributes flagged as USB_ENDPOINT_XFER_INT - this cannot be the
VideoControl interface's control endpoint, as the default endpoint
0 is used for that purpose. This is instead the optional interrupt
endpoint that can be contained by a VideoControl interface.
Rename the variables to make that clear.
Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
---
drivers/usb/gadget/function/f_uvc.c | 24 ++++++++++++------------
drivers/usb/gadget/function/uvc.h | 2 +-
2 files changed, 13 insertions(+), 13 deletions(-)
diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
index 6e196e06181e..49b7231742d6 100644
--- a/drivers/usb/gadget/function/f_uvc.c
+++ b/drivers/usb/gadget/function/f_uvc.c
@@ -86,7 +86,7 @@ static struct usb_interface_descriptor uvc_control_intf = {
.iInterface = 0,
};
-static struct usb_endpoint_descriptor uvc_control_ep = {
+static struct usb_endpoint_descriptor uvc_interrupt_ep = {
.bLength = USB_DT_ENDPOINT_SIZE,
.bDescriptorType = USB_DT_ENDPOINT,
.bEndpointAddress = USB_DIR_IN,
@@ -290,14 +290,14 @@ uvc_function_set_alt(struct usb_function *f, unsigned interface, unsigned alt)
if (alt)
return -EINVAL;
- uvcg_info(f, "reset UVC Control\n");
- usb_ep_disable(uvc->control_ep);
+ uvcg_info(f, "reset UVC interrupt endpoint\n");
+ usb_ep_disable(uvc->interrupt_ep);
- if (!uvc->control_ep->desc)
- if (config_ep_by_speed(cdev->gadget, f, uvc->control_ep))
+ if (!uvc->interrupt_ep->desc)
+ if (config_ep_by_speed(cdev->gadget, f, uvc->interrupt_ep))
return -EINVAL;
- usb_ep_enable(uvc->control_ep);
+ usb_ep_enable(uvc->interrupt_ep);
if (uvc->state == UVC_STATE_DISCONNECTED) {
memset(&v4l2_event, 0, sizeof(v4l2_event));
@@ -375,7 +375,7 @@ uvc_function_disable(struct usb_function *f)
uvc->state = UVC_STATE_DISCONNECTED;
usb_ep_disable(uvc->video.ep);
- usb_ep_disable(uvc->control_ep);
+ usb_ep_disable(uvc->interrupt_ep);
}
/* --------------------------------------------------------------------------
@@ -511,7 +511,7 @@ uvc_copy_descriptors(struct uvc_device *uvc, enum usb_device_speed speed)
* uvc_iad
* uvc_control_intf
* Class-specific UVC control descriptors
- * uvc_control_ep
+ * uvc_interrupt_ep
* uvc_control_cs_ep
* uvc_ss_control_comp (for SS only)
* uvc_streaming_intf_alt0
@@ -523,7 +523,7 @@ 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_control_ep.bLength + uvc_control_cs_ep.bLength
+ + uvc_interrupt_ep.bLength + uvc_control_cs_ep.bLength
+ uvc_streaming_intf_alt0.bLength;
if (speed == USB_SPEED_SUPER) {
@@ -569,7 +569,7 @@ 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_control_ep);
+ UVC_COPY_DESCRIPTOR(mem, dst, &uvc_interrupt_ep);
if (speed == USB_SPEED_SUPER)
UVC_COPY_DESCRIPTOR(mem, dst, &uvc_ss_control_comp);
@@ -656,12 +656,12 @@ uvc_function_bind(struct usb_configuration *c, struct usb_function *f)
(opts->streaming_maxburst + 1));
/* Allocate endpoints. */
- ep = usb_ep_autoconfig(cdev->gadget, &uvc_control_ep);
+ ep = usb_ep_autoconfig(cdev->gadget, &uvc_interrupt_ep);
if (!ep) {
uvcg_info(f, "Unable to allocate control EP\n");
goto error;
}
- uvc->control_ep = ep;
+ uvc->interrupt_ep = ep;
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 40226b1f7e14..48b71e04c2b1 100644
--- a/drivers/usb/gadget/function/uvc.h
+++ b/drivers/usb/gadget/function/uvc.h
@@ -146,7 +146,7 @@ struct uvc_device {
} desc;
unsigned int control_intf;
- struct usb_ep *control_ep;
+ struct usb_ep *interrupt_ep;
struct usb_request *control_req;
void *control_buf;
--
2.34.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 2/3] usb: gadget: uvc: Add new disable_interrupt_ep attribute
2022-12-05 14:37 [PATCH 0/3] Add ability to disable UVC Gadget's interrupt endpoint Daniel Scally
2022-12-05 14:37 ` [PATCH 1/3] usb: gadget: uvc: Rename uvc_control_ep Daniel Scally
@ 2022-12-05 14:37 ` Daniel Scally
2022-12-08 15:32 ` Greg KH
2022-12-28 2:02 ` Laurent Pinchart
2022-12-05 14:37 ` [PATCH 3/3] usb: gadget: uvc: Allow disabling of interrupt endpoint Daniel Scally
2 siblings, 2 replies; 13+ messages in thread
From: Daniel Scally @ 2022-12-05 14:37 UTC (permalink / raw
To: linux-usb; +Cc: laurent.pinchart, gregkh, mgr, kieran.bingham, Daniel Scally
Add a new attribute to the default control config group that allows
users to specify whether they want to disable the default interrupt
endpoint for the VideoControl interface.
Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
---
drivers/usb/gadget/function/u_uvc.h | 2 +
drivers/usb/gadget/function/uvc_configfs.c | 53 ++++++++++++++++++++++
2 files changed, 55 insertions(+)
diff --git a/drivers/usb/gadget/function/u_uvc.h b/drivers/usb/gadget/function/u_uvc.h
index 24b8681b0d6f..7e6d31a8fad7 100644
--- a/drivers/usb/gadget/function/u_uvc.h
+++ b/drivers/usb/gadget/function/u_uvc.h
@@ -29,6 +29,8 @@ struct f_uvc_opts {
unsigned int streaming_interface;
char function_name[32];
+ bool disable_interrupt_ep;
+
/*
* Control descriptors array pointers for full-/high-speed and
* super-speed. They point by default to the uvc_fs_control_cls and
diff --git a/drivers/usb/gadget/function/uvc_configfs.c b/drivers/usb/gadget/function/uvc_configfs.c
index 4303a3283ba0..644d87eee164 100644
--- a/drivers/usb/gadget/function/uvc_configfs.c
+++ b/drivers/usb/gadget/function/uvc_configfs.c
@@ -716,8 +716,61 @@ static ssize_t uvcg_default_control_b_interface_number_show(
UVC_ATTR_RO(uvcg_default_control_, b_interface_number, bInterfaceNumber);
+static ssize_t uvcg_default_control_disable_interrupt_ep_show(
+ struct config_item *item, char *page)
+{
+ struct config_group *group = to_config_group(item);
+ struct mutex *su_mutex = &group->cg_subsys->su_mutex;
+ struct config_item *opts_item;
+ struct f_uvc_opts *opts;
+ int result = 0;
+
+ mutex_lock(su_mutex); /* for navigating configfs hierarchy */
+
+ opts_item = item->ci_parent;
+ opts = to_f_uvc_opts(opts_item);
+
+ mutex_lock(&opts->lock);
+ result += sprintf(page, "%u\n", opts->disable_interrupt_ep);
+ mutex_unlock(&opts->lock);
+
+ mutex_unlock(su_mutex);
+
+ return result;
+}
+
+static ssize_t uvcg_default_control_disable_interrupt_ep_store(
+ struct config_item *item, const char *page, size_t len)
+{
+ struct config_group *group = to_config_group(item);
+ struct mutex *su_mutex = &group->cg_subsys->su_mutex;
+ struct config_item *opts_item;
+ struct f_uvc_opts *opts;
+ ssize_t ret;
+ u8 num;
+
+ ret = kstrtou8(page, 0, &num);
+ if (ret)
+ return ret;
+
+ mutex_lock(su_mutex); /* for navigating configfs hierarchy */
+
+ opts_item = item->ci_parent;
+ opts = to_f_uvc_opts(opts_item);
+
+ mutex_lock(&opts->lock);
+ opts->disable_interrupt_ep = num;
+ mutex_unlock(&opts->lock);
+
+ mutex_unlock(su_mutex);
+
+ return len;
+}
+UVC_ATTR(uvcg_default_control_, disable_interrupt_ep, disable_interrupt_ep);
+
static struct configfs_attribute *uvcg_default_control_attrs[] = {
&uvcg_default_control_attr_b_interface_number,
+ &uvcg_default_control_attr_disable_interrupt_ep,
NULL,
};
--
2.34.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 3/3] usb: gadget: uvc: Allow disabling of interrupt endpoint
2022-12-05 14:37 [PATCH 0/3] Add ability to disable UVC Gadget's interrupt endpoint Daniel Scally
2022-12-05 14:37 ` [PATCH 1/3] usb: gadget: uvc: Rename uvc_control_ep Daniel Scally
2022-12-05 14:37 ` [PATCH 2/3] usb: gadget: uvc: Add new disable_interrupt_ep attribute Daniel Scally
@ 2022-12-05 14:37 ` Daniel Scally
2022-12-28 2:09 ` Laurent Pinchart
2 siblings, 1 reply; 13+ messages in thread
From: Daniel Scally @ 2022-12-05 14:37 UTC (permalink / raw
To: linux-usb; +Cc: laurent.pinchart, gregkh, mgr, kieran.bingham, Daniel Scally
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
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))
+ 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;
+
+ 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);
@@ -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;
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;
--
2.34.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] usb: gadget: uvc: Add new disable_interrupt_ep attribute
2022-12-05 14:37 ` [PATCH 2/3] usb: gadget: uvc: Add new disable_interrupt_ep attribute Daniel Scally
@ 2022-12-08 15:32 ` Greg KH
2022-12-28 2:02 ` Laurent Pinchart
1 sibling, 0 replies; 13+ messages in thread
From: Greg KH @ 2022-12-08 15:32 UTC (permalink / raw
To: Daniel Scally; +Cc: linux-usb, laurent.pinchart, mgr, kieran.bingham
On Mon, Dec 05, 2022 at 02:37:57PM +0000, Daniel Scally wrote:
> Add a new attribute to the default control config group that allows
> users to specify whether they want to disable the default interrupt
> endpoint for the VideoControl interface.
>
> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
> ---
> drivers/usb/gadget/function/u_uvc.h | 2 +
> drivers/usb/gadget/function/uvc_configfs.c | 53 ++++++++++++++++++++++
> 2 files changed, 55 insertions(+)
Where is the userspace documentation for this new configfs attribute?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] usb: gadget: uvc: Rename uvc_control_ep
2022-12-05 14:37 ` [PATCH 1/3] usb: gadget: uvc: Rename uvc_control_ep Daniel Scally
@ 2022-12-08 15:33 ` Greg KH
2022-12-08 15:39 ` Dan Scally
2022-12-28 1:59 ` Laurent Pinchart
1 sibling, 1 reply; 13+ messages in thread
From: Greg KH @ 2022-12-08 15:33 UTC (permalink / raw
To: Daniel Scally; +Cc: linux-usb, laurent.pinchart, mgr, kieran.bingham
On Mon, Dec 05, 2022 at 02:37:56PM +0000, Daniel Scally wrote:
> The f_uvc code defines an endpoint named "uvc_control_ep" but it
> is configured with a non-zero endpoint address and has its
> bmAttributes flagged as USB_ENDPOINT_XFER_INT - this cannot be the
> VideoControl interface's control endpoint, as the default endpoint
> 0 is used for that purpose. This is instead the optional interrupt
> endpoint that can be contained by a VideoControl interface.
>
> Rename the variables to make that clear.
Does userspace documentation also need to be updated to reflect this
change?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] usb: gadget: uvc: Rename uvc_control_ep
2022-12-08 15:33 ` Greg KH
@ 2022-12-08 15:39 ` Dan Scally
0 siblings, 0 replies; 13+ messages in thread
From: Dan Scally @ 2022-12-08 15:39 UTC (permalink / raw
To: Greg KH; +Cc: linux-usb, laurent.pinchart, mgr, kieran.bingham
Hi Greg
On 08/12/2022 15:33, Greg KH wrote:
> On Mon, Dec 05, 2022 at 02:37:56PM +0000, Daniel Scally wrote:
>> The f_uvc code defines an endpoint named "uvc_control_ep" but it
>> is configured with a non-zero endpoint address and has its
>> bmAttributes flagged as USB_ENDPOINT_XFER_INT - this cannot be the
>> VideoControl interface's control endpoint, as the default endpoint
>> 0 is used for that purpose. This is instead the optional interrupt
>> endpoint that can be contained by a VideoControl interface.
>>
>> Rename the variables to make that clear.
> Does userspace documentation also need to be updated to reflect this
> change?
Not for this one I don't think; the data sent to userspace was already
correct and isn't changing, it's just the internal variable name that
is...but for #2/3 absolutely; I'll update the documentation in a v2.
> thanks,
>
> greg k-h
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] usb: gadget: uvc: Rename uvc_control_ep
2022-12-05 14:37 ` [PATCH 1/3] usb: gadget: uvc: Rename uvc_control_ep Daniel Scally
2022-12-08 15:33 ` Greg KH
@ 2022-12-28 1:59 ` Laurent Pinchart
2023-01-01 20:25 ` Dan Scally
1 sibling, 1 reply; 13+ messages in thread
From: Laurent Pinchart @ 2022-12-28 1:59 UTC (permalink / raw
To: Daniel Scally; +Cc: linux-usb, gregkh, mgr, kieran.bingham
Hi Dan,
Thank you for the patch.
On Mon, Dec 05, 2022 at 02:37:56PM +0000, Daniel Scally wrote:
> The f_uvc code defines an endpoint named "uvc_control_ep" but it
> is configured with a non-zero endpoint address and has its
> bmAttributes flagged as USB_ENDPOINT_XFER_INT - this cannot be the
> VideoControl interface's control endpoint, as the default endpoint
> 0 is used for that purpose. This is instead the optional interrupt
> endpoint that can be contained by a VideoControl interface.
>
> Rename the variables to make that clear.
>
> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
> ---
> drivers/usb/gadget/function/f_uvc.c | 24 ++++++++++++------------
> drivers/usb/gadget/function/uvc.h | 2 +-
> 2 files changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
> index 6e196e06181e..49b7231742d6 100644
> --- a/drivers/usb/gadget/function/f_uvc.c
> +++ b/drivers/usb/gadget/function/f_uvc.c
> @@ -86,7 +86,7 @@ static struct usb_interface_descriptor uvc_control_intf = {
> .iInterface = 0,
> };
>
> -static struct usb_endpoint_descriptor uvc_control_ep = {
> +static struct usb_endpoint_descriptor uvc_interrupt_ep = {
> .bLength = USB_DT_ENDPOINT_SIZE,
> .bDescriptorType = USB_DT_ENDPOINT,
> .bEndpointAddress = USB_DIR_IN,
> @@ -290,14 +290,14 @@ uvc_function_set_alt(struct usb_function *f, unsigned interface, unsigned alt)
> if (alt)
> return -EINVAL;
>
> - uvcg_info(f, "reset UVC Control\n");
> - usb_ep_disable(uvc->control_ep);
> + uvcg_info(f, "reset UVC interrupt endpoint\n");
> + usb_ep_disable(uvc->interrupt_ep);
Technically it's called the "VideoControl Interrupt Endpoint", but that
would be quite long, and given that UVC has a single interrupt endpoint
in all interfaces, I think the name is fine.
>
> - if (!uvc->control_ep->desc)
> - if (config_ep_by_speed(cdev->gadget, f, uvc->control_ep))
> + if (!uvc->interrupt_ep->desc)
> + if (config_ep_by_speed(cdev->gadget, f, uvc->interrupt_ep))
> return -EINVAL;
>
> - usb_ep_enable(uvc->control_ep);
> + usb_ep_enable(uvc->interrupt_ep);
>
> if (uvc->state == UVC_STATE_DISCONNECTED) {
> memset(&v4l2_event, 0, sizeof(v4l2_event));
> @@ -375,7 +375,7 @@ uvc_function_disable(struct usb_function *f)
> uvc->state = UVC_STATE_DISCONNECTED;
>
> usb_ep_disable(uvc->video.ep);
> - usb_ep_disable(uvc->control_ep);
> + usb_ep_disable(uvc->interrupt_ep);
> }
>
> /* --------------------------------------------------------------------------
> @@ -511,7 +511,7 @@ uvc_copy_descriptors(struct uvc_device *uvc, enum usb_device_speed speed)
> * uvc_iad
> * uvc_control_intf
> * Class-specific UVC control descriptors
> - * uvc_control_ep
> + * uvc_interrupt_ep
> * uvc_control_cs_ep
> * uvc_ss_control_comp (for SS only)
Shouldn't you also rename those two variables ?
Ideally I would also rename struct uvc_control_endpoint_descriptor and
UVC_DT_CONTROL_ENDPOINT_SIZE, but those are exposed to userspace :-(
> * uvc_streaming_intf_alt0
> @@ -523,7 +523,7 @@ 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_control_ep.bLength + uvc_control_cs_ep.bLength
> + + uvc_interrupt_ep.bLength + uvc_control_cs_ep.bLength
> + uvc_streaming_intf_alt0.bLength;
>
> if (speed == USB_SPEED_SUPER) {
> @@ -569,7 +569,7 @@ 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_control_ep);
> + UVC_COPY_DESCRIPTOR(mem, dst, &uvc_interrupt_ep);
> if (speed == USB_SPEED_SUPER)
> UVC_COPY_DESCRIPTOR(mem, dst, &uvc_ss_control_comp);
>
> @@ -656,12 +656,12 @@ uvc_function_bind(struct usb_configuration *c, struct usb_function *f)
> (opts->streaming_maxburst + 1));
>
> /* Allocate endpoints. */
> - ep = usb_ep_autoconfig(cdev->gadget, &uvc_control_ep);
> + ep = usb_ep_autoconfig(cdev->gadget, &uvc_interrupt_ep);
> if (!ep) {
> uvcg_info(f, "Unable to allocate control EP\n");
> goto error;
> }
> - uvc->control_ep = ep;
> + uvc->interrupt_ep = ep;
>
> 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 40226b1f7e14..48b71e04c2b1 100644
> --- a/drivers/usb/gadget/function/uvc.h
> +++ b/drivers/usb/gadget/function/uvc.h
> @@ -146,7 +146,7 @@ struct uvc_device {
> } desc;
>
> unsigned int control_intf;
> - struct usb_ep *control_ep;
> + struct usb_ep *interrupt_ep;
> struct usb_request *control_req;
> void *control_buf;
>
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] usb: gadget: uvc: Add new disable_interrupt_ep attribute
2022-12-05 14:37 ` [PATCH 2/3] usb: gadget: uvc: Add new disable_interrupt_ep attribute Daniel Scally
2022-12-08 15:32 ` Greg KH
@ 2022-12-28 2:02 ` Laurent Pinchart
1 sibling, 0 replies; 13+ messages in thread
From: Laurent Pinchart @ 2022-12-28 2:02 UTC (permalink / raw
To: Daniel Scally; +Cc: linux-usb, gregkh, mgr, kieran.bingham
Hi Dan,
Thank you for the patch.
On Mon, Dec 05, 2022 at 02:37:57PM +0000, Daniel Scally wrote:
> Add a new attribute to the default control config group that allows
> users to specify whether they want to disable the default interrupt
> endpoint for the VideoControl interface.
>
> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
Apart from the missing documentation, this looks good to me.
> ---
> drivers/usb/gadget/function/u_uvc.h | 2 +
> drivers/usb/gadget/function/uvc_configfs.c | 53 ++++++++++++++++++++++
> 2 files changed, 55 insertions(+)
>
> diff --git a/drivers/usb/gadget/function/u_uvc.h b/drivers/usb/gadget/function/u_uvc.h
> index 24b8681b0d6f..7e6d31a8fad7 100644
> --- a/drivers/usb/gadget/function/u_uvc.h
> +++ b/drivers/usb/gadget/function/u_uvc.h
> @@ -29,6 +29,8 @@ struct f_uvc_opts {
> unsigned int streaming_interface;
> char function_name[32];
>
> + bool disable_interrupt_ep;
> +
> /*
> * Control descriptors array pointers for full-/high-speed and
> * super-speed. They point by default to the uvc_fs_control_cls and
> diff --git a/drivers/usb/gadget/function/uvc_configfs.c b/drivers/usb/gadget/function/uvc_configfs.c
> index 4303a3283ba0..644d87eee164 100644
> --- a/drivers/usb/gadget/function/uvc_configfs.c
> +++ b/drivers/usb/gadget/function/uvc_configfs.c
> @@ -716,8 +716,61 @@ static ssize_t uvcg_default_control_b_interface_number_show(
>
> UVC_ATTR_RO(uvcg_default_control_, b_interface_number, bInterfaceNumber);
>
> +static ssize_t uvcg_default_control_disable_interrupt_ep_show(
> + struct config_item *item, char *page)
> +{
> + struct config_group *group = to_config_group(item);
> + struct mutex *su_mutex = &group->cg_subsys->su_mutex;
> + struct config_item *opts_item;
> + struct f_uvc_opts *opts;
> + int result = 0;
> +
> + mutex_lock(su_mutex); /* for navigating configfs hierarchy */
> +
> + opts_item = item->ci_parent;
> + opts = to_f_uvc_opts(opts_item);
> +
> + mutex_lock(&opts->lock);
> + result += sprintf(page, "%u\n", opts->disable_interrupt_ep);
> + mutex_unlock(&opts->lock);
> +
> + mutex_unlock(su_mutex);
> +
> + return result;
> +}
> +
> +static ssize_t uvcg_default_control_disable_interrupt_ep_store(
> + struct config_item *item, const char *page, size_t len)
> +{
> + struct config_group *group = to_config_group(item);
> + struct mutex *su_mutex = &group->cg_subsys->su_mutex;
> + struct config_item *opts_item;
> + struct f_uvc_opts *opts;
> + ssize_t ret;
> + u8 num;
> +
> + ret = kstrtou8(page, 0, &num);
> + if (ret)
> + return ret;
> +
> + mutex_lock(su_mutex); /* for navigating configfs hierarchy */
> +
> + opts_item = item->ci_parent;
> + opts = to_f_uvc_opts(opts_item);
> +
> + mutex_lock(&opts->lock);
> + opts->disable_interrupt_ep = num;
> + mutex_unlock(&opts->lock);
> +
> + mutex_unlock(su_mutex);
> +
> + return len;
> +}
> +UVC_ATTR(uvcg_default_control_, disable_interrupt_ep, disable_interrupt_ep);
> +
> static struct configfs_attribute *uvcg_default_control_attrs[] = {
> &uvcg_default_control_attr_b_interface_number,
> + &uvcg_default_control_attr_disable_interrupt_ep,
> NULL,
> };
>
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] usb: gadget: uvc: Allow disabling of interrupt endpoint
2022-12-05 14:37 ` [PATCH 3/3] usb: gadget: uvc: Allow disabling of interrupt endpoint Daniel Scally
@ 2022-12-28 2:09 ` Laurent Pinchart
2023-01-01 20:46 ` Dan Scally
0 siblings, 1 reply; 13+ messages in thread
From: Laurent Pinchart @ 2022-12-28 2:09 UTC (permalink / raw
To: Daniel Scally; +Cc: linux-usb, gregkh, mgr, kieran.bingham
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.
>
> 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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] usb: gadget: uvc: Rename uvc_control_ep
2022-12-28 1:59 ` Laurent Pinchart
@ 2023-01-01 20:25 ` Dan Scally
0 siblings, 0 replies; 13+ messages in thread
From: Dan Scally @ 2023-01-01 20:25 UTC (permalink / raw
To: Laurent Pinchart; +Cc: linux-usb, gregkh, mgr, kieran.bingham
Hi Laurent - thank you for the review!
On 28/12/2022 01:59, Laurent Pinchart wrote:
> Hi Dan,
>
> Thank you for the patch.
>
> On Mon, Dec 05, 2022 at 02:37:56PM +0000, Daniel Scally wrote:
>> The f_uvc code defines an endpoint named "uvc_control_ep" but it
>> is configured with a non-zero endpoint address and has its
>> bmAttributes flagged as USB_ENDPOINT_XFER_INT - this cannot be the
>> VideoControl interface's control endpoint, as the default endpoint
>> 0 is used for that purpose. This is instead the optional interrupt
>> endpoint that can be contained by a VideoControl interface.
>>
>> Rename the variables to make that clear.
>>
>> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
>> ---
>> drivers/usb/gadget/function/f_uvc.c | 24 ++++++++++++------------
>> drivers/usb/gadget/function/uvc.h | 2 +-
>> 2 files changed, 13 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
>> index 6e196e06181e..49b7231742d6 100644
>> --- a/drivers/usb/gadget/function/f_uvc.c
>> +++ b/drivers/usb/gadget/function/f_uvc.c
>> @@ -86,7 +86,7 @@ static struct usb_interface_descriptor uvc_control_intf = {
>> .iInterface = 0,
>> };
>>
>> -static struct usb_endpoint_descriptor uvc_control_ep = {
>> +static struct usb_endpoint_descriptor uvc_interrupt_ep = {
>> .bLength = USB_DT_ENDPOINT_SIZE,
>> .bDescriptorType = USB_DT_ENDPOINT,
>> .bEndpointAddress = USB_DIR_IN,
>> @@ -290,14 +290,14 @@ uvc_function_set_alt(struct usb_function *f, unsigned interface, unsigned alt)
>> if (alt)
>> return -EINVAL;
>>
>> - uvcg_info(f, "reset UVC Control\n");
>> - usb_ep_disable(uvc->control_ep);
>> + uvcg_info(f, "reset UVC interrupt endpoint\n");
>> + usb_ep_disable(uvc->interrupt_ep);
> Technically it's called the "VideoControl Interrupt Endpoint", but that
> would be quite long, and given that UVC has a single interrupt endpoint
> in all interfaces, I think the name is fine.
>
>>
>> - if (!uvc->control_ep->desc)
>> - if (config_ep_by_speed(cdev->gadget, f, uvc->control_ep))
>> + if (!uvc->interrupt_ep->desc)
>> + if (config_ep_by_speed(cdev->gadget, f, uvc->interrupt_ep))
>> return -EINVAL;
>>
>> - usb_ep_enable(uvc->control_ep);
>> + usb_ep_enable(uvc->interrupt_ep);
>>
>> if (uvc->state == UVC_STATE_DISCONNECTED) {
>> memset(&v4l2_event, 0, sizeof(v4l2_event));
>> @@ -375,7 +375,7 @@ uvc_function_disable(struct usb_function *f)
>> uvc->state = UVC_STATE_DISCONNECTED;
>>
>> usb_ep_disable(uvc->video.ep);
>> - usb_ep_disable(uvc->control_ep);
>> + usb_ep_disable(uvc->interrupt_ep);
>> }
>>
>> /* --------------------------------------------------------------------------
>> @@ -511,7 +511,7 @@ uvc_copy_descriptors(struct uvc_device *uvc, enum usb_device_speed speed)
>> * uvc_iad
>> * uvc_control_intf
>> * Class-specific UVC control descriptors
>> - * uvc_control_ep
>> + * uvc_interrupt_ep
>> * uvc_control_cs_ep
>> * uvc_ss_control_comp (for SS only)
> Shouldn't you also rename those two variables ?
Yes, I hadn't realised that they were related. That's fixed in the v2.
>
> Ideally I would also rename struct uvc_control_endpoint_descriptor and
> UVC_DT_CONTROL_ENDPOINT_SIZE, but those are exposed to userspace :-(
>
>> * uvc_streaming_intf_alt0
>> @@ -523,7 +523,7 @@ 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_control_ep.bLength + uvc_control_cs_ep.bLength
>> + + uvc_interrupt_ep.bLength + uvc_control_cs_ep.bLength
>> + uvc_streaming_intf_alt0.bLength;
>>
>> if (speed == USB_SPEED_SUPER) {
>> @@ -569,7 +569,7 @@ 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_control_ep);
>> + UVC_COPY_DESCRIPTOR(mem, dst, &uvc_interrupt_ep);
>> if (speed == USB_SPEED_SUPER)
>> UVC_COPY_DESCRIPTOR(mem, dst, &uvc_ss_control_comp);
>>
>> @@ -656,12 +656,12 @@ uvc_function_bind(struct usb_configuration *c, struct usb_function *f)
>> (opts->streaming_maxburst + 1));
>>
>> /* Allocate endpoints. */
>> - ep = usb_ep_autoconfig(cdev->gadget, &uvc_control_ep);
>> + ep = usb_ep_autoconfig(cdev->gadget, &uvc_interrupt_ep);
>> if (!ep) {
>> uvcg_info(f, "Unable to allocate control EP\n");
>> goto error;
>> }
>> - uvc->control_ep = ep;
>> + uvc->interrupt_ep = ep;
>>
>> 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 40226b1f7e14..48b71e04c2b1 100644
>> --- a/drivers/usb/gadget/function/uvc.h
>> +++ b/drivers/usb/gadget/function/uvc.h
>> @@ -146,7 +146,7 @@ struct uvc_device {
>> } desc;
>>
>> unsigned int control_intf;
>> - struct usb_ep *control_ep;
>> + struct usb_ep *interrupt_ep;
>> struct usb_request *control_req;
>> void *control_buf;
>>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] usb: gadget: uvc: Allow disabling of interrupt endpoint
2022-12-28 2:09 ` Laurent Pinchart
@ 2023-01-01 20:46 ` Dan Scally
2023-01-02 10:24 ` Laurent Pinchart
0 siblings, 1 reply; 13+ messages in thread
From: Dan Scally @ 2023-01-01 20:46 UTC (permalink / raw
To: Laurent Pinchart; +Cc: linux-usb, gregkh, mgr, kieran.bingham
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;
>>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] usb: gadget: uvc: Allow disabling of interrupt endpoint
2023-01-01 20:46 ` Dan Scally
@ 2023-01-02 10:24 ` Laurent Pinchart
0 siblings, 0 replies; 13+ messages in thread
From: Laurent Pinchart @ 2023-01-02 10:24 UTC (permalink / raw
To: Dan Scally; +Cc: linux-usb, gregkh, mgr, kieran.bingham
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
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2023-01-02 10:24 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-12-05 14:37 [PATCH 0/3] Add ability to disable UVC Gadget's interrupt endpoint Daniel Scally
2022-12-05 14:37 ` [PATCH 1/3] usb: gadget: uvc: Rename uvc_control_ep Daniel Scally
2022-12-08 15:33 ` Greg KH
2022-12-08 15:39 ` Dan Scally
2022-12-28 1:59 ` Laurent Pinchart
2023-01-01 20:25 ` Dan Scally
2022-12-05 14:37 ` [PATCH 2/3] usb: gadget: uvc: Add new disable_interrupt_ep attribute Daniel Scally
2022-12-08 15:32 ` Greg KH
2022-12-28 2:02 ` Laurent Pinchart
2022-12-05 14:37 ` [PATCH 3/3] usb: gadget: uvc: Allow disabling of interrupt endpoint Daniel Scally
2022-12-28 2:09 ` Laurent Pinchart
2023-01-01 20:46 ` Dan Scally
2023-01-02 10:24 ` Laurent Pinchart
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.