| Message ID | 20231024183605.908253-1-arakesh@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v8,1/4] usb: gadget: uvc: prevent use of disabled endpoint | expand |
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@pendragon.ideasonboard.com/ > Link: https://lore.kernel.org/20230531085544.253363-1-dan.scally@ideasonboard.com/ > Reviewed-by: Michael Grzeschik <m.grzeschik@pengutronix.de> > Tested-by: Michael Grzeschik <m.grzeschik@pengutronix.de> > Signed-off-by: Avichal Rakesh <arakesh@google.com> > --- > 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.
On Thu, Oct 26, 2023 at 01:23:44PM -0700, Avichal Rakesh wrote: > 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. I would like their review first please. thanks, greg k-h
Good morning On 27/10/2023 11:51, Greg KH wrote: > On Thu, Oct 26, 2023 at 01:23:44PM -0700, Avichal Rakesh wrote: >> 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. > I would like their review first please. Apologies for the delay - I am reviewing it now. > > thanks, > > greg k-h
Hi Avichal On 24/10/2023 19: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@pendragon.ideasonboard.com/ > Link: https://lore.kernel.org/20230531085544.253363-1-dan.scally@ideasonboard.com/ Thank you for picking this up! And I'm sorry to have been so remiss looking at your patches - I will try to be more responsive to them. I spotted a typo below, but otherwise: Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com> > Reviewed-by: Michael Grzeschik <m.grzeschik@pengutronix.de> > Tested-by: Michael Grzeschik <m.grzeschik@pengutronix.de> > Signed-off-by: Avichal Rakesh <arakesh@google.com> > --- > 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); s/disale_ep/disable_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
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.