Hi Laurent
On Mon, 1 May 2023 at 14:01, Laurent Pinchart
<laurent.pinchart@xxxxxxxxxxxxxxxx> wrote:
>
> Hi Ricardo,
>
> On Mon, May 01, 2023 at 01:44:11PM +0200, Ricardo Ribalda wrote:
> > Hi Laurent
> >
> > I agree with the intent of the patch but am not sure that this will work.
> >
> > Regards!
> >
> > On Thu, 20 Apr 2023 at 12:07, Laurent Pinchart wrote:
> > >
> > > When the uvcvideo driver encounters a format descriptor with an unknown
> > > format GUID, it creates a corresponding struct uvc_format instance with
> > > the fcc field set to 0. Since commit 50459f103edf ("media: uvcvideo:
> > > Remove format descriptions"), the driver relies on the V4L2 core to
> > > provide the format description string, which the V4L2 core can't do
> > > without a valid 4CC. This triggers a WARN_ON.
> > >
> > > As a format with a zero 4CC can't be selected, it is unusable for
> > > applications. Ignore the format completely without creating a uvc_format
> > > instance, which fixes the warning.
> > >
> > > Fixes: 50459f103edf ("media: uvcvideo: Remove format descriptions")
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> > > ---
> > > drivers/media/usb/uvc/uvc_driver.c | 13 +++++++++----
> > > 1 file changed, 9 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > > index de64c9d789fd..25b8199f4e82 100644
> > > --- a/drivers/media/usb/uvc/uvc_driver.c
> > > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > > @@ -251,14 +251,17 @@ static int uvc_parse_format(struct uvc_device *dev,
> > > /* Find the format descriptor from its GUID. */
> > > fmtdesc = uvc_format_by_guid(&buffer[5]);
> > >
> > > - if (fmtdesc != NULL) {
> > > - format->fcc = fmtdesc->fcc;
> > > - } else {
> > > + if (!fmtdesc) {
> > > + /*
> > > + * Unknown video formats are not fatal errors, the
> > > + * caller will skip this descriptor.
> > > + */
> > > dev_info(&streaming->intf->dev,
> > > "Unknown video format %pUl\n", &buffer[5]);
> > > - format->fcc = 0;
> > > + return 0;
> > > }
> > >
> > > + format->fcc = fmtdesc->fcc;
> > > format->bpp = buffer[21];
> > >
> > > /*
> > > @@ -689,6 +692,8 @@ static int uvc_parse_streaming(struct uvc_device *dev,
> > > &interval, buffer, buflen);
> >
> > For devices with unknown formats streaming->nformats will not be
> > valid, it will be higher than the actual formats on
> > streaming->formats.
>
> Indeed. I think this could be handled quite simply:
We will still be overallocating. I guess it is not a big deal...
But what about a helper function. I think it will be less hacky
diff --git a/drivers/media/usb/uvc/uvc_driver.c
b/drivers/media/usb/uvc/uvc_driver.c
index 7aefa76a42b3..b49c2b911d03 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -483,6 +483,33 @@ static int uvc_parse_format(struct uvc_device *dev,
return buffer - start;
}
+static bool is_valid_format(unsigned char *buffer, unsigned int buflen)
+{
+ if (buflen < 2)
+ return false;
+
+ switch (buffer[2]) {
+ case UVC_VS_FORMAT_UNCOMPRESSED:
+ case UVC_VS_FORMAT_MJPEG:
+ case UVC_VS_FORMAT_DV:
+ case UVC_VS_FORMAT_FRAME_BASED:
+ {
+ const struct uvc_format_desc *fmtdesc;
+
+ if (buflen < 6)
+ return false;
+ fmtdesc = uvc_format_by_guid(&buffer[5]);
+ if (!fmtdesc)
+ return false;
+ return true;
+ }
+ default:
+ break;
+ }
+
+ return false;
+}
+
static int uvc_parse_streaming(struct uvc_device *dev,
struct usb_interface *intf)
{
@@ -613,19 +640,15 @@ static int uvc_parse_streaming(struct uvc_device *dev,
/* Count the format and frame descriptors. */
while (_buflen > 2 && _buffer[1] == USB_DT_CS_INTERFACE) {
- switch (_buffer[2]) {
- case UVC_VS_FORMAT_UNCOMPRESSED:
- case UVC_VS_FORMAT_MJPEG:
- case UVC_VS_FORMAT_FRAME_BASED:
+ if (is_valid_format(_buffer, _buflen))
nformats++;
- break;
+ switch (_buffer[2]) {
case UVC_VS_FORMAT_DV:
/*
* DV format has no frame descriptor. We will create a
* dummy frame descriptor with a dummy frame interval.
*/
- nformats++;
nframes++;
nintervals++;
break;
@@ -679,11 +702,7 @@ static int uvc_parse_streaming(struct uvc_device *dev,
/* Parse the format descriptors. */
while (buflen > 2 && buffer[1] == USB_DT_CS_INTERFACE) {
- switch (buffer[2]) {
- case UVC_VS_FORMAT_UNCOMPRESSED:
- case UVC_VS_FORMAT_MJPEG:
- case UVC_VS_FORMAT_DV:
- case UVC_VS_FORMAT_FRAME_BASED:
+ if (!is_valid_format(buffer, buflen)) {
format->frame = frame;
ret = uvc_parse_format(dev, streaming, format,
&interval, buffer, buflen);
@@ -692,13 +711,6 @@ static int uvc_parse_streaming(struct uvc_device *dev,
frame += format->nframes;
format++;
-
- buflen -= ret;
- buffer += ret;
- continue;
-
- default:
- break;
}
buflen -= buffer[0];
>
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index d966d003632b..cc6608eb3ab9 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -679,7 +679,7 @@ static int uvc_parse_streaming(struct uvc_device *dev,
> interval = (u32 *)&frame[nframes];
>
> streaming->formats = format;
> - streaming->nformats = nformats;
> + streaming->nformats = 0;
>
> /* Parse the format descriptors. */
> while (buflen > 2 && buffer[1] == USB_DT_CS_INTERFACE) {
> @@ -695,6 +695,7 @@ static int uvc_parse_streaming(struct uvc_device *dev,
> if (!ret)
> break;
>
> + streaming->nformats++;
> frame += format->nframes;
> format++;
>
> What do you think ? I can submit a v2 with this change.
>
> > > if (ret < 0)
> > > goto error;
> > > + if (!ret)
> > > + break;
> > >
> > > frame += format->nframes;
> > > format++;
>
> --
> Regards,
>
> Laurent Pinchart
--
Ricardo Ribalda
