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

[PATCH 0/7] 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.

Michael Grzeschik (6):
  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
  usb: gadget: udc: ensure the gadget is still available

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

 drivers/usb/gadget/composite.c          |  4 ++--
 drivers/usb/gadget/function/f_uvc.c     |  7 ++++---
 drivers/usb/gadget/function/uvc_v4l2.c  |  3 ++-
 drivers/usb/gadget/function/uvc_video.c | 23 ++++++++++++++++-------
 drivers/usb/gadget/udc/core.c           |  3 +++
 5 files changed, 27 insertions(+), 13 deletions(-)

-- 
2.30.2

[PATCH 1/7] 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.

Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
---
 drivers/usb/gadget/function/uvc_video.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
index b4a763e5f70e1..da93b46df464d 100644
--- a/drivers/usb/gadget/function/uvc_video.c
+++ b/drivers/usb/gadget/function/uvc_video.c
@@ -302,7 +302,9 @@ 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,
+			       DIV_ROUND_UP(req_size - UVCG_REQUEST_HEADER_LEN,
+					    PAGE_SIZE) +
+			       UVCG_REQUEST_HEADER_LEN,
 			       GFP_KERNEL);
 	}
 
-- 
2.30.2

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

[paul.elder]

Hi Michael,

On Thu, Sep 30, 2021 at 12:27:11PM +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.
> 
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>

Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

> ---
>  drivers/usb/gadget/function/uvc_video.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
> index b4a763e5f70e1..da93b46df464d 100644
> --- a/drivers/usb/gadget/function/uvc_video.c
> +++ b/drivers/usb/gadget/function/uvc_video.c
> @@ -302,7 +302,9 @@ 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,
> +			       DIV_ROUND_UP(req_size - UVCG_REQUEST_HEADER_LEN,
> +					    PAGE_SIZE) +
> +			       UVCG_REQUEST_HEADER_LEN,
>  			       GFP_KERNEL);
>  	}
>  
> -- 
> 2.30.2
> 

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

[Laurent Pinchart]

Hi Michael,

Thank you for the patch.

On Thu, Sep 30, 2021 at 12:27:11PM +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.
> 
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> ---
>  drivers/usb/gadget/function/uvc_video.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
> index b4a763e5f70e1..da93b46df464d 100644
> --- a/drivers/usb/gadget/function/uvc_video.c
> +++ b/drivers/usb/gadget/function/uvc_video.c
> @@ -302,7 +302,9 @@ 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,
> +			       DIV_ROUND_UP(req_size - UVCG_REQUEST_HEADER_LEN,

This is correct, we remove the header size.

> +					    PAGE_SIZE) +
> +			       UVCG_REQUEST_HEADER_LEN,

But here we add two entries to the sgt, not two bytes (see the above
comment), so it's unrelated to the header size.

>  			       GFP_KERNEL);
>  	}
>  

-- 
Regards,

Laurent Pinchart

[PATCH 2/7] 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.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
---
 drivers/usb/gadget/function/f_uvc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
index 9d87c0fb8f92e..1e93ab5c0c88d 100644
--- a/drivers/usb/gadget/function/f_uvc.c
+++ b/drivers/usb/gadget/function/f_uvc.c
@@ -884,12 +884,12 @@ 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 +943,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

Re: [PATCH 2/7] usb: gadget: uvc: rename function to be more consistent

[paul.elder]

Hi Michael,

On Thu, Sep 30, 2021 at 12:27:12PM +0200, Michael Grzeschik wrote:
> 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.
> 
> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>

Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

> ---
>  drivers/usb/gadget/function/f_uvc.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
> index 9d87c0fb8f92e..1e93ab5c0c88d 100644
> --- a/drivers/usb/gadget/function/f_uvc.c
> +++ b/drivers/usb/gadget/function/f_uvc.c
> @@ -884,12 +884,12 @@ 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 +943,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
> 

Re: [PATCH 2/7] usb: gadget: uvc: rename function to be more consistent

