Instead of suspending/resume the USB device at open()/close(), do it
when the device is actually used.
This way we can reduce the power consumption when a service is holding
the video device and leaving it in an idle state.
And now that all the access to the hardware, has a common entry path,
use it to fix the race conditions to hardware disconnects.
To: Mauro Carvalho Chehab <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: Laurent Pinchart <[email protected]>
Cc: Tomasz Figa <[email protected]>
Cc: Alan Stern <[email protected]>
Cc: Hans Verkuil <[email protected]>
Cc: Guenter Roeck <[email protected]>
Cc: Max Staudt <[email protected]>
Signed-off-by: Ricardo Ribalda <[email protected]>
---
Changes in v3:
- Rebase on top of uvc/next
- Reorder series, and put "controversial" patches at the end.
- Fix "use-before-set" bug. Thanks Max!
- Link to v2: https://lore.kernel.org/r/[email protected]
Changes in v2:
- Make access to uvc_status contitional
- Merge with Guenter race condition patchset: https://lore.kernel.org/lkml/[email protected]/
- Link to v1: https://lore.kernel.org/r/[email protected]
---
Guenter Roeck (4):
media: uvcvideo: Cancel async worker earlier
media: uvcvideo: Release stream queue when unregistering video device
media: uvcvideo: Lock video streams and queues while unregistering
media: uvcvideo: Protect uvc queue file operations against disconnect
Ricardo Ribalda (3):
media: uvcvideo: Refactor streamon/streamoff
media: uvcvideo: Do power management granularly
media: uvcvideo: Only call status ep if hw supports it
drivers/media/usb/uvc/uvc_ctrl.c | 11 +-
drivers/media/usb/uvc/uvc_driver.c | 35 +++++--
drivers/media/usb/uvc/uvc_queue.c | 32 +++++-
drivers/media/usb/uvc/uvc_status.c | 11 +-
drivers/media/usb/uvc/uvc_v4l2.c | 206 ++++++++++++++++++++++++++++++-------
drivers/media/usb/uvc/uvcvideo.h | 2 +
6 files changed, 248 insertions(+), 49 deletions(-)
---
base-commit: 58540610e464d8b2ba46a11b81c3e6fcc4118fae
change-id: 20220920-resend-powersave-5981719ed267
Best regards,
--
Ricardo Ribalda <[email protected]>
From: Guenter Roeck <[email protected]>
The following traceback was observed when stress testing the uvcvideo
driver.
config->interface[0] is NULL
WARNING: CPU: 0 PID: 2757 at drivers/usb/core/usb.c:285 usb_ifnum_to_if+0x81/0x85
^^^ added log message and WARN() to prevent crash
Modules linked in: <...>
CPU: 0 PID: 2757 Comm: inotify_reader Not tainted 4.19.139 #20
Hardware name: Google Phaser/Phaser, BIOS Google_Phaser.10952.0.0 08/09/2018
RIP: 0010:usb_ifnum_to_if+0x81/0x85
Code: <...>
RSP: 0018:ffff9ee20141fa58 EFLAGS: 00010246
RAX: 438a0e5a525f1800 RBX: 0000000000000000 RCX: 0000000000000000
RDX: ffff975477a1de90 RSI: ffff975477a153d0 RDI: ffff975477a153d0
RBP: ffff9ee20141fa70 R08: 000000000000002c R09: 002daec189138d78
R10: 0000001000000000 R11: ffffffffa7da42e6 R12: ffff975459594800
R13: 0000000000000001 R14: 0000000000000001 R15: ffff975465376400
FS: 00007ddebffd6700(0000) GS:ffff975477a00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000012c83000 CR3: 00000001bbaf8000 CR4: 0000000000340ef0
Call Trace:
usb_set_interface+0x3b/0x2f3
uvc_video_stop_streaming+0x38/0x81 [uvcvideo]
uvc_stop_streaming+0x21/0x49 [uvcvideo]
__vb2_queue_cancel+0x33/0x249 [videobuf2_common]
vb2_core_queue_release+0x1c/0x45 [videobuf2_common]
uvc_queue_release+0x26/0x32 [uvcvideo]
uvc_v4l2_release+0x41/0xdd [uvcvideo]
v4l2_release+0x99/0xed
__fput+0xf0/0x1ea
task_work_run+0x7f/0xa7
do_exit+0x1d1/0x6eb
do_group_exit+0x9d/0xac
get_signal+0x135/0x482
do_signal+0x4a/0x63c
exit_to_usermode_loop+0x86/0xa5
do_syscall_64+0x171/0x269
? __do_page_fault+0x272/0x474
entry_SYSCALL_64_after_hwframe+0x44/0xa9
RIP: 0033:0x7ddec156dc26
Code: Bad RIP value.
RSP: 002b:00007ddebffd5c10 EFLAGS: 00000293 ORIG_RAX: 0000000000000017
RAX: fffffffffffffdfe RBX: 000000000000000a RCX: 00007ddec156dc26
RDX: 0000000000000000 RSI: 00007ddebffd5d28 RDI: 000000000000000a
RBP: 00007ddebffd5c50 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000293 R12: 00007ddebffd5d28
R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
When this was observed, a USB disconnect was in progress, uvc_disconnect()
had returned, but usb_disable_device() was still running.
Drivers are not supposed to call any USB functions after the driver
disconnect function has been called. The uvcvideo driver does not follow
that convention and tries to write to the disconnected USB interface
anyway. This results in a variety of race conditions.
To solve this specific problem, release the uvc queue from
uvc_unregister_video() after the associated video devices have been
unregistered. Since the function already holds the uvc queue mutex,
bypass uvc_queue_release() and call vb2_queue_release() directly.
Cc: Laurent Pinchart <[email protected]>
Cc: Alan Stern <[email protected]>
Cc: Hans Verkuil <[email protected]>
Signed-off-by: Guenter Roeck <[email protected]>
diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index 70d081c9a9fe..dc43100d0f95 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -1897,6 +1897,8 @@ static void uvc_unregister_video(struct uvc_device *dev)
video_unregister_device(&stream->meta.vdev);
uvc_debugfs_cleanup_stream(stream);
+
+ vb2_queue_release(&stream->queue.queue);
}
if (dev->int_ep)
--
b4 0.11.0-dev-d93f8
From: Guenter Roeck <[email protected]>
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 <[email protected]>
Cc: Alan Stern <[email protected]>
Cc: Hans Verkuil <[email protected]>
Signed-off-by: Guenter Roeck <[email protected]>
diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index c95a2229f4fa..c7af3e08b2c6 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -2597,14 +2597,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 ac87dee77f44..70d081c9a9fe 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -1901,6 +1901,7 @@ static void uvc_unregister_video(struct uvc_device *dev)
if (dev->int_ep)
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 45310f55475f..6b07abac7034 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -739,6 +739,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,
--
b4 0.11.0-dev-d93f8
Instead of calling uvc_status_* regardless if the hw supports it or not,
make all the calls conditional.
This simplifies the locking during suspend/resume.
Signed-off-by: Ricardo Ribalda <[email protected]>
diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index bd3716a359b0..ac87dee77f44 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -1837,7 +1837,8 @@ static void uvc_delete(struct kref *kref)
struct uvc_device *dev = container_of(kref, struct uvc_device, ref);
struct list_head *p, *n;
- uvc_status_cleanup(dev);
+ if (dev->int_ep)
+ uvc_status_cleanup(dev);
uvc_ctrl_cleanup_device(dev);
usb_put_intf(dev->intf);
@@ -1898,7 +1899,8 @@ static void uvc_unregister_video(struct uvc_device *dev)
uvc_debugfs_cleanup_stream(stream);
}
- uvc_status_unregister(dev);
+ if (dev->int_ep)
+ uvc_status_unregister(dev);
if (dev->vdev.dev)
v4l2_device_unregister(&dev->vdev);
@@ -2199,10 +2201,13 @@ static int uvc_probe(struct usb_interface *intf,
usb_set_intfdata(intf, dev);
/* Initialize the interrupt URB. */
- if ((ret = uvc_status_init(dev)) < 0) {
- dev_info(&dev->udev->dev,
- "Unable to initialize the status endpoint (%d), status interrupt will not be supported.\n",
- ret);
+ if (dev->int_ep) {
+ ret = uvc_status_init(dev);
+ if (ret < 0) {
+ dev_info(&dev->udev->dev,
+ "Unable to initialize the status endpoint (%d), status interrupt will not be supported.\n",
+ ret);
+ }
}
ret = uvc_gpio_init_irq(dev);
@@ -2251,6 +2256,8 @@ static int uvc_suspend(struct usb_interface *intf, pm_message_t message)
/* Controls are cached on the fly so they don't need to be saved. */
if (intf->cur_altsetting->desc.bInterfaceSubClass ==
UVC_SC_VIDEOCONTROL) {
+ if (!dev->int_ep)
+ return 0;
mutex_lock(&dev->lock);
if (dev->users)
uvc_status_stop(dev);
@@ -2285,6 +2292,9 @@ static int __uvc_resume(struct usb_interface *intf, int reset)
return ret;
}
+ if (!dev->int_ep)
+ return ret;
+
mutex_lock(&dev->lock);
if (dev->users)
ret = uvc_status_start(dev, GFP_NOIO);
diff --git a/drivers/media/usb/uvc/uvc_status.c b/drivers/media/usb/uvc/uvc_status.c
index cb90aff344bc..627cf11066e7 100644
--- a/drivers/media/usb/uvc/uvc_status.c
+++ b/drivers/media/usb/uvc/uvc_status.c
@@ -277,7 +277,7 @@ int uvc_status_init(struct uvc_device *dev)
unsigned int pipe;
int interval;
- if (ep == NULL)
+ if (WARN_ON(!ep))
return 0;
uvc_input_init(dev);
@@ -312,19 +312,23 @@ int uvc_status_init(struct uvc_device *dev)
void uvc_status_unregister(struct uvc_device *dev)
{
+ if (WARN_ON(!dev->int_ep))
+ return;
usb_kill_urb(dev->int_urb);
uvc_input_unregister(dev);
}
void uvc_status_cleanup(struct uvc_device *dev)
{
+ if (WARN_ON(!dev->int_ep))
+ return;
usb_free_urb(dev->int_urb);
kfree(dev->status);
}
int uvc_status_start(struct uvc_device *dev, gfp_t flags)
{
- if (dev->int_urb == NULL)
+ if (WARN_ON(!dev->int_ep) || !dev->int_urb)
return 0;
return usb_submit_urb(dev->int_urb, flags);
@@ -332,5 +336,8 @@ int uvc_status_start(struct uvc_device *dev, gfp_t flags)
void uvc_status_stop(struct uvc_device *dev)
{
+ if (WARN_ON(!dev->int_ep) || !dev->int_urb)
+ return;
+
usb_kill_urb(dev->int_urb);
}
diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
index f5b0f1905962..77b687c46082 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -37,6 +37,9 @@ static int uvc_pm_get(struct uvc_streaming *stream)
if (ret)
return ret;
+ if (!stream->dev->int_ep)
+ return 0;
+
mutex_lock(&stream->dev->lock);
if (!stream->dev->users)
ret = uvc_status_start(stream->dev, GFP_KERNEL);
@@ -52,6 +55,9 @@ static int uvc_pm_get(struct uvc_streaming *stream)
static void uvc_pm_put(struct uvc_streaming *stream)
{
+ if (!stream->dev->int_ep)
+ goto done;
+
mutex_lock(&stream->dev->lock);
if (WARN_ON(!stream->dev->users)) {
mutex_unlock(&stream->dev->lock);
@@ -62,6 +68,7 @@ static void uvc_pm_put(struct uvc_streaming *stream)
uvc_status_stop(stream->dev);
mutex_unlock(&stream->dev->lock);
+done:
usb_autopm_put_interface(stream->dev->intf);
}
--
b4 0.11.0-dev-d93f8
From: Guenter Roeck <[email protected]>
uvc queue file operations have no mutex protection against USB
disconnect. This is questionable at least for the poll operation,
which has to wait for the uvc queue mutex. By the time that mutex
has been acquired, is it possible that the video device has been
unregistered.
Protect all file operations by using the queue mutex to avoid
possible race conditions. After acquiring the mutex, check if
the video device is still registered, and bail out if not.
Cc: Laurent Pinchart <[email protected]>
Cc: Alan Stern <[email protected]>
Cc: Hans Verkuil <[email protected]>
Signed-off-by: Guenter Roeck <[email protected]>
diff --git a/drivers/media/usb/uvc/uvc_queue.c b/drivers/media/usb/uvc/uvc_queue.c
index 16fa17bbd15e..aed45cbc814e 100644
--- a/drivers/media/usb/uvc/uvc_queue.c
+++ b/drivers/media/usb/uvc/uvc_queue.c
@@ -354,24 +354,52 @@ int uvc_queue_streamoff(struct uvc_video_queue *queue, enum v4l2_buf_type type)
int uvc_queue_mmap(struct uvc_video_queue *queue, struct vm_area_struct *vma)
{
- return vb2_mmap(&queue->queue, vma);
+ struct uvc_streaming *stream = uvc_queue_to_stream(queue);
+ int ret;
+
+ mutex_lock(&queue->mutex);
+ if (!video_is_registered(&stream->vdev)) {
+ ret = -ENODEV;
+ goto unlock;
+ }
+ ret = vb2_mmap(&queue->queue, vma);
+unlock:
+ mutex_unlock(&queue->mutex);
+ return ret;
}
#ifndef CONFIG_MMU
unsigned long uvc_queue_get_unmapped_area(struct uvc_video_queue *queue,
unsigned long pgoff)
{
- return vb2_get_unmapped_area(&queue->queue, 0, 0, pgoff, 0);
+ struct uvc_streaming *stream = uvc_queue_to_stream(queue);
+ unsigned long ret;
+
+ mutex_lock(&queue->mutex);
+ if (!video_is_registered(&stream->vdev)) {
+ ret = -ENODEV;
+ goto unlock;
+ }
+ ret = vb2_get_unmapped_area(&queue->queue, 0, 0, pgoff, 0);
+unlock:
+ mutex_unlock(&queue->mutex);
+ return ret;
}
#endif
__poll_t uvc_queue_poll(struct uvc_video_queue *queue, struct file *file,
poll_table *wait)
{
+ struct uvc_streaming *stream = uvc_queue_to_stream(queue);
__poll_t ret;
mutex_lock(&queue->mutex);
+ if (!video_is_registered(&stream->vdev)) {
+ ret = EPOLLERR;
+ goto unlock;
+ }
ret = vb2_poll(&queue->queue, file, wait);
+unlock:
mutex_unlock(&queue->mutex);
return ret;
--
b4 0.11.0-dev-d93f8
Add a new variable to handle the streaming state and handle the
streamoff errors, that were not handled before.
Suggested-by: Laurent Pinchart <[email protected]>
Signed-off-by: Ricardo Ribalda <[email protected]>
diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
index f4d4c33b6dfb..1389a87b8ae1 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -840,13 +840,19 @@ static int uvc_ioctl_streamon(struct file *file, void *fh,
{
struct uvc_fh *handle = fh;
struct uvc_streaming *stream = handle->stream;
- int ret;
+ int ret = -EBUSY;
if (!uvc_has_privileges(handle))
return -EBUSY;
mutex_lock(&stream->mutex);
+
+ if (handle->is_streaming)
+ goto unlock;
ret = uvc_queue_streamon(&stream->queue, type);
+ handle->is_streaming = !ret;
+
+unlock:
mutex_unlock(&stream->mutex);
return ret;
@@ -857,15 +863,22 @@ static int uvc_ioctl_streamoff(struct file *file, void *fh,
{
struct uvc_fh *handle = fh;
struct uvc_streaming *stream = handle->stream;
+ int ret = 0;
if (!uvc_has_privileges(handle))
return -EBUSY;
mutex_lock(&stream->mutex);
- uvc_queue_streamoff(&stream->queue, type);
+
+ if (!handle->is_streaming)
+ goto unlock;
+ ret = uvc_queue_streamoff(&stream->queue, type);
+ handle->is_streaming = !!ret;
+
+unlock:
mutex_unlock(&stream->mutex);
- return 0;
+ return ret;
}
static int uvc_ioctl_enum_input(struct file *file, void *fh,
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index df93db259312..45310f55475f 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -581,6 +581,7 @@ enum uvc_handle_state {
struct uvc_fh {
struct v4l2_fh vfh;
+ bool is_streaming;
struct uvc_video_chain *chain;
struct uvc_streaming *stream;
enum uvc_handle_state state;
--
b4 0.11.0-dev-d93f8
Instead of suspending/resume the USB device at open()/close(), do it
when the device is actually used.
This way we can reduce the power consumption when a service is holding
the video device and leaving it in an idle state.
Reviewed-by: Tomasz Figa <[email protected]>
Signed-off-by: Ricardo Ribalda <[email protected]>
diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
index 1389a87b8ae1..f5b0f1905962 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -25,6 +25,46 @@
#include "uvcvideo.h"
+/* ------------------------------------------------------------------------
+ * UVC power management
+ */
+
+static int uvc_pm_get(struct uvc_streaming *stream)
+{
+ int ret;
+
+ ret = usb_autopm_get_interface(stream->dev->intf);
+ if (ret)
+ return ret;
+
+ mutex_lock(&stream->dev->lock);
+ if (!stream->dev->users)
+ ret = uvc_status_start(stream->dev, GFP_KERNEL);
+ if (!ret)
+ stream->dev->users++;
+ mutex_unlock(&stream->dev->lock);
+
+ if (ret)
+ usb_autopm_put_interface(stream->dev->intf);
+
+ return ret;
+}
+
+static void uvc_pm_put(struct uvc_streaming *stream)
+{
+ mutex_lock(&stream->dev->lock);
+ if (WARN_ON(!stream->dev->users)) {
+ mutex_unlock(&stream->dev->lock);
+ return;
+ }
+ stream->dev->users--;
+ if (!stream->dev->users)
+ uvc_status_stop(stream->dev);
+ mutex_unlock(&stream->dev->lock);
+
+ usb_autopm_put_interface(stream->dev->intf);
+}
+
/* ------------------------------------------------------------------------
* UVC ioctls
*/
@@ -249,6 +289,9 @@ static int uvc_v4l2_try_format(struct uvc_streaming *stream,
* developers test their webcams with the Linux driver as well as with
* the Windows driver).
*/
+ ret = uvc_pm_get(stream);
+ if (ret)
+ return ret;
mutex_lock(&stream->mutex);
if (stream->dev->quirks & UVC_QUIRK_PROBE_EXTRAFIELDS)
probe->dwMaxVideoFrameSize =
@@ -257,6 +300,7 @@ static int uvc_v4l2_try_format(struct uvc_streaming *stream,
/* Probe the device. */
ret = uvc_probe_video(stream, probe);
mutex_unlock(&stream->mutex);
+ uvc_pm_put(stream);
if (ret < 0)
return ret;
@@ -468,7 +512,13 @@ static int uvc_v4l2_set_streamparm(struct uvc_streaming *stream,
}
/* Probe the device with the new settings. */
+ ret = uvc_pm_get(stream);
+ if (ret) {
+ mutex_unlock(&stream->mutex);
+ return ret;
+ }
ret = uvc_probe_video(stream, &probe);
+ uvc_pm_put(stream);
if (ret < 0) {
mutex_unlock(&stream->mutex);
return ret;
@@ -559,36 +609,29 @@ static int uvc_v4l2_open(struct file *file)
{
struct uvc_streaming *stream;
struct uvc_fh *handle;
- int ret = 0;
stream = video_drvdata(file);
uvc_dbg(stream->dev, CALLS, "%s\n", __func__);
- ret = usb_autopm_get_interface(stream->dev->intf);
- if (ret < 0)
- return ret;
-
/* Create the device handle. */
handle = kzalloc(sizeof(*handle), GFP_KERNEL);
- if (handle == NULL) {
- usb_autopm_put_interface(stream->dev->intf);
+ if (!handle)
return -ENOMEM;
- }
- mutex_lock(&stream->dev->lock);
- if (stream->dev->users == 0) {
- ret = uvc_status_start(stream->dev, GFP_KERNEL);
- if (ret < 0) {
- mutex_unlock(&stream->dev->lock);
- usb_autopm_put_interface(stream->dev->intf);
+ /*
+ * If the uvc evdev exists we cannot suspend when the device
+ * is idle. Otherwise we will miss button actions.
+ */
+ if (stream->dev->input) {
+ int ret;
+
+ ret = uvc_pm_get(stream);
+ if (ret) {
kfree(handle);
return ret;
}
}
- stream->dev->users++;
- mutex_unlock(&stream->dev->lock);
-
v4l2_fh_init(&handle->vfh, &stream->vdev);
v4l2_fh_add(&handle->vfh);
handle->chain = stream->chain;
@@ -610,6 +653,12 @@ static int uvc_v4l2_release(struct file *file)
if (uvc_has_privileges(handle))
uvc_queue_release(&stream->queue);
+ if (handle->is_streaming)
+ uvc_pm_put(stream);
+
+ if (stream->dev->input)
+ uvc_pm_put(stream);
+
/* Release the file handle. */
uvc_dismiss_privileges(handle);
v4l2_fh_del(&handle->vfh);
@@ -617,12 +666,6 @@ static int uvc_v4l2_release(struct file *file)
kfree(handle);
file->private_data = NULL;
- mutex_lock(&stream->dev->lock);
- if (--stream->dev->users == 0)
- uvc_status_stop(stream->dev);
- mutex_unlock(&stream->dev->lock);
-
- usb_autopm_put_interface(stream->dev->intf);
return 0;
}
@@ -849,9 +892,17 @@ static int uvc_ioctl_streamon(struct file *file, void *fh,
if (handle->is_streaming)
goto unlock;
+
+ ret = uvc_pm_get(stream);
+ if (ret)
+ goto unlock;
+
ret = uvc_queue_streamon(&stream->queue, type);
handle->is_streaming = !ret;
+ if (!handle->is_streaming)
+ uvc_pm_put(stream);
+
unlock:
mutex_unlock(&stream->mutex);
@@ -875,6 +926,9 @@ static int uvc_ioctl_streamoff(struct file *file, void *fh,
ret = uvc_queue_streamoff(&stream->queue, type);
handle->is_streaming = !!ret;
+ if (!handle->is_streaming)
+ uvc_pm_put(stream);
+
unlock:
mutex_unlock(&stream->mutex);
@@ -928,6 +982,7 @@ static int uvc_ioctl_g_input(struct file *file, void *fh, unsigned int *input)
{
struct uvc_fh *handle = fh;
struct uvc_video_chain *chain = handle->chain;
+ struct uvc_streaming *stream = handle->stream;
u8 *buf;
int ret;
@@ -941,9 +996,16 @@ static int uvc_ioctl_g_input(struct file *file, void *fh, unsigned int *input)
if (!buf)
return -ENOMEM;
+ ret = uvc_pm_get(stream);
+ if (ret) {
+ kfree(buf);
+ return ret;
+ }
+
ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR, chain->selector->id,
chain->dev->intfnum, UVC_SU_INPUT_SELECT_CONTROL,
buf, 1);
+ uvc_pm_put(stream);
if (!ret)
*input = *buf - 1;
@@ -956,6 +1018,7 @@ static int uvc_ioctl_s_input(struct file *file, void *fh, unsigned int input)
{
struct uvc_fh *handle = fh;
struct uvc_video_chain *chain = handle->chain;
+ struct uvc_streaming *stream = handle->stream;
u8 *buf;
int ret;
@@ -977,10 +1040,17 @@ static int uvc_ioctl_s_input(struct file *file, void *fh, unsigned int input)
if (!buf)
return -ENOMEM;
+ ret = uvc_pm_get(stream);
+ if (ret) {
+ kfree(buf);
+ return ret;
+ }
+
*buf = input + 1;
ret = uvc_query_ctrl(chain->dev, UVC_SET_CUR, chain->selector->id,
chain->dev->intfnum, UVC_SU_INPUT_SELECT_CONTROL,
buf, 1);
+ uvc_pm_put(stream);
kfree(buf);
return ret;
@@ -991,8 +1061,15 @@ static int uvc_ioctl_queryctrl(struct file *file, void *fh,
{
struct uvc_fh *handle = fh;
struct uvc_video_chain *chain = handle->chain;
+ struct uvc_streaming *stream = handle->stream;
+ int ret;
- return uvc_query_v4l2_ctrl(chain, qc);
+ ret = uvc_pm_get(stream);
+ if (ret)
+ return ret;
+ ret = uvc_query_v4l2_ctrl(chain, qc);
+ uvc_pm_put(stream);
+ return ret;
}
static int uvc_ioctl_query_ext_ctrl(struct file *file, void *fh,
@@ -1000,10 +1077,15 @@ static int uvc_ioctl_query_ext_ctrl(struct file *file, void *fh,
{
struct uvc_fh *handle = fh;
struct uvc_video_chain *chain = handle->chain;
+ struct uvc_streaming *stream = handle->stream;
struct v4l2_queryctrl qc = { qec->id };
int ret;
+ ret = uvc_pm_get(stream);
+ if (ret)
+ return ret;
ret = uvc_query_v4l2_ctrl(chain, &qc);
+ uvc_pm_put(stream);
if (ret)
return ret;
@@ -1049,6 +1131,7 @@ static int uvc_ioctl_g_ext_ctrls(struct file *file, void *fh,
{
struct uvc_fh *handle = fh;
struct uvc_video_chain *chain = handle->chain;
+ struct uvc_streaming *stream = handle->stream;
struct v4l2_ext_control *ctrl = ctrls->controls;
unsigned int i;
int ret;
@@ -1073,22 +1156,30 @@ static int uvc_ioctl_g_ext_ctrls(struct file *file, void *fh,
return 0;
}
+ ret = uvc_pm_get(stream);
+ if (ret)
+ return ret;
ret = uvc_ctrl_begin(chain);
- if (ret < 0)
+ if (ret < 0) {
+ uvc_pm_put(stream);
return ret;
+ }
for (i = 0; i < ctrls->count; ++ctrl, ++i) {
ret = uvc_ctrl_get(chain, ctrl);
if (ret < 0) {
uvc_ctrl_rollback(handle);
ctrls->error_idx = i;
- return ret;
+ goto done;
}
}
ctrls->error_idx = 0;
- return uvc_ctrl_rollback(handle);
+ ret = uvc_ctrl_rollback(handle);
+done:
+ uvc_pm_put(stream);
+ return ret;
}
static int uvc_ioctl_s_try_ext_ctrls(struct uvc_fh *handle,
@@ -1097,6 +1188,7 @@ static int uvc_ioctl_s_try_ext_ctrls(struct uvc_fh *handle,
{
struct v4l2_ext_control *ctrl = ctrls->controls;
struct uvc_video_chain *chain = handle->chain;
+ struct uvc_streaming *stream = handle->stream;
unsigned int i;
int ret;
@@ -1104,9 +1196,15 @@ static int uvc_ioctl_s_try_ext_ctrls(struct uvc_fh *handle,
if (ret < 0)
return ret;
+ ret = uvc_pm_get(stream);
+ if (ret)
+ return ret;
+
ret = uvc_ctrl_begin(chain);
- if (ret < 0)
+ if (ret < 0) {
+ uvc_pm_put(stream);
return ret;
+ }
for (i = 0; i < ctrls->count; ++ctrl, ++i) {
ret = uvc_ctrl_set(handle, ctrl);
@@ -1114,16 +1212,20 @@ static int uvc_ioctl_s_try_ext_ctrls(struct uvc_fh *handle,
uvc_ctrl_rollback(handle);
ctrls->error_idx = ioctl == VIDIOC_S_EXT_CTRLS ?
ctrls->count : i;
- return ret;
+ goto done;
}
}
ctrls->error_idx = 0;
if (ioctl == VIDIOC_S_EXT_CTRLS)
- return uvc_ctrl_commit(handle, ctrls);
+ ret = uvc_ctrl_commit(handle, ctrls);
else
- return uvc_ctrl_rollback(handle);
+ ret = uvc_ctrl_rollback(handle);
+
+done:
+ uvc_pm_put(stream);
+ return ret;
}
static int uvc_ioctl_s_ext_ctrls(struct file *file, void *fh,
@@ -1147,8 +1249,16 @@ static int uvc_ioctl_querymenu(struct file *file, void *fh,
{
struct uvc_fh *handle = fh;
struct uvc_video_chain *chain = handle->chain;
+ struct uvc_streaming *stream = handle->stream;
+ int ret;
+
+ ret = uvc_pm_get(stream);
+ if (ret)
+ return ret;
+ ret = uvc_query_v4l2_menu(chain, qm);
+ uvc_pm_put(stream);
- return uvc_query_v4l2_menu(chain, qm);
+ return ret;
}
static int uvc_ioctl_g_selection(struct file *file, void *fh,
--
b4 0.11.0-dev-d93f8
From: Guenter Roeck <[email protected]>
The call to uvc_disconnect() is not protected by any mutex.
This means it can and will be called while other accesses to the video
device are in progress. This can cause all kinds of race conditions,
including crashes such as the following.
usb 1-4: USB disconnect, device number 3
BUG: unable to handle kernel NULL pointer dereference at 0000000000000000
PGD 0 P4D 0
Oops: 0000 [#1] PREEMPT SMP PTI
CPU: 0 PID: 5633 Comm: V4L2CaptureThre Not tainted 4.19.113-08536-g5d29ca36db06 #1
Hardware name: GOOGLE Edgar, BIOS Google_Edgar.7287.167.156 03/25/2019
RIP: 0010:usb_ifnum_to_if+0x29/0x40
Code: <...>
RSP: 0018:ffffa46f42a47a80 EFLAGS: 00010246
RAX: 0000000000000000 RBX: 0000000000000000 RCX: ffff904a396c9000
RDX: ffff904a39641320 RSI: 0000000000000001 RDI: 0000000000000000
RBP: ffffa46f42a47a80 R08: 0000000000000002 R09: 0000000000000000
R10: 0000000000009975 R11: 0000000000000009 R12: 0000000000000000
R13: ffff904a396b3800 R14: ffff904a39e88000 R15: 0000000000000000
FS: 00007f396448e700(0000) GS:ffff904a3ba00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000000 CR3: 000000016cb46000 CR4: 00000000001006f0
Call Trace:
usb_hcd_alloc_bandwidth+0x1ee/0x30f
usb_set_interface+0x1a3/0x2b7
uvc_video_start_transfer+0x29b/0x4b8 [uvcvideo]
uvc_video_start_streaming+0x91/0xdd [uvcvideo]
uvc_start_streaming+0x28/0x5d [uvcvideo]
vb2_start_streaming+0x61/0x143 [videobuf2_common]
vb2_core_streamon+0xf7/0x10f [videobuf2_common]
uvc_queue_streamon+0x2e/0x41 [uvcvideo]
uvc_ioctl_streamon+0x42/0x5c [uvcvideo]
__video_do_ioctl+0x33d/0x42a
video_usercopy+0x34e/0x5ff
? video_ioctl2+0x16/0x16
v4l2_ioctl+0x46/0x53
do_vfs_ioctl+0x50a/0x76f
ksys_ioctl+0x58/0x83
__x64_sys_ioctl+0x1a/0x1e
do_syscall_64+0x54/0xde
usb_set_interface() should not be called after the USB device has been
unregistered. However, in the above case the disconnect happened after
v4l2_ioctl() was called, but before the call to usb_ifnum_to_if().
Acquire various mutexes in uvc_unregister_video() to fix the majority
(maybe all) of the observed race conditions.
The uvc_device lock prevents races against suspend and resume calls
and the poll function.
The uvc_streaming lock prevents races against stream related functions;
for the most part, those are ioctls. This lock also requires other
functions using this lock to check if a video device is still registered
after acquiring it. For example, it was observed that the video device
was already unregistered by the time the stream lock was acquired in
uvc_ioctl_streamon().
The uvc_queue lock prevents races against queue functions, Most of
those are already protected by the uvc_streaming lock, but some
are called directly. This is done as added protection; an actual race
was not (yet) observed.
Cc: Laurent Pinchart <[email protected]>
Cc: Alan Stern <[email protected]>
Cc: Hans Verkuil <[email protected]>
Signed-off-by: Guenter Roeck <[email protected]>
diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index dc43100d0f95..b67d753d27ae 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -1889,16 +1889,24 @@ static void uvc_unregister_video(struct uvc_device *dev)
{
struct uvc_streaming *stream;
+ mutex_lock(&dev->lock);
+
list_for_each_entry(stream, &dev->streams, list) {
if (!video_is_registered(&stream->vdev))
continue;
+ mutex_lock(&stream->mutex);
+ mutex_lock(&stream->queue.mutex);
+
video_unregister_device(&stream->vdev);
video_unregister_device(&stream->meta.vdev);
uvc_debugfs_cleanup_stream(stream);
vb2_queue_release(&stream->queue.queue);
+
+ mutex_unlock(&stream->queue.mutex);
+ mutex_unlock(&stream->mutex);
}
if (dev->int_ep)
@@ -1911,6 +1919,8 @@ static void uvc_unregister_video(struct uvc_device *dev)
if (media_devnode_is_registered(dev->mdev.devnode))
media_device_unregister(&dev->mdev);
#endif
+
+ mutex_unlock(&dev->lock);
}
int uvc_register_video_device(struct uvc_device *dev,
diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
index 77b687c46082..b4f2da336def 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -37,14 +37,20 @@ static int uvc_pm_get(struct uvc_streaming *stream)
if (ret)
return ret;
+ mutex_lock(&stream->dev->lock);
+
+ if (!video_is_registered(&stream->vdev))
+ goto done;
+
if (!stream->dev->int_ep)
- return 0;
+ goto done;
- mutex_lock(&stream->dev->lock);
if (!stream->dev->users)
ret = uvc_status_start(stream->dev, GFP_KERNEL);
if (!ret)
stream->dev->users++;
+
+done:
mutex_unlock(&stream->dev->lock);
if (ret)
--
b4 0.11.0-dev-d93f8
Thanks Ricardo!
I've reviewed v2 for locking issues etc., albeit without much background
on V4L, UVC, USB yet.
If this is good enough, then please have a
Reviewed-by: Max Staudt <[email protected]>