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 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: Lock video streams and queues while unregistering
media: uvcvideo: Release stream queue when unregistering video device
media: uvcvideo: Protect uvc queue file operations against disconnect
Ricardo Ribalda (4):
media: uvcvideo: Only create input devs if hw supports it
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 | 28 +++++-
drivers/media/usb/uvc/uvc_queue.c | 32 +++++-
drivers/media/usb/uvc/uvc_status.c | 34 ++++++-
drivers/media/usb/uvc/uvc_v4l2.c | 201 ++++++++++++++++++++++++++++++-------
drivers/media/usb/uvc/uvcvideo.h | 2 +
6 files changed, 262 insertions(+), 46 deletions(-)
---
base-commit: 1a2dcbdde82e3a5f1db9b2f4c48aa1aeba534fb2
change-id: 20220920-resend-powersave-5981719ed267
Best regards,
--
Ricardo Ribalda <[email protected]>
Examine the stream headers to figure out if the device has a button and
can be used as an input.
Reviewed-by: Laurent Pinchart <[email protected]>
Signed-off-by: Ricardo Ribalda <[email protected]>
diff --git a/drivers/media/usb/uvc/uvc_status.c b/drivers/media/usb/uvc/uvc_status.c
index 7518ffce22ed..cb90aff344bc 100644
--- a/drivers/media/usb/uvc/uvc_status.c
+++ b/drivers/media/usb/uvc/uvc_status.c
@@ -18,11 +18,34 @@
* Input device
*/
#ifdef CONFIG_USB_VIDEO_CLASS_INPUT_EVDEV
+
+static bool uvc_input_has_button(struct uvc_device *dev)
+{
+ struct uvc_streaming *stream;
+
+ /*
+ * The device has button events if both bTriggerSupport and
+ * bTriggerUsage are one. Otherwise the camera button does not
+ * exist or is handled automatically by the camera without host
+ * driver or client application intervention.
+ */
+ list_for_each_entry(stream, &dev->streams, list) {
+ if (stream->header.bTriggerSupport == 1 &&
+ stream->header.bTriggerUsage == 1)
+ return true;
+ }
+
+ return false;
+}
+
static int uvc_input_init(struct uvc_device *dev)
{
struct input_dev *input;
int ret;
+ if (!uvc_input_has_button(dev))
+ return 0;
+
input = input_allocate_device();
if (input == NULL)
return -ENOMEM;
--
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
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 14b66019208d..f15fbdbcd8bb 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -1912,6 +1912,8 @@ static void uvc_unregister_video(struct uvc_device *dev)
uvc_debugfs_cleanup_stream(stream);
+ vb2_queue_release(&stream->queue.queue);
+
mutex_unlock(&stream->queue.mutex);
mutex_unlock(&stream->mutex);
}
--
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 f15fbdbcd8bb..d2841f0d9132 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -1847,7 +1847,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);
@@ -1918,7 +1919,8 @@ static void uvc_unregister_video(struct uvc_device *dev)
mutex_unlock(&stream->mutex);
}
- uvc_status_unregister(dev);
+ if (dev->int_ep)
+ uvc_status_unregister(dev);
uvc_ctrl_stop_device(dev);
if (dev->vdev.dev)
@@ -2222,7 +2224,9 @@ static int uvc_probe(struct usb_interface *intf,
usb_set_intfdata(intf, dev);
/* Initialize the interrupt URB. */
- if ((ret = uvc_status_init(dev)) < 0) {
+ 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);
@@ -2274,6 +2278,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);
@@ -2308,6 +2314,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 c250b628fc4f..2861ab122beb 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -44,7 +44,7 @@ static int uvc_pm_get(struct uvc_streaming *stream)
goto done;
}
- if (!stream->dev->users)
+ if (!stream->dev->users && stream->dev->int_ep)
ret = uvc_status_start(stream->dev, GFP_KERNEL);
if (!ret)
stream->dev->users++;
@@ -66,7 +66,7 @@ static void uvc_pm_put(struct uvc_streaming *stream)
return;
}
stream->dev->users--;
- if (!stream->dev->users)
+ if (!stream->dev->users && stream->dev->int_ep)
uvc_status_stop(stream->dev);
mutex_unlock(&stream->dev->lock);
--
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 fb58f80f0b37..14b66019208d 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -1898,14 +1898,22 @@ 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);
+
+ mutex_unlock(&stream->queue.mutex);
+ mutex_unlock(&stream->mutex);
}
uvc_status_unregister(dev);
@@ -1917,6 +1925,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 f5b0f1905962..c250b628fc4f 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -38,10 +38,18 @@ static int uvc_pm_get(struct uvc_streaming *stream)
return ret;
mutex_lock(&stream->dev->lock);
+
+ if (!video_is_registered(&stream->vdev)) {
+ ret = -ENODEV;
+ goto done;
+ }
+
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