[PATCH v3 0/6] usb: gadget: uvc: smaller fixes for stability

[PATCH v3 0/6] usb: gadget: uvc: smaller fixes for stability

[Michael Grzeschik]

This series improves the uvc video gadget overal stability and code
quality. Including a fix for the configfs udc callbacks.

v3: - dropped patch: usb: gadget: udc: ensure the gadget is still available

Michael Grzeschik (5):
  usb: gadget: uvc: consistently use define for headerlen
  usb: gadget: uvc: test if ep->desc is valid on ep_queue
  usb: gadget: uvc: only schedule stream in streaming state
  usb: gadget: uvc: only pump video data if necessary
  usb: gadget: uvc: ensure the vdev is unset

Michael Tretter (1):
  usb: gadget: uvc: rename function to be more consistent

 drivers/usb/gadget/function/f_uvc.c     |  8 +++++---
 drivers/usb/gadget/function/uvc_v4l2.c  |  3 ++-
 drivers/usb/gadget/function/uvc_video.c | 26 ++++++++++++++++---------
 3 files changed, 24 insertions(+), 13 deletions(-)

-- 
2.30.2

[PATCH v3 1/6] usb: gadget: uvc: consistently use define for headerlen

[Michael Grzeschik]

The uvc request headerlen of 2 was defined as UVCG_REQUEST_HEADER_LEN
in commit e81e7f9a0eb9 ("usb: gadget: uvc: add scatter gather support").
We missed to use it consistently. This patch fixes that.

Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>

---
v3: - fixed wrong use of define
    - added missing use of define in uvc_video_encode_header

 drivers/usb/gadget/function/uvc_video.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
index b4a763e5f70e1..f3e97a4fc0303 100644
--- a/drivers/usb/gadget/function/uvc_video.c
+++ b/drivers/usb/gadget/function/uvc_video.c
@@ -33,7 +33,7 @@ uvc_video_encode_header(struct uvc_video *video, struct uvc_buffer *buf,
 	if (buf->bytesused - video->queue.buf_used <= len - UVCG_REQUEST_HEADER_LEN)
 		data[1] |= UVC_STREAM_EOF;
 
-	return 2;
+	return UVCG_REQUEST_HEADER_LEN;
 }
 
 static int
@@ -302,8 +302,8 @@ uvc_video_alloc_requests(struct uvc_video *video)
 		list_add_tail(&video->ureq[i].req->list, &video->req_free);
 		/* req_size/PAGE_SIZE + 1 for overruns and + 1 for header */
 		sg_alloc_table(&video->ureq[i].sgt,
-			       DIV_ROUND_UP(req_size - 2, PAGE_SIZE) + 2,
-			       GFP_KERNEL);
+			       DIV_ROUND_UP(req_size - UVCG_REQUEST_HEADER_LEN,
+					    PAGE_SIZE) + 2, GFP_KERNEL);
 	}
 
 	video->req_size = req_size;
-- 
2.30.2

[PATCH v3 2/6] usb: gadget: uvc: rename function to be more consistent

[Michael Grzeschik]

From: Michael Tretter <m.tretter@pengutronix.de>

When enabling info debugging for the uvc gadget, the bind and unbind
infos use different formats. Change the unbind to visually match the
bind.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>

---
v3: - wrapped the function line

 drivers/usb/gadget/function/f_uvc.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
index 9d87c0fb8f92e..e6de56afec6ea 100644
--- a/drivers/usb/gadget/function/f_uvc.c
+++ b/drivers/usb/gadget/function/f_uvc.c
@@ -884,12 +884,13 @@ static void uvc_free(struct usb_function *f)
 	kfree(uvc);
 }
 
