Hi Michael,
Thank you for the patch.
On Mon, Sep 11, 2023 at 04:05:28PM +0200, Michael Grzeschik wrote:
> Since the uvc-video gadget driver is using the v4l2 interface,
> the streamon and streamoff can be triggered at any times. To ensure
> that the pump worker will be closed as soon the userspace is
> calling streamoff we synchronize the state of the gadget ensuring
> the pump worker to bail out.
I'm sorry but I really dislike this. Not only does the patch fail to
ensure real synchronization, as the uvcg_video_pump() function still
runs asynchronously, it messes up the usage of the state field that now
tracks the state both from a host point of view (which it was doing so
far, updating the state based on callbacks from the UDC), and from a
gadget userspace point of view. This lacks clarity and is confusing.
Furthermore, the commit message doesn't even explain what issue is being
fixed here.
Greg, I think this series has been merged too soon :-(
> Signed-off-by: Michael Grzeschik <m.grzeschik@xxxxxxxxxxxxxx>
> ---
> v1 -> v2: Fixed the missing uvc variable in uvcg_video_enable
>
> drivers/usb/gadget/function/uvc_video.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
> index 91af3b1ef0d412..4b68a3a9815d73 100644
> --- a/drivers/usb/gadget/function/uvc_video.c
> +++ b/drivers/usb/gadget/function/uvc_video.c
> @@ -384,13 +384,14 @@ static void uvcg_video_pump(struct work_struct *work)
> struct uvc_video_queue *queue = &video->queue;
> /* video->max_payload_size is only set when using bulk transfer */
> bool is_bulk = video->max_payload_size;
> + struct uvc_device *uvc = video->uvc;
> struct usb_request *req = NULL;
> struct uvc_buffer *buf;
> unsigned long flags;
> bool buf_done;
> int ret;
>
> - while (video->ep->enabled) {
> + while (video->ep->enabled && uvc->state == UVC_STATE_STREAMING) {
> /*
> * Retrieve the first available USB request, protected by the
> * request lock.
> @@ -488,6 +489,7 @@ static void uvcg_video_pump(struct work_struct *work)
> */
> int uvcg_video_enable(struct uvc_video *video, int enable)
> {
> + struct uvc_device *uvc = video->uvc;
> unsigned int i;
> int ret;
>
> @@ -498,6 +500,8 @@ int uvcg_video_enable(struct uvc_video *video, int enable)
> }
>
> if (!enable) {
> + uvc->state = UVC_STATE_CONNECTED;
> +
> cancel_work_sync(&video->pump);
> uvcg_queue_cancel(&video->queue, 0);
>
> @@ -523,6 +527,8 @@ int uvcg_video_enable(struct uvc_video *video, int enable)
> video->encode = video->queue.use_sg ?
> uvc_video_encode_isoc_sg : uvc_video_encode_isoc;
>
> + uvc->state = UVC_STATE_STREAMING;
> +
You're now setting the state to UVC_STATE_STREAMING both here and in
uvc_v4l2_streamon(). That's confusing.
> video->req_int_count = 0;
>
> queue_work(video->async_wq, &video->pump);
--
Regards,
Laurent Pinchart
