| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <alpine.LFD.2.21.2108031201460.28227@fields.fields.utoronto.ca>
Date: Tue, 3 Aug 2021 12:04:29 -0400 (EDT)
From: Philip Spencer <pspencer@...lds.utoronto.ca>
To: Laurent Pinchart <laurent.pinchart@...asonboard.com>
cc: Mauro Carvalho Chehab <mchehab@...nel.org>,
linux-media@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] uvcvideo: Support devices that require SET_INTERFACE(0)
before/after streaming
On Mon, 2 Aug 2021, Philip Spencer wrote:
> (This is my first kernel-related mailing list posting; my apologies if I have
> targeted wrong maintainers and/or lists. This is posted on the Ubuntu
> launchpad bug tracker at
> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1938669 and it was
> suggested there that I post directly to the maintainers/mailing lists).
My apologies; I thought I had set my mailer not to mangle the patches but
I hadn't. Resending properly formatted patch (I hope):
--- a/drivers/media/usb/uvc/uvc_video.c 2021-08-01 10:19:19.343564026 -0400
+++ b/drivers/media/usb/uvc/uvc_video.c 2021-08-01 10:38:54.234311440 -0400
@@ -2108,6 +2081,15 @@ int uvc_video_start_streaming(struct uvc
{
int ret;
+ /* On a bulk-based device where there is only one alternate
+ * setting possibility, set it explicitly to 0. This should be
+ * the default value, but some devices (e.g. Epiphan Systems
+ * framegrabbers) freeze and won't restart streaming until they
+ * receive a SET_INTERFACE(0) request.
+ */
+ if (stream->intf->num_altsetting == 1)
+ usb_set_interface(stream->dev->udev, stream->intfnum, 0);
+
ret = uvc_video_clock_init(stream);
if (ret < 0)
return ret;
@@ -2135,9 +2117,17 @@ void uvc_video_stop_streaming(struct uvc
{
uvc_video_stop_transfer(stream, 1);
- if (stream->intf->num_altsetting > 1) {
- usb_set_interface(stream->dev->udev, stream->intfnum, 0);
- } else {
+ /* On isochronous devices, switch back to interface 0 to move
+ * the device out of the "streaming" state.
+ *
+ * On bulk-based devices, this interface will already be selected
+ * but we re-select it explicitly because some devices seem to need
+ * a SET_INTERFACE(0) request to prepare them for receiving other
+ * control requests and/or to tell them to stop streaming.
+ */
+ usb_set_interface(stream->dev->udev, stream->intfnum, 0);
+
+ if (stream->intf->num_altsetting == 1) {
/* UVC doesn't specify how to inform a bulk-based device
* when the video stream is stopped. Windows sends a
* CLEAR_FEATURE(HALT) request to the video streaming
>
> Video capture devices made by Epiphan Systems (vendor id 0x2b77) work once,
> but as soon as the video device is closed (or even if it is kept open but the
> application issues a VIDIOC_STREAMOFF ioctl) it won't work again - subsequent
> calls to VIDOC_DQBUF simply hang - until the device is unbound from and
> rebound to the uvcvideo module. (modprobe -r uvcvideo; modprobe uvcvideo also
> works).
>
> For example:
>
> ffplay /dev/video0 -- works fine and shows the captured stream.
>
> ffplay /dev/video0 -- when run a second time: hangs and does not capture
> anything
>
> modprobe -r uvcvideo ; modprobe uvcvideo; ffplay /dev/video0 -- works fine
> again.
>
> Experimenting with the device and the uvcvideo module source code reveals that
> problem is the device is expecting SET_INTERFACE(0) to be sent to return it to
> a state where it can accept control requests and start streaming again.
>
> The code in uvc_video.c has several comments stating that some bulk-transfer
> devices require a SET_INTERFACE(0) call to be made before any control
> commands, even though 0 is already the default and only valid interface value.
> And, the function uvc_video_init makes such a call (which is why the device
> starts working again after rebinding to the uvcvideo module). But no such call
> is made when streaming is stopped then restarted.
>
> Furthermore, SET_INTERFACE(0) is the mechanism by which isochronous devices
> are told to stop streaming, and the comments in uvc_video_stop_streaming state
> that the UVC specification is unclear on how a bulk-based device should be
> told to stop streaming, so it is reasonable to imagine this particular
> bulk-based device might be expecting the same SET_INTERFACE(0) call that an
> isochronous device would get as means of being told to stop streaming.
>
> The attached patch fixes the problem for these Epiphan devices by adding a
> SET_INTERFACE(0) call in two places. Either one by itself is sufficient to
> resolve the symptoms but I think it is probably safest to include both.
>
> The first hunk adds a SET_INTERFACE(0) call in uvc_video_start_streaming, but
> only in the bulk-based case where 0 is the only possible interface value (it
> won't mess with an isochronous device that might be set to a different
> interface).
>
> The second hunk modifies the behaviour of uvc_video_stop_streaming to call
> SET_INTERFACE(0) unconditionally instead of only calling it for isochronous
> devices. Since interface 0 should already be set on non-isochronous devices,
> it should be safe to set it again, and this way devices that are expecting it
> as a signal to stop streaming will get it.
>
> The patch is against 5.4.137 but also applies cleanly to 5.14-rc3.
>
> --- a/drivers/media/usb/uvc/uvc_video.c 2021-08-01 10:19:19.343564026 -0400
> +++ b/drivers/media/usb/uvc/uvc_video.c 2021-08-01 10:38:54.234311440 -0400
> @@ -2108,6 +2081,15 @@ int uvc_video_start_streaming(struct uvc
> {
> int ret;
>
> + /* On a bulk-based device where there is only one alternate
> + * setting possibility, set it explicitly to 0. This should be
> + * the default value, but some devices (e.g. Epiphan Systems
> + * framegrabbers) freeze and won't restart streaming until they
> + * receive a SET_INTERFACE(0) request.
> + */
> + if (stream->intf->num_altsetting == 1) +
> usb_set_interface(stream->dev->udev, stream->intfnum, 0);
> +
> ret = uvc_video_clock_init(stream);
> if (ret < 0)
> return ret;
> @@ -2135,9 +2117,17 @@ void uvc_video_stop_streaming(struct uvc
> {
> uvc_video_stop_transfer(stream, 1);
>
> - if (stream->intf->num_altsetting > 1) {
> - usb_set_interface(stream->dev->udev, stream->intfnum, 0);
> - } else {
> + /* On isochronous devices, switch back to interface 0 to move
> + * the device out of the "streaming" state.
> + *
> + * On bulk-based devices, this interface will already be selected
> + * but we re-select it explicitly because some devices seem to need
> + * a SET_INTERFACE(0) request to prepare them for receiving other
> + * control requests and/or to tell them to stop streaming.
> + */
> + usb_set_interface(stream->dev->udev, stream->intfnum, 0);
> +
> + if (stream->intf->num_altsetting == 1) {
> /* UVC doesn't specify how to inform a bulk-based device
> * when the video stream is stopped. Windows sends a
> * CLEAR_FEATURE(HALT) request to the video streaming
>
> Philip
>
> --------------------------------------------+-------------------------------
> Philip Spencer pspencer@...lds.utoronto.ca | Director of Computing Services
> Room 348 (416)-348-9710 ext3048 | The Fields Institute for
> 222 College St, Toronto ON M5T 3J1 Canada | Research in Mathematical
> Sciences
>
--------------------------------------------+-------------------------------
Philip Spencer pspencer@...lds.utoronto.ca | Director of Computing Services
Room 348 (416)-348-9710 ext3048 | The Fields Institute for
222 College St, Toronto ON M5T 3J1 Canada | Research in Mathematical Sciences
Powered by blists - more mailing lists