-static void uvc_unbind(struct usb_configuration *c, struct usb_function *f)
+static void uvc_function_unbind(struct usb_configuration *c,
+				struct usb_function *f)
 {
 	struct usb_composite_dev *cdev = c->cdev;
 	struct uvc_device *uvc = to_uvc(f);
 
-	uvcg_info(f, "%s\n", __func__);
+	uvcg_info(f, "%s()\n", __func__);
 
 	device_remove_file(&uvc->vdev.dev, &dev_attr_function_name);
 	video_unregister_device(&uvc->vdev);
@@ -943,7 +944,7 @@ static struct usb_function *uvc_alloc(struct usb_function_instance *fi)
 	/* Register the function. */
 	uvc->func.name = "uvc";
 	uvc->func.bind = uvc_function_bind;
-	uvc->func.unbind = uvc_unbind;
+	uvc->func.unbind = uvc_function_unbind;
 	uvc->func.get_alt = uvc_function_get_alt;
 	uvc->func.set_alt = uvc_function_set_alt;
 	uvc->func.disable = uvc_function_disable;
-- 
2.30.2

[PATCH v3 3/6] usb: gadget: uvc: test if ep->desc is valid on ep_queue

[Michael Grzeschik]

The reason that the ep_queue has failed could be a disabled endpoint.
In that case it is not guaranteed that the ep->desc is still valid.
This patch adds a check for NULL.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>

---
v3: - fixed commit description
    - added comment to condition

 drivers/usb/gadget/function/uvc_video.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
index f3e97a4fc0303..4222192fa624c 100644
--- a/drivers/usb/gadget/function/uvc_video.c
+++ b/drivers/usb/gadget/function/uvc_video.c
@@ -199,9 +199,12 @@ static int uvcg_video_ep_queue(struct uvc_video *video, struct usb_request *req)
 		uvcg_err(&video->uvc->func, "Failed to queue request (%d).\n",
 			 ret);
 
-		/* Isochronous endpoints can't be halted. */
-		if (usb_endpoint_xfer_bulk(video->ep->desc))
-			usb_ep_set_halt(video->ep);
+		/* If the endpoint is disabled the descriptor may be NULL. */
+		if (video->ep->desc) {
+			/* Isochronous endpoints can't be halted. */
+			if (usb_endpoint_xfer_bulk(video->ep->desc))
+				usb_ep_set_halt(video->ep);
+		}
 	}
 
 	return ret;
-- 
2.30.2

[PATCH v3 4/6] usb: gadget: uvc: only schedule stream in streaming state

[Michael Grzeschik]

This patch ensures that the video pump thread will only be scheduled if
the uvc is really in streaming state. This way the worker will not have
to run on an empty queue.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
---
 drivers/usb/gadget/function/uvc_v4l2.c  | 3 ++-
 drivers/usb/gadget/function/uvc_video.c | 4 +++-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/function/uvc_v4l2.c b/drivers/usb/gadget/function/uvc_v4l2.c
index 197c26f7aec63..a2c78690c5c28 100644
--- a/drivers/usb/gadget/function/uvc_v4l2.c
+++ b/drivers/usb/gadget/function/uvc_v4l2.c
@@ -169,7 +169,8 @@ uvc_v4l2_qbuf(struct file *file, void *fh, struct v4l2_buffer *b)
 	if (ret < 0)
 		return ret;
 
-	schedule_work(&video->pump);
+	if (uvc->state == UVC_STATE_STREAMING)
+		schedule_work(&video->pump);
 
 	return ret;
 }
diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
index 4222192fa624c..167145687af49 100644
--- a/drivers/usb/gadget/function/uvc_video.c
+++ b/drivers/usb/gadget/function/uvc_video.c
@@ -216,6 +216,7 @@ uvc_video_complete(struct usb_ep *ep, struct usb_request *req)
 	struct uvc_request *ureq = req->context;
 	struct uvc_video *video = ureq->video;
 	struct uvc_video_queue *queue = &video->queue;
+	struct uvc_device *uvc = video->uvc;
 	unsigned long flags;
 
 	switch (req->status) {
@@ -238,7 +239,8 @@ uvc_video_complete(struct usb_ep *ep, struct usb_request *req)
 	list_add_tail(&req->list, &video->req_free);
 	spin_unlock_irqrestore(&video->req_lock, flags);
 
-	schedule_work(&video->pump);
+	if (uvc->state == UVC_STATE_STREAMING)
+		schedule_work(&video->pump);
 }
 
 static int
-- 
2.30.2

[PATCH v3 5/6] usb: gadget: uvc: only pump video data if necessary

[Michael Grzeschik]

If the streaming endpoint is not enabled, the worker has nothing to do.
In the case buffers are still queued, this patch ensures that it will bail
out without handling any data.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>

