Hi Dan,
Thank you for the patch.
On Thu, Jun 15, 2023 at 10:34:06AM +0100, Daniel Scally wrote:
> Currently the UVC Gadget's .set_alt() callback disables the USB
> endpoint, following which a V4L2 event is queued that closes
> down the workqueue. This ordering results in repeated calls to
> usb_ep_queue() from the workqueue whilst usb_ep_disable() is
> running - behaviour that the documentation for usb_ep_disable()
> specifically prohibits.
>
> Move the call to usb_ep_disable() until after cancel_work_sync(),
> which will guarantee the endpoint is no longer in use when
> usb_ep_disable() is called.
>
> Signed-off-by: Daniel Scally <dan.scally@xxxxxxxxxxxxxxxx>
> ---
> drivers/usb/gadget/function/f_uvc.c | 3 ---
> drivers/usb/gadget/function/uvc_video.c | 4 ++++
> 2 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
> index 5e919fb65833..4b91bd572a83 100644
> --- a/drivers/usb/gadget/function/f_uvc.c
> +++ b/drivers/usb/gadget/function/f_uvc.c
> @@ -337,9 +337,6 @@ uvc_function_set_alt(struct usb_function *f, unsigned interface, unsigned alt)
> if (uvc->state != UVC_STATE_STREAMING)
> return 0;
>
> - if (uvc->video.ep)
> - usb_ep_disable(uvc->video.ep);
> -
> memset(&v4l2_event, 0, sizeof(v4l2_event));
> v4l2_event.type = UVC_EVENT_STREAMOFF;
> v4l2_event_queue(&uvc->vdev, &v4l2_event);
If we don't disable the endpoint here, should we return
USB_GADGET_DELAYED_STATUS here (and call uvc_function_setup_continue()
in uvc_v4l2_streamoff()) or is that not needed ? The uvc->state updated
should then possibly be moved to uvc_v4l2_streamoff() (checking if this
would cause any race condition would also be a nice bonus :-)).
> diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
> index 91af3b1ef0d4..c7e38fa26492 100644
> --- a/drivers/usb/gadget/function/uvc_video.c
> +++ b/drivers/usb/gadget/function/uvc_video.c
> @@ -499,6 +499,10 @@ int uvcg_video_enable(struct uvc_video *video, int enable)
>
> if (!enable) {
> cancel_work_sync(&video->pump);
> +
> + if (video->ep)
> + usb_ep_disable(video->ep);
> +
> uvcg_queue_cancel(&video->queue, 0);
>
> for (i = 0; i < video->uvc_num_requests; ++i)
--
Regards,
Laurent Pinchart
