| Message ID | 20231121-guenter-mini-v3-1-d8a5eae2312b@chromium.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | uvcvideo: Attempt N to land UVC race conditions fixes | expand |
Hi Sergey On Wed, 22 Nov 2023 at 08:21, Sergey Senozhatsky <senozhatsky@chromium.org> wrote: > > On (23/11/21 19:53), Ricardo Ribalda wrote: > > uvc_status_stop() handles properly the race conditions with the > > asynchronous worker. > > Let's use uvc_status_stop() for all the code paths that require stopping > > it. > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > --- > > drivers/media/usb/uvc/uvc_ctrl.c | 4 ---- > > drivers/media/usb/uvc/uvc_status.c | 2 +- > > 2 files changed, 1 insertion(+), 5 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_status.c b/drivers/media/usb/uvc/uvc_status.c > > index a78a88c710e2..0208612a9f12 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); > > Sort of feels like this needs dev->lock somewhere here. Should we move 3/3 > to the head of the series? > > The question is, can this be called in parallel with uvc_v4l2_release(), > for instance? I can be called in parallel with uvc_v4l2_release(), but uvc_status_stop() is thread-safe and does not take any locks after: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=619d9b710cf06f7a00a17120ca92333684ac45a8 So this "should" be good. key-word here is should :P > > > uvc_input_unregister(dev); > > }
Hi Sergey On Wed, 22 Nov 2023 at 08:35, Ricardo Ribalda <ribalda@chromium.org> wrote: > > Hi Sergey > > On Wed, 22 Nov 2023 at 08:21, Sergey Senozhatsky > <senozhatsky@chromium.org> wrote: > > > > On (23/11/21 19:53), Ricardo Ribalda wrote: > > > uvc_status_stop() handles properly the race conditions with the > > > asynchronous worker. > > > Let's use uvc_status_stop() for all the code paths that require stopping > > > it. > > > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > > --- > > > drivers/media/usb/uvc/uvc_ctrl.c | 4 ---- > > > drivers/media/usb/uvc/uvc_status.c | 2 +- > > > 2 files changed, 1 insertion(+), 5 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_status.c b/drivers/media/usb/uvc/uvc_status.c > > > index a78a88c710e2..0208612a9f12 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); > > > > Sort of feels like this needs dev->lock somewhere here. Should we move 3/3 > > to the head of the series? > > > > The question is, can this be called in parallel with uvc_v4l2_release(), > > for instance? > > I can be called in parallel with uvc_v4l2_release(), but > uvc_status_stop() is thread-safe and does not take any locks after: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=619d9b710cf06f7a00a17120ca92333684ac45a8 > > So this "should" be good. key-word here is should :P To be on the safe side I am not going to run the async work on the release path. will send a new revision > > > > > > > uvc_input_unregister(dev); > > > } > > > > -- > Ricardo Ribalda
Hi Sakari On Wed, 22 Nov 2023 at 10:58, Sakari Ailus <sakari.ailus@iki.fi> wrote: > > Hi Ricardo, > > On Tue, Nov 21, 2023 at 07:53:48PM +0000, Ricardo Ribalda wrote: > > uvc_status_stop() handles properly the race conditions with the > > asynchronous worker. > > Let's use uvc_status_stop() for all the code paths that require stopping > > it. > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com> Thanks! I have slightly changed the code in v3 and kept your tag, hope that it is fine. Regards! > > -- > Sakari Ailus
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_status.c b/drivers/media/usb/uvc/uvc_status.c index a78a88c710e2..0208612a9f12 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); uvc_input_unregister(dev); }
uvc_status_stop() handles properly the race conditions with the asynchronous worker. Let's use uvc_status_stop() for all the code paths that require stopping it. Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> --- drivers/media/usb/uvc/uvc_ctrl.c | 4 ---- drivers/media/usb/uvc/uvc_status.c | 2 +- 2 files changed, 1 insertion(+), 5 deletions(-)