On 10/24/23 11:36, Avichal Rakesh wrote: > Currently the set_alt callback immediately disables the endpoint and queues > the v4l2 streamoff event. However, as the streamoff event is processed > asynchronously, it is possible that the video_pump thread attempts to queue > requests to an already disabled endpoint. > > This change moves disabling usb endpoint to the end of streamoff event > callback. As the endpoint's state can no longer be used, video_pump is > now guarded by uvc->state as well. To be consistent with the actual > streaming state, uvc->state is now toggled between CONNECTED and STREAMING > from the v4l2 event callback only. > > Link: https://lore.kernel.org/20230615171558.GK741@xxxxxxxxxxxxxxxxxxxxxxxxxx/ > Link: https://lore.kernel.org/20230531085544.253363-1-dan.scally@xxxxxxxxxxxxxxxx/ > Reviewed-by: Michael Grzeschik <m.grzeschik@xxxxxxxxxxxxxx> > Tested-by: Michael Grzeschik <m.grzeschik@xxxxxxxxxxxxxx> > Signed-off-by: Avichal Rakesh <arakesh@xxxxxxxxxx> > --- > v1 -> v2: Rebased to ToT and reworded commit message. > v2 -> v3: Fix email threading goof-up > v3 -> v4: Address review comments & re-rebase to ToT > v4 -> v5: Add Reviewed-by & Tested-by > v5 -> v6: No change > v6 -> v7: No change > v7 -> v8: No change. Getting back in review queue > > drivers/usb/gadget/function/f_uvc.c | 11 +++++------ > drivers/usb/gadget/function/f_uvc.h | 2 +- > drivers/usb/gadget/function/uvc.h | 2 +- > drivers/usb/gadget/function/uvc_v4l2.c | 20 +++++++++++++++++--- > drivers/usb/gadget/function/uvc_video.c | 3 ++- > 5 files changed, 26 insertions(+), 12 deletions(-) > > diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c > index faa398109431..ae08341961eb 100644 > --- a/drivers/usb/gadget/function/f_uvc.c > +++ b/drivers/usb/gadget/function/f_uvc.c > @@ -263,10 +263,13 @@ uvc_function_setup(struct usb_function *f, const struct usb_ctrlrequest *ctrl) > return 0; > } > > -void uvc_function_setup_continue(struct uvc_device *uvc) > +void uvc_function_setup_continue(struct uvc_device *uvc, int disable_ep) > { > struct usb_composite_dev *cdev = uvc->func.config->cdev; > > + if (disable_ep && uvc->video.ep) > + usb_ep_disable(uvc->video.ep); > + > usb_composite_setup_continue(cdev); > } > > @@ -337,15 +340,11 @@ 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); > > - uvc->state = UVC_STATE_CONNECTED; > - return 0; > + return USB_GADGET_DELAYED_STATUS; > > case 1: > if (uvc->state != UVC_STATE_CONNECTED) > diff --git a/drivers/usb/gadget/function/f_uvc.h b/drivers/usb/gadget/function/f_uvc.h > index 1db972d4beeb..e7f9f13f14dc 100644 > --- a/drivers/usb/gadget/function/f_uvc.h > +++ b/drivers/usb/gadget/function/f_uvc.h > @@ -11,7 +11,7 @@ > > struct uvc_device; > > -void uvc_function_setup_continue(struct uvc_device *uvc); > +void uvc_function_setup_continue(struct uvc_device *uvc, int disale_ep); > > void uvc_function_connect(struct uvc_device *uvc); > > diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h > index 6751de8b63ad..989bc6b4e93d 100644 > --- a/drivers/usb/gadget/function/uvc.h > +++ b/drivers/usb/gadget/function/uvc.h > @@ -177,7 +177,7 @@ struct uvc_file_handle { > * Functions > */ > > -extern void uvc_function_setup_continue(struct uvc_device *uvc); > +extern void uvc_function_setup_continue(struct uvc_device *uvc, int disable_ep); > extern void uvc_function_connect(struct uvc_device *uvc); > extern void uvc_function_disconnect(struct uvc_device *uvc); > > diff --git a/drivers/usb/gadget/function/uvc_v4l2.c b/drivers/usb/gadget/function/uvc_v4l2.c > index 3f0a9795c0d4..7cb8d027ff0c 100644 > --- a/drivers/usb/gadget/function/uvc_v4l2.c > +++ b/drivers/usb/gadget/function/uvc_v4l2.c > @@ -451,7 +451,7 @@ uvc_v4l2_streamon(struct file *file, void *fh, enum v4l2_buf_type type) > * Complete the alternate setting selection setup phase now that > * userspace is ready to provide video frames. > */ > - uvc_function_setup_continue(uvc); > + uvc_function_setup_continue(uvc, 0); > uvc->state = UVC_STATE_STREAMING; > > return 0; > @@ -463,11 +463,18 @@ uvc_v4l2_streamoff(struct file *file, void *fh, enum v4l2_buf_type type) > struct video_device *vdev = video_devdata(file); > struct uvc_device *uvc = video_get_drvdata(vdev); > struct uvc_video *video = &uvc->video; > + int ret = 0; > > if (type != video->queue.queue.type) > return -EINVAL; > > - return uvcg_video_enable(video, 0); > + uvc->state = UVC_STATE_CONNECTED; > + ret = uvcg_video_enable(video, 0); > + if (ret < 0) > + return ret; > + > + uvc_function_setup_continue(uvc, 1); > + return 0; > } > > static int > @@ -500,6 +507,14 @@ uvc_v4l2_subscribe_event(struct v4l2_fh *fh, > static void uvc_v4l2_disable(struct uvc_device *uvc) > { > uvc_function_disconnect(uvc); > + /* > + * Drop uvc->state to CONNECTED if it was streaming before. > + * This ensures that the usb_requests are no longer queued > + * to the controller. > + */ > + if (uvc->state == UVC_STATE_STREAMING) > + uvc->state = UVC_STATE_CONNECTED; > + > uvcg_video_enable(&uvc->video, 0); > uvcg_free_buffers(&uvc->video.queue); > uvc->func_connected = false; > @@ -647,4 +662,3 @@ const struct v4l2_file_operations uvc_v4l2_fops = { > .get_unmapped_area = uvcg_v4l2_get_unmapped_area, > #endif > }; > - > diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c > index 91af3b1ef0d4..c334802ac0a4 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 (uvc->state == UVC_STATE_STREAMING && video->ep->enabled) { > /* > * Retrieve the first available USB request, protected by the > * request lock. > -- > 2.42.0.758.gaed0368e0e-goog Hey Greg, Considering Laurent and Dan haven't responded, and Michael and I have tested this change, would it be possible to merge this patch set if the changes look OK to you? I don't think there are any outstanding items to be done around these fixes. Thank you! - Avi.