[Laurent Pinchart]

Hi Michael,

Thank you for the patch.

On Thu, Sep 30, 2021 at 12:27:12PM +0200, Michael Grzeschik wrote:
> 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.
> 
> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> ---
>  drivers/usb/gadget/function/f_uvc.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
> index 9d87c0fb8f92e..1e93ab5c0c88d 100644
> --- a/drivers/usb/gadget/function/f_uvc.c
> +++ b/drivers/usb/gadget/function/f_uvc.c
> @@ -884,12 +884,12 @@ 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)

I'd wrap this line.

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

>  {
>  	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 +943,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;

-- 
Regards,

Laurent Pinchart

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

[Michael Grzeschik]

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

Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
---
 drivers/usb/gadget/function/uvc_video.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
index da93b46df464d..cdfd3726a86ae 100644
--- a/drivers/usb/gadget/function/uvc_video.c
+++ b/drivers/usb/gadget/function/uvc_video.c
@@ -199,9 +199,11 @@ 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 (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

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

[paul.elder]

Hi Michael,

On Thu, Sep 30, 2021 at 12:27:13PM +0200, Michael Grzeschik wrote:
> The reason that the ep_queue has failed could be an disabled endpoint.

s/an/a/

> In that case it is not guaranteed that the ep->desc is still valid.
> This patch adds an check for NULL.
> 
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>

Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

> ---
>  drivers/usb/gadget/function/uvc_video.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
> index da93b46df464d..cdfd3726a86ae 100644
> --- a/drivers/usb/gadget/function/uvc_video.c
> +++ b/drivers/usb/gadget/function/uvc_video.c
> @@ -199,9 +199,11 @@ 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 (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
> 

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

[Laurent Pinchart]

Hi Michael,

Thank you for the patch.

On Thu, Sep 30, 2021 at 12:27:13PM +0200, Michael Grzeschik wrote:
> The reason that the ep_queue has failed could be an disabled endpoint.

s/an/a/

> In that case it is not guaranteed that the ep->desc is still valid.
> This patch adds an check for NULL.

s/an/a/

> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> ---
>  drivers/usb/gadget/function/uvc_video.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
> index da93b46df464d..cdfd3726a86ae 100644
> --- a/drivers/usb/gadget/function/uvc_video.c
> +++ b/drivers/usb/gadget/function/uvc_video.c
> @@ -199,9 +199,11 @@ 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);

I'd add a comment here:

		/* If the endpoint is disabled the descriptor may be NULL. */

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

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

-- 
Regards,

Laurent Pinchart

[PATCH 4/7] 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.

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 4ca89eab61590..67922b1355e69 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 cdfd3726a86ae..ccee35177411d 100644
--- a/drivers/usb/gadget/function/uvc_video.c
+++ b/drivers/usb/gadget/function/uvc_video.c
@@ -215,6 +215,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) {
@@ -237,7 +238,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

Re: [PATCH 4/7] usb: gadget: uvc: only schedule stream in streaming state

[paul.elder]

Hi Michael,

On Thu, Sep 30, 2021 at 12:27:14PM +0200, Michael Grzeschik wrote:
> 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.
> 
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>

Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

> ---
>  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 4ca89eab61590..67922b1355e69 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 cdfd3726a86ae..ccee35177411d 100644
> --- a/drivers/usb/gadget/function/uvc_video.c
> +++ b/drivers/usb/gadget/function/uvc_video.c
> @@ -215,6 +215,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) {
> @@ -237,7 +238,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
> 

Re: [PATCH 4/7] usb: gadget: uvc: only schedule stream in streaming state

[Laurent Pinchart]

Hi Michael,

Thank you for the patch.

On Thu, Sep 30, 2021 at 12:27:14PM +0200, Michael Grzeschik wrote:
> 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.
> 
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>

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

> ---
>  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 4ca89eab61590..67922b1355e69 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 cdfd3726a86ae..ccee35177411d 100644
> --- a/drivers/usb/gadget/function/uvc_video.c
> +++ b/drivers/usb/gadget/function/uvc_video.c
> @@ -215,6 +215,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) {
> @@ -237,7 +238,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

-- 
Regards,

Laurent Pinchart

[PATCH 5/7] 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 it still got enqueued, this patch ensueres that it will bail
out without handling any data.

Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
---
 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 ccee35177411d..152647495fa61 100644
--- a/drivers/usb/gadget/function/uvc_video.c
+++ b/drivers/usb/gadget/function/uvc_video.c
@@ -335,12 +335,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.
 		 */
@@ -390,6 +390,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

Re: [PATCH 5/7] usb: gadget: uvc: only pump video data if necessary

[paul.elder]

Hi Michael,

On Thu, Sep 30, 2021 at 12:27:15PM +0200, Michael Grzeschik wrote:
> If the streaming endpoint is not enabled, the worker has nothing to do.
> In the case it still got enqueued, this patch ensueres that it will bail
> out without handling any data.
> 
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>

Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

> ---
>  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 ccee35177411d..152647495fa61 100644
> --- a/drivers/usb/gadget/function/uvc_video.c
> +++ b/drivers/usb/gadget/function/uvc_video.c
> @@ -335,12 +335,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.
>  		 */
> @@ -390,6 +390,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
> 

Re: [PATCH 5/7] usb: gadget: uvc: only pump video data if necessary

[Laurent Pinchart]

Hi Michael,

Thank you for the patch.

On Thu, Sep 30, 2021 at 12:27:15PM +0200, Michael Grzeschik wrote:
> If the streaming endpoint is not enabled, the worker has nothing to do.
> In the case it still got enqueued, this patch ensueres that it will bail

s/it still got enqueued/buffers are still queued/
s/ensueres/ensures/

> out without handling any data.

Does this happen when uvc_function_set_alt() calls usb_ep_disable() ?
The current implementation will cause usb_ep_queue() (called from
uvcg_video_ep_queue(), from uvcg_video_pump()) to return an error in
that case, which will result in uvcg_queue_cancel() being called in
uvcg_video_pump(). With this patch, I believe this will still work fine
as userspace is notified of the stream off event and calls
VIDIOC_STREAMOFF, which in turn calls uvcg_video_enable(0) from
uvc_v4l2_streamoff() (or uvcg_video_enable(0) gets called from
uvc_v4l2_release() in the worst case if the application closes the
device). Could you confirm that your understanding matches this analysis
? If so,

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

> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> ---
>  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 ccee35177411d..152647495fa61 100644
> --- a/drivers/usb/gadget/function/uvc_video.c
> +++ b/drivers/usb/gadget/function/uvc_video.c
> @@ -335,12 +335,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.
>  		 */
> @@ -390,6 +390,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);

-- 
Regards,

Laurent Pinchart

Re: [PATCH 5/7] usb: gadget: uvc: only pump video data if necessary

[Michael Grzeschik]

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

On Tue, Oct 05, 2021 at 01:29:25AM +0300, Laurent Pinchart wrote:
>Hi Michael,
>
>Thank you for the patch.
>
>On Thu, Sep 30, 2021 at 12:27:15PM +0200, Michael Grzeschik wrote:
>> If the streaming endpoint is not enabled, the worker has nothing to do.
>> In the case it still got enqueued, this patch ensueres that it will bail
>
>s/it still got enqueued/buffers are still queued/
>s/ensueres/ensures/

Thanks, will fix this and your other comments on the patches in v3.

>> out without handling any data.
>
>Does this happen when uvc_function_set_alt() calls usb_ep_disable() ?
>The current implementation will cause usb_ep_queue() (called from
>uvcg_video_ep_queue(), from uvcg_video_pump()) to return an error in
>that case, which will result in uvcg_queue_cancel() being called in
>uvcg_video_pump(). With this patch, I believe this will still work fine
>as userspace is notified of the stream off event and calls
>VIDIOC_STREAMOFF, which in turn calls uvcg_video_enable(0) from
>uvc_v4l2_streamoff() (or uvcg_video_enable(0) gets called from
>uvc_v4l2_release() in the worst case if the application closes the
>device). Could you confirm that your understanding matches this analysis
>? If so,

Yes! I can confirm your analysis correct.

>
>Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
>> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
>> ---
>>  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 ccee35177411d..152647495fa61 100644
>> --- a/drivers/usb/gadget/function/uvc_video.c
>> +++ b/drivers/usb/gadget/function/uvc_video.c
>> @@ -335,12 +335,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.
>>  		 */
>> @@ -390,6 +390,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);
>
>-- 
>Regards,
>
>Laurent Pinchart
>

