| 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
| ||
|
Date: Tue, 3 Jan 2023 17:13:14 +0100
From: Ricardo Ribalda <ribalda@...omium.org>
To: Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc: Mauro Carvalho Chehab <mchehab@...nel.org>,
"hn.chen" <hn.chen@...plusit.com>, linux-media@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH RESEND v2 5/8] media: uvcvideo: Quirk for autosuspend in
Logi C910
Hi Laurent
On Fri, 30 Dec 2022 at 14:52, Laurent Pinchart
<laurent.pinchart@...asonboard.com> wrote:
>
> Hi Ricardo,
>
> Thank you for the patch.
>
> s/C910/B910 and C910/ in the subject line.
>
> On Fri, Dec 02, 2022 at 06:02:45PM +0100, Ricardo Ribalda wrote:
> > Logitech C910 firmware is unable to recover from a usb autosuspend. When
>
> s/C910/B910 and C910/
> s/usb/USB/
>
> > it resumes, the device is in a state where it only produces invalid
> > frames. Eg:
> >
> > $ echo 0xFFFF > /sys/module/uvcvideo/parameters/trace # enable verbose log
> > $ yavta -c1 -n1 --file='frame#.jpg' --format MJPEG --size=1920x1080 /dev/video1
>
> Is this true for YUYV frames too ?
I believe so. This is just the script that I typically use to play
with my cameras.
>
> > [350438.435219] uvcvideo: uvc_v4l2_open
> > [350438.529794] uvcvideo: Resuming interface 2
> > [350438.529801] uvcvideo: Resuming interface 3
> > [350438.529991] uvcvideo: Trying format 0x47504a4d (MJPG): 1920x1080.
> > [350438.529996] uvcvideo: Using default frame interval 33333.3 us (30.0 fps).
> > [350438.551496] uvcvideo: uvc_v4l2_mmap
> > [350438.555890] uvcvideo: Device requested 3060 B/frame bandwidth.
> > [350438.555896] uvcvideo: Selecting alternate setting 11 (3060 B/frame bandwidth).
> > [350438.556362] uvcvideo: Allocated 5 URB buffers of 32x3060 bytes each.
> > [350439.316468] uvcvideo: Marking buffer as bad (error bit set).
> > [350439.316475] uvcvideo: Frame complete (EOF found).
> > [350439.316477] uvcvideo: EOF in empty payload.
> > [350439.316484] uvcvideo: frame 1 stats: 149/261/417 packets, 1/149/417 pts (early initial), 416/417 scr, last pts/stc/sof 2976325734/2978107243/249
> > [350439.384510] uvcvideo: Marking buffer as bad (error bit set).
> > [350439.384516] uvcvideo: Frame complete (EOF found).
> > [350439.384518] uvcvideo: EOF in empty payload.
> > [350439.384525] uvcvideo: frame 2 stats: 265/379/533 packets, 1/265/533 pts (early initial), 532/533 scr, last pts/stc/sof 2979524454/2981305193/316
> > [350439.448472] uvcvideo: Marking buffer as bad (error bit set).
> > [350439.448478] uvcvideo: Frame complete (EOF found).
> > [350439.448480] uvcvideo: EOF in empty payload.
> > [350439.448487] uvcvideo: frame 3 stats: 265/377/533 packets, 1/265/533 pts (early initial), 532/533 scr, last pts/stc/sof 2982723174/2984503144/382
> > ...(loop)...
> >
> > The devices can leave this invalid state if its altstate is toggled.
>
> s/its altstate/the alternate setting of the streaming interface/
>
> How did you figure this out ?
A deterministic process, where we analysed all the variables involved
in the streaming process....
(aka, educated guess + luck + trial and error :P)
I triggered a whole reconfiguration of the device and that worked, and
then I started removing parts of it until I ended up with the minimum
thing that made the device work.
>
> > This patch addes a quirk for this device so it can be autosuspended
> > properly.
> >
> > lsusb -v:
> > Bus 001 Device 049: ID 046d:0821 Logitech, Inc. HD Webcam C910
> > Device Descriptor:
> > bLength 18
> > bDescriptorType 1
> > bcdUSB 2.00
> > bDeviceClass 239 Miscellaneous Device
> > bDeviceSubClass 2
> > bDeviceProtocol 1 Interface Association
> > bMaxPacketSize0 64
> > idVendor 0x046d Logitech, Inc.
> > idProduct 0x0821 HD Webcam C910
> > bcdDevice 0.10
> > iManufacturer 0
> > iProduct 0
> > iSerial 1 390022B0
> > bNumConfigurations 1
> >
> > Signed-off-by: Ricardo Ribalda <ribalda@...omium.org>
> > ---
> > drivers/media/usb/uvc/uvc_driver.c | 18 ++++++++++++++++++
> > drivers/media/usb/uvc/uvc_video.c | 5 +++++
> > drivers/media/usb/uvc/uvcvideo.h | 1 +
> > 3 files changed, 24 insertions(+)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > index 4512316c8748..d2a158a1ce35 100644
> > --- a/drivers/media/usb/uvc/uvc_driver.c
> > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > @@ -2823,6 +2823,24 @@ static const struct usb_device_id uvc_ids[] = {
> > .bInterfaceSubClass = 1,
> > .bInterfaceProtocol = 0,
> > .driver_info = (kernel_ulong_t)&uvc_quirk_probe_minmax },
> > + /* Logitech, Webcam C910 */
> > + { .match_flags = USB_DEVICE_ID_MATCH_DEVICE
> > + | USB_DEVICE_ID_MATCH_INT_INFO,
> > + .idVendor = 0x046d,
> > + .idProduct = 0x0821,
> > + .bInterfaceClass = USB_CLASS_VIDEO,
> > + .bInterfaceSubClass = 1,
> > + .bInterfaceProtocol = 0,
> > + .driver_info = UVC_INFO_QUIRK(UVC_QUIRK_WAKE_AUTOSUSPEND)},
> > + /* Logitech, Webcam B910 */
> > + { .match_flags = USB_DEVICE_ID_MATCH_DEVICE
> > + | USB_DEVICE_ID_MATCH_INT_INFO,
> > + .idVendor = 0x046d,
> > + .idProduct = 0x0823,
> > + .bInterfaceClass = USB_CLASS_VIDEO,
> > + .bInterfaceSubClass = 1,
> > + .bInterfaceProtocol = 0,
> > + .driver_info = UVC_INFO_QUIRK(UVC_QUIRK_WAKE_AUTOSUSPEND)},
> > /* Logitech Quickcam Fusion */
> > { .match_flags = USB_DEVICE_ID_MATCH_DEVICE
> > | USB_DEVICE_ID_MATCH_INT_INFO,
> > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> > index d387d6335344..75c32e232f5d 100644
> > --- a/drivers/media/usb/uvc/uvc_video.c
> > +++ b/drivers/media/usb/uvc/uvc_video.c
> > @@ -1983,6 +1983,11 @@ static int uvc_video_start_transfer(struct uvc_streaming *stream,
> > "Selecting alternate setting %u (%u B/frame bandwidth)\n",
> > altsetting, best_psize);
> >
>
> Please add a comment to explain the issue.
>
> > + if (stream->dev->quirks & UVC_QUIRK_WAKE_AUTOSUSPEND) {
> > + usb_set_interface(stream->dev->udev, intfnum,
> > + altsetting);
> > + usb_set_interface(stream->dev->udev, intfnum, 0);
> > + }
>
> Missing blank line.
>
> > ret = usb_set_interface(stream->dev->udev, intfnum, altsetting);
> > if (ret < 0)
> > return ret;
> > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > index e41289605d0e..14daa7111953 100644
> > --- a/drivers/media/usb/uvc/uvcvideo.h
> > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > @@ -214,6 +214,7 @@
> > #define UVC_QUIRK_FORCE_BPP 0x00001000
> > #define UVC_QUIRK_IGNORE_EMPTY_TS 0x00002000
> > #define UVC_QUIRK_INVALID_DEVICE_SOF 0x00004000
> > +#define UVC_QUIRK_WAKE_AUTOSUSPEND 0x00008000
> >
> > /* Format flags */
> > #define UVC_FMT_FLAG_COMPRESSED 0x00000001
> >
>
> --
> Regards,
>
> Laurent Pinchart
--
Ricardo Ribalda
Powered by blists - more mailing lists