diff mbox series

[v2,1/3] media: uvcvideo: Cancel async worker earlier

Message ID	20230309-guenter-mini-v2-1-e6410d590d43@chromium.org
State	New
Headers	Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; From: Ricardo Ribalda <ribalda@chromium.org> Date: Thu, 09 Mar 2023 15:44:05 +0100 Subject: [PATCH v2 1/3] media: uvcvideo: Cancel async worker earlier MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Message-Id: <20230309-guenter-mini-v2-1-e6410d590d43@chromium.org> References: <20230309-guenter-mini-v2-0-e6410d590d43@chromium.org> In-Reply-To: <20230309-guenter-mini-v2-0-e6410d590d43@chromium.org> To: Mauro Carvalho Chehab <mchehab@kernel.org> Cc: Ricardo Ribalda <ribalda@chromium.org>, Laurent Pinchart <laurent.pinchart@ideasonboard.com>, Max Staudt <mstaudt@chromium.org>, Alan Stern <stern@rowland.harvard.edu>, Sakari Ailus <sakari.ailus@linux.intel.com>, Guenter Roeck <linux@roeck-us.net>, linux-media@vger.kernel.org, linux-kernel@vger.kernel.org, Sean Paul <seanpaul@chromium.org>, Tomasz Figa <tfiga@chromium.org>, Hans Verkuil <hverkuil-cisco@xs4all.nl> Precedence: bulk
Series	uvcvideo: Attempt N to land UVC race conditions fixes \| [v2,0/3] uvcvideo: Attempt N to land UVC race conditions fixes [v2,1/3] media: uvcvideo: Cancel async worker earlier [v2,2/3] media: uvcvideo: Lock video streams and queues while unregistering [v2,3/3] media: uvcvideo: Release stream queue when unregistering video device

Commit Message

Ricardo Ribalda March 9, 2023, 2:44 p.m. UTC

  From: Guenter Roeck <linux@roeck-us.net>

So far the asynchronous control worker was canceled only in
uvc_ctrl_cleanup_device. This is much later than the call to
uvc_disconnect. However, after the call to uvc_disconnect returns,
there must be no more USB activity. This can result in all kinds
of problems in the USB code. One observed example:

URB ffff993e83d0bc00 submitted while active
WARNING: CPU: 0 PID: 4046 at drivers/usb/core/urb.c:364 usb_submit_urb+0x4ba/0x55e
Modules linked in: <...>
CPU: 0 PID: 4046 Comm: kworker/0:35 Not tainted 4.19.139 #18
Hardware name: Google Phaser/Phaser, BIOS Google_Phaser.10952.0.0 08/09/2018
Workqueue: events uvc_ctrl_status_event_work [uvcvideo]
RIP: 0010:usb_submit_urb+0x4ba/0x55e
Code: <...>
RSP: 0018:ffffb08d471ebde8 EFLAGS: 00010246
RAX: a6da85d923ea5d00 RBX: ffff993e71985928 RCX: 0000000000000000
RDX: ffff993f37a1de90 RSI: ffff993f37a153d0 RDI: ffff993f37a153d0
RBP: ffffb08d471ebe28 R08: 000000000000003b R09: 001424bf85822e96
R10: 0000001000000000 R11: ffffffff975a4398 R12: ffff993e83d0b000
R13: ffff993e83d0bc00 R14: 0000000000000000 R15: 00000000fffffff0
FS:  0000000000000000(0000) GS:ffff993f37a00000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00000000ec9c0000 CR3: 000000025b160000 CR4: 0000000000340ef0
Call Trace:
 uvc_ctrl_status_event_work+0xd6/0x107 [uvcvideo]
 process_one_work+0x19b/0x4c5
 worker_thread+0x10d/0x286
 kthread+0x138/0x140
 ? process_one_work+0x4c5/0x4c5
 ? kthread_associate_blkcg+0xc1/0xc1
 ret_from_fork+0x1f/0x40

