On Thu, 2 Jan 2020, Takashi Iwai wrote: > On Thu, 02 Jan 2020 12:20:45 +0100, > Johan Hovold wrote: > > > > On Wed, Jan 01, 2020 at 03:09:35PM -0500, Alan Stern wrote: > > > On Wed, 1 Jan 2020, Greg KH wrote: > > > > On Wed, Jan 01, 2020 at 08:35:59PM +0200, Laurent Pinchart wrote: > > > > > > > [ 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 <stern@xxxxxxxxxxxxxxxxxxx> > > > > > 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 <stern@xxxxxxxxxxxxxxxxxxx> > > > > > > > > > > Link: https://lore.kernel.org/r/Pine.LNX.4.44L0.1910281050420.1485-100000@xxxxxxxxxxxxxxxxxxxx > > > > > Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> > > > > > > > > > > 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? > > > > > > 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.) > > > > 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. > > 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