---
v3: - fixed commit description

 drivers/usb/gadget/function/uvc_video.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
index 167145687af49..7bc865eab27b3 100644
--- a/drivers/usb/gadget/function/uvc_video.c
+++ b/drivers/usb/gadget/function/uvc_video.c
@@ -334,12 +334,12 @@ static void uvcg_video_pump(struct work_struct *work)
 {
 	struct uvc_video *video = container_of(work, struct uvc_video, pump);
 	struct uvc_video_queue *queue = &video->queue;
-	struct usb_request *req;
+	struct usb_request *req = NULL;
 	struct uvc_buffer *buf;
 	unsigned long flags;
 	int ret;
 
-	while (1) {
+	while (video->ep->enabled) {
 		/* Retrieve the first available USB request, protected by the
 		 * request lock.
 		 */
@@ -389,6 +389,9 @@ static void uvcg_video_pump(struct work_struct *work)
 		video->req_int_count++;
 	}
 
+	if (!req)
+		return;
+
 	spin_lock_irqsave(&video->req_lock, flags);
 	list_add_tail(&req->list, &video->req_free);
 	spin_unlock_irqrestore(&video->req_lock, flags);
-- 
2.30.2

[PATCH v3 6/6] usb: gadget: uvc: ensure the vdev is unset

[Michael Grzeschik]

Since the uvc video device is created on demand, we have to ensure
that the struct is always zeroed. Otherwise the previous settings
might collide with the new values.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>

---
v3: - fixed commit description
    - using sizeof(uvc->video) on memset
---
 drivers/usb/gadget/function/f_uvc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
index e6de56afec6ea..71bb5e477dbad 100644
--- a/drivers/usb/gadget/function/f_uvc.c
+++ b/drivers/usb/gadget/function/f_uvc.c
@@ -417,6 +417,7 @@ uvc_register_video(struct uvc_device *uvc)
 	int ret;
 
 	/* TODO reference counting. */
+	memset(&uvc->vdev, 0, sizeof(uvc->video));
 	uvc->vdev.v4l2_dev = &uvc->v4l2_dev;
 	uvc->vdev.v4l2_dev->dev = &cdev->gadget->dev;
 	uvc->vdev.fops = &uvc_v4l2_fops;
-- 
2.30.2

[PATCH] usb: gadget: uvc: consistently use define for headerlen

[Michael Grzeschik]

The uvc request headerlen of 2 was defined as UVCG_REQUEST_HEADER_LEN
in commit e81e7f9a0eb9 ("usb: gadget: uvc: add scatter gather support").
We missed to use it consistently. This patch fixes that.

Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>

---
v3: - fixed wrong use of define
    - added missing use of define in uvc_video_encode_header
v4: - fixed headersize in struct uvc_request to also use the define

 drivers/usb/gadget/function/uvc.h       | 4 +++-
 drivers/usb/gadget/function/uvc_video.c | 6 +++---
 drivers/usb/gadget/function/uvc_video.h | 2 --
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h
index 9d5f17b551bbd..c3607a32b9862 100644
--- a/drivers/usb/gadget/function/uvc.h
+++ b/drivers/usb/gadget/function/uvc.h
@@ -68,6 +68,8 @@ extern unsigned int uvc_gadget_trace_param;
 #define UVC_MAX_REQUEST_SIZE			64
 #define UVC_MAX_EVENTS				4
 
+#define UVCG_REQUEST_HEADER_LEN			2
+
 /* ------------------------------------------------------------------------
  * Structures
  */
@@ -76,7 +78,7 @@ struct uvc_request {
 	u8 *req_buffer;
 	struct uvc_video *video;
 	struct sg_table sgt;
-	u8 header[2];
+	u8 header[UVCG_REQUEST_HEADER_LEN];
 };
 
 struct uvc_video {
diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
index 1bfca3cb42f9b..7bc865eab27b3 100644
--- a/drivers/usb/gadget/function/uvc_video.c
+++ b/drivers/usb/gadget/function/uvc_video.c
@@ -33,7 +33,7 @@ uvc_video_encode_header(struct uvc_video *video, struct uvc_buffer *buf,
 	if (buf->bytesused - video->queue.buf_used <= len - UVCG_REQUEST_HEADER_LEN)
 		data[1] |= UVC_STREAM_EOF;
 
-	return 2;
+	return UVCG_REQUEST_HEADER_LEN;
 }
 
 static int
@@ -307,8 +307,8 @@ uvc_video_alloc_requests(struct uvc_video *video)
 		list_add_tail(&video->ureq[i].req->list, &video->req_free);
 		/* req_size/PAGE_SIZE + 1 for overruns and + 1 for header */
 		sg_alloc_table(&video->ureq[i].sgt,
-			       DIV_ROUND_UP(req_size - 2, PAGE_SIZE) + 2,
-			       GFP_KERNEL);
+			       DIV_ROUND_UP(req_size - UVCG_REQUEST_HEADER_LEN,
+					    PAGE_SIZE) + 2, GFP_KERNEL);
 	}
 
 	video->req_size = req_size;
diff --git a/drivers/usb/gadget/function/uvc_video.h b/drivers/usb/gadget/function/uvc_video.h
index 9bf19475f6f9a..03adeefa343b7 100644
--- a/drivers/usb/gadget/function/uvc_video.h
+++ b/drivers/usb/gadget/function/uvc_video.h
@@ -12,8 +12,6 @@
 #ifndef __UVC_VIDEO_H__
 #define __UVC_VIDEO_H__
 
-#define UVCG_REQUEST_HEADER_LEN			2
-
 struct uvc_video;
 
 int uvcg_video_enable(struct uvc_video *video, int enable);
-- 
2.30.2

Re: [PATCH] usb: gadget: uvc: consistently use define for headerlen

[Michael Grzeschik]

[-- Attachment #1: Type: text/plain, Size: 3313 bytes --]

Oh, I forgot v4 in the subject. Will resend. Ignore this.

On Mon, Oct 18, 2021 at 02:11:39AM +0200, Michael Grzeschik wrote:
>The uvc request headerlen of 2 was defined as UVCG_REQUEST_HEADER_LEN
>in commit e81e7f9a0eb9 ("usb: gadget: uvc: add scatter gather support").
>We missed to use it consistently. This patch fixes that.
>
>Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
>Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
>
>---
>v3: - fixed wrong use of define
>    - added missing use of define in uvc_video_encode_header
>v4: - fixed headersize in struct uvc_request to also use the define
>
> drivers/usb/gadget/function/uvc.h       | 4 +++-
> drivers/usb/gadget/function/uvc_video.c | 6 +++---
> drivers/usb/gadget/function/uvc_video.h | 2 --
> 3 files changed, 6 insertions(+), 6 deletions(-)
>
>diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h
>index 9d5f17b551bbd..c3607a32b9862 100644
>--- a/drivers/usb/gadget/function/uvc.h
>+++ b/drivers/usb/gadget/function/uvc.h
>@@ -68,6 +68,8 @@ extern unsigned int uvc_gadget_trace_param;
> #define UVC_MAX_REQUEST_SIZE			64
> #define UVC_MAX_EVENTS				4
>
>+#define UVCG_REQUEST_HEADER_LEN			2
>+
> /* ------------------------------------------------------------------------
>  * Structures
>  */
>@@ -76,7 +78,7 @@ struct uvc_request {
> 	u8 *req_buffer;
> 	struct uvc_video *video;
> 	struct sg_table sgt;
>-	u8 header[2];
>+	u8 header[UVCG_REQUEST_HEADER_LEN];
> };
>
> struct uvc_video {
>diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
>index 1bfca3cb42f9b..7bc865eab27b3 100644
>--- a/drivers/usb/gadget/function/uvc_video.c
>+++ b/drivers/usb/gadget/function/uvc_video.c
>@@ -33,7 +33,7 @@ uvc_video_encode_header(struct uvc_video *video, struct uvc_buffer *buf,
> 	if (buf->bytesused - video->queue.buf_used <= len - UVCG_REQUEST_HEADER_LEN)
> 		data[1] |= UVC_STREAM_EOF;
>
>-	return 2;
>+	return UVCG_REQUEST_HEADER_LEN;
> }
>
> static int
>@@ -307,8 +307,8 @@ uvc_video_alloc_requests(struct uvc_video *video)
> 		list_add_tail(&video->ureq[i].req->list, &video->req_free);
> 		/* req_size/PAGE_SIZE + 1 for overruns and + 1 for header */
> 		sg_alloc_table(&video->ureq[i].sgt,
>-			       DIV_ROUND_UP(req_size - 2, PAGE_SIZE) + 2,
>-			       GFP_KERNEL);
>+			       DIV_ROUND_UP(req_size - UVCG_REQUEST_HEADER_LEN,
>+					    PAGE_SIZE) + 2, GFP_KERNEL);
> 	}
>
> 	video->req_size = req_size;
>diff --git a/drivers/usb/gadget/function/uvc_video.h b/drivers/usb/gadget/function/uvc_video.h
>index 9bf19475f6f9a..03adeefa343b7 100644
>--- a/drivers/usb/gadget/function/uvc_video.h
>+++ b/drivers/usb/gadget/function/uvc_video.h
>@@ -12,8 +12,6 @@
> #ifndef __UVC_VIDEO_H__
> #define __UVC_VIDEO_H__
>
>-#define UVCG_REQUEST_HEADER_LEN			2
>-
> struct uvc_video;
>
> int uvcg_video_enable(struct uvc_video *video, int enable);
>-- 
>2.30.2
>
>
>

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v4] usb: gadget: uvc: consistently use define for headerlen