Introduce new function uvc_ctrl_stop_device() to cancel the worker
and call it from uvc_unregister_video() to solve the problem.

Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Alan Stern <stern@rowland.harvard.edu>
Cc: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Reviewed-by: Tomasz Figa <tfiga@chromium.org>
Reviewed-by: Sean Paul <seanpaul@chromium.org>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_ctrl.c   | 11 +++++++----
 drivers/media/usb/uvc/uvc_driver.c |  1 +
 drivers/media/usb/uvc/uvcvideo.h   |  1 +
 3 files changed, 9 insertions(+), 4 deletions(-)

Comments

Laurent Pinchart March 9, 2023, 2:57 p.m. UTC | #1

Hi Ricardo and Guenter,

Thank you for the patch.

On Thu, Mar 09, 2023 at 03:44:05PM +0100, Ricardo Ribalda wrote:
> From: Guenter Roeck <linux@roeck-us.net>
> 
> So far the asynchronous control worker was canceled only in
> uvc_ctrl_cleanup_device. This is much later than the call to
> uvc_disconnect. However, after the call to uvc_disconnect returns,
> there must be no more USB activity. This can result in all kinds
> of problems in the USB code. One observed example:
> 
> URB ffff993e83d0bc00 submitted while active
> WARNING: CPU: 0 PID: 4046 at drivers/usb/core/urb.c:364 usb_submit_urb+0x4ba/0x55e
> Modules linked in: <...>
> CPU: 0 PID: 4046 Comm: kworker/0:35 Not tainted 4.19.139 #18
> Hardware name: Google Phaser/Phaser, BIOS Google_Phaser.10952.0.0 08/09/2018
> Workqueue: events uvc_ctrl_status_event_work [uvcvideo]
> RIP: 0010:usb_submit_urb+0x4ba/0x55e
> Code: <...>
> RSP: 0018:ffffb08d471ebde8 EFLAGS: 00010246
> RAX: a6da85d923ea5d00 RBX: ffff993e71985928 RCX: 0000000000000000
> RDX: ffff993f37a1de90 RSI: ffff993f37a153d0 RDI: ffff993f37a153d0
> RBP: ffffb08d471ebe28 R08: 000000000000003b R09: 001424bf85822e96
> R10: 0000001000000000 R11: ffffffff975a4398 R12: ffff993e83d0b000
> R13: ffff993e83d0bc00 R14: 0000000000000000 R15: 00000000fffffff0
> FS:  0000000000000000(0000) GS:ffff993f37a00000(0000) knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00000000ec9c0000 CR3: 000000025b160000 CR4: 0000000000340ef0
> Call Trace:
>  uvc_ctrl_status_event_work+0xd6/0x107 [uvcvideo]
>  process_one_work+0x19b/0x4c5
>  worker_thread+0x10d/0x286
>  kthread+0x138/0x140
>  ? process_one_work+0x4c5/0x4c5
>  ? kthread_associate_blkcg+0xc1/0xc1
>  ret_from_fork+0x1f/0x40
> 
> Introduce new function uvc_ctrl_stop_device() to cancel the worker
> and call it from uvc_unregister_video() to solve the problem.
> 
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Alan Stern <stern@rowland.harvard.edu>
> Cc: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Reviewed-by: Tomasz Figa <tfiga@chromium.org>
> Reviewed-by: Sean Paul <seanpaul@chromium.org>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_ctrl.c   | 11 +++++++----
>  drivers/media/usb/uvc/uvc_driver.c |  1 +
>  drivers/media/usb/uvc/uvcvideo.h   |  1 +
>  3 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index 5e9d3da862dd..769c1d2a2f45 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -2757,14 +2757,17 @@ static void uvc_ctrl_cleanup_mappings(struct uvc_device *dev,
>  	}
>  }
>  
> -void uvc_ctrl_cleanup_device(struct uvc_device *dev)
> +void uvc_ctrl_stop_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);
> +}

