Re: Certain cameras no longer working with uvcvideo on recent (openSUSE) kernels

Laurent Pinchart

author

2020-01-01 18:36 UTC #3 lore raw

Hi Roger,

(CCin'g Alan Stern and linux-usb)

Quoted 6 lines · Roger Whittaker

Quoted 3 lines · Laurent Pinchart
The last message is worse. Could you send me the output of lsusb -v (you can restrict it to the camera with -d), if possible running as root, for both the working and non-working kernels ?

Quoted 7 lines

Thanks very much for your reply.
The lsusb outputs are attached - they are in fact identical to each other.
Also attached, the dmesg lines when replugging the camera on both kernels.

Thank you for the information.

I had missed the following message:

[  470.351700] usb 1-1.4.3.1: config 1 interface 2 altsetting 0 endpoint 0x82 has wMaxPacketSize 0, skipping

This seems to be the culprit, and it points to the USB core. One interface is ignored due to its wMaxPacketSize value, and the uvcvideo driver then fails to find it.

The wMaxPacketSize check was added in

commit d482c7bb0541d19dea8bff437a9f3c5563b5b2d2

Author: Alan Stern <[email protected]>
Date:   Mon Oct 28 10:52:35 2019 -0400

USB: Skip endpoints with 0 maxpacket length

Endpoints with a maxpacket length of 0 are probably useless. They can't transfer any data, and it's not at all unlikely that an HCD will crash or hang when trying to handle an URB for such an endpoint.

Currently the USB core does not check for endpoints having a maxpacket value of 0. This patch adds a check, printing a warning and skipping over any endpoints it catches.

Now, the USB spec does not rule out endpoints having maxpacket = 0. But since they wouldn't have any practical use, there doesn't seem to be any good reason for us to accept them.

Signed-off-by Alan Stern <[email protected]>

Link https://lore.kernel.org/r/[email protected] open here

Signed-off-by Greg Kroah-Hartman <[email protected]>

The commit was merged in v5.4 and backported to v5.3.11 in 47aaab6377204cdbcd16f52a23c584f994fd0d15.

For reference for Alan and linux-usb, the issue being discussed is described in https://bugzilla.suse.com/show_bug.cgi?id=1159811. The above commit seems to cause a regression with several cameras. I've attached to this e-mail the lsusb output provided by Roger.

Signature · 3 lines

Regards,

Laurent Pinchart

Bus 001 Device 010: ID 1778:0214
Device Descriptor:
bLength 18
bDescriptorType 1
bcdUSB 2.00
bDeviceClass 239 Miscellaneous Device
bDeviceSubClass 2
bDeviceProtocol 1 Interface Association
bMaxPacketSize0 64
idVendor 0x1778
idProduct 0x0214
bcdDevice 7.07

  iManufacturer           1 IPEVO Inc.
  iProduct                2 IPEVO Point 2 View
  iSerial                 0 
  bNumConfigurations      1

Configuration Descriptor:
bLength 9
bDescriptorType 2
wTotalLength 0x0299
bNumInterfaces 3
bConfigurationValue 1
iConfiguration 0
bmAttributes 0x80
(Bus Powered)
MaxPower 500mA
Interface Descriptor:
bLength 9
bDescriptorType 4
bInterfaceNumber 0
bAlternateSetting 0
bNumEndpoints 1
bInterfaceClass 3 Human Interface Device
bInterfaceSubClass 0
bInterfaceProtocol 0
iInterface 0
HID Device Descriptor:
bLength 9
bDescriptorType 33
bcdHID 1.10
bCountryCode 0 Not supported
bNumDescriptors 1
bDescriptorType 34 Report
wDescriptorLength 64
Report Descriptors:
** UNAVAILABLE **
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x85 EP 5 IN
bmAttributes 3
Transfer Type Interrupt
Synch Type None
Usage Type Data
wMaxPacketSize 0x0008 1x 8 bytes
bInterval 10
Interface Association:
bLength 8
bDescriptorType 11
bFirstInterface 1
bInterfaceCount 2
bFunctionClass 14 Video
bFunctionSubClass 3 Video Interface Collection
bFunctionProtocol 0
iFunction 2 IPEVO Point 2 View
Interface Descriptor:
bLength 9
bDescriptorType 4
bInterfaceNumber 1
bAlternateSetting 0
bNumEndpoints 1
bInterfaceClass 14 Video
bInterfaceSubClass 1 Video Control
bInterfaceProtocol 0
iInterface 2 IPEVO Point 2 View
VideoControl Interface Descriptor:
bLength 13
bDescriptorType 36
bDescriptorSubtype 1 (HEADER)
bcdUVC 1.00
wTotalLength 0x0050
dwClockFrequency 6.000000MHz
bInCollection 1
baInterfaceNr( 0) 2
VideoControl Interface Descriptor:
bLength 18
bDescriptorType 36
bDescriptorSubtype 2 (INPUT_TERMINAL)
bTerminalID 1
wTerminalType 0x0201 Camera Sensor
bAssocTerminal 0
iTerminal 0
wObjectiveFocalLengthMin 0
wObjectiveFocalLengthMax 0
wOcularFocalLength 0
bControlSize 3
bmControls 0x000200a2
Auto-Exposure Mode
Focus (Absolute)
Iris (Absolute)
Focus, Auto
VideoControl Interface Descriptor:
bLength 11
bDescriptorType 36
bDescriptorSubtype 5 (PROCESSING_UNIT)
Warning: Descriptor too short
bUnitID 3
bSourceID 1
wMaxMultiplier 0
bControlSize 2
bmControls 0x0000147f
Brightness
Contrast
Hue
Saturation
Sharpness
Gamma
White Balance Temperature
Power Line Frequency
White Balance Temperature, Auto
iProcessing 0
bmVideoStandards 0x1d
None
PAL - 625/50
SECAM - 625/50
NTSC - 625/50
VideoControl Interface Descriptor:
bLength 29
bDescriptorType 36
bDescriptorSubtype 6 (EXTENSION_UNIT)
bUnitID 4
guidExtensionCode {5a215226-3289-4156-894a-5c557cdf9664}
bNumControl 4
bNrPins 1
baSourceID( 0) 3
bControlSize 4
bmControls( 0) 0xff
bmControls( 1) 0xff
bmControls( 2) 0xff
bmControls( 3) 0xff
iExtension 0
VideoControl Interface Descriptor:
bLength 9
bDescriptorType 36
bDescriptorSubtype 3 (OUTPUT_TERMINAL)
bTerminalID 2
wTerminalType 0x0101 USB Streaming
bAssocTerminal 0
bSourceID 4
iTerminal 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x81 EP 1 IN
bmAttributes 3
Transfer Type Interrupt
Synch Type None
Usage Type Data
wMaxPacketSize 0x0008 1x 8 bytes
bInterval 9
Interface Descriptor:
bLength 9
bDescriptorType 4
bInterfaceNumber 2
bAlternateSetting 0
bNumEndpoints 1
bInterfaceClass 14 Video
bInterfaceSubClass 2 Video Streaming
bInterfaceProtocol 0
iInterface 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x82 EP 2 IN
bmAttributes 5
Transfer Type Isochronous
Synch Type Asynchronous
Usage Type Data
wMaxPacketSize 0x0000 1x 0 bytes
bInterval 1
INTERFACE CLASS: 0f 24 01 02 ea 01 82 00 02 02 01 00 01 00 00
INTERFACE CLASS: 1b 24 04 01 07 59 55 59 32 00 00 10 00 80 00 00 aa 00 38 9b 71 10 01 00 00 00 00 INTERFACE CLASS: 1e 24 05 01 00 80 02 e0 01 00 00 0d 2f 00 00 0d 2f 00 60 09 00 15 16 05 00 01 15 16 05 00 INTERFACE CLASS: 1e 24 05 02 00 40 01 f0 00 c0 00 03 4b c0 00 03 4b 00 58 02 00 15 16 05 00 01 15 16 05 00 INTERFACE CLASS: 1e 24 05 03 00 20 03 58 02 70 00 14 99 70 00 14 99 00 a6 0e 00 9a 5b 06 00 01 9a 5b 06 00 INTERFACE CLASS: 1e 24 05 04 00 00 04 00 03 00 00 16 80 00 00 16 80 00 00 18 00 2a 2c 0a 00 01 2a 2c 0a 00 INTERFACE CLASS: 1e 24 05 05 00 00 05 00 04 00 00 12 c0 00 00 12 c0 00 00 28 00 d0 12 13 00 01 d0 12 13 00 INTERFACE CLASS: 1e 24 05 06 00 40 06 b0 04 00 00 0e a6 00 00 0e a6 00 98 3a 00 a0 25 26 00 01 a0 25 26 00 INTERFACE CLASS: 1e 24 05 01 00 80 02 e0 01 00 00 0d 2f 00 00 0d 2f 00 60 09 00 15 16 05 00 01 15 16 05 00 INTERFACE CLASS: 0b 24 03 00 01 80 02 e0 01 01 00
INTERFACE CLASS: 0b 24 06 02 07 00 01 00 00 00 00
INTERFACE CLASS: 1e 24 07 01 00 80 02 e0 01 00 00 0d 2f 00 00 0d 2f 00 60 09 00 0e 64 03 00 01 0e 64 03 00 INTERFACE CLASS: 1e 24 07 02 00 40 01 f0 00 c0 00 03 4b c0 00 03 4b 00 58 02 00 0e 64 03 00 01 0e 64 03 00 INTERFACE CLASS: 1e 24 07 03 00 20 03 58 02 70 00 14 99 70 00 14 99 00 a6 0e 00 0e 64 03 00 01 0e 64 03 00 INTERFACE CLASS: 1e 24 07 04 00 00 04 00 03 00 00 16 80 00 00 16 80 00 00 18 00 15 16 05 00 01 15 16 05 00 INTERFACE CLASS: 1e 24 07 05 00 00 05 00 04 00 00 12 c0 00 00 12 c0 00 00 28 00 2a 2c 0a 00 01 2a 2c 0a 00 INTERFACE CLASS: 1e 24 07 06 00 40 06 b0 04 00 00 0e a6 00 00 0e a6 00 98 3a 00 d0 12 13 00 01 d0 12 13 00 INTERFACE CLASS: 1e 24 07 01 00 80 02 e0 01 00 00 0d 2f 00 00 0d 2f 00 60 09 00 0e 64 03 00 01 0e 64 03 00 INTERFACE CLASS: 06 24 0d 00 00 00
Interface Descriptor:
bLength 9
bDescriptorType 4
bInterfaceNumber 2
bAlternateSetting 1
bNumEndpoints 1
bInterfaceClass 14 Video
bInterfaceSubClass 2 Video Streaming
bInterfaceProtocol 0
iInterface 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x82 EP 2 IN
bmAttributes 5
Transfer Type Isochronous
Synch Type Asynchronous
Usage Type Data
wMaxPacketSize 0x13fc 3x 1020 bytes
bInterval 1
Device Qualifier (for other device speed):
bLength 10
bDescriptorType 6
bcdUSB 2.00
bDeviceClass 239 Miscellaneous Device
bDeviceSubClass 2
bDeviceProtocol 1 Interface Association
bMaxPacketSize0 64
bNumConfigurations 1
Device Status: 0x0000
(Bus Powered)

Greg KH

maintainer

2020-01-01 18:47 UTC #4 lore raw

Quoted 54 lines · Laurent Pinchart

Hi Roger,
(CCin'g Alan Stern and linux-usb)
Quoted 6 lines · Roger Whittaker
Quoted 3 lines · Laurent Pinchart
The last message is worse. Could you send me the output of lsusb -v (you can restrict it to the camera with -d), if possible running as root, for both the working and non-working kernels ?
Quoted 7 lines
Thanks very much for your reply.
The lsusb outputs are attached - they are in fact identical to each other.
Also attached, the dmesg lines when replugging the camera on both kernels.
Thank you for the information.
I had missed the following message:
[  470.351700] usb 1-1.4.3.1: config 1 interface 2 altsetting 0 endpoint 0x82 has wMaxPacketSize 0, skipping
This seems to be the culprit, and it points to the USB core. One interface is ignored due to its wMaxPacketSize value, and the uvcvideo driver then fails to find it.
The wMaxPacketSize check was added in
commit d482c7bb0541d19dea8bff437a9f3c5563b5b2d2
Author: Alan Stern <[email protected]>
Date:   Mon Oct 28 10:52:35 2019 -0400
USB: Skip endpoints with 0 maxpacket length
Endpoints with a maxpacket length of 0 are probably useless. They can't transfer any data, and it's not at all unlikely that an HCD will crash or hang when trying to handle an URB for such an endpoint.
Currently the USB core does not check for endpoints having a maxpacket value of 0. This patch adds a check, printing a warning and skipping over any endpoints it catches.
Now, the USB spec does not rule out endpoints having maxpacket = 0. But since they wouldn't have any practical use, there doesn't seem to be any good reason for us to accept them.
Signed-off-by Alan Stern <[email protected]>
Link https://lore.kernel.org/r/[email protected] open here
Signed-off-by Greg Kroah-Hartman <[email protected]>

Quoted 7 lines

The commit was merged in v5.4 and backported to v5.3.11 in 47aaab6377204cdbcd16f52a23c584f994fd0d15.
For reference for Alan and linux-usb, the issue being discussed is described in https://bugzilla.suse.com/show_bug.cgi?id=1159811. The above commit seems to cause a regression with several cameras. I've attached to this e-mail the lsusb output provided by Roger.

How can a device work with an endpoint of 0 length?

What does the driver expect to do with those endpoints? Does it expect it to be present but just ignore it?

thanks,

greg k-h

Alan Stern

2020-01-01 20:09 UTC #5 lore raw

Quoted 63 lines · Greg KH

Quoted 54 lines · Laurent Pinchart
Hi Roger,
(CCin'g Alan Stern and linux-usb)
Quoted 6 lines · Roger Whittaker
Quoted 3 lines · Laurent Pinchart
The last message is worse. Could you send me the output of lsusb -v (you can restrict it to the camera with -d), if possible running as root, for both the working and non-working kernels ?
Quoted 7 lines
Thanks very much for your reply.
The lsusb outputs are attached - they are in fact identical to each other.
Also attached, the dmesg lines when replugging the camera on both kernels.
Thank you for the information.
I had missed the following message:
[  470.351700] usb 1-1.4.3.1: config 1 interface 2 altsetting 0 endpoint 0x82 has wMaxPacketSize 0, skipping
This seems to be the culprit, and it points to the USB core. One interface is ignored due to its wMaxPacketSize value, and the uvcvideo driver then fails to find it.
The wMaxPacketSize check was added in
commit d482c7bb0541d19dea8bff437a9f3c5563b5b2d2
Author: Alan Stern <[email protected]>
Date:   Mon Oct 28 10:52:35 2019 -0400
USB: Skip endpoints with 0 maxpacket length
Endpoints with a maxpacket length of 0 are probably useless. They can't transfer any data, and it's not at all unlikely that an HCD will crash or hang when trying to handle an URB for such an endpoint.
Currently the USB core does not check for endpoints having a maxpacket value of 0. This patch adds a check, printing a warning and skipping over any endpoints it catches.
Now, the USB spec does not rule out endpoints having maxpacket = 0. But since they wouldn't have any practical use, there doesn't seem to be any good reason for us to accept them.
Signed-off-by Alan Stern <[email protected]>
Link https://lore.kernel.org/r/[email protected] open here
Signed-off-by Greg Kroah-Hartman <[email protected]>
Quoted 7 lines
The commit was merged in v5.4 and backported to v5.3.11 in 47aaab6377204cdbcd16f52a23c584f994fd0d15.
For reference for Alan and linux-usb, the issue being discussed is described in https://bugzilla.suse.com/show_bug.cgi?id=1159811. The above commit seems to cause a regression with several cameras. I've attached to this e-mail the lsusb output provided by Roger.

Quoted 4 lines

How can a device work with an endpoint of 0 length?
What does the driver expect to do with those endpoints? Does it expect it to be present but just ignore it?

I see what's going on. The endpoint in question is isochronous, and the bAlternateSetting value is 0, which makes this the default altsetting for that interface. The USB spec says (at the end of section 5.6.3):

All device default interface settings must not include any isochronous endpoints with non-zero data payload sizes (specified via wMaxPacketSize in the endpoint descriptor). Alternate interface settings may specify non-zero data payload sizes for isochronous endpoints.

That explains why the maxpacket size is set to 0.

So it looks like the endpoint-descriptor parsing code might want to make a special case to accept isochronous endpoints with maxpacket 0 if bAlternateSetting is 0. But whether we do this or not, I would expect the uvcvideo driver to look for isochronous endpoints in the alternate settings it will actually use, not in altsetting 0. Then the presence or absence of that endpoint descriptor would make no difference to uvcvideo.

(Unless the UVC spec _requires_ these endpoint descriptors to be present. If it does then we should simply change the core parsing code and leave uvcvideo alone.)

Alan Stern

Johan Hovold

2020-01-02 11:20 UTC #6 lore raw

Quoted 68 lines · Alan Stern

Quoted 41 lines · Greg KH
Quoted 31 lines · Laurent Pinchart
[  470.351700] usb 1-1.4.3.1: config 1 interface 2 altsetting 0 endpoint 0x82 has wMaxPacketSize 0, skipping
This seems to be the culprit, and it points to the USB core. One interface is ignored due to its wMaxPacketSize value, and the uvcvideo driver then fails to find it.
The wMaxPacketSize check was added in
commit d482c7bb0541d19dea8bff437a9f3c5563b5b2d2
Author: Alan Stern <[email protected]>
Date:   Mon Oct 28 10:52:35 2019 -0400
USB: Skip endpoints with 0 maxpacket length
Endpoints with a maxpacket length of 0 are probably useless. They can't transfer any data, and it's not at all unlikely that an HCD will crash or hang when trying to handle an URB for such an endpoint.
Currently the USB core does not check for endpoints having a maxpacket value of 0. This patch adds a check, printing a warning and skipping over any endpoints it catches.
Now, the USB spec does not rule out endpoints having maxpacket = 0. But since they wouldn't have any practical use, there doesn't seem to be any good reason for us to accept them.
Signed-off-by Alan Stern <[email protected]>
Link https://lore.kernel.org/r/[email protected] open here
Signed-off-by Greg Kroah-Hartman <[email protected]>
Quoted 7 lines
The commit was merged in v5.4 and backported to v5.3.11 in 47aaab6377204cdbcd16f52a23c584f994fd0d15.
For reference for Alan and linux-usb, the issue being discussed is described in https://bugzilla.suse.com/show_bug.cgi?id=1159811. The above commit seems to cause a regression with several cameras. I've attached to this e-mail the lsusb output provided by Roger.
Quoted 4 lines
How can a device work with an endpoint of 0 length?
What does the driver expect to do with those endpoints? Does it expect it to be present but just ignore it?
I see what's going on. The endpoint in question is isochronous, and the bAlternateSetting value is 0, which makes this the default altsetting for that interface. The USB spec says (at the end of section 5.6.3):
All device default interface settings must not include any isochronous endpoints with non-zero data payload sizes (specified via wMaxPacketSize in the endpoint descriptor). Alternate interface settings may specify non-zero data payload sizes for isochronous endpoints.
That explains why the maxpacket size is set to 0.
So it looks like the endpoint-descriptor parsing code might want to make a special case to accept isochronous endpoints with maxpacket 0 if bAlternateSetting is 0. But whether we do this or not, I would expect the uvcvideo driver to look for isochronous endpoints in the alternate settings it will actually use, not in altsetting 0. Then the presence or absence of that endpoint descriptor would make no difference to uvcvideo.

Quoted 3 lines

(Unless the UVC spec _requires_ these endpoint descriptors to be present. If it does then we should simply change the core parsing code and leave uvcvideo alone.)

Note that we also have this little gem in the ftdi usb-serial driver (since 2009) overriding a zero max packet size for devices with broken descriptors:

895f28badce9 ("USB: ftdi_sio: fix hi-speed device packet size calculation")

Note sure how common those are but they will no longer work after the new sanity check in core. I guess we could add quirks for them (to core) in case we get any reports, but perhaps reverting the check should be considered.

There doesn't seem to be any other drivers accepting and explicitly handling zero max packet sizes like this in mainline however.

Johan

Takashi Iwai

2020-01-02 13:11 UTC #7 lore raw

Quoted 78 lines · Johan Hovold

Quoted 68 lines · Alan Stern
Quoted 41 lines · Greg KH
Quoted 31 lines · Laurent Pinchart
[  470.351700] usb 1-1.4.3.1: config 1 interface 2 altsetting 0 endpoint 0x82 has wMaxPacketSize 0, skipping
This seems to be the culprit, and it points to the USB core. One interface is ignored due to its wMaxPacketSize value, and the uvcvideo driver then fails to find it.
The wMaxPacketSize check was added in
commit d482c7bb0541d19dea8bff437a9f3c5563b5b2d2
Author: Alan Stern <[email protected]>
Date:   Mon Oct 28 10:52:35 2019 -0400
USB: Skip endpoints with 0 maxpacket length
Endpoints with a maxpacket length of 0 are probably useless. They can't transfer any data, and it's not at all unlikely that an HCD will crash or hang when trying to handle an URB for such an endpoint.
Currently the USB core does not check for endpoints having a maxpacket value of 0. This patch adds a check, printing a warning and skipping over any endpoints it catches.
Now, the USB spec does not rule out endpoints having maxpacket = 0. But since they wouldn't have any practical use, there doesn't seem to be any good reason for us to accept them.
Signed-off-by Alan Stern <[email protected]>
Link https://lore.kernel.org/r/[email protected] open here
Signed-off-by Greg Kroah-Hartman <[email protected]>
Quoted 7 lines
The commit was merged in v5.4 and backported to v5.3.11 in 47aaab6377204cdbcd16f52a23c584f994fd0d15.
For reference for Alan and linux-usb, the issue being discussed is described in https://bugzilla.suse.com/show_bug.cgi?id=1159811. The above commit seems to cause a regression with several cameras. I've attached to this e-mail the lsusb output provided by Roger.
Quoted 4 lines
How can a device work with an endpoint of 0 length?
What does the driver expect to do with those endpoints? Does it expect it to be present but just ignore it?
I see what's going on. The endpoint in question is isochronous, and the bAlternateSetting value is 0, which makes this the default altsetting for that interface. The USB spec says (at the end of section 5.6.3):
All device default interface settings must not include any isochronous endpoints with non-zero data payload sizes (specified via wMaxPacketSize in the endpoint descriptor). Alternate interface settings may specify non-zero data payload sizes for isochronous endpoints.
That explains why the maxpacket size is set to 0.
So it looks like the endpoint-descriptor parsing code might want to make a special case to accept isochronous endpoints with maxpacket 0 if bAlternateSetting is 0. But whether we do this or not, I would expect the uvcvideo driver to look for isochronous endpoints in the alternate settings it will actually use, not in altsetting 0. Then the presence or absence of that endpoint descriptor would make no difference to uvcvideo.
Quoted 3 lines
(Unless the UVC spec _requires_ these endpoint descriptors to be present. If it does then we should simply change the core parsing code and leave uvcvideo alone.)
Note that we also have this little gem in the ftdi usb-serial driver (since 2009) overriding a zero max packet size for devices with broken descriptors:

Quoted 6 lines

895f28badce9 ("USB: ftdi_sio: fix hi-speed device packet size calculation")
Note sure how common those are but they will no longer work after the new sanity check in core. I guess we could add quirks for them (to core) in case we get any reports, but perhaps reverting the check should be considered.

FWIW, Roger confirmed that reverting the commit d482c7bb0541 does indeed fix the issue (with the latest 5.4.y kernel).

thanks,

Takashi

Alan Stern

2020-01-02 15:06 UTC #8 lore raw

Quoted 82 lines · Takashi Iwai

Quoted 80 lines · Johan Hovold
Quoted 68 lines · Alan Stern
Quoted 41 lines · Greg KH
Quoted 31 lines · Laurent Pinchart
[  470.351700] usb 1-1.4.3.1: config 1 interface 2 altsetting 0 endpoint 0x82 has wMaxPacketSize 0, skipping
This seems to be the culprit, and it points to the USB core. One interface is ignored due to its wMaxPacketSize value, and the uvcvideo driver then fails to find it.
The wMaxPacketSize check was added in
commit d482c7bb0541d19dea8bff437a9f3c5563b5b2d2
Author: Alan Stern <[email protected]>
Date:   Mon Oct 28 10:52:35 2019 -0400
USB: Skip endpoints with 0 maxpacket length
Endpoints with a maxpacket length of 0 are probably useless. They can't transfer any data, and it's not at all unlikely that an HCD will crash or hang when trying to handle an URB for such an endpoint.
Currently the USB core does not check for endpoints having a maxpacket value of 0. This patch adds a check, printing a warning and skipping over any endpoints it catches.
Now, the USB spec does not rule out endpoints having maxpacket = 0. But since they wouldn't have any practical use, there doesn't seem to be any good reason for us to accept them.
Signed-off-by Alan Stern <[email protected]>
Link https://lore.kernel.org/r/[email protected] open here
Signed-off-by Greg Kroah-Hartman <[email protected]>
Quoted 7 lines
The commit was merged in v5.4 and backported to v5.3.11 in 47aaab6377204cdbcd16f52a23c584f994fd0d15.
For reference for Alan and linux-usb, the issue being discussed is described in https://bugzilla.suse.com/show_bug.cgi?id=1159811. The above commit seems to cause a regression with several cameras. I've attached to this e-mail the lsusb output provided by Roger.
Quoted 4 lines
How can a device work with an endpoint of 0 length?
What does the driver expect to do with those endpoints? Does it expect it to be present but just ignore it?
I see what's going on. The endpoint in question is isochronous, and the bAlternateSetting value is 0, which makes this the default altsetting for that interface. The USB spec says (at the end of section 5.6.3):
All device default interface settings must not include any isochronous endpoints with non-zero data payload sizes (specified via wMaxPacketSize in the endpoint descriptor). Alternate interface settings may specify non-zero data payload sizes for isochronous endpoints.
That explains why the maxpacket size is set to 0.
So it looks like the endpoint-descriptor parsing code might want to make a special case to accept isochronous endpoints with maxpacket 0 if bAlternateSetting is 0. But whether we do this or not, I would expect the uvcvideo driver to look for isochronous endpoints in the alternate settings it will actually use, not in altsetting 0. Then the presence or absence of that endpoint descriptor would make no difference to uvcvideo.
Quoted 3 lines
(Unless the UVC spec _requires_ these endpoint descriptors to be present. If it does then we should simply change the core parsing code and leave uvcvideo alone.)
Note that we also have this little gem in the ftdi usb-serial driver (since 2009) overriding a zero max packet size for devices with broken descriptors:
895f28badce9 ("USB: ftdi_sio: fix hi-speed device packet size calculation")

Quoted 7 lines

Quoted 4 lines
Note sure how common those are but they will no longer work after the new sanity check in core. I guess we could add quirks for them (to core) in case we get any reports, but perhaps reverting the check should be considered.
FWIW, Roger confirmed that reverting the commit d482c7bb0541 does indeed fix the issue (with the latest 5.4.y kernel).

All right. Suppose instead of reverting that commit, I change the code so that it only logs a warning when it finds an endpoint descriptor with maxpacket = 0 (and it skips the warning for isochronous endpoints in altsetting 0). At the same time, we can add a check to usb_submit_urb() to refuse URBs if the endpoint's maxpacket is 0.

Sounds good?

Alan Stern

Johan Hovold

2020-01-02 15:32 UTC #9 lore raw

Quoted 17 lines · Alan Stern

Quoted 8 lines · Takashi Iwai
Quoted 6 lines · Johan Hovold
Note that we also have this little gem in the ftdi usb-serial driver (since 2009) overriding a zero max packet size for devices with broken descriptors:
895f28badce9 ("USB: ftdi_sio: fix hi-speed device packet size calculation")
Quoted 7 lines
Quoted 4 lines
Note sure how common those are but they will no longer work after the new sanity check in core. I guess we could add quirks for them (to core) in case we get any reports, but perhaps reverting the check should be considered.
FWIW, Roger confirmed that reverting the commit d482c7bb0541 does indeed fix the issue (with the latest 5.4.y kernel).

Quoted 7 lines

All right. Suppose instead of reverting that commit, I change the code so that it only logs a warning when it finds an endpoint descriptor with maxpacket = 0 (and it skips the warning for isochronous endpoints in altsetting 0). At the same time, we can add a check to usb_submit_urb() to refuse URBs if the endpoint's maxpacket is 0.
Sounds good?

Sounds good to me.

Just make sure not to add a WARN() in usb_submit_urb() so that we end up having to add maxpacket checks to every USB driver when syzbot starts hitting this (only driver's doing maxpacket divisions or similar should need that).

Johan

Alan Stern

2020-01-02 18:24 UTC #10 lore raw

Quoted 9 lines · Johan Hovold

Quoted 9 lines
Quoted 2 lines
FWIW, Roger confirmed that reverting the commit d482c7bb0541 does indeed fix the issue (with the latest 5.4.y kernel).
All right. Suppose instead of reverting that commit, I change the code so that it only logs a warning when it finds an endpoint descriptor with maxpacket = 0 (and it skips the warning for isochronous endpoints in altsetting 0). At the same time, we can add a check to usb_submit_urb() to refuse URBs if the endpoint's maxpacket is 0.

Quoted 8 lines

Quoted 1 line
Sounds good?
Sounds good to me.
Just make sure not to add a WARN() in usb_submit_urb() so that we end up having to add maxpacket checks to every USB driver when syzbot starts hitting this (only driver's doing maxpacket divisions or similar should need that).

Heh. Yes, syzbot interprets WARN() very differently from dev_warn().

Okay, here's a patch for testing. This is meant to go on top of the existing commit; instead of rejecting bogus endpoints entirely it just issues a warning.

And it turns out that usb_submit_urb() already checks for maxpacket = 0, so no changes are needed there.

Alan Stern

drivers/usb/core/config.c +8 -4

Index: usb-devel/drivers/usb/core/config.c
===================================================================
@@ -346,12 +346,16 @@ static int usb_parse_endpoint(struct dev
 			endpoint->desc.wMaxPacketSize = cpu_to_le16(8);
 	}
 
-	/* Validate the wMaxPacketSize field */
+	/*
+	 * Validate the wMaxPacketSize field.
+	 * Some devices have isochronous endpoints in altsetting 0;
+	 * the USB-2 spec requires such endpoints to have wMaxPacketSize = 0
+	 * (see the end of section 5.6.3), so don't warn about them.
+	 */
 	maxp = usb_endpoint_maxp(&endpoint->desc);
-	if (maxp == 0) {
-		dev_warn(ddev, "config %d interface %d altsetting %d endpoint 0x%X has wMaxPacketSize 0, skipping\n",
+	if (maxp == 0 && !(usb_endpoint_xfer_isoc(d) && asnum == 0)) {
+		dev_warn(ddev, "config %d interface %d altsetting %d endpoint 0x%X has invalid wMaxPacketSize 0\n",
 		    cfgno, inum, asnum, d->bEndpointAddress);
-		goto skip_to_next_endpoint_or_interface_descriptor;
 	}
 
 	/* Find the highest legal maxpacket size for this endpoint */

Laurent Pinchart

author

2020-01-02 16:38 UTC #11 lore raw

In reply to Alan Stern

Hi Alan,

Quoted 89 lines · Alan Stern

Quoted 80 lines · Takashi Iwai
Quoted 79 lines · Johan Hovold
Quoted 68 lines · Alan Stern
Quoted 41 lines · Greg KH
Quoted 31 lines · Laurent Pinchart
[  470.351700] usb 1-1.4.3.1: config 1 interface 2 altsetting 0 endpoint 0x82 has wMaxPacketSize 0, skipping
This seems to be the culprit, and it points to the USB core. One interface is ignored due to its wMaxPacketSize value, and the uvcvideo driver then fails to find it.
The wMaxPacketSize check was added in
commit d482c7bb0541d19dea8bff437a9f3c5563b5b2d2
Author: Alan Stern <[email protected]>
Date:   Mon Oct 28 10:52:35 2019 -0400
USB: Skip endpoints with 0 maxpacket length
Endpoints with a maxpacket length of 0 are probably useless. They can't transfer any data, and it's not at all unlikely that an HCD will crash or hang when trying to handle an URB for such an endpoint.
Currently the USB core does not check for endpoints having a maxpacket value of 0. This patch adds a check, printing a warning and skipping over any endpoints it catches.
Now, the USB spec does not rule out endpoints having maxpacket = 0. But since they wouldn't have any practical use, there doesn't seem to be any good reason for us to accept them.
Signed-off-by Alan Stern <[email protected]>
Link https://lore.kernel.org/r/[email protected] open here
Signed-off-by Greg Kroah-Hartman <[email protected]>
Quoted 7 lines
The commit was merged in v5.4 and backported to v5.3.11 in 47aaab6377204cdbcd16f52a23c584f994fd0d15.
For reference for Alan and linux-usb, the issue being discussed is described in https://bugzilla.suse.com/show_bug.cgi?id=1159811. The above commit seems to cause a regression with several cameras. I've attached to this e-mail the lsusb output provided by Roger.
Quoted 4 lines
How can a device work with an endpoint of 0 length?
What does the driver expect to do with those endpoints? Does it expect it to be present but just ignore it?
I see what's going on. The endpoint in question is isochronous, and the bAlternateSetting value is 0, which makes this the default altsetting for that interface. The USB spec says (at the end of section 5.6.3):
All device default interface settings must not include any isochronous endpoints with non-zero data payload sizes (specified via wMaxPacketSize in the endpoint descriptor). Alternate interface settings may specify non-zero data payload sizes for isochronous endpoints.
That explains why the maxpacket size is set to 0.
So it looks like the endpoint-descriptor parsing code might want to make a special case to accept isochronous endpoints with maxpacket 0 if bAlternateSetting is 0. But whether we do this or not, I would expect the uvcvideo driver to look for isochronous endpoints in the alternate settings it will actually use, not in altsetting 0. Then the presence or absence of that endpoint descriptor would make no difference to uvcvideo.
Quoted 3 lines
(Unless the UVC spec _requires_ these endpoint descriptors to be present. If it does then we should simply change the core parsing code and leave uvcvideo alone.)
Note that we also have this little gem in the ftdi usb-serial driver (since 2009) overriding a zero max packet size for devices with broken descriptors:
895f28badce9 ("USB: ftdi_sio: fix hi-speed device packet size calculation")
Quoted 7 lines
Quoted 4 lines
Note sure how common those are but they will no longer work after the new sanity check in core. I guess we could add quirks for them (to core) in case we get any reports, but perhaps reverting the check should be considered.
FWIW, Roger confirmed that reverting the commit d482c7bb0541 does indeed fix the issue (with the latest 5.4.y kernel).

Quoted 7 lines

All right. Suppose instead of reverting that commit, I change the code so that it only logs a warning when it finds an endpoint descriptor with maxpacket = 0 (and it skips the warning for isochronous endpoints in altsetting 0). At the same time, we can add a check to usb_submit_urb() to refuse URBs if the endpoint's maxpacket is 0.
Sounds good?

It makes it easier for me as I won't have to adapt the uvcvideo driver, so it certainly sounds good :-)

The USB specification allows different endpoint settings in different alternate settings, and unless I'm mistaken, it includes a different number of endpoints and different endpoint numbers. The UVC specification isn't very clear on this topic, but in practice most isochronous devices don't include any isochronous endpoint in altsetting
0. Only 5 devices in my database of ~400 has an isochronous endpoint in altsetting 0, all of them with a wMaxPacketSize value set to 0. The above commit, if it only ignored the endpoint, should thus be fine, as the uvcvideo driver checks for endpoints in other alternate settings (the related code is the loop at the end of uvc_parse_streaming() in drivers/media/usb/uvc/uvc_driver.c). I thus wonder what's going wrong, and if the commit couldn't cause the interface to be mistakenly ignored as a whole ?

Roger, would you be able to set the uvcvideo trace module parameter to 0xffff before plugging the device, and provide the messages printed by the driver to the kernel log both with and without the above commit ?

Signature · 3 lines

Regards,

Laurent Pinchart

Roger Whittaker

2020-01-02 17:18 UTC #12 lore raw

Quoted 3 lines · Laurent Pinchart

Roger, would you be able to set the uvcvideo trace module parameter to 0xffff before plugging the device, and provide the messages printed by the driver to the kernel log both with and without the above commit ?

With 5.3.12-2-default, loading uvcvideo with

options uvcvideo trace=0xffff

On plugging:

[   73.571566] usb 1-1.4.3.1: new high-speed USB device number 12 using xhci_hcd
[   73.729180] usb 1-1.4.3.1: config 1 interface 2 altsetting 0 endpoint 0x82 has wMaxPacketSize 0, skipping
[   73.729552] usb 1-1.4.3.1: New USB device found, idVendor=1778, idProduct=0214, bcdDevice= 7.07
[   73.729558] usb 1-1.4.3.1: New USB device strings: Mfr=1, Product=2, SerialNumber=0
[   73.729561] usb 1-1.4.3.1: Product: IPEVO Point 2 View
[   73.729564] usb 1-1.4.3.1: Manufacturer: IPEVO Inc.
[   73.732670] hid-generic 0003:1778:0214.0009: hiddev98,hidraw8: USB HID v1.10 Device [IPEVO Inc. IPEVO Point 2 View] on usb-0000:00:14.0-1.4.3.1/input0
[   73.781765] videodev: Linux video capture interface: v2.00
[   73.807553] uvcvideo: Probing generic UVC device 1.4.3.1
[   73.807693] uvcvideo: no class-specific streaming interface descriptors found.
[   73.807728] uvcvideo: Found a Status endpoint (addr 81).
[   73.807730] uvcvideo: Found UVC 1.00 device IPEVO Point 2 View (1778:0214)
[   73.807759] uvcvideo: Failed to query (GET_INFO) UVC control 2 on unit 1: -32 (exp. 1).
[   73.807832] uvcvideo: Control error 6
[   73.807834] uvcvideo: Added control 00000000-0000-0000-0000-000000000001/2 to device 1.4.3.1 entity 1
[   73.807835] uvcvideo: Adding mapping 'Exposure, Auto' to control 00000000-0000-0000-0000-000000000001/2.
[   73.807876] uvcvideo: Added control 00000000-0000-0000-0000-000000000001/6 to device 1.4.3.1 entity 1
[   73.807877] uvcvideo: Adding mapping 'Focus (absolute)' to control 00000000-0000-0000-0000-000000000001/6.
[   73.807918] uvcvideo: Failed to query (GET_INFO) UVC control 9 on unit 1: -32 (exp. 1).
[   73.807996] uvcvideo: Control error 6
[   73.807997] uvcvideo: Added control 00000000-0000-0000-0000-000000000001/9 to device 1.4.3.1 entity 1
[   73.807998] uvcvideo: Adding mapping 'Iris, Absolute' to control 00000000-0000-0000-0000-000000000001/9.
[   73.808037] uvcvideo: Added control 00000000-0000-0000-0000-000000000001/8 to device 1.4.3.1 entity 1
[   73.808038] uvcvideo: Adding mapping 'Focus, Auto' to control 00000000-0000-0000-0000-000000000001/8.
[   73.808079] uvcvideo: Added control 00000000-0000-0000-0000-000000000101/2 to device 1.4.3.1 entity 3
[   73.808080] uvcvideo: Adding mapping 'Brightness' to control 00000000-0000-0000-0000-000000000101/2.
[   73.808119] uvcvideo: Added control 00000000-0000-0000-0000-000000000101/3 to device 1.4.3.1 entity 3
[   73.808120] uvcvideo: Adding mapping 'Contrast' to control 00000000-0000-0000-0000-000000000101/3.
[   73.808159] uvcvideo: Added control 00000000-0000-0000-0000-000000000101/6 to device 1.4.3.1 entity 3
[   73.808160] uvcvideo: Adding mapping 'Hue' to control 00000000-0000-0000-0000-000000000101/6.
[   73.808205] uvcvideo: Added control 00000000-0000-0000-0000-000000000101/7 to device 1.4.3.1 entity 3
[   73.808206] uvcvideo: Adding mapping 'Saturation' to control 00000000-0000-0000-0000-000000000101/7.
[   73.808255] uvcvideo: Added control 00000000-0000-0000-0000-000000000101/8 to device 1.4.3.1 entity 3
[   73.808257] uvcvideo: Adding mapping 'Sharpness' to control 00000000-0000-0000-0000-000000000101/8.
[   73.808308] uvcvideo: Added control 00000000-0000-0000-0000-000000000101/9 to device 1.4.3.1 entity 3
[   73.808309] uvcvideo: Adding mapping 'Gamma' to control 00000000-0000-0000-0000-000000000101/9.
[   73.808349] uvcvideo: Added control 00000000-0000-0000-0000-000000000101/10 to device 1.4.3.1 entity 3
[   73.808350] uvcvideo: Adding mapping 'White Balance Temperature' to control 00000000-0000-0000-0000-000000000101/10.
[   73.808389] uvcvideo: Added control 00000000-0000-0000-0000-000000000101/5 to device 1.4.3.1 entity 3
[   73.808390] uvcvideo: Adding mapping 'Power Line Frequency' to control 00000000-0000-0000-0000-000000000101/5.
[   73.808431] uvcvideo: Added control 00000000-0000-0000-0000-000000000101/11 to device 1.4.3.1 entity 3
[   73.808432] uvcvideo: Adding mapping 'White Balance Temperature, Auto' to control 00000000-0000-0000-0000-000000000101/11.
[   73.808434] uvcvideo: Scanning UVC chain: OT 2 <- XU 4 <- PU 3 <- IT 1
[   73.808437] uvcvideo: Found a valid video chain (1 -> 2).
[   73.808438] uvcvideo: No streaming interface found for terminal 2.
[   73.808442] uvcvideo 1-1.4.3.1:1.1: Entity type for entity Extension 4 was not initialized!
[   73.808444] uvcvideo 1-1.4.3.1:1.1: Entity type for entity Processing 3 was not initialized!
[   73.808446] uvcvideo 1-1.4.3.1:1.1: Entity type for entity Camera 1 was not initialized!
[   73.808542] input: IPEVO Point 2 View: IPEVO Point as /devices/pci0000:00/0000:00:14.0/usb1/1-1/1-1.4/1-1.4.3/1-1.4.3.1/1-1.4.3.1:1.1/input/input21
[   73.808590] uvcvideo: UVC device initialized.
[   73.808636] usbcore: registered new interface driver uvcvideo
[   73.808637] USB Video Class driver (1.1.1)
[   75.899721] uvcvideo: Suspending interface 1

------------------------------------------------------------------------

With 5.4.7-1.g43720a7-default (the kernel Takashi built with commit d482c7bb0541 reverted), loading uvcvideo with

options uvcvideo trace=0xffff

On plugging:

[  267.765563] usb 1-1.4.3.1: new high-speed USB device number 13 using xhci_hcd
[  267.879567] usb 1-1.4.3.1: New USB device found, idVendor=1778, idProduct=0214, bcdDevice= 7.07
[  267.879573] usb 1-1.4.3.1: New USB device strings: Mfr=1, Product=2, SerialNumber=0
[  267.879577] usb 1-1.4.3.1: Product: IPEVO Point 2 View
[  267.879580] usb 1-1.4.3.1: Manufacturer: IPEVO Inc.
[  267.882718] hid-generic 0003:1778:0214.000A: hiddev98,hidraw7: USB HID v1.10 Device [IPEVO Inc. IPEVO Point 2 View] on usb-0000:00:14.0-1.4.3.1/input0
[  267.883135] uvcvideo: Probing generic UVC device 1.4.3.1
[  267.883260] uvcvideo: trying extra data from endpoint 0.
[  267.883265] uvcvideo: Found format YUV 4:2:2 (YUYV).
[  267.883268] uvcvideo: - 640x480 (30.0 fps)
[  267.883277] uvcvideo: - 320x240 (30.0 fps)
[  267.883278] uvcvideo: - 800x600 (24.0 fps)
[  267.883280] uvcvideo: - 1024x768 (15.0 fps)
[  267.883282] uvcvideo: - 1280x1024 (8.0 fps)
[  267.883284] uvcvideo: - 1600x1200 (4.0 fps)
[  267.883286] uvcvideo: - 640x480 (30.0 fps)
[  267.883288] uvcvideo: Found format MJPEG.
[  267.883290] uvcvideo: - 640x480 (45.0 fps)
[  267.883292] uvcvideo: - 320x240 (45.0 fps)
[  267.883293] uvcvideo: - 800x600 (45.0 fps)
[  267.883295] uvcvideo: - 1024x768 (30.0 fps)
[  267.883297] uvcvideo: - 1280x1024 (15.0 fps)
[  267.883299] uvcvideo: - 1600x1200 (8.0 fps)
[  267.883301] uvcvideo: - 640x480 (45.0 fps)
[  267.883310] uvcvideo: Found a Status endpoint (addr 81).
[  267.883314] uvcvideo: Found UVC 1.00 device IPEVO Point 2 View (1778:0214)
[  267.883380] uvcvideo: Failed to query (GET_INFO) UVC control 2 on unit 1: -32 (exp. 1).
[  267.883411] uvcvideo: Control error 6
[  267.883416] uvcvideo: Added control 00000000-0000-0000-0000-000000000001/2 to device 1.4.3.1 entity 1
[  267.883419] uvcvideo: Adding mapping 'Exposure, Auto' to control 00000000-0000-0000-0000-000000000001/2.
[  267.883468] uvcvideo: Added control 00000000-0000-0000-0000-000000000001/6 to device 1.4.3.1 entity 1
[  267.883471] uvcvideo: Adding mapping 'Focus (absolute)' to control 00000000-0000-0000-0000-000000000001/6.
[  267.883512] uvcvideo: Failed to query (GET_INFO) UVC control 9 on unit 1: -32 (exp. 1).
[  267.883588] uvcvideo: Control error 6
[  267.883590] uvcvideo: Added control 00000000-0000-0000-0000-000000000001/9 to device 1.4.3.1 entity 1
[  267.883593] uvcvideo: Adding mapping 'Iris, Absolute' to control 00000000-0000-0000-0000-000000000001/9.
[  267.883642] uvcvideo: Added control 00000000-0000-0000-0000-000000000001/8 to device 1.4.3.1 entity 1
[  267.883645] uvcvideo: Adding mapping 'Focus, Auto' to control 00000000-0000-0000-0000-000000000001/8.
[  267.883694] uvcvideo: Added control 00000000-0000-0000-0000-000000000101/2 to device 1.4.3.1 entity 3
[  267.883696] uvcvideo: Adding mapping 'Brightness' to control 00000000-0000-0000-0000-000000000101/2.
[  267.883745] uvcvideo: Added control 00000000-0000-0000-0000-000000000101/3 to device 1.4.3.1 entity 3
[  267.883747] uvcvideo: Adding mapping 'Contrast' to control 00000000-0000-0000-0000-000000000101/3.
[  267.883795] uvcvideo: Added control 00000000-0000-0000-0000-000000000101/6 to device 1.4.3.1 entity 3
[  267.883797] uvcvideo: Adding mapping 'Hue' to control 00000000-0000-0000-0000-000000000101/6.
[  267.883846] uvcvideo: Added control 00000000-0000-0000-0000-000000000101/7 to device 1.4.3.1 entity 3
[  267.883848] uvcvideo: Adding mapping 'Saturation' to control 00000000-0000-0000-0000-000000000101/7.
[  267.883895] uvcvideo: Added control 00000000-0000-0000-0000-000000000101/8 to device 1.4.3.1 entity 3
[  267.883898] uvcvideo: Adding mapping 'Sharpness' to control 00000000-0000-0000-0000-000000000101/8.
[  267.883947] uvcvideo: Added control 00000000-0000-0000-0000-000000000101/9 to device 1.4.3.1 entity 3
[  267.883949] uvcvideo: Adding mapping 'Gamma' to control 00000000-0000-0000-0000-000000000101/9.
[  267.883999] uvcvideo: Added control 00000000-0000-0000-0000-000000000101/10 to device 1.4.3.1 entity 3
[  267.884002] uvcvideo: Adding mapping 'White Balance Temperature' to control 00000000-0000-0000-0000-000000000101/10.
[  267.884050] uvcvideo: Added control 00000000-0000-0000-0000-000000000101/5 to device 1.4.3.1 entity 3
[  267.884053] uvcvideo: Adding mapping 'Power Line Frequency' to control 00000000-0000-0000-0000-000000000101/5.
[  267.884101] uvcvideo: Added control 00000000-0000-0000-0000-000000000101/11 to device 1.4.3.1 entity 3
[  267.884104] uvcvideo: Adding mapping 'White Balance Temperature, Auto' to control 00000000-0000-0000-0000-000000000101/11.
[  267.884108] uvcvideo: Scanning UVC chain: OT 2 <- XU 4 <- PU 3 <- IT 1
[  267.884117] uvcvideo: Found a valid video chain (1 -> 2).
[  267.885020] uvcvideo 1-1.4.3.1:1.1: Entity type for entity Extension 4 was not initialized!
[  267.885025] uvcvideo 1-1.4.3.1:1.1: Entity type for entity Processing 3 was not initialized!
[  267.885028] uvcvideo 1-1.4.3.1:1.1: Entity type for entity Camera 1 was not initialized!
[  267.885188] input: IPEVO Point 2 View: IPEVO Point as /devices/pci0000:00/0000:00:14.0/usb1/1-1/1-1.4/1-1.4.3/1-1.4.3.1/1-1.4.3.1:1.1/input/input22
[  267.885266] uvcvideo: UVC device initialized.
[  267.919845] uvcvideo: uvc_v4l2_open
[  267.919884] uvcvideo: uvc_v4l2_release
[  270.387236] uvcvideo: Suspending interface 2
[  270.387241] uvcvideo: Suspending interface 1

Signature · 13 lines

============================================
Roger Whittaker
SUSE Linux Premium Support Engineer
 
[email protected]
+44 7802 357081
 
SUSE Linux
One Station Square
Bracknell
RG12 1QB
United Kingdom
============================================

Laurent Pinchart

author

2020-01-02 17:03 UTC #13 lore raw

Hi Roger,

Quoted 9 lines · Roger Whittaker

Quoted 3 lines · Laurent Pinchart
Roger, would you be able to set the uvcvideo trace module parameter to 0xffff before plugging the device, and provide the messages printed by the driver to the kernel log both with and without the above commit ?
With 5.3.12-2-default, loading uvcvideo with
options uvcvideo trace=0xffff

Thank you.

Quoted 12 lines

On plugging:

[   73.571566] usb 1-1.4.3.1: new high-speed USB device number 12 using xhci_hcd
[   73.729180] usb 1-1.4.3.1: config 1 interface 2 altsetting 0 endpoint 0x82 has wMaxPacketSize 0, skipping
[   73.729552] usb 1-1.4.3.1: New USB device found, idVendor=1778, idProduct=0214, bcdDevice= 7.07
[   73.729558] usb 1-1.4.3.1: New USB device strings: Mfr=1, Product=2, SerialNumber=0
[   73.729561] usb 1-1.4.3.1: Product: IPEVO Point 2 View
[   73.729564] usb 1-1.4.3.1: Manufacturer: IPEVO Inc.
[   73.732670] hid-generic 0003:1778:0214.0009: hiddev98,hidraw8: USB HID v1.10 Device [IPEVO Inc. IPEVO Point 2 View] on usb-0000:00:14.0-1.4.3.1/input0
[   73.781765] videodev: Linux video capture interface: v2.00
[   73.807553] uvcvideo: Probing generic UVC device 1.4.3.1
[   73.807693] uvcvideo: no class-specific streaming interface descriptors found.

It seems that Alan's patch causes more than the endpoint to be ignored.

Quoted 121 lines

[   73.807728] uvcvideo: Found a Status endpoint (addr 81).
[   73.807730] uvcvideo: Found UVC 1.00 device IPEVO Point 2 View (1778:0214)
[   73.807759] uvcvideo: Failed to query (GET_INFO) UVC control 2 on unit 1: -32 (exp. 1).
[   73.807832] uvcvideo: Control error 6
[   73.807834] uvcvideo: Added control 00000000-0000-0000-0000-000000000001/2 to device 1.4.3.1 entity 1
[   73.807835] uvcvideo: Adding mapping 'Exposure, Auto' to control 00000000-0000-0000-0000-000000000001/2.
[   73.807876] uvcvideo: Added control 00000000-0000-0000-0000-000000000001/6 to device 1.4.3.1 entity 1
[   73.807877] uvcvideo: Adding mapping 'Focus (absolute)' to control 00000000-0000-0000-0000-000000000001/6.
[   73.807918] uvcvideo: Failed to query (GET_INFO) UVC control 9 on unit 1: -32 (exp. 1).
[   73.807996] uvcvideo: Control error 6
[   73.807997] uvcvideo: Added control 00000000-0000-0000-0000-000000000001/9 to device 1.4.3.1 entity 1
[   73.807998] uvcvideo: Adding mapping 'Iris, Absolute' to control 00000000-0000-0000-0000-000000000001/9.
[   73.808037] uvcvideo: Added control 00000000-0000-0000-0000-000000000001/8 to device 1.4.3.1 entity 1
[   73.808038] uvcvideo: Adding mapping 'Focus, Auto' to control 00000000-0000-0000-0000-000000000001/8.
[   73.808079] uvcvideo: Added control 00000000-0000-0000-0000-000000000101/2 to device 1.4.3.1 entity 3
[   73.808080] uvcvideo: Adding mapping 'Brightness' to control 00000000-0000-0000-0000-000000000101/2.
[   73.808119] uvcvideo: Added control 00000000-0000-0000-0000-000000000101/3 to device 1.4.3.1 entity 3
[   73.808120] uvcvideo: Adding mapping 'Contrast' to control 00000000-0000-0000-0000-000000000101/3.
[   73.808159] uvcvideo: Added control 00000000-0000-0000-0000-000000000101/6 to device 1.4.3.1 entity 3
[   73.808160] uvcvideo: Adding mapping 'Hue' to control 00000000-0000-0000-0000-000000000101/6.
[   73.808205] uvcvideo: Added control 00000000-0000-0000-0000-000000000101/7 to device 1.4.3.1 entity 3
[   73.808206] uvcvideo: Adding mapping 'Saturation' to control 00000000-0000-0000-0000-000000000101/7.
[   73.808255] uvcvideo: Added control 00000000-0000-0000-0000-000000000101/8 to device 1.4.3.1 entity 3
[   73.808257] uvcvideo: Adding mapping 'Sharpness' to control 00000000-0000-0000-0000-000000000101/8.
[   73.808308] uvcvideo: Added control 00000000-0000-0000-0000-000000000101/9 to device 1.4.3.1 entity 3
[   73.808309] uvcvideo: Adding mapping 'Gamma' to control 00000000-0000-0000-0000-000000000101/9.
[   73.808349] uvcvideo: Added control 00000000-0000-0000-0000-000000000101/10 to device 1.4.3.1 entity 3
[   73.808350] uvcvideo: Adding mapping 'White Balance Temperature' to control 00000000-0000-0000-0000-000000000101/10.
[   73.808389] uvcvideo: Added control 00000000-0000-0000-0000-000000000101/5 to device 1.4.3.1 entity 3
[   73.808390] uvcvideo: Adding mapping 'Power Line Frequency' to control 00000000-0000-0000-0000-000000000101/5.
[   73.808431] uvcvideo: Added control 00000000-0000-0000-0000-000000000101/11 to device 1.4.3.1 entity 3
[   73.808432] uvcvideo: Adding mapping 'White Balance Temperature, Auto' to control 00000000-0000-0000-0000-000000000101/11.
[   73.808434] uvcvideo: Scanning UVC chain: OT 2 <- XU 4 <- PU 3 <- IT 1
[   73.808437] uvcvideo: Found a valid video chain (1 -> 2).
[   73.808438] uvcvideo: No streaming interface found for terminal 2.
[   73.808442] uvcvideo 1-1.4.3.1:1.1: Entity type for entity Extension 4 was not initialized!
[   73.808444] uvcvideo 1-1.4.3.1:1.1: Entity type for entity Processing 3 was not initialized!
[   73.808446] uvcvideo 1-1.4.3.1:1.1: Entity type for entity Camera 1 was not initialized!
[   73.808542] input: IPEVO Point 2 View: IPEVO Point as /devices/pci0000:00/0000:00:14.0/usb1/1-1/1-1.4/1-1.4.3/1-1.4.3.1/1-1.4.3.1:1.1/input/input21
[   73.808590] uvcvideo: UVC device initialized.
[   73.808636] usbcore: registered new interface driver uvcvideo
[   73.808637] USB Video Class driver (1.1.1)
[   75.899721] uvcvideo: Suspending interface 1

------------------------------------------------------------------------

With 5.4.7-1.g43720a7-default (the kernel Takashi built with commit d482c7bb0541 reverted), loading uvcvideo with

options uvcvideo trace=0xffff

On plugging:

[  267.765563] usb 1-1.4.3.1: new high-speed USB device number 13 using xhci_hcd
[  267.879567] usb 1-1.4.3.1: New USB device found, idVendor=1778, idProduct=0214, bcdDevice= 7.07
[  267.879573] usb 1-1.4.3.1: New USB device strings: Mfr=1, Product=2, SerialNumber=0
[  267.879577] usb 1-1.4.3.1: Product: IPEVO Point 2 View
[  267.879580] usb 1-1.4.3.1: Manufacturer: IPEVO Inc.
[  267.882718] hid-generic 0003:1778:0214.000A: hiddev98,hidraw7: USB HID v1.10 Device [IPEVO Inc. IPEVO Point 2 View] on usb-0000:00:14.0-1.4.3.1/input0
[  267.883135] uvcvideo: Probing generic UVC device 1.4.3.1
[  267.883260] uvcvideo: trying extra data from endpoint 0.
[  267.883265] uvcvideo: Found format YUV 4:2:2 (YUYV).
[  267.883268] uvcvideo: - 640x480 (30.0 fps)
[  267.883277] uvcvideo: - 320x240 (30.0 fps)
[  267.883278] uvcvideo: - 800x600 (24.0 fps)
[  267.883280] uvcvideo: - 1024x768 (15.0 fps)
[  267.883282] uvcvideo: - 1280x1024 (8.0 fps)
[  267.883284] uvcvideo: - 1600x1200 (4.0 fps)
[  267.883286] uvcvideo: - 640x480 (30.0 fps)
[  267.883288] uvcvideo: Found format MJPEG.
[  267.883290] uvcvideo: - 640x480 (45.0 fps)
[  267.883292] uvcvideo: - 320x240 (45.0 fps)
[  267.883293] uvcvideo: - 800x600 (45.0 fps)
[  267.883295] uvcvideo: - 1024x768 (30.0 fps)
[  267.883297] uvcvideo: - 1280x1024 (15.0 fps)
[  267.883299] uvcvideo: - 1600x1200 (8.0 fps)
[  267.883301] uvcvideo: - 640x480 (45.0 fps)
[  267.883310] uvcvideo: Found a Status endpoint (addr 81).
[  267.883314] uvcvideo: Found UVC 1.00 device IPEVO Point 2 View (1778:0214)
[  267.883380] uvcvideo: Failed to query (GET_INFO) UVC control 2 on unit 1: -32 (exp. 1).
[  267.883411] uvcvideo: Control error 6
[  267.883416] uvcvideo: Added control 00000000-0000-0000-0000-000000000001/2 to device 1.4.3.1 entity 1
[  267.883419] uvcvideo: Adding mapping 'Exposure, Auto' to control 00000000-0000-0000-0000-000000000001/2.
[  267.883468] uvcvideo: Added control 00000000-0000-0000-0000-000000000001/6 to device 1.4.3.1 entity 1
[  267.883471] uvcvideo: Adding mapping 'Focus (absolute)' to control 00000000-0000-0000-0000-000000000001/6.
[  267.883512] uvcvideo: Failed to query (GET_INFO) UVC control 9 on unit 1: -32 (exp. 1).
[  267.883588] uvcvideo: Control error 6
[  267.883590] uvcvideo: Added control 00000000-0000-0000-0000-000000000001/9 to device 1.4.3.1 entity 1
[  267.883593] uvcvideo: Adding mapping 'Iris, Absolute' to control 00000000-0000-0000-0000-000000000001/9.
[  267.883642] uvcvideo: Added control 00000000-0000-0000-0000-000000000001/8 to device 1.4.3.1 entity 1
[  267.883645] uvcvideo: Adding mapping 'Focus, Auto' to control 00000000-0000-0000-0000-000000000001/8.
[  267.883694] uvcvideo: Added control 00000000-0000-0000-0000-000000000101/2 to device 1.4.3.1 entity 3
[  267.883696] uvcvideo: Adding mapping 'Brightness' to control 00000000-0000-0000-0000-000000000101/2.
[  267.883745] uvcvideo: Added control 00000000-0000-0000-0000-000000000101/3 to device 1.4.3.1 entity 3
[  267.883747] uvcvideo: Adding mapping 'Contrast' to control 00000000-0000-0000-0000-000000000101/3.
[  267.883795] uvcvideo: Added control 00000000-0000-0000-0000-000000000101/6 to device 1.4.3.1 entity 3
[  267.883797] uvcvideo: Adding mapping 'Hue' to control 00000000-0000-0000-0000-000000000101/6.
[  267.883846] uvcvideo: Added control 00000000-0000-0000-0000-000000000101/7 to device 1.4.3.1 entity 3
[  267.883848] uvcvideo: Adding mapping 'Saturation' to control 00000000-0000-0000-0000-000000000101/7.
[  267.883895] uvcvideo: Added control 00000000-0000-0000-0000-000000000101/8 to device 1.4.3.1 entity 3
[  267.883898] uvcvideo: Adding mapping 'Sharpness' to control 00000000-0000-0000-0000-000000000101/8.
[  267.883947] uvcvideo: Added control 00000000-0000-0000-0000-000000000101/9 to device 1.4.3.1 entity 3
[  267.883949] uvcvideo: Adding mapping 'Gamma' to control 00000000-0000-0000-0000-000000000101/9.
[  267.883999] uvcvideo: Added control 00000000-0000-0000-0000-000000000101/10 to device 1.4.3.1 entity 3
[  267.884002] uvcvideo: Adding mapping 'White Balance Temperature' to control 00000000-0000-0000-0000-000000000101/10.
[  267.884050] uvcvideo: Added control 00000000-0000-0000-0000-000000000101/5 to device 1.4.3.1 entity 3
[  267.884053] uvcvideo: Adding mapping 'Power Line Frequency' to control 00000000-0000-0000-0000-000000000101/5.
[  267.884101] uvcvideo: Added control 00000000-0000-0000-0000-000000000101/11 to device 1.4.3.1 entity 3
[  267.884104] uvcvideo: Adding mapping 'White Balance Temperature, Auto' to control 00000000-0000-0000-0000-000000000101/11.
[  267.884108] uvcvideo: Scanning UVC chain: OT 2 <- XU 4 <- PU 3 <- IT 1
[  267.884117] uvcvideo: Found a valid video chain (1 -> 2).
[  267.885020] uvcvideo 1-1.4.3.1:1.1: Entity type for entity Extension 4 was not initialized!
[  267.885025] uvcvideo 1-1.4.3.1:1.1: Entity type for entity Processing 3 was not initialized!
[  267.885028] uvcvideo 1-1.4.3.1:1.1: Entity type for entity Camera 1 was not initialized!
[  267.885188] input: IPEVO Point 2 View: IPEVO Point as /devices/pci0000:00/0000:00:14.0/usb1/1-1/1-1.4/1-1.4.3/1-1.4.3.1/1-1.4.3.1:1.1/input/input22
[  267.885266] uvcvideo: UVC device initialized.
[  267.919845] uvcvideo: uvc_v4l2_open
[  267.919884] uvcvideo: uvc_v4l2_release
[  270.387236] uvcvideo: Suspending interface 2
[  270.387241] uvcvideo: Suspending interface 1

Signature · 3 lines

Regards,

Laurent Pinchart

Alan Stern

2020-01-02 17:49 UTC #14 lore raw

Quoted 28 lines · Laurent Pinchart

Hi Roger,
Quoted 9 lines · Roger Whittaker
Quoted 3 lines · Laurent Pinchart
Roger, would you be able to set the uvcvideo trace module parameter to 0xffff before plugging the device, and provide the messages printed by the driver to the kernel log both with and without the above commit ?
With 5.3.12-2-default, loading uvcvideo with
options uvcvideo trace=0xffff
Thank you.
Quoted 12 lines
On plugging:
[   73.571566] usb 1-1.4.3.1: new high-speed USB device number 12 using xhci_hcd
[   73.729180] usb 1-1.4.3.1: config 1 interface 2 altsetting 0 endpoint 0x82 has wMaxPacketSize 0, skipping
[   73.729552] usb 1-1.4.3.1: New USB device found, idVendor=1778, idProduct=0214, bcdDevice= 7.07
[   73.729558] usb 1-1.4.3.1: New USB device strings: Mfr=1, Product=2, SerialNumber=0
[   73.729561] usb 1-1.4.3.1: Product: IPEVO Point 2 View
[   73.729564] usb 1-1.4.3.1: Manufacturer: IPEVO Inc.
[   73.732670] hid-generic 0003:1778:0214.0009: hiddev98,hidraw8: USB HID v1.10 Device [IPEVO Inc. IPEVO Point 2 View] on usb-0000:00:14.0-1.4.3.1/input0
[   73.781765] videodev: Linux video capture interface: v2.00
[   73.807553] uvcvideo: Probing generic UVC device 1.4.3.1
[   73.807693] uvcvideo: no class-specific streaming interface descriptors found.

Quoted 1 line

It seems that Alan's patch causes more than the endpoint to be ignored.

Roger, you can get more information by plugging in the device and then posting the contents of /sys/kernel/debug/usb/devices (or just the portion that refers to the camera). It would be interesting to compare the values from the kernel with the commit present and the kernel with the commit reverted.

Alan Stern

Roger Whittaker

2020-01-02 21:52 UTC #15 lore raw

Quoted 5 lines · Alan Stern

Roger, you can get more information by plugging in the device and then posting the contents of /sys/kernel/debug/usb/devices (or just the portion that refers to the camera). It would be interesting to compare the values from the kernel with the commit present and the kernel with the commit reverted.

5.3.12-2-default (commit present)

T:  Bus=01 Lev=04 Prnt=08 Port=00 Cnt=01 Dev#= 10 Spd=480  MxCh= 0
D:  Ver= 2.00 Cls=ef(misc ) Sub=02 Prot=01 MxPS=64 #Cfgs=  1
P:  Vendor=1778 ProdID=0214 Rev= 7.07
S:  Manufacturer=IPEVO Inc.
S:  Product=IPEVO Point 2 View

C:* #Ifs= 3 Cfg#= 1 Atr=80 MxPwr=500mA
A: FirstIf#= 1 IfCount= 2 Cls=0e(video) Sub=03 Prot=00
I:* If#= 0 Alt= 0 #EPs= 1 Cls=03(HID ) Sub=00 Prot=00 Driver=usbhid
E: Ad=85(I) Atr=03(Int.) MxPS= 8 Ivl=64ms
I:* If#= 1 Alt= 0 #EPs= 1 Cls=0e(video) Sub=01 Prot=00 Driver=uvcvideo
E: Ad=81(I) Atr=03(Int.) MxPS= 8 Ivl=32ms
I:* If#= 2 Alt= 0 #EPs= 1 Cls=0e(video) Sub=02 Prot=00 Driver=(none)

E:  Ad=82(I) Atr=05(Isoc) MxPS=   0 Ivl=125us
I:  If#= 2 Alt= 1 #EPs= 1 Cls=0e(video) Sub=02 Prot=00 Driver=(none)
E:  Ad=82(I) Atr=05(Isoc) MxPS=3060 Ivl=125us

----------------

5.4.7-1.g43720a7-default (commit reverted)

T:  Bus=01 Lev=04 Prnt=08 Port=00 Cnt=01 Dev#= 13 Spd=480  MxCh= 0
D:  Ver= 2.00 Cls=ef(misc ) Sub=02 Prot=01 MxPS=64 #Cfgs=  1
P:  Vendor=1778 ProdID=0214 Rev= 7.07
S:  Manufacturer=IPEVO Inc.
S:  Product=IPEVO Point 2 View

C:* #Ifs= 3 Cfg#= 1 Atr=80 MxPwr=500mA
A: FirstIf#= 1 IfCount= 2 Cls=0e(video) Sub=03 Prot=00
I:* If#= 0 Alt= 0 #EPs= 1 Cls=03(HID ) Sub=00 Prot=00 Driver=usbhid
E: Ad=85(I) Atr=03(Int.) MxPS= 8 Ivl=64ms
I:* If#= 1 Alt= 0 #EPs= 1 Cls=0e(video) Sub=01 Prot=00 Driver=uvcvideo
E: Ad=81(I) Atr=03(Int.) MxPS= 8 Ivl=32ms
I:* If#= 2 Alt= 0 #EPs= 1 Cls=0e(video) Sub=02 Prot=00 Driver=uvcvideo

E:  Ad=82(I) Atr=05(Isoc) MxPS=   0 Ivl=125us
I:  If#= 2 Alt= 1 #EPs= 1 Cls=0e(video) Sub=02 Prot=00 Driver=uvcvideo
E:  Ad=82(I) Atr=05(Isoc) MxPS=3060 Ivl=125us

Signature · 13 lines

============================================
Roger Whittaker
SUSE Linux Premium Support Engineer
 
[email protected]
+44 7802 357081
 
SUSE Linux
One Station Square
Bracknell
RG12 1QB
United Kingdom
============================================

Laurent Pinchart

author

2020-01-02 23:11 UTC #16 lore raw

In reply to Alan Stern

Hi Alan,

Quoted 27 lines · Alan Stern

Quoted 26 lines · Laurent Pinchart
Quoted 9 lines · Roger Whittaker
Quoted 3 lines · Laurent Pinchart
Roger, would you be able to set the uvcvideo trace module parameter to 0xffff before plugging the device, and provide the messages printed by the driver to the kernel log both with and without the above commit ?
With 5.3.12-2-default, loading uvcvideo with
options uvcvideo trace=0xffff
Thank you.
Quoted 12 lines
On plugging:
[   73.571566] usb 1-1.4.3.1: new high-speed USB device number 12 using xhci_hcd
[   73.729180] usb 1-1.4.3.1: config 1 interface 2 altsetting 0 endpoint 0x82 has wMaxPacketSize 0, skipping
[   73.729552] usb 1-1.4.3.1: New USB device found, idVendor=1778, idProduct=0214, bcdDevice= 7.07
[   73.729558] usb 1-1.4.3.1: New USB device strings: Mfr=1, Product=2, SerialNumber=0
[   73.729561] usb 1-1.4.3.1: Product: IPEVO Point 2 View
[   73.729564] usb 1-1.4.3.1: Manufacturer: IPEVO Inc.
[   73.732670] hid-generic 0003:1778:0214.0009: hiddev98,hidraw8: USB HID v1.10 Device [IPEVO Inc. IPEVO Point 2 View] on usb-0000:00:14.0-1.4.3.1/input0
[   73.781765] videodev: Linux video capture interface: v2.00
[   73.807553] uvcvideo: Probing generic UVC device 1.4.3.1
[   73.807693] uvcvideo: no class-specific streaming interface descriptors found.

Quoted 7 lines

Quoted 1 line
It seems that Alan's patch causes more than the endpoint to be ignored.
Roger, you can get more information by plugging in the device and then posting the contents of /sys/kernel/debug/usb/devices (or just the portion that refers to the camera). It would be interesting to compare the values from the kernel with the commit present and the kernel with the commit reverted.

I've investigated this a bit further.

UVC defines class-specific interface descriptors that are usually located right after the standard interface descriptor in altsetting 0. The uvcvideo driver accesses those descriptor through intf->altsetting[0].extra. However, some devices insert an isochronous endpoint descriptor with wMaxPAcketSize set to 0 between the standard interface descriptor and the UVC class-specific interface descriptors.

Before your patch, these descriptors were recorded in the extra field of the endpoint, as they're located right after it:

static int usb_parse_endpoint(struct device *ddev, int cfgno, int inum,
int asnum, struct usb_host_interface *ifp, int num_ep, unsigned char *buffer, int size)
{
...

        /* Skip over any Class Specific or Vendor Specific descriptors;
         * find the next endpoint or interface descriptor */
        endpoint->extra = buffer;
        i = find_next_descriptor(buffer, size, USB_DT_ENDPOINT,
                        USB_DT_INTERFACE, &n);
        endpoint->extralen = i;

...
}

The uvcvideo driver looks at endpoint->extra when altsetting[0] has no extra data.

With your patch, the endpoint is skipped, and the class-specific interface descriptors are dropped with it. The uvcvideo driver can't access them and fails.

One solution may be to add to altsetting[0].extra all the extra class-specific descriptors, regardless of whether they're located before or after endpoints. I however fear we may not always be able to identify those descriptors properly, especially with the CS_INTERFACE descriptor type being defined in class specifications, not in the USB core specification. There's also a risk of breaking working devices if we do so (the uvcvideo driver should be able to cope, but other drivers may always look for descriptors in the endpoint).

Signature · 3 lines

Regards,

Laurent Pinchart

Alan Stern

2020-01-03 15:13 UTC #17 lore raw

Quoted 26 lines · Laurent Pinchart

I've investigated this a bit further.
UVC defines class-specific interface descriptors that are usually located right after the standard interface descriptor in altsetting 0. The uvcvideo driver accesses those descriptor through intf->altsetting[0].extra. However, some devices insert an isochronous endpoint descriptor with wMaxPAcketSize set to 0 between the standard interface descriptor and the UVC class-specific interface descriptors.
Before your patch, these descriptors were recorded in the extra field of the endpoint, as they're located right after it:
static int usb_parse_endpoint(struct device *ddev, int cfgno, int inum,
int asnum, struct usb_host_interface *ifp, int num_ep, unsigned char *buffer, int size)
{
...
        /* Skip over any Class Specific or Vendor Specific descriptors;
         * find the next endpoint or interface descriptor */
        endpoint->extra = buffer;
        i = find_next_descriptor(buffer, size, USB_DT_ENDPOINT,
                        USB_DT_INTERFACE, &n);
        endpoint->extralen = i;
...
}

Quoted 6 lines

The uvcvideo driver looks at endpoint->extra when altsetting[0] has no extra data.
With your patch, the endpoint is skipped, and the class-specific interface descriptors are dropped with it. The uvcvideo driver can't access them and fails.

Ah, a very tricky and unexpected interaction!

Quoted 8 lines

One solution may be to add to altsetting[0].extra all the extra class-specific descriptors, regardless of whether they're located before or after endpoints. I however fear we may not always be able to identify those descriptors properly, especially with the CS_INTERFACE descriptor type being defined in class specifications, not in the USB core specification. There's also a risk of breaking working devices if we do so (the uvcvideo driver should be able to cope, but other drivers may always look for descriptors in the endpoint).

With the patch I posted yesterday, everything should go back to working the way it used to. Have you had a chance to test it?

Alan Stern

Laurent Pinchart

author

2020-01-04 18:22 UTC #18 lore raw

Hi Alan,

Quoted 46 lines · Alan Stern

Quoted 26 lines · Laurent Pinchart
I've investigated this a bit further.
UVC defines class-specific interface descriptors that are usually located right after the standard interface descriptor in altsetting 0. The uvcvideo driver accesses those descriptor through intf->altsetting[0].extra. However, some devices insert an isochronous endpoint descriptor with wMaxPAcketSize set to 0 between the standard interface descriptor and the UVC class-specific interface descriptors.
Before your patch, these descriptors were recorded in the extra field of the endpoint, as they're located right after it:
static int usb_parse_endpoint(struct device *ddev, int cfgno, int inum,
int asnum, struct usb_host_interface *ifp, int num_ep, unsigned char *buffer, int size)
{
...
        /* Skip over any Class Specific or Vendor Specific descriptors;
         * find the next endpoint or interface descriptor */
        endpoint->extra = buffer;
        i = find_next_descriptor(buffer, size, USB_DT_ENDPOINT,
                        USB_DT_INTERFACE, &n);
        endpoint->extralen = i;
...
}
Quoted 6 lines
The uvcvideo driver looks at endpoint->extra when altsetting[0] has no extra data.
With your patch, the endpoint is skipped, and the class-specific interface descriptors are dropped with it. The uvcvideo driver can't access them and fails.
Ah, a very tricky and unexpected interaction!
Quoted 8 lines
One solution may be to add to altsetting[0].extra all the extra class-specific descriptors, regardless of whether they're located before or after endpoints. I however fear we may not always be able to identify those descriptors properly, especially with the CS_INTERFACE descriptor type being defined in class specifications, not in the USB core specification. There's also a risk of breaking working devices if we do so (the uvcvideo driver should be able to cope, but other drivers may always look for descriptors in the endpoint).

Quoted 2 lines

With the patch I posted yesterday, everything should go back to working the way it used to. Have you had a chance to test it?

I don't have any camera affected by this issue, so I can't test it I'm afraid. Roger, would you be able to give it a try ?

Signature · 3 lines

Regards,

Laurent Pinchart

Roger Whittaker

2020-01-05 12:29 UTC #19 lore raw

Quoted 7 lines · Laurent Pinchart

[...]
Quoted 2 lines
With the patch I posted yesterday, everything should go back to working the way it used to. Have you had a chance to test it?
I don't have any camera affected by this issue, so I can't test it I'm afraid. Roger, would you be able to give it a try ?

With 5.4.7-1.g8211231-default that Takashi built with the patch mentioned
(http://download.opensuse.org/repositories/home:/tiwai:/bsc1159811-fix2/standard/x86_64/)

output of lsusb -v -d 1778:0214

Bus 001 Device 013: ID 1778:0214
Device Descriptor:
bLength 18
bDescriptorType 1
bcdUSB 2.00
bDeviceClass 239 Miscellaneous Device
bDeviceSubClass 2
bDeviceProtocol 1 Interface Association
bMaxPacketSize0 64
idVendor 0x1778
idProduct 0x0214
bcdDevice 7.07

  iManufacturer           1 IPEVO Inc.
  iProduct                2 IPEVO Point 2 View
  iSerial                 0 
  bNumConfigurations      1

Configuration Descriptor:
bLength 9
bDescriptorType 2
wTotalLength 0x0299
bNumInterfaces 3
bConfigurationValue 1
iConfiguration 0
bmAttributes 0x80
(Bus Powered)
MaxPower 500mA
Interface Descriptor:
bLength 9
bDescriptorType 4
bInterfaceNumber 0
bAlternateSetting 0
bNumEndpoints 1
bInterfaceClass 3 Human Interface Device
bInterfaceSubClass 0
bInterfaceProtocol 0
iInterface 0
HID Device Descriptor:
bLength 9
bDescriptorType 33
bcdHID 1.10
bCountryCode 0 Not supported
bNumDescriptors 1
bDescriptorType 34 Report
wDescriptorLength 64
Report Descriptors:
** UNAVAILABLE **
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x85 EP 5 IN
bmAttributes 3
Transfer Type Interrupt
Synch Type None
Usage Type Data
wMaxPacketSize 0x0008 1x 8 bytes
bInterval 10
Interface Association:
bLength 8
bDescriptorType 11
bFirstInterface 1
bInterfaceCount 2
bFunctionClass 14 Video
bFunctionSubClass 3 Video Interface Collection
bFunctionProtocol 0
iFunction 2 IPEVO Point 2 View
Interface Descriptor:
bLength 9
bDescriptorType 4
bInterfaceNumber 1
bAlternateSetting 0
bNumEndpoints 1
bInterfaceClass 14 Video
bInterfaceSubClass 1 Video Control
bInterfaceProtocol 0
iInterface 2 IPEVO Point 2 View
VideoControl Interface Descriptor:
bLength 13
bDescriptorType 36
bDescriptorSubtype 1 (HEADER)
bcdUVC 1.00
wTotalLength 0x0050
dwClockFrequency 6.000000MHz
bInCollection 1
baInterfaceNr( 0) 2
VideoControl Interface Descriptor:
bLength 18
bDescriptorType 36
bDescriptorSubtype 2 (INPUT_TERMINAL)
bTerminalID 1
wTerminalType 0x0201 Camera Sensor
bAssocTerminal 0
iTerminal 0
wObjectiveFocalLengthMin 0
wObjectiveFocalLengthMax 0
wOcularFocalLength 0
bControlSize 3
bmControls 0x000200a2
Auto-Exposure Mode
Focus (Absolute)
Iris (Absolute)
Focus, Auto
VideoControl Interface Descriptor:
bLength 11
bDescriptorType 36
bDescriptorSubtype 5 (PROCESSING_UNIT)
Warning: Descriptor too short
bUnitID 3
bSourceID 1
wMaxMultiplier 0
bControlSize 2
bmControls 0x0000147f
Brightness
Contrast
Hue
Saturation
Sharpness
Gamma
White Balance Temperature
Power Line Frequency
White Balance Temperature, Auto
iProcessing 0
bmVideoStandards 0x1d
None
PAL - 625/50
SECAM - 625/50
NTSC - 625/50
VideoControl Interface Descriptor:
bLength 29
bDescriptorType 36
bDescriptorSubtype 6 (EXTENSION_UNIT)
bUnitID 4
guidExtensionCode {5a215226-3289-4156-894a-5c557cdf9664}
bNumControl 4
bNrPins 1
baSourceID( 0) 3
bControlSize 4
bmControls( 0) 0xff
bmControls( 1) 0xff
bmControls( 2) 0xff
bmControls( 3) 0xff
iExtension 0
VideoControl Interface Descriptor:
bLength 9
bDescriptorType 36
bDescriptorSubtype 3 (OUTPUT_TERMINAL)
bTerminalID 2
wTerminalType 0x0101 USB Streaming
bAssocTerminal 0
bSourceID 4
iTerminal 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x81 EP 1 IN
bmAttributes 3
Transfer Type Interrupt
Synch Type None
Usage Type Data
wMaxPacketSize 0x0008 1x 8 bytes
bInterval 9
Interface Descriptor:
bLength 9
bDescriptorType 4
bInterfaceNumber 2
bAlternateSetting 0
bNumEndpoints 1
bInterfaceClass 14 Video
bInterfaceSubClass 2 Video Streaming
bInterfaceProtocol 0
iInterface 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x82 EP 2 IN
bmAttributes 5
Transfer Type Isochronous
Synch Type Asynchronous
Usage Type Data
wMaxPacketSize 0x0000 1x 0 bytes
bInterval 1
INTERFACE CLASS: 0f 24 01 02 ea 01 82 00 02 02 01 00 01 00 00
INTERFACE CLASS: 1b 24 04 01 07 59 55 59 32 00 00 10 00 80 00 00 aa 00 38 9b 71 10 01 00 00 00 00 INTERFACE CLASS: 1e 24 05 01 00 80 02 e0 01 00 00 0d 2f 00 00 0d 2f 00 60 09 00 15 16 05 00 01 15 16 05 00 INTERFACE CLASS: 1e 24 05 02 00 40 01 f0 00 c0 00 03 4b c0 00 03 4b 00 58 02 00 15 16 05 00 01 15 16 05 00 INTERFACE CLASS: 1e 24 05 03 00 20 03 58 02 70 00 14 99 70 00 14 99 00 a6 0e 00 9a 5b 06 00 01 9a 5b 06 00 INTERFACE CLASS: 1e 24 05 04 00 00 04 00 03 00 00 16 80 00 00 16 80 00 00 18 00 2a 2c 0a 00 01 2a 2c 0a 00 INTERFACE CLASS: 1e 24 05 05 00 00 05 00 04 00 00 12 c0 00 00 12 c0 00 00 28 00 d0 12 13 00 01 d0 12 13 00 INTERFACE CLASS: 1e 24 05 06 00 40 06 b0 04 00 00 0e a6 00 00 0e a6 00 98 3a 00 a0 25 26 00 01 a0 25 26 00 INTERFACE CLASS: 1e 24 05 01 00 80 02 e0 01 00 00 0d 2f 00 00 0d 2f 00 60 09 00 15 16 05 00 01 15 16 05 00 INTERFACE CLASS: 0b 24 03 00 01 80 02 e0 01 01 00
INTERFACE CLASS: 0b 24 06 02 07 00 01 00 00 00 00
INTERFACE CLASS: 1e 24 07 01 00 80 02 e0 01 00 00 0d 2f 00 00 0d 2f 00 60 09 00 0e 64 03 00 01 0e 64 03 00 INTERFACE CLASS: 1e 24 07 02 00 40 01 f0 00 c0 00 03 4b c0 00 03 4b 00 58 02 00 0e 64 03 00 01 0e 64 03 00 INTERFACE CLASS: 1e 24 07 03 00 20 03 58 02 70 00 14 99 70 00 14 99 00 a6 0e 00 0e 64 03 00 01 0e 64 03 00 INTERFACE CLASS: 1e 24 07 04 00 00 04 00 03 00 00 16 80 00 00 16 80 00 00 18 00 15 16 05 00 01 15 16 05 00 INTERFACE CLASS: 1e 24 07 05 00 00 05 00 04 00 00 12 c0 00 00 12 c0 00 00 28 00 2a 2c 0a 00 01 2a 2c 0a 00 INTERFACE CLASS: 1e 24 07 06 00 40 06 b0 04 00 00 0e a6 00 00 0e a6 00 98 3a 00 d0 12 13 00 01 d0 12 13 00 INTERFACE CLASS: 1e 24 07 01 00 80 02 e0 01 00 00 0d 2f 00 00 0d 2f 00 60 09 00 0e 64 03 00 01 0e 64 03 00 INTERFACE CLASS: 06 24 0d 00 00 00
Interface Descriptor:
bLength 9
bDescriptorType 4
bInterfaceNumber 2
bAlternateSetting 1
bNumEndpoints 1
bInterfaceClass 14 Video
bInterfaceSubClass 2 Video Streaming
bInterfaceProtocol 0
iInterface 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x82 EP 2 IN
bmAttributes 5
Transfer Type Isochronous
Synch Type Asynchronous
Usage Type Data
wMaxPacketSize 0x13fc 3x 1020 bytes
bInterval 1
Device Qualifier (for other device speed):
bLength 10
bDescriptorType 6
bcdUSB 2.00
bDeviceClass 239 Miscellaneous Device
bDeviceSubClass 2
bDeviceProtocol 1 Interface Association
bMaxPacketSize0 64
bNumConfigurations 1
Device Status: 0x0000
(Bus Powered)

dmesg lines on plugging:

[   95.016139] usb 1-1.4.3.3: new high-speed USB device number 13 using xhci_hcd
[   95.130236] usb 1-1.4.3.3: New USB device found, idVendor=1778, idProduct=0214, bcdDevice= 7.07
[   95.130241] usb 1-1.4.3.3: New USB device strings: Mfr=1, Product=2, SerialNumber=0
[   95.130244] usb 1-1.4.3.3: Product: IPEVO Point 2 View
[   95.130246] usb 1-1.4.3.3: Manufacturer: IPEVO Inc.
[   95.133103] hid-generic 0003:1778:0214.000A: hiddev97,hidraw6: USB HID v1.10 Device [IPEVO Inc. IPEVO Point 2 View] on usb-0000:00:14.0-1.4.3.3/input0
[   95.133500] uvcvideo: Probing generic UVC device 1.4.3.3
[   95.133618] uvcvideo: trying extra data from endpoint 0.
[   95.133623] uvcvideo: Found format YUV 4:2:2 (YUYV).
[   95.133626] uvcvideo: - 640x480 (30.0 fps)
[   95.133629] uvcvideo: - 320x240 (30.0 fps)
[   95.133631] uvcvideo: - 800x600 (24.0 fps)
[   95.133633] uvcvideo: - 1024x768 (15.0 fps)
[   95.133635] uvcvideo: - 1280x1024 (8.0 fps)
[   95.133636] uvcvideo: - 1600x1200 (4.0 fps)
[   95.133638] uvcvideo: - 640x480 (30.0 fps)
[   95.133639] uvcvideo: Found format MJPEG.
[   95.133641] uvcvideo: - 640x480 (45.0 fps)
[   95.133642] uvcvideo: - 320x240 (45.0 fps)
[   95.133644] uvcvideo: - 800x600 (45.0 fps)
[   95.133645] uvcvideo: - 1024x768 (30.0 fps)
[   95.133647] uvcvideo: - 1280x1024 (15.0 fps)
[   95.133648] uvcvideo: - 1600x1200 (8.0 fps)
[   95.133649] uvcvideo: - 640x480 (45.0 fps)
[   95.133656] uvcvideo: Found a Status endpoint (addr 81).
[   95.133658] uvcvideo: Found UVC 1.00 device IPEVO Point 2 View (1778:0214)
[   95.133698] uvcvideo: Failed to query (GET_INFO) UVC control 2 on unit 1: -32 (exp. 1).
[   95.133763] uvcvideo: Control error 6
[   95.133766] uvcvideo: Added control 00000000-0000-0000-0000-000000000001/2 to device 1.4.3.3 entity 1
[   95.133769] uvcvideo: Adding mapping 'Exposure, Auto' to control 00000000-0000-0000-0000-000000000001/2.
[   95.133815] uvcvideo: Added control 00000000-0000-0000-0000-000000000001/6 to device 1.4.3.3 entity 1
[   95.133818] uvcvideo: Adding mapping 'Focus (absolute)' to control 00000000-0000-0000-0000-000000000001/6.
[   95.133860] uvcvideo: Failed to query (GET_INFO) UVC control 9 on unit 1: -32 (exp. 1).
[   95.133934] uvcvideo: Control error 6
[   95.133936] uvcvideo: Added control 00000000-0000-0000-0000-000000000001/9 to device 1.4.3.3 entity 1
[   95.133939] uvcvideo: Adding mapping 'Iris, Absolute' to control 00000000-0000-0000-0000-000000000001/9.
[   95.133984] uvcvideo: Added control 00000000-0000-0000-0000-000000000001/8 to device 1.4.3.3 entity 1
[   95.133986] uvcvideo: Adding mapping 'Focus, Auto' to control 00000000-0000-0000-0000-000000000001/8.
[   95.134031] uvcvideo: Added control 00000000-0000-0000-0000-000000000101/2 to device 1.4.3.3 entity 3
[   95.134033] uvcvideo: Adding mapping 'Brightness' to control 00000000-0000-0000-0000-000000000101/2.
[   95.134078] uvcvideo: Added control 00000000-0000-0000-0000-000000000101/3 to device 1.4.3.3 entity 3
[   95.134080] uvcvideo: Adding mapping 'Contrast' to control 00000000-0000-0000-0000-000000000101/3.
[   95.134130] uvcvideo: Added control 00000000-0000-0000-0000-000000000101/6 to device 1.4.3.3 entity 3
[   95.134132] uvcvideo: Adding mapping 'Hue' to control 00000000-0000-0000-0000-000000000101/6.
[   95.134178] uvcvideo: Added control 00000000-0000-0000-0000-000000000101/7 to device 1.4.3.3 entity 3
[   95.134180] uvcvideo: Adding mapping 'Saturation' to control 00000000-0000-0000-0000-000000000101/7.
[   95.134225] uvcvideo: Added control 00000000-0000-0000-0000-000000000101/8 to device 1.4.3.3 entity 3
[   95.134227] uvcvideo: Adding mapping 'Sharpness' to control 00000000-0000-0000-0000-000000000101/8.
[   95.134266] uvcvideo: Added control 00000000-0000-0000-0000-000000000101/9 to device 1.4.3.3 entity 3
[   95.134269] uvcvideo: Adding mapping 'Gamma' to control 00000000-0000-0000-0000-000000000101/9.
[   95.134313] uvcvideo: Added control 00000000-0000-0000-0000-000000000101/10 to device 1.4.3.3 entity 3
[   95.134316] uvcvideo: Adding mapping 'White Balance Temperature' to control 00000000-0000-0000-0000-000000000101/10.
[   95.134361] uvcvideo: Added control 00000000-0000-0000-0000-000000000101/5 to device 1.4.3.3 entity 3
[   95.134364] uvcvideo: Adding mapping 'Power Line Frequency' to control 00000000-0000-0000-0000-000000000101/5.
[   95.134409] uvcvideo: Added control 00000000-0000-0000-0000-000000000101/11 to device 1.4.3.3 entity 3
[   95.134411] uvcvideo: Adding mapping 'White Balance Temperature, Auto' to control 00000000-0000-0000-0000-000000000101/11.
[   95.134415] uvcvideo: Scanning UVC chain: OT 2 <- XU 4 <- PU 3 <- IT 1
[   95.134422] uvcvideo: Found a valid video chain (1 -> 2).
[   95.135278] uvcvideo 1-1.4.3.3:1.1: Entity type for entity Extension 4 was not initialized!
[   95.135284] uvcvideo 1-1.4.3.3:1.1: Entity type for entity Processing 3 was not initialized!
[   95.135289] uvcvideo 1-1.4.3.3:1.1: Entity type for entity Camera 1 was not initialized!
[   95.135468] input: IPEVO Point 2 View: IPEVO Point as /devices/pci0000:00/0000:00:14.0/usb1/1-1/1-1.4/1-1.4.3/1-1.4.3.3/1-1.4.3.3:1.1/input/input22
[   95.135632] uvcvideo: UVC device initialized.
[   95.173185] uvcvideo: uvc_v4l2_open
[   95.173224] uvcvideo: uvc_v4l2_release
[   97.532205] uvcvideo: Suspending interface 2
[   97.532210] uvcvideo: Suspending interface 1

# ls -l /dev/video*
crw-rw----+ 1 root video 81, 0 Jan  5 12:20 /dev/video0
crw-rw----+ 1 root video 81, 1 Jan  5 12:20 /dev/video1

Camera works.

Signature · 13 lines

============================================
Roger Whittaker
SUSE Linux Premium Support Engineer
 
[email protected]
+44 7802 357081
 
SUSE Linux
One Station Square
Bracknell
RG12 1QB
United Kingdom
============================================

Alan Stern

2020-01-06 15:43 UTC #20 lore raw

PATCH 1/1 USB: Fix: Don't skip endpoint descriptors with maxpacket=0

It turns out that even though endpoints with a maxpacket length of 0 aren't useful for data transfer, the descriptors do serve other purposes. In particular, skipping them will also skip over other class-specific descriptors for classes such as UVC. This unexpected side effect has caused some UVC cameras to stop working.

In addition, the USB spec requires that when isochronous endpoint descriptors are present in an interface's altsetting 0 (which is true on some devices), the maxpacket size _must_ be set to 0. Warning about such things seems like a bad idea.

This patch updates an earlier commit which would log a warning and skip these endpoint descriptors. Now we only log a warning, and we don't even do that for isochronous endpoints in altsetting 0.

We don't need to worry about preventing endpoints with maxpacket = 0 from ever being used for data transfers; usb_submit_urb() already checks for this.

Reported-and-tested-by Roger Whittaker <[email protected]>

Fixes d482c7bb0541 ("USB: Skip endpoints with 0 maxpacket length")

Signed-off-by Alan Stern <[email protected]>

CC Laurent Pinchart <[email protected]>

Link https://marc.info/?l=linux-usb&m=157790377329882&w=2

---

[as1928]

 drivers/usb/core/config.c |   12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

drivers/usb/core/config.c +8 -4

Index: usb-devel/drivers/usb/core/config.c
===================================================================
@@ -346,12 +346,16 @@ static int usb_parse_endpoint(struct dev
 			endpoint->desc.wMaxPacketSize = cpu_to_le16(8);
 	}
 
-	/* Validate the wMaxPacketSize field */
+	/*
+	 * Validate the wMaxPacketSize field.
+	 * Some devices have isochronous endpoints in altsetting 0;
+	 * the USB-2 spec requires such endpoints to have wMaxPacketSize = 0
+	 * (see the end of section 5.6.3), so don't warn about them.
+	 */
 	maxp = usb_endpoint_maxp(&endpoint->desc);
-	if (maxp == 0) {
-		dev_warn(ddev, "config %d interface %d altsetting %d endpoint 0x%X has wMaxPacketSize 0, skipping\n",
+	if (maxp == 0 && !(usb_endpoint_xfer_isoc(d) && asnum == 0)) {
+		dev_warn(ddev, "config %d interface %d altsetting %d endpoint 0x%X has invalid wMaxPacketSize 0\n",
 		    cfgno, inum, asnum, d->bEndpointAddress);
-		goto skip_to_next_endpoint_or_interface_descriptor;
 	}
 
 	/* Find the highest legal maxpacket size for this endpoint */

Johan Hovold

Acked-by on v1 1/1

2020-01-06 16:03 UTC #21 lore raw

Re: [PATCH] USB: Fix: Don't skip endpoint descriptors with maxpacket=0

Quoted 19 lines · Alan Stern

It turns out that even though endpoints with a maxpacket length of 0 aren't useful for data transfer, the descriptors do serve other purposes. In particular, skipping them will also skip over other class-specific descriptors for classes such as UVC. This unexpected side effect has caused some UVC cameras to stop working.
In addition, the USB spec requires that when isochronous endpoint descriptors are present in an interface's altsetting 0 (which is true on some devices), the maxpacket size _must_ be set to 0. Warning about such things seems like a bad idea.
This patch updates an earlier commit which would log a warning and skip these endpoint descriptors. Now we only log a warning, and we don't even do that for isochronous endpoints in altsetting 0.
We don't need to worry about preventing endpoints with maxpacket = 0 from ever being used for data transfers; usb_submit_urb() already checks for this.

Quoted 5 lines

Reported-and-tested-by Roger Whittaker <[email protected]>
Fixes d482c7bb0541 ("USB: Skip endpoints with 0 maxpacket length")
Signed-off-by Alan Stern <[email protected]>
CC Laurent Pinchart <[email protected]>
Link https://marc.info/?l=linux-usb&m=157790377329882&w=2

Acked-by Johan Hovold <[email protected]>

We also need

Cc stable <[email protected]>

as d482c7bb0541 ("USB: Skip endpoints with 0 maxpacket length") ended up being (auto- ?) selected for stable.

Johan

Alan Stern

2020-01-06 16:17 UTC #22 lore raw

Re: [PATCH] USB: Fix: Don't skip endpoint descriptors with maxpacket=0

Quoted 26 lines · Johan Hovold

Quoted 19 lines · Alan Stern
It turns out that even though endpoints with a maxpacket length of 0 aren't useful for data transfer, the descriptors do serve other purposes. In particular, skipping them will also skip over other class-specific descriptors for classes such as UVC. This unexpected side effect has caused some UVC cameras to stop working.
In addition, the USB spec requires that when isochronous endpoint descriptors are present in an interface's altsetting 0 (which is true on some devices), the maxpacket size _must_ be set to 0. Warning about such things seems like a bad idea.
This patch updates an earlier commit which would log a warning and skip these endpoint descriptors. Now we only log a warning, and we don't even do that for isochronous endpoints in altsetting 0.
We don't need to worry about preventing endpoints with maxpacket = 0 from ever being used for data transfers; usb_submit_urb() already checks for this.
Quoted 5 lines
Reported-and-tested-by Roger Whittaker <[email protected]>
Fixes d482c7bb0541 ("USB: Skip endpoints with 0 maxpacket length")
Signed-off-by Alan Stern <[email protected]>
CC Laurent Pinchart <[email protected]>
Link https://marc.info/?l=linux-usb&m=157790377329882&w=2

Quoted 8 lines

Acked-by Johan Hovold <[email protected]>
We also need
Cc stable <[email protected]>
as d482c7bb0541 ("USB: Skip endpoints with 0 maxpacket length") ended up being (auto- ?) selected for stable.

Absolutely -- I had intended to add that CC: but it slipped my mind when the email was being prepared.

Alan Stern

Greg KH

2020-01-06 19:13 UTC #23 lore raw

Re: [PATCH] USB: Fix: Don't skip endpoint descriptors with maxpacket=0

Quoted 32 lines · Alan Stern

Quoted 30 lines · Johan Hovold
Quoted 19 lines · Alan Stern
It turns out that even though endpoints with a maxpacket length of 0 aren't useful for data transfer, the descriptors do serve other purposes. In particular, skipping them will also skip over other class-specific descriptors for classes such as UVC. This unexpected side effect has caused some UVC cameras to stop working.
In addition, the USB spec requires that when isochronous endpoint descriptors are present in an interface's altsetting 0 (which is true on some devices), the maxpacket size _must_ be set to 0. Warning about such things seems like a bad idea.
This patch updates an earlier commit which would log a warning and skip these endpoint descriptors. Now we only log a warning, and we don't even do that for isochronous endpoints in altsetting 0.
We don't need to worry about preventing endpoints with maxpacket = 0 from ever being used for data transfers; usb_submit_urb() already checks for this.
Quoted 5 lines
Reported-and-tested-by Roger Whittaker <[email protected]>
Fixes d482c7bb0541 ("USB: Skip endpoints with 0 maxpacket length")
Signed-off-by Alan Stern <[email protected]>
CC Laurent Pinchart <[email protected]>
Link https://marc.info/?l=linux-usb&m=157790377329882&w=2
Acked-by Johan Hovold <[email protected]>
We also need

Quoted 7 lines

Quoted 4 lines
Cc stable <[email protected]>
as d482c7bb0541 ("USB: Skip endpoints with 0 maxpacket length") ended up being (auto- ?) selected for stable.
Absolutely -- I had intended to add that CC: but it slipped my mind when the email was being prepared.

I'll catch this when it hits Linus's tree.

thanks,

greg k-h

Laurent Pinchart

author Reviewed-by on v1 1/1

2020-01-06 16:13 UTC #24 lore raw

Re: [PATCH] USB: Fix: Don't skip endpoint descriptors with maxpacket=0

In reply to Alan Stern

Hi Alan,

Thank you for the patch.

Quoted 19 lines · Alan Stern

It turns out that even though endpoints with a maxpacket length of 0 aren't useful for data transfer, the descriptors do serve other purposes. In particular, skipping them will also skip over other class-specific descriptors for classes such as UVC. This unexpected side effect has caused some UVC cameras to stop working.
In addition, the USB spec requires that when isochronous endpoint descriptors are present in an interface's altsetting 0 (which is true on some devices), the maxpacket size _must_ be set to 0. Warning about such things seems like a bad idea.
This patch updates an earlier commit which would log a warning and skip these endpoint descriptors. Now we only log a warning, and we don't even do that for isochronous endpoints in altsetting 0.
We don't need to worry about preventing endpoints with maxpacket = 0 from ever being used for data transfers; usb_submit_urb() already checks for this.

Quoted 5 lines

Reported-and-tested-by Roger Whittaker <[email protected]>
Fixes d482c7bb0541 ("USB: Skip endpoints with 0 maxpacket length")
Signed-off-by Alan Stern <[email protected]>
CC Laurent Pinchart <[email protected]>
Link https://marc.info/?l=linux-usb&m=157790377329882&w=2

The patch looks good to me, so

Reviewed-by Laurent Pinchart <[email protected]>

But shouldn't we also warn if maxp != 0 && usb_endpoint_xfer_isoc(d) && asnum == 0 ?

Quoted 34 lines

---

[as1928]

 drivers/usb/core/config.c |   12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

Index: usb-devel/drivers/usb/core/config.c
===================================================================

drivers/usb/core/config.c +8 -4

@@ -346,12 +346,16 @@ static int usb_parse_endpoint(struct dev
 			endpoint->desc.wMaxPacketSize = cpu_to_le16(8);
 	}
 
-	/* Validate the wMaxPacketSize field */
+	/*
+	 * Validate the wMaxPacketSize field.
+	 * Some devices have isochronous endpoints in altsetting 0;
+	 * the USB-2 spec requires such endpoints to have wMaxPacketSize = 0
+	 * (see the end of section 5.6.3), so don't warn about them.
+	 */
 	maxp = usb_endpoint_maxp(&endpoint->desc);
-	if (maxp == 0) {
-		dev_warn(ddev, "config %d interface %d altsetting %d endpoint 0x%X has wMaxPacketSize 0, skipping\n",
+	if (maxp == 0 && !(usb_endpoint_xfer_isoc(d) && asnum == 0)) {
+		dev_warn(ddev, "config %d interface %d altsetting %d endpoint 0x%X has invalid wMaxPacketSize 0\n",
 		    cfgno, inum, asnum, d->bEndpointAddress);
-		goto skip_to_next_endpoint_or_interface_descriptor;
 	}
 
 	/* Find the highest legal maxpacket size for this endpoint */

Signature · 3 lines

Regards,

Laurent Pinchart

Alan Stern

2020-01-06 16:21 UTC #25 lore raw

Re: [PATCH] USB: Fix: Don't skip endpoint descriptors with maxpacket=0

Quoted 30 lines · Laurent Pinchart

Hi Alan,
Thank you for the patch.
Quoted 19 lines · Alan Stern
It turns out that even though endpoints with a maxpacket length of 0 aren't useful for data transfer, the descriptors do serve other purposes. In particular, skipping them will also skip over other class-specific descriptors for classes such as UVC. This unexpected side effect has caused some UVC cameras to stop working.
In addition, the USB spec requires that when isochronous endpoint descriptors are present in an interface's altsetting 0 (which is true on some devices), the maxpacket size _must_ be set to 0. Warning about such things seems like a bad idea.
This patch updates an earlier commit which would log a warning and skip these endpoint descriptors. Now we only log a warning, and we don't even do that for isochronous endpoints in altsetting 0.
We don't need to worry about preventing endpoints with maxpacket = 0 from ever being used for data transfers; usb_submit_urb() already checks for this.
Quoted 5 lines
Reported-and-tested-by Roger Whittaker <[email protected]>
Fixes d482c7bb0541 ("USB: Skip endpoints with 0 maxpacket length")
Signed-off-by Alan Stern <[email protected]>
CC Laurent Pinchart <[email protected]>
Link https://marc.info/?l=linux-usb&m=157790377329882&w=2

Quoted 6 lines

The patch looks good to me, so
Reviewed-by Laurent Pinchart <[email protected]>
But shouldn't we also warn if maxp != 0 && usb_endpoint_xfer_isoc(d) && asnum == 0 ?

We could, but that's a different kind of issue. That would be more a question of not adhering to the standard than of possibly failing to work.

In theory the USBCV tests already check for this. Manufacturers that don't run those tests probably also won't care if the Linux kernel complains. But as far as I know, none of our drivers will malfunction if there's an isochronous endpoint in altsetting 0 with maxpacket > 0.

Alan Stern