| Message ID | 20221025-usb-string-v1-1-4c351b6907bb@chromium.org |
|---|---|
| State | New |
| Headers | show |
| Series | media: uvcvideo: Handle cameras with invalid usb_strings | expand |
Hi Ricardo and Guenter, Thank you for the patch. On Tue, Oct 25, 2022 at 04:41:01PM +0200, Ricardo Ribalda wrote: > From: Guenter Roeck <linux@roeck-us.net> > > On a Webcam from Quanta, we see the following error. > > usb 3-5: New USB device found, idVendor=0408, idProduct=30d2, bcdDevice= 0.03 > usb 3-5: New USB device strings: Mfr=3, Product=1, SerialNumber=2 > usb 3-5: Product: USB2.0 HD UVC WebCam > usb 3-5: Manufacturer: Quanta > usb 3-5: SerialNumber: 0x0001 > ... > uvcvideo: Found UVC 1.10 device USB2.0 HD UVC WebCam (0408:30d2) > uvcvideo: Failed to initialize entity for entity 5 > uvcvideo: Failed to register entities (-22). > > The Webcam reports an entity of type UVC_VC_EXTENSION_UNIT. It reports a > string index of '7' associated with that entity. The attempt to read that > string from the camera fails with error -32 (-EPIPE). usb_string() returns > that error, but it is ignored. As result, the entity name is empty. This > later causes v4l2_device_register_subdev() to return -EINVAL, and no > entities are registered as result. > > While this appears to be a firmware problem with the camera, the kernel > should still handle the situation gracefully. To do that, check the return > value from usb_string(). If it reports an error, assign the entity's > default name. Sounds good. The perfect world in which firmwares could be fixed doesn't exist. > Signed-off-by: Guenter Roeck <linux@roeck-us.net> > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c > index 215fb483efb0..828c443faaa9 100644 > --- a/drivers/media/usb/uvc/uvc_driver.c > +++ b/drivers/media/usb/uvc/uvc_driver.c > @@ -879,10 +879,8 @@ static int uvc_parse_vendor_control(struct uvc_device *dev, > + n; > memcpy(unit->extension.bmControls, &buffer[23+p], 2*n); > > - if (buffer[24+p+2*n] != 0) > - usb_string(udev, buffer[24+p+2*n], unit->name, > - sizeof(unit->name)); > - else > + if (buffer[24+p+2*n] == 0 || > + usb_string(udev, buffer[24+p+2*n], unit->name, sizeof(unit->name)) < 0) > sprintf(unit->name, "Extension %u", buffer[3]); There's quite a bit of common code between all the cases. I'll submit a patch that refactors it on top. There's thus no need for nitpicking here, so Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > list_add_tail(&unit->list, &dev->entities); > @@ -1006,15 +1004,15 @@ static int uvc_parse_standard_control(struct uvc_device *dev, > memcpy(term->media.bmTransportModes, &buffer[10+n], p); > } > > - if (buffer[7] != 0) > - usb_string(udev, buffer[7], term->name, > - sizeof(term->name)); > - else if (UVC_ENTITY_TYPE(term) == UVC_ITT_CAMERA) > - sprintf(term->name, "Camera %u", buffer[3]); > - else if (UVC_ENTITY_TYPE(term) == UVC_ITT_MEDIA_TRANSPORT_INPUT) > - sprintf(term->name, "Media %u", buffer[3]); > - else > - sprintf(term->name, "Input %u", buffer[3]); > + if (buffer[7] == 0 || > + usb_string(udev, buffer[7], term->name, sizeof(term->name)) < 0) { > + if (UVC_ENTITY_TYPE(term) == UVC_ITT_CAMERA) > + sprintf(term->name, "Camera %u", buffer[3]); > + if (UVC_ENTITY_TYPE(term) == UVC_ITT_MEDIA_TRANSPORT_INPUT) > + sprintf(term->name, "Media %u", buffer[3]); > + else > + sprintf(term->name, "Input %u", buffer[3]); > + } > > list_add_tail(&term->list, &dev->entities); > break; > @@ -1047,10 +1045,8 @@ static int uvc_parse_standard_control(struct uvc_device *dev, > > memcpy(term->baSourceID, &buffer[7], 1); > > - if (buffer[8] != 0) > - usb_string(udev, buffer[8], term->name, > - sizeof(term->name)); > - else > + if (buffer[8] == 0 || > + usb_string(udev, buffer[8], term->name, sizeof(term->name)) < 0) > sprintf(term->name, "Output %u", buffer[3]); > > list_add_tail(&term->list, &dev->entities); > @@ -1072,10 +1068,8 @@ static int uvc_parse_standard_control(struct uvc_device *dev, > > memcpy(unit->baSourceID, &buffer[5], p); > > - if (buffer[5+p] != 0) > - usb_string(udev, buffer[5+p], unit->name, > - sizeof(unit->name)); > - else > + if (buffer[5+p] == 0 || > + usb_string(udev, buffer[5+p], unit->name, sizeof(unit->name)) < 0) > sprintf(unit->name, "Selector %u", buffer[3]); > > list_add_tail(&unit->list, &dev->entities); > @@ -1105,10 +1099,8 @@ static int uvc_parse_standard_control(struct uvc_device *dev, > if (dev->uvc_version >= 0x0110) > unit->processing.bmVideoStandards = buffer[9+n]; > > - if (buffer[8+n] != 0) > - usb_string(udev, buffer[8+n], unit->name, > - sizeof(unit->name)); > - else > + if (buffer[8+n] == 0 || > + usb_string(udev, buffer[8+n], unit->name, sizeof(unit->name)) < 0) > sprintf(unit->name, "Processing %u", buffer[3]); > > list_add_tail(&unit->list, &dev->entities); > @@ -1136,10 +1128,8 @@ static int uvc_parse_standard_control(struct uvc_device *dev, > unit->extension.bmControls = (u8 *)unit + sizeof(*unit); > memcpy(unit->extension.bmControls, &buffer[23+p], n); > > - if (buffer[23+p+n] != 0) > - usb_string(udev, buffer[23+p+n], unit->name, > - sizeof(unit->name)); > - else > + if (buffer[23+p+n] == 0 || > + usb_string(udev, buffer[23+p+n], unit->name, sizeof(unit->name)) < 0) > sprintf(unit->name, "Extension %u", buffer[3]); > > list_add_tail(&unit->list, &dev->entities); >
diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c index 215fb483efb0..828c443faaa9 100644 --- a/drivers/media/usb/uvc/uvc_driver.c +++ b/drivers/media/usb/uvc/uvc_driver.c @@ -879,10 +879,8 @@ static int uvc_parse_vendor_control(struct uvc_device *dev, + n; memcpy(unit->extension.bmControls, &buffer[23+p], 2*n); - if (buffer[24+p+2*n] != 0) - usb_string(udev, buffer[24+p+2*n], unit->name, - sizeof(unit->name)); - else + if (buffer[24+p+2*n] == 0 || + usb_string(udev, buffer[24+p+2*n], unit->name, sizeof(unit->name)) < 0) sprintf(unit->name, "Extension %u", buffer[3]); list_add_tail(&unit->list, &dev->entities); @@ -1006,15 +1004,15 @@ static int uvc_parse_standard_control(struct uvc_device *dev, memcpy(term->media.bmTransportModes, &buffer[10+n], p); } - if (buffer[7] != 0) - usb_string(udev, buffer[7], term->name, - sizeof(term->name)); - else if (UVC_ENTITY_TYPE(term) == UVC_ITT_CAMERA) - sprintf(term->name, "Camera %u", buffer[3]); - else if (UVC_ENTITY_TYPE(term) == UVC_ITT_MEDIA_TRANSPORT_INPUT) - sprintf(term->name, "Media %u", buffer[3]); - else - sprintf(term->name, "Input %u", buffer[3]); + if (buffer[7] == 0 || + usb_string(udev, buffer[7], term->name, sizeof(term->name)) < 0) { + if (UVC_ENTITY_TYPE(term) == UVC_ITT_CAMERA) + sprintf(term->name, "Camera %u", buffer[3]); + if (UVC_ENTITY_TYPE(term) == UVC_ITT_MEDIA_TRANSPORT_INPUT) + sprintf(term->name, "Media %u", buffer[3]); + else + sprintf(term->name, "Input %u", buffer[3]); + } list_add_tail(&term->list, &dev->entities); break; @@ -1047,10 +1045,8 @@ static int uvc_parse_standard_control(struct uvc_device *dev, memcpy(term->baSourceID, &buffer[7], 1); - if (buffer[8] != 0) - usb_string(udev, buffer[8], term->name, - sizeof(term->name)); - else + if (buffer[8] == 0 || + usb_string(udev, buffer[8], term->name, sizeof(term->name)) < 0) sprintf(term->name, "Output %u", buffer[3]); list_add_tail(&term->list, &dev->entities); @@ -1072,10 +1068,8 @@ static int uvc_parse_standard_control(struct uvc_device *dev, memcpy(unit->baSourceID, &buffer[5], p); - if (buffer[5+p] != 0) - usb_string(udev, buffer[5+p], unit->name, - sizeof(unit->name)); - else + if (buffer[5+p] == 0 || + usb_string(udev, buffer[5+p], unit->name, sizeof(unit->name)) < 0) sprintf(unit->name, "Selector %u", buffer[3]); list_add_tail(&unit->list, &dev->entities); @@ -1105,10 +1099,8 @@ static int uvc_parse_standard_control(struct uvc_device *dev, if (dev->uvc_version >= 0x0110) unit->processing.bmVideoStandards = buffer[9+n]; - if (buffer[8+n] != 0) - usb_string(udev, buffer[8+n], unit->name, - sizeof(unit->name)); - else + if (buffer[8+n] == 0 || + usb_string(udev, buffer[8+n], unit->name, sizeof(unit->name)) < 0) sprintf(unit->name, "Processing %u", buffer[3]); list_add_tail(&unit->list, &dev->entities); @@ -1136,10 +1128,8 @@ static int uvc_parse_standard_control(struct uvc_device *dev, unit->extension.bmControls = (u8 *)unit + sizeof(*unit); memcpy(unit->extension.bmControls, &buffer[23+p], n); - if (buffer[23+p+n] != 0) - usb_string(udev, buffer[23+p+n], unit->name, - sizeof(unit->name)); - else + if (buffer[23+p+n] == 0 || + usb_string(udev, buffer[23+p+n], unit->name, sizeof(unit->name)) < 0) sprintf(unit->name, "Extension %u", buffer[3]); list_add_tail(&unit->list, &dev->entities);