[Michael Grzeschik]

The uvc request headerlen of 2 was defined as UVCG_REQUEST_HEADER_LEN
in commit e81e7f9a0eb9 ("usb: gadget: uvc: add scatter gather support").
We missed to use it consistently. This patch fixes that.

Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>

---
v3: - fixed wrong use of define
    - added missing use of define in uvc_video_encode_header
v4: - fixed headersize in struct uvc_request to also use the define

 drivers/usb/gadget/function/uvc.h       | 4 +++-
 drivers/usb/gadget/function/uvc_video.c | 6 +++---
 drivers/usb/gadget/function/uvc_video.h | 2 --
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h
index 9d5f17b551bbd..b05de36e2c605 100644
--- a/drivers/usb/gadget/function/uvc.h
+++ b/drivers/usb/gadget/function/uvc.h
@@ -68,6 +68,8 @@ extern unsigned int uvc_gadget_trace_param;
 #define UVC_MAX_REQUEST_SIZE			64
 #define UVC_MAX_EVENTS				4
 
+#define UVCG_REQUEST_HEADER_LEN			2
+
 /* ------------------------------------------------------------------------
  * Structures
  */
@@ -76,7 +78,7 @@ struct uvc_request {
 	u8 *req_buffer;
 	struct uvc_video *video;
 	struct sg_table sgt;
-	u8 header[2];
+	u8 header[UVCG_REQUEST_HEADER_LEN];
 };
 
 struct uvc_video {
diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
index b4a763e5f70e1..f3e97a4fc0303 100644
--- a/drivers/usb/gadget/function/uvc_video.c
+++ b/drivers/usb/gadget/function/uvc_video.c
@@ -33,7 +33,7 @@ uvc_video_encode_header(struct uvc_video *video, struct uvc_buffer *buf,
 	if (buf->bytesused - video->queue.buf_used <= len - UVCG_REQUEST_HEADER_LEN)
 		data[1] |= UVC_STREAM_EOF;
 
-	return 2;
+	return UVCG_REQUEST_HEADER_LEN;
 }
 
 static int
@@ -302,8 +302,8 @@ uvc_video_alloc_requests(struct uvc_video *video)
 		list_add_tail(&video->ureq[i].req->list, &video->req_free);
 		/* req_size/PAGE_SIZE + 1 for overruns and + 1 for header */
 		sg_alloc_table(&video->ureq[i].sgt,
-			       DIV_ROUND_UP(req_size - 2, PAGE_SIZE) + 2,
-			       GFP_KERNEL);
+			       DIV_ROUND_UP(req_size - UVCG_REQUEST_HEADER_LEN,
+					    PAGE_SIZE) + 2, GFP_KERNEL);
 	}
 
 	video->req_size = req_size;