-- 
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 6/7] usb: gadget: uvc: ensure the vdev is unset

[Michael Grzeschik]

Since the uvc video device will be created on demand, we have to ensure
that the struct is always unprepared. Otherwise the previous settings
will colide with the new perparation.

Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
---
 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 1e93ab5c0c88d..b3279ba357331 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(struct video_device));
 	uvc->vdev.v4l2_dev = &uvc->v4l2_dev;
 	uvc->vdev.v4l2_dev->dev = &cdev->gadget->dev;
 	uvc->vdev.fops = &uvc_v4l2_fops;
-- 
2.30.2

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

[paul.elder]

On Thu, Sep 30, 2021 at 12:27:16PM +0200, Michael Grzeschik wrote:
> Since the uvc video device will be created on demand, we have to ensure

s/will be/is/

> that the struct is always unprepared. Otherwise the previous settings

s/unprepared/zeroed/

> will colide with the new perparation.

s/will/might/

s/colide/collide/

s/perparation/values/ ? Or s/perparation/preparation/

> 
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>

Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

> ---
>  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 1e93ab5c0c88d..b3279ba357331 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(struct video_device));
>  	uvc->vdev.v4l2_dev = &uvc->v4l2_dev;
>  	uvc->vdev.v4l2_dev->dev = &cdev->gadget->dev;
>  	uvc->vdev.fops = &uvc_v4l2_fops;
> -- 
> 2.30.2
> 

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

