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