| Message ID | 20231122-guenter-mini-v5-2-15d8cd8ed74f@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | uvcvideo: Attempt N to land UVC race conditions fixes | expand |
Hi Ricardo, Thank you for the patch. On Wed, Nov 22, 2023 at 11:45:48AM +0000, Ricardo Ribalda wrote: > The only thread safe way to stop the status handler is with uvc_status. The commit message should explain why. > Let's remove all the code paths partially stopping uvc_status. And should explain what the implications are. > Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > --- > drivers/media/usb/uvc/uvc_ctrl.c | 4 ---- > drivers/media/usb/uvc/uvc_driver.c | 2 +- > drivers/media/usb/uvc/uvc_status.c | 8 ++++---- > drivers/media/usb/uvc/uvc_v4l2.c | 2 +- > drivers/media/usb/uvc/uvcvideo.h | 2 +- > 5 files changed, 7 insertions(+), 11 deletions(-) > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c > index e59a463c2761..8e22a07e3e7b 100644 > --- a/drivers/media/usb/uvc/uvc_ctrl.c > +++ b/drivers/media/usb/uvc/uvc_ctrl.c > @@ -2765,10 +2765,6 @@ void uvc_ctrl_cleanup_device(struct uvc_device *dev) > struct uvc_entity *entity; > unsigned int i; > > - /* Can be uninitialized if we are aborting on probe error. */ > - if (dev->async_ctrl.work.func) > - cancel_work_sync(&dev->async_ctrl.work); > - > /* Free controls and control mappings for all entities. */ > list_for_each_entry(entity, &dev->entities, list) { > for (i = 0; i < entity->ncontrols; ++i) { > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c > index ded2cb6ce14f..d5dbf2644272 100644 > --- a/drivers/media/usb/uvc/uvc_driver.c > +++ b/drivers/media/usb/uvc/uvc_driver.c > @@ -2282,7 +2282,7 @@ static int uvc_suspend(struct usb_interface *intf, pm_message_t message) > UVC_SC_VIDEOCONTROL) { > mutex_lock(&dev->lock); > if (dev->users) > - uvc_status_stop(dev); > + uvc_status_stop(dev, true); > mutex_unlock(&dev->lock); > return 0; > } > diff --git a/drivers/media/usb/uvc/uvc_status.c b/drivers/media/usb/uvc/uvc_status.c > index a78a88c710e2..9c5da1244999 100644 > --- a/drivers/media/usb/uvc/uvc_status.c > +++ b/drivers/media/usb/uvc/uvc_status.c > @@ -292,7 +292,7 @@ int uvc_status_init(struct uvc_device *dev) > > void uvc_status_unregister(struct uvc_device *dev) > { > - usb_kill_urb(dev->int_urb); > + uvc_status_stop(dev, false); > uvc_input_unregister(dev); > } > > @@ -310,7 +310,7 @@ int uvc_status_start(struct uvc_device *dev, gfp_t flags) > return usb_submit_urb(dev->int_urb, flags); > } > > -void uvc_status_stop(struct uvc_device *dev) > +void uvc_status_stop(struct uvc_device *dev, bool run_async_work) > { > struct uvc_ctrl_work *w = &dev->async_ctrl; > > @@ -326,7 +326,7 @@ void uvc_status_stop(struct uvc_device *dev) > * Cancel any pending asynchronous work. If any status event was queued, > * process it synchronously. > */ > - if (cancel_work_sync(&w->work)) > + if (cancel_work_sync(&w->work) && run_async_work) > uvc_ctrl_status_event(w->chain, w->ctrl, w->data); > > /* Kill the urb. */ > @@ -338,7 +338,7 @@ void uvc_status_stop(struct uvc_device *dev) > * cancelled before returning or it could then race with a future > * uvc_status_start() call. > */ > - if (cancel_work_sync(&w->work)) > + if (cancel_work_sync(&w->work) && run_async_work) > uvc_ctrl_status_event(w->chain, w->ctrl, w->data); > > /* > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c > index f4988f03640a..f90206263ff4 100644 > --- a/drivers/media/usb/uvc/uvc_v4l2.c > +++ b/drivers/media/usb/uvc/uvc_v4l2.c > @@ -672,7 +672,7 @@ static int uvc_v4l2_release(struct file *file) > > mutex_lock(&stream->dev->lock); > if (--stream->dev->users == 0) > - uvc_status_stop(stream->dev); > + uvc_status_stop(stream->dev, false); > mutex_unlock(&stream->dev->lock); > > usb_autopm_put_interface(stream->dev->intf); > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h > index 6fb0a78b1b00..ba8f8c1f2c83 100644 > --- a/drivers/media/usb/uvc/uvcvideo.h > +++ b/drivers/media/usb/uvc/uvcvideo.h > @@ -745,7 +745,7 @@ int uvc_status_init(struct uvc_device *dev); > void uvc_status_unregister(struct uvc_device *dev); > void uvc_status_cleanup(struct uvc_device *dev); > int uvc_status_start(struct uvc_device *dev, gfp_t flags); > -void uvc_status_stop(struct uvc_device *dev); > +void uvc_status_stop(struct uvc_device *dev, bool run_async_work); > > /* Controls */ > extern const struct uvc_control_mapping uvc_ctrl_power_line_mapping_limited; >
diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c index e59a463c2761..8e22a07e3e7b 100644 --- a/drivers/media/usb/uvc/uvc_ctrl.c +++ b/drivers/media/usb/uvc/uvc_ctrl.c @@ -2765,10 +2765,6 @@ void uvc_ctrl_cleanup_device(struct uvc_device *dev) struct uvc_entity *entity; unsigned int i; - /* Can be uninitialized if we are aborting on probe error. */ - if (dev->async_ctrl.work.func) - cancel_work_sync(&dev->async_ctrl.work); - /* Free controls and control mappings for all entities. */ list_for_each_entry(entity, &dev->entities, list) { for (i = 0; i < entity->ncontrols; ++i) { diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c index ded2cb6ce14f..d5dbf2644272 100644 --- a/drivers/media/usb/uvc/uvc_driver.c +++ b/drivers/media/usb/uvc/uvc_driver.c @@ -2282,7 +2282,7 @@ static int uvc_suspend(struct usb_interface *intf, pm_message_t message) UVC_SC_VIDEOCONTROL) { mutex_lock(&dev->lock); if (dev->users) - uvc_status_stop(dev); + uvc_status_stop(dev, true); mutex_unlock(&dev->lock); return 0; } diff --git a/drivers/media/usb/uvc/uvc_status.c b/drivers/media/usb/uvc/uvc_status.c index a78a88c710e2..9c5da1244999 100644 --- a/drivers/media/usb/uvc/uvc_status.c +++ b/drivers/media/usb/uvc/uvc_status.c @@ -292,7 +292,7 @@ int uvc_status_init(struct uvc_device *dev) void uvc_status_unregister(struct uvc_device *dev) { - usb_kill_urb(dev->int_urb); + uvc_status_stop(dev, false); uvc_input_unregister(dev); } @@ -310,7 +310,7 @@ int uvc_status_start(struct uvc_device *dev, gfp_t flags) return usb_submit_urb(dev->int_urb, flags); } -void uvc_status_stop(struct uvc_device *dev) +void uvc_status_stop(struct uvc_device *dev, bool run_async_work) { struct uvc_ctrl_work *w = &dev->async_ctrl; @@ -326,7 +326,7 @@ void uvc_status_stop(struct uvc_device *dev) * Cancel any pending asynchronous work. If any status event was queued, * process it synchronously. */ - if (cancel_work_sync(&w->work)) + if (cancel_work_sync(&w->work) && run_async_work) uvc_ctrl_status_event(w->chain, w->ctrl, w->data); /* Kill the urb. */ @@ -338,7 +338,7 @@ void uvc_status_stop(struct uvc_device *dev) * cancelled before returning or it could then race with a future * uvc_status_start() call. */ - if (cancel_work_sync(&w->work)) + if (cancel_work_sync(&w->work) && run_async_work) uvc_ctrl_status_event(w->chain, w->ctrl, w->data); /* diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c index f4988f03640a..f90206263ff4 100644 --- a/drivers/media/usb/uvc/uvc_v4l2.c +++ b/drivers/media/usb/uvc/uvc_v4l2.c @@ -672,7 +672,7 @@ static int uvc_v4l2_release(struct file *file) mutex_lock(&stream->dev->lock); if (--stream->dev->users == 0) - uvc_status_stop(stream->dev); + uvc_status_stop(stream->dev, false); mutex_unlock(&stream->dev->lock); usb_autopm_put_interface(stream->dev->intf); diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h index 6fb0a78b1b00..ba8f8c1f2c83 100644 --- a/drivers/media/usb/uvc/uvcvideo.h +++ b/drivers/media/usb/uvc/uvcvideo.h @@ -745,7 +745,7 @@ int uvc_status_init(struct uvc_device *dev); void uvc_status_unregister(struct uvc_device *dev); void uvc_status_cleanup(struct uvc_device *dev); int uvc_status_start(struct uvc_device *dev, gfp_t flags); -void uvc_status_stop(struct uvc_device *dev); +void uvc_status_stop(struct uvc_device *dev, bool run_async_work); /* Controls */ extern const struct uvc_control_mapping uvc_ctrl_power_line_mapping_limited;