On Thu, Jul 05, 2018 at 11:13:42AM +0200, Landry Breuil wrote:
> On Thu, Jul 05, 2018 at 11:04:34AM +0200, Martin Pieuchot wrote:
> > On 05/07/18(Thu) 09:28, Landry Breuil wrote:
> > > On Fri, Jun 29, 2018 at 01:50:24PM +0200, Landry Breuil wrote:
> > > > On Fri, Jun 29, 2018 at 09:05:56AM +0200, Remco wrote:
> > > > > > > If you don't find any better solution, I'd suggest using a device
> > > > > > > ID
> > > > > > > check rather than adding a quirk. Because such quirk cannot be
> > > > > > > generic.
> > > > > > > In that case you have an off-by-one, but another device might
> > > > > > > have a
> > > > > > > different problem.
> > > > > >
> > > > > > That was in the case we'd encounter other devices where the
> > > > > > descriptor
> > > > > > is bogus and has the same issue, but i agree this is unlikely.
> > > > > >
> > > > >
> > > > > More likely for Logitech USB webcams I'm afraid.
> > > > > My dad's Logitech C500 showed the same issue several years back.
> > > > > (I'll try to find some time to borrow the device and test your patch)
> > > > > There's also a report that suggests a C200 may be affected as well.
> > > > > (https://marc.info/?l=openbsd-misc&m=127952275021290&w=2)
> > > >
> > > > Aaah, very interesting data point. Thanks for the pointer.. maybe a
> > > > 'generic fix' would be to ignore size checks for all logitech webcams,
> > > > or to print a warning but not error out in this case. Is there some kind
> > > > of 'usbdevs -v/lsusb -v' database somewhere ?
> > >
> > > So after feedback from lots of logitech webcam users (thanks!), i got a
> > > list
> > > for working vs non-working devices, and i think a new quirk would do as
> > > they
> > > all expose the same issue (bLength for FEATURE_UNIT is 9 while it is 8 on
> > > working devices) - and they would all be 'fixed' by the attached diff.
> >
> > If you know that some descriptors have an off-by-one, I'd suggest you add
> > 1 to the corresponding buffer length instead of completely removing the
> > check. Removing it completely now open the door to other overflows and
> > adding 1 auto-document the hardware bug ;)
>
> Well that was the intent of the original diff. New version...
New diff adding the C250 which also exhibits the same issue. I have an
okay from mpi@ on the previous diff, but i'd like actual reports that it
makes the audio device actually working (mine doesnt now, i'm digging
into it, but i'm pretty sure at some point i had it working...)
To test:
sysctl kern.audio.record=1
aucat -f rsnd/1 -o /tmp/foo.wav (if the uaudio device attaches as audio1)
If this actually records sound (mplayer /tmp/foo.wav to check), you've
won. if foo.wav is 68 bytes long, then recording doesnt work, interrupts
are triggered but no transfers happen..
Index: uaudio.c
===================================================================
RCS file: /cvs/src/sys/dev/usb/uaudio.c,v
retrieving revision 1.128
diff -u -r1.128 uaudio.c
--- uaudio.c 30 Dec 2017 23:08:29 -0000 1.128
+++ uaudio.c 6 Jul 2018 11:49:36 -0000
@@ -172,6 +172,7 @@
#define UAUDIO_FLAG_VENDOR_CLASS 0x0010 /* claims vendor class but
works */
#define UAUDIO_FLAG_DEPENDENT 0x0020 /* play and record params must equal */
#define UAUDIO_FLAG_EMU0202 0x0040
+#define UAUDIO_FLAG_BAD_ADC_LEN 0x0080 /* bad audio control descriptor
size */
struct uaudio_devs {
struct usb_devno uv_dev;
@@ -222,6 +223,18 @@
UAUDIO_FLAG_BAD_AUDIO },
{ { USB_VENDOR_LOGITECH, USB_PRODUCT_LOGITECH_QUICKCAMZOOM },
UAUDIO_FLAG_BAD_AUDIO },
+ { { USB_VENDOR_LOGITECH, USB_PRODUCT_LOGITECH_WEBCAMC200 },
+ UAUDIO_FLAG_BAD_ADC_LEN },
+ { { USB_VENDOR_LOGITECH, USB_PRODUCT_LOGITECH_WEBCAMC210 },
+ UAUDIO_FLAG_BAD_ADC_LEN },
+ { { USB_VENDOR_LOGITECH, USB_PRODUCT_LOGITECH_WEBCAMC250 },
+ UAUDIO_FLAG_BAD_ADC_LEN },
+ { { USB_VENDOR_LOGITECH, USB_PRODUCT_LOGITECH_WEBCAMC270 },
+ UAUDIO_FLAG_BAD_ADC_LEN },
+ { { USB_VENDOR_LOGITECH, USB_PRODUCT_LOGITECH_WEBCAMC310 },
+ UAUDIO_FLAG_BAD_ADC_LEN },
+ { { USB_VENDOR_LOGITECH, USB_PRODUCT_LOGITECH_WEBCAMC500 },
+ UAUDIO_FLAG_BAD_ADC_LEN },
{ { USB_VENDOR_TELEX, USB_PRODUCT_TELEX_MIC1 },
UAUDIO_FLAG_NO_FRAC }
};
@@ -1814,6 +1827,10 @@
sc->sc_audio_rev = UGETW(acdp->bcdADC);
DPRINTFN(2,("%s: found AC header, vers=%03x, len=%d\n",
__func__, sc->sc_audio_rev, aclen));
+
+ /* Some webcams descriptors advertise an off-by-one wTotalLength */
+ if (sc->sc_quirks & UAUDIO_FLAG_BAD_ADC_LEN)
+ aclen++;
sc->sc_nullalt = -1;