[Laurent Pinchart]

Hi Michael,

Thank you for the patch.

On Thu, Sep 30, 2021 at 12:27:16PM +0200, Michael Grzeschik wrote:
> Since the uvc video device will be created on demand, we have to ensure
> that the struct is always unprepared. Otherwise the previous settings
> will colide with the new perparation.
> 
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> ---
>  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 1e93ab5c0c88d..b3279ba357331 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(struct video_device));

	memset(&uvc->vdev, 0, sizeof(uvc->video));

With this and the commit message changes pointed out by Paul,

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

>  	uvc->vdev.v4l2_dev = &uvc->v4l2_dev;
>  	uvc->vdev.v4l2_dev->dev = &cdev->gadget->dev;
>  	uvc->vdev.fops = &uvc_v4l2_fops;

-- 
Regards,

Laurent Pinchart

[PATCH 7/7] usb: gadget: udc: ensure the gadget is still available

[Michael Grzeschik]

It is possible that the configfs gadget layer will be calling the unbind
functions of all gadget functions on gadget_dev_desc_UDC_store and
cleaned up the cdev structures pointer to the gadget. This will not
prevent the usage of the usb_function_de/activate functions.

f_obex and f_uvc are the candidates to still call the functions with no
valid gadget set. This patch prevents the usage of the gadget if it was
already unset.

Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
---
 drivers/usb/gadget/composite.c | 4 ++--
 drivers/usb/gadget/udc/core.c  | 3 +++
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index 504c1cbc255d1..1b4cd01e2ae13 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -393,7 +393,7 @@ int usb_function_deactivate(struct usb_function *function)
 
 	spin_lock_irqsave(&cdev->lock, flags);
 