diff --git a/drivers/usb/gadget/function/uvc_video.h b/drivers/usb/gadget/function/uvc_video.h
index 9bf19475f6f9a..03adeefa343b7 100644
--- a/drivers/usb/gadget/function/uvc_video.h
+++ b/drivers/usb/gadget/function/uvc_video.h
@@ -12,8 +12,6 @@
 #ifndef __UVC_VIDEO_H__
 #define __UVC_VIDEO_H__
 
-#define UVCG_REQUEST_HEADER_LEN			2
-
 struct uvc_video;
 
 int uvcg_video_enable(struct uvc_video *video, int enable);
-- 
2.30.2

Re: [PATCH v4] usb: gadget: uvc: consistently use define for headerlen

[Laurent Pinchart]

Hi Michael,

Thank you for the patch.

On Mon, Oct 18, 2021 at 09:20:59AM +0200, Michael Grzeschik wrote:
> The uvc request headerlen of 2 was defined as UVCG_REQUEST_HEADER_LEN
> in commit e81e7f9a0eb9 ("usb: gadget: uvc: add scatter gather support").
> We missed to use it consistently. This patch fixes that.
> 
> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
> v3: - fixed wrong use of define
>     - added missing use of define in uvc_video_encode_header
> v4: - fixed headersize in struct uvc_request to also use the define
> 
>  drivers/usb/gadget/function/uvc.h       | 4 +++-
>  drivers/usb/gadget/function/uvc_video.c | 6 +++---
>  drivers/usb/gadget/function/uvc_video.h | 2 --
>  3 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h
> index 9d5f17b551bbd..b05de36e2c605 100644
> --- a/drivers/usb/gadget/function/uvc.h
> +++ b/drivers/usb/gadget/function/uvc.h
> @@ -68,6 +68,8 @@ extern unsigned int uvc_gadget_trace_param;
>  #define UVC_MAX_REQUEST_SIZE			64
>  #define UVC_MAX_EVENTS				4
>  
> +#define UVCG_REQUEST_HEADER_LEN			2
> +
>  /* ------------------------------------------------------------------------
>   * Structures
>   */
> @@ -76,7 +78,7 @@ struct uvc_request {
>  	u8 *req_buffer;
>  	struct uvc_video *video;
>  	struct sg_table sgt;
> -	u8 header[2];
> +	u8 header[UVCG_REQUEST_HEADER_LEN];
>  };
>  
>  struct uvc_video {
> diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
> index b4a763e5f70e1..f3e97a4fc0303 100644
> --- a/drivers/usb/gadget/function/uvc_video.c
> +++ b/drivers/usb/gadget/function/uvc_video.c
> @@ -33,7 +33,7 @@ uvc_video_encode_header(struct uvc_video *video, struct uvc_buffer *buf,
>  	if (buf->bytesused - video->queue.buf_used <= len - UVCG_REQUEST_HEADER_LEN)
>  		data[1] |= UVC_STREAM_EOF;
>  
> -	return 2;
> +	return UVCG_REQUEST_HEADER_LEN;
>  }
>  
>  static int
> @@ -302,8 +302,8 @@ uvc_video_alloc_requests(struct uvc_video *video)
>  		list_add_tail(&video->ureq[i].req->list, &video->req_free);
>  		/* req_size/PAGE_SIZE + 1 for overruns and + 1 for header */
>  		sg_alloc_table(&video->ureq[i].sgt,
> -			       DIV_ROUND_UP(req_size - 2, PAGE_SIZE) + 2,
> -			       GFP_KERNEL);
> +			       DIV_ROUND_UP(req_size - UVCG_REQUEST_HEADER_LEN,
> +					    PAGE_SIZE) + 2, GFP_KERNEL);
>  	}
>  
>  	video->req_size = req_size;
> diff --git a/drivers/usb/gadget/function/uvc_video.h b/drivers/usb/gadget/function/uvc_video.h
> index 9bf19475f6f9a..03adeefa343b7 100644
> --- a/drivers/usb/gadget/function/uvc_video.h
> +++ b/drivers/usb/gadget/function/uvc_video.h
> @@ -12,8 +12,6 @@
>  #ifndef __UVC_VIDEO_H__
>  #define __UVC_VIDEO_H__
>  
> -#define UVCG_REQUEST_HEADER_LEN			2
> -
>  struct uvc_video;
>  
>  int uvcg_video_enable(struct uvc_video *video, int enable);

-- 
Regards,

Laurent Pinchart