There may be an opportunity for refactoring, as we have
uvc_status_stop() that stops the work queue, but I think this is good
enough for now. I'm wondering, though, if there could be a race
condition here similar to the one that the recent changes to
uvc_status_stop() have fixed ? As uvc_ctrl_stop_device() is called at
release time I assume that URBs have been cancelled, so there should be
no race, but a second pair of eyeballs to confirm this would be
appreciated.

Other than that, the patch looks good to me, and fixes an issue
independent from the rest of the series, so

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

I will wait for a reply regarding the race condition before queuing this
up though.

> +
> +void uvc_ctrl_cleanup_device(struct uvc_device *dev)
> +{
> +	struct uvc_entity *entity;
> +	unsigned int i;
>  
>  	/* Free controls and control mappings for all entities. */
>  	list_for_each_entry(entity, &dev->entities, list) {
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index 7aefa76a42b3..4be6dfeaa295 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -1893,6 +1893,7 @@ static void uvc_unregister_video(struct uvc_device *dev)
>  	}
>  
>  	uvc_status_unregister(dev);
> +	uvc_ctrl_stop_device(dev);
>  
>  	if (dev->vdev.dev)
>  		v4l2_device_unregister(&dev->vdev);
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index 9a596c8d894a..50f171e7381b 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -760,6 +760,7 @@ int uvc_query_v4l2_menu(struct uvc_video_chain *chain,
>  int uvc_ctrl_add_mapping(struct uvc_video_chain *chain,
>  			 const struct uvc_control_mapping *mapping);
>  int uvc_ctrl_init_device(struct uvc_device *dev);
> +void uvc_ctrl_stop_device(struct uvc_device *dev);
>  void uvc_ctrl_cleanup_device(struct uvc_device *dev);
>  int uvc_ctrl_restore_values(struct uvc_device *dev);
>  bool uvc_ctrl_status_event_async(struct urb *urb, struct uvc_video_chain *chain,

Ricardo Ribalda March 28, 2023, 7:52 a.m. UTC | #2

Hi Laurent

I have not tested it yet... but maybe something like this might be
slightly better?


diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index 5e9d3da862dd..3944404b2de2 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -2762,10 +2762,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);
 }


The benefit from Guenter patch is that it has been tested for years...

What do you think? Shall we try this approach instead?

Regards!

On Thu, 9 Mar 2023 at 15:57, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Ricardo and Guenter,
>
> Thank you for the patch.
>
> On Thu, Mar 09, 2023 at 03:44:05PM +0100, Ricardo Ribalda wrote:
> > From: Guenter Roeck <linux@roeck-us.net>
> >
> > So far the asynchronous control worker was canceled only in
> > uvc_ctrl_cleanup_device. This is much later than the call to
> > uvc_disconnect. However, after the call to uvc_disconnect returns,
> > there must be no more USB activity. This can result in all kinds
> > of problems in the USB code. One observed example:
> >
> > URB ffff993e83d0bc00 submitted while active
> > WARNING: CPU: 0 PID: 4046 at drivers/usb/core/urb.c:364 usb_submit_urb+0x4ba/0x55e
> > Modules linked in: <...>
> > CPU: 0 PID: 4046 Comm: kworker/0:35 Not tainted 4.19.139 #18
> > Hardware name: Google Phaser/Phaser, BIOS Google_Phaser.10952.0.0 08/09/2018
> > Workqueue: events uvc_ctrl_status_event_work [uvcvideo]
> > RIP: 0010:usb_submit_urb+0x4ba/0x55e
> > Code: <...>
> > RSP: 0018:ffffb08d471ebde8 EFLAGS: 00010246
> > RAX: a6da85d923ea5d00 RBX: ffff993e71985928 RCX: 0000000000000000
> > RDX: ffff993f37a1de90 RSI: ffff993f37a153d0 RDI: ffff993f37a153d0
> > RBP: ffffb08d471ebe28 R08: 000000000000003b R09: 001424bf85822e96
> > R10: 0000001000000000 R11: ffffffff975a4398 R12: ffff993e83d0b000
> > R13: ffff993e83d0bc00 R14: 0000000000000000 R15: 00000000fffffff0
> > FS:  0000000000000000(0000) GS:ffff993f37a00000(0000) knlGS:0000000000000000
> > CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > CR2: 00000000ec9c0000 CR3: 000000025b160000 CR4: 0000000000340ef0
> > Call Trace:
> >  uvc_ctrl_status_event_work+0xd6/0x107 [uvcvideo]
> >  process_one_work+0x19b/0x4c5
> >  worker_thread+0x10d/0x286
> >  kthread+0x138/0x140
> >  ? process_one_work+0x4c5/0x4c5
> >  ? kthread_associate_blkcg+0xc1/0xc1
> >  ret_from_fork+0x1f/0x40
> >
> > Introduce new function uvc_ctrl_stop_device() to cancel the worker
> > and call it from uvc_unregister_video() to solve the problem.
> >
> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Cc: Alan Stern <stern@rowland.harvard.edu>
> > Cc: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> > Reviewed-by: Tomasz Figa <tfiga@chromium.org>
> > Reviewed-by: Sean Paul <seanpaul@chromium.org>
> > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> >  drivers/media/usb/uvc/uvc_ctrl.c   | 11 +++++++----
> >  drivers/media/usb/uvc/uvc_driver.c |  1 +
> >  drivers/media/usb/uvc/uvcvideo.h   |  1 +
> >  3 files changed, 9 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> > index 5e9d3da862dd..769c1d2a2f45 100644
> > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > @@ -2757,14 +2757,17 @@ static void uvc_ctrl_cleanup_mappings(struct uvc_device *dev,
> >       }
> >  }
> >
> > -void uvc_ctrl_cleanup_device(struct uvc_device *dev)
> > +void uvc_ctrl_stop_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);
> > +}
>
> There may be an opportunity for refactoring, as we have
> uvc_status_stop() that stops the work queue, but I think this is good
> enough for now. I'm wondering, though, if there could be a race
> condition here similar to the one that the recent changes to
> uvc_status_stop() have fixed ? As uvc_ctrl_stop_device() is called at
> release time I assume that URBs have been cancelled, so there should be
> no race, but a second pair of eyeballs to confirm this would be
> appreciated.
>
> Other than that, the patch looks good to me, and fixes an issue
> independent from the rest of the series, so
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> I will wait for a reply regarding the race condition before queuing this
> up though.
>
> > +
> > +void uvc_ctrl_cleanup_device(struct uvc_device *dev)
> > +{
> > +     struct uvc_entity *entity;
> > +     unsigned int i;
> >
> >       /* Free controls and control mappings for all entities. */
> >       list_for_each_entry(entity, &dev->entities, list) {
> > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > index 7aefa76a42b3..4be6dfeaa295 100644
> > --- a/drivers/media/usb/uvc/uvc_driver.c
> > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > @@ -1893,6 +1893,7 @@ static void uvc_unregister_video(struct uvc_device *dev)
> >       }
> >
> >       uvc_status_unregister(dev);
> > +     uvc_ctrl_stop_device(dev);
> >
> >       if (dev->vdev.dev)
> >               v4l2_device_unregister(&dev->vdev);
> > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > index 9a596c8d894a..50f171e7381b 100644
> > --- a/drivers/media/usb/uvc/uvcvideo.h
> > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > @@ -760,6 +760,7 @@ int uvc_query_v4l2_menu(struct uvc_video_chain *chain,
> >  int uvc_ctrl_add_mapping(struct uvc_video_chain *chain,
> >                        const struct uvc_control_mapping *mapping);
> >  int uvc_ctrl_init_device(struct uvc_device *dev);
> > +void uvc_ctrl_stop_device(struct uvc_device *dev);
> >  void uvc_ctrl_cleanup_device(struct uvc_device *dev);
> >  int uvc_ctrl_restore_values(struct uvc_device *dev);
> >  bool uvc_ctrl_status_event_async(struct urb *urb, struct uvc_video_chain *chain,
>
> --
> Regards,
>
> Laurent Pinchart

Ricardo Ribalda May 1, 2023, 1:29 p.m. UTC | #3

Hi Laurent

Friendly ping!

On Tue, 28 Mar 2023 at 09:52, Ricardo Ribalda <ribalda@chromium.org> wrote:
>
> Hi Laurent
>
> I have not tested it yet... but maybe something like this might be
> slightly better?
>
>
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index 5e9d3da862dd..3944404b2de2 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -2762,10 +2762,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);
>  }
>
>
> The benefit from Guenter patch is that it has been tested for years...
>
> What do you think? Shall we try this approach instead?
>
> Regards!
>
> On Thu, 9 Mar 2023 at 15:57, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> >
> > Hi Ricardo and Guenter,
> >
> > Thank you for the patch.
> >
> > On Thu, Mar 09, 2023 at 03:44:05PM +0100, Ricardo Ribalda wrote:
> > > From: Guenter Roeck <linux@roeck-us.net>
> > >
> > > So far the asynchronous control worker was canceled only in
> > > uvc_ctrl_cleanup_device. This is much later than the call to
> > > uvc_disconnect. However, after the call to uvc_disconnect returns,
> > > there must be no more USB activity. This can result in all kinds
> > > of problems in the USB code. One observed example:
> > >
> > > URB ffff993e83d0bc00 submitted while active
> > > WARNING: CPU: 0 PID: 4046 at drivers/usb/core/urb.c:364 usb_submit_urb+0x4ba/0x55e
> > > Modules linked in: <...>
> > > CPU: 0 PID: 4046 Comm: kworker/0:35 Not tainted 4.19.139 #18
> > > Hardware name: Google Phaser/Phaser, BIOS Google_Phaser.10952.0.0 08/09/2018
> > > Workqueue: events uvc_ctrl_status_event_work [uvcvideo]
> > > RIP: 0010:usb_submit_urb+0x4ba/0x55e
> > > Code: <...>
> > > RSP: 0018:ffffb08d471ebde8 EFLAGS: 00010246
> > > RAX: a6da85d923ea5d00 RBX: ffff993e71985928 RCX: 0000000000000000
> > > RDX: ffff993f37a1de90 RSI: ffff993f37a153d0 RDI: ffff993f37a153d0
> > > RBP: ffffb08d471ebe28 R08: 000000000000003b R09: 001424bf85822e96
> > > R10: 0000001000000000 R11: ffffffff975a4398 R12: ffff993e83d0b000
> > > R13: ffff993e83d0bc00 R14: 0000000000000000 R15: 00000000fffffff0
> > > FS:  0000000000000000(0000) GS:ffff993f37a00000(0000) knlGS:0000000000000000
> > > CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > > CR2: 00000000ec9c0000 CR3: 000000025b160000 CR4: 0000000000340ef0
> > > Call Trace:
> > >  uvc_ctrl_status_event_work+0xd6/0x107 [uvcvideo]
> > >  process_one_work+0x19b/0x4c5
> > >  worker_thread+0x10d/0x286
> > >  kthread+0x138/0x140
> > >  ? process_one_work+0x4c5/0x4c5
> > >  ? kthread_associate_blkcg+0xc1/0xc1
> > >  ret_from_fork+0x1f/0x40
> > >
> > > Introduce new function uvc_ctrl_stop_device() to cancel the worker
> > > and call it from uvc_unregister_video() to solve the problem.
> > >
> > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > Cc: Alan Stern <stern@rowland.harvard.edu>
> > > Cc: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> > > Reviewed-by: Tomasz Figa <tfiga@chromium.org>
> > > Reviewed-by: Sean Paul <seanpaul@chromium.org>
> > > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > ---
> > >  drivers/media/usb/uvc/uvc_ctrl.c   | 11 +++++++----
> > >  drivers/media/usb/uvc/uvc_driver.c |  1 +
> > >  drivers/media/usb/uvc/uvcvideo.h   |  1 +
> > >  3 files changed, 9 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> > > index 5e9d3da862dd..769c1d2a2f45 100644
> > > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > > @@ -2757,14 +2757,17 @@ static void uvc_ctrl_cleanup_mappings(struct uvc_device *dev,
> > >       }
> > >  }
> > >
> > > -void uvc_ctrl_cleanup_device(struct uvc_device *dev)
> > > +void uvc_ctrl_stop_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);
> > > +}
> >
> > There may be an opportunity for refactoring, as we have
> > uvc_status_stop() that stops the work queue, but I think this is good
> > enough for now. I'm wondering, though, if there could be a race
> > condition here similar to the one that the recent changes to
> > uvc_status_stop() have fixed ? As uvc_ctrl_stop_device() is called at
> > release time I assume that URBs have been cancelled, so there should be
> > no race, but a second pair of eyeballs to confirm this would be
> > appreciated.
> >
> > Other than that, the patch looks good to me, and fixes an issue
> > independent from the rest of the series, so
> >
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >
> > I will wait for a reply regarding the race condition before queuing this
> > up though.
> >
> > > +
> > > +void uvc_ctrl_cleanup_device(struct uvc_device *dev)
> > > +{
> > > +     struct uvc_entity *entity;
> > > +     unsigned int i;
> > >
> > >       /* Free controls and control mappings for all entities. */
> > >       list_for_each_entry(entity, &dev->entities, list) {
> > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > > index 7aefa76a42b3..4be6dfeaa295 100644
> > > --- a/drivers/media/usb/uvc/uvc_driver.c
> > > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > > @@ -1893,6 +1893,7 @@ static void uvc_unregister_video(struct uvc_device *dev)
> > >       }
> > >
> > >       uvc_status_unregister(dev);
> > > +     uvc_ctrl_stop_device(dev);
> > >
> > >       if (dev->vdev.dev)
> > >               v4l2_device_unregister(&dev->vdev);
> > > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > > index 9a596c8d894a..50f171e7381b 100644
> > > --- a/drivers/media/usb/uvc/uvcvideo.h
> > > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > > @@ -760,6 +760,7 @@ int uvc_query_v4l2_menu(struct uvc_video_chain *chain,
> > >  int uvc_ctrl_add_mapping(struct uvc_video_chain *chain,
> > >                        const struct uvc_control_mapping *mapping);
> > >  int uvc_ctrl_init_device(struct uvc_device *dev);
> > > +void uvc_ctrl_stop_device(struct uvc_device *dev);
> > >  void uvc_ctrl_cleanup_device(struct uvc_device *dev);
> > >  int uvc_ctrl_restore_values(struct uvc_device *dev);
> > >  bool uvc_ctrl_status_event_async(struct urb *urb, struct uvc_video_chain *chain,
> >
> > --
> > Regards,
> >
> > Laurent Pinchart
>
>
>
> --
> Ricardo Ribalda

Tomasz Figa May 17, 2023, 4:05 a.m. UTC | #4

On Mon, May 1, 2023 at 10:30 PM Ricardo Ribalda <ribalda@chromium.org> wrote:
>
> Hi Laurent
>
> Friendly ping!

+Kieran and +Mauro, could you take a look at this please, since
Laurent has not responded to this for almost 2 months?

>
> On Tue, 28 Mar 2023 at 09:52, Ricardo Ribalda <ribalda@chromium.org> wrote:
> >
> > Hi Laurent
> >
> > I have not tested it yet... but maybe something like this might be
> > slightly better?
> >
> >
> > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> > index 5e9d3da862dd..3944404b2de2 100644
> > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > @@ -2762,10 +2762,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);
> >  }
> >
> >
> > The benefit from Guenter patch is that it has been tested for years...
> >
> > What do you think? Shall we try this approach instead?
> >
> > Regards!
> >
> > On Thu, 9 Mar 2023 at 15:57, Laurent Pinchart
> > <laurent.pinchart@ideasonboard.com> wrote:
> > >
> > > Hi Ricardo and Guenter,
> > >
> > > Thank you for the patch.
> > >
> > > On Thu, Mar 09, 2023 at 03:44:05PM +0100, Ricardo Ribalda wrote:
> > > > From: Guenter Roeck <linux@roeck-us.net>
> > > >
> > > > So far the asynchronous control worker was canceled only in
> > > > uvc_ctrl_cleanup_device. This is much later than the call to
> > > > uvc_disconnect. However, after the call to uvc_disconnect returns,
> > > > there must be no more USB activity. This can result in all kinds
> > > > of problems in the USB code. One observed example:
> > > >
> > > > URB ffff993e83d0bc00 submitted while active
> > > > WARNING: CPU: 0 PID: 4046 at drivers/usb/core/urb.c:364 usb_submit_urb+0x4ba/0x55e
> > > > Modules linked in: <...>
> > > > CPU: 0 PID: 4046 Comm: kworker/0:35 Not tainted 4.19.139 #18
> > > > Hardware name: Google Phaser/Phaser, BIOS Google_Phaser.10952.0.0 08/09/2018
> > > > Workqueue: events uvc_ctrl_status_event_work [uvcvideo]
> > > > RIP: 0010:usb_submit_urb+0x4ba/0x55e
> > > > Code: <...>
> > > > RSP: 0018:ffffb08d471ebde8 EFLAGS: 00010246
> > > > RAX: a6da85d923ea5d00 RBX: ffff993e71985928 RCX: 0000000000000000
> > > > RDX: ffff993f37a1de90 RSI: ffff993f37a153d0 RDI: ffff993f37a153d0
> > > > RBP: ffffb08d471ebe28 R08: 000000000000003b R09: 001424bf85822e96
> > > > R10: 0000001000000000 R11: ffffffff975a4398 R12: ffff993e83d0b000
> > > > R13: ffff993e83d0bc00 R14: 0000000000000000 R15: 00000000fffffff0
> > > > FS:  0000000000000000(0000) GS:ffff993f37a00000(0000) knlGS:0000000000000000
> > > > CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > > > CR2: 00000000ec9c0000 CR3: 000000025b160000 CR4: 0000000000340ef0
> > > > Call Trace:
> > > >  uvc_ctrl_status_event_work+0xd6/0x107 [uvcvideo]
> > > >  process_one_work+0x19b/0x4c5
> > > >  worker_thread+0x10d/0x286
> > > >  kthread+0x138/0x140
> > > >  ? process_one_work+0x4c5/0x4c5
> > > >  ? kthread_associate_blkcg+0xc1/0xc1
> > > >  ret_from_fork+0x1f/0x40
> > > >
> > > > Introduce new function uvc_ctrl_stop_device() to cancel the worker
> > > > and call it from uvc_unregister_video() to solve the problem.
> > > >
> > > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > Cc: Alan Stern <stern@rowland.harvard.edu>
> > > > Cc: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> > > > Reviewed-by: Tomasz Figa <tfiga@chromium.org>
> > > > Reviewed-by: Sean Paul <seanpaul@chromium.org>
> > > > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > > ---
> > > >  drivers/media/usb/uvc/uvc_ctrl.c   | 11 +++++++----
> > > >  drivers/media/usb/uvc/uvc_driver.c |  1 +
> > > >  drivers/media/usb/uvc/uvcvideo.h   |  1 +
> > > >  3 files changed, 9 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> > > > index 5e9d3da862dd..769c1d2a2f45 100644
> > > > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > > > @@ -2757,14 +2757,17 @@ static void uvc_ctrl_cleanup_mappings(struct uvc_device *dev,
> > > >       }
> > > >  }
> > > >
> > > > -void uvc_ctrl_cleanup_device(struct uvc_device *dev)
> > > > +void uvc_ctrl_stop_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);
> > > > +}
> > >
> > > There may be an opportunity for refactoring, as we have
> > > uvc_status_stop() that stops the work queue, but I think this is good
> > > enough for now. I'm wondering, though, if there could be a race
> > > condition here similar to the one that the recent changes to
> > > uvc_status_stop() have fixed ? As uvc_ctrl_stop_device() is called at
> > > release time I assume that URBs have been cancelled, so there should be
> > > no race, but a second pair of eyeballs to confirm this would be
> > > appreciated.
> > >
> > > Other than that, the patch looks good to me, and fixes an issue
> > > independent from the rest of the series, so
> > >
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > >
> > > I will wait for a reply regarding the race condition before queuing this
> > > up though.
> > >
> > > > +
> > > > +void uvc_ctrl_cleanup_device(struct uvc_device *dev)
> > > > +{
> > > > +     struct uvc_entity *entity;
> > > > +     unsigned int i;
> > > >
> > > >       /* Free controls and control mappings for all entities. */
> > > >       list_for_each_entry(entity, &dev->entities, list) {
> > > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > > > index 7aefa76a42b3..4be6dfeaa295 100644
> > > > --- a/drivers/media/usb/uvc/uvc_driver.c
> > > > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > > > @@ -1893,6 +1893,7 @@ static void uvc_unregister_video(struct uvc_device *dev)
> > > >       }
> > > >
> > > >       uvc_status_unregister(dev);
> > > > +     uvc_ctrl_stop_device(dev);
> > > >
> > > >       if (dev->vdev.dev)
> > > >               v4l2_device_unregister(&dev->vdev);
> > > > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > > > index 9a596c8d894a..50f171e7381b 100644
> > > > --- a/drivers/media/usb/uvc/uvcvideo.h
> > > > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > > > @@ -760,6 +760,7 @@ int uvc_query_v4l2_menu(struct uvc_video_chain *chain,
> > > >  int uvc_ctrl_add_mapping(struct uvc_video_chain *chain,
> > > >                        const struct uvc_control_mapping *mapping);
> > > >  int uvc_ctrl_init_device(struct uvc_device *dev);
> > > > +void uvc_ctrl_stop_device(struct uvc_device *dev);
> > > >  void uvc_ctrl_cleanup_device(struct uvc_device *dev);
> > > >  int uvc_ctrl_restore_values(struct uvc_device *dev);
> > > >  bool uvc_ctrl_status_event_async(struct urb *urb, struct uvc_video_chain *chain,
> > >
> > > --
> > > Regards,
> > >
> > > Laurent Pinchart
> >
> >
> >
> > --
> > Ricardo Ribalda
>
>
>
> --
> Ricardo Ribalda

diff mbox series

Patch

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index 5e9d3da862dd..769c1d2a2f45 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -2757,14 +2757,17 @@  static void uvc_ctrl_cleanup_mappings(struct uvc_device *dev,
 	}
 }
 
-void uvc_ctrl_cleanup_device(struct uvc_device *dev)
+void uvc_ctrl_stop_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);
+}
+
+void uvc_ctrl_cleanup_device(struct uvc_device *dev)
+{
+	struct uvc_entity *entity;
+	unsigned int i;
 
 	/* Free controls and control mappings for all entities. */
 	list_for_each_entry(entity, &dev->entities, list) {
diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index 7aefa76a42b3..4be6dfeaa295 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -1893,6 +1893,7 @@  static void uvc_unregister_video(struct uvc_device *dev)
 	}
 
 	uvc_status_unregister(dev);
+	uvc_ctrl_stop_device(dev);
 
 	if (dev->vdev.dev)
 		v4l2_device_unregister(&dev->vdev);
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index 9a596c8d894a..50f171e7381b 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -760,6 +760,7 @@  int uvc_query_v4l2_menu(struct uvc_video_chain *chain,
 int uvc_ctrl_add_mapping(struct uvc_video_chain *chain,
 			 const struct uvc_control_mapping *mapping);
 int uvc_ctrl_init_device(struct uvc_device *dev);
+void uvc_ctrl_stop_device(struct uvc_device *dev);
 void uvc_ctrl_cleanup_device(struct uvc_device *dev);
 int uvc_ctrl_restore_values(struct uvc_device *dev);
 bool uvc_ctrl_status_event_async(struct urb *urb, struct uvc_video_chain *chain,