-	if (cdev->deactivations == 0) {
+	if (cdev->deactivations == 0 && cdev->gadget) {
 		spin_unlock_irqrestore(&cdev->lock, flags);
 		status = usb_gadget_deactivate(cdev->gadget);
 		spin_lock_irqsave(&cdev->lock, flags);
@@ -428,7 +428,7 @@ int usb_function_activate(struct usb_function *function)
 		status = -EINVAL;
 	else {
 		cdev->deactivations--;
-		if (cdev->deactivations == 0) {
+		if (cdev->deactivations == 0 && cdev->gadget) {
 			spin_unlock_irqrestore(&cdev->lock, flags);
 			status = usb_gadget_activate(cdev->gadget);
 			spin_lock_irqsave(&cdev->lock, flags);
diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
index 14fdf918ecfeb..52964d0e26fa6 100644
--- a/drivers/usb/gadget/udc/core.c
+++ b/drivers/usb/gadget/udc/core.c
@@ -756,6 +756,9 @@ int usb_gadget_deactivate(struct usb_gadget *gadget)
 {
 	int ret = 0;
 
+	if (!gadget)
+		return ret;
+
 	if (gadget->deactivated)
 		goto out;
 
-- 
2.30.2

Re: [PATCH 7/7] usb: gadget: udc: ensure the gadget is still available

[Laurent Pinchart]

Hi Michael,

Thank you for the patch.

On Thu, Sep 30, 2021 at 12:27:17PM +0200, Michael Grzeschik wrote:
> It is possible that the configfs gadget layer will be calling the unbind
> functions of all gadget functions on gadget_dev_desc_UDC_store and
> cleaned up the cdev structures pointer to the gadget. This will not
> prevent the usage of the usb_function_de/activate functions.
> 
> f_obex and f_uvc are the candidates to still call the functions with no
> valid gadget set. This patch prevents the usage of the gadget if it was
> already unset.
> 
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> ---
>  drivers/usb/gadget/composite.c | 4 ++--
>  drivers/usb/gadget/udc/core.c  | 3 +++
>  2 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
> index 504c1cbc255d1..1b4cd01e2ae13 100644
> --- a/drivers/usb/gadget/composite.c
> +++ b/drivers/usb/gadget/composite.c
> @@ -393,7 +393,7 @@ int usb_function_deactivate(struct usb_function *function)
>  
>  	spin_lock_irqsave(&cdev->lock, flags);
>  
> -	if (cdev->deactivations == 0) {
> +	if (cdev->deactivations == 0 && cdev->gadget) {
>  		spin_unlock_irqrestore(&cdev->lock, flags);
>  		status = usb_gadget_deactivate(cdev->gadget);
>  		spin_lock_irqsave(&cdev->lock, flags);
> @@ -428,7 +428,7 @@ int usb_function_activate(struct usb_function *function)
>  		status = -EINVAL;
>  	else {
>  		cdev->deactivations--;
> -		if (cdev->deactivations == 0) {
> +		if (cdev->deactivations == 0 && cdev->gadget) {
>  			spin_unlock_irqrestore(&cdev->lock, flags);
>  			status = usb_gadget_activate(cdev->gadget);
>  			spin_lock_irqsave(&cdev->lock, flags);
> diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
> index 14fdf918ecfeb..52964d0e26fa6 100644
> --- a/drivers/usb/gadget/udc/core.c
> +++ b/drivers/usb/gadget/udc/core.c
> @@ -756,6 +756,9 @@ int usb_gadget_deactivate(struct usb_gadget *gadget)
>  {
>  	int ret = 0;
>  
> +	if (!gadget)
> +		return ret;
> +

As far as I can tell, the only caller of usb_gadget_deactivate() is
usb_function_deactivate(). With the NULL check in
usb_function_deactivate(), do we need one here ? If so, I would expect a
similar NULL check in usb_gadget_activate().

Apart from this the patch looks ok to me, but I'm not sure if it's
really fixing the problem globally or only in specific places. I'll let
Felipe comment on this.

>  	if (gadget->deactivated)
>  		goto out;
>  

-- 
Regards,

Laurent Pinchart