[PATCH] media: usb: uvc: fill in description for unknown pixelformats

[PATCH] media: usb: uvc: fill in description for unknown pixelformats

[Hans Verkuil]

If the fcc is 0 (indicating an unknown GUID format), then fill in the
description field in ENUM_FMT. Otherwise the V4L2 core will WARN.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Fixes: 50459f103edf ("media: uvcvideo: Remove format descriptions")
---
diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index 7aefa76a42b3..2f1ced1212cd 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -256,6 +256,9 @@ static int uvc_parse_format(struct uvc_device *dev,
 		} else {
 			dev_info(&streaming->intf->dev,
 				 "Unknown video format %pUl\n", &buffer[5]);
+			snprintf(format->name, sizeof(format->name), "%pUl\n",
+				 &buffer[5]);
+
 			format->fcc = 0;
 		}

diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
index 35453f81c1d9..fc6f9e7d8506 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -713,6 +713,10 @@ static int uvc_ioctl_enum_fmt(struct uvc_streaming *stream,
 	if (format->flags & UVC_FMT_FLAG_COMPRESSED)
 		fmt->flags |= V4L2_FMT_FLAG_COMPRESSED;
 	fmt->pixelformat = format->fcc;
+	if (format->name[0])
+		strscpy(fmt->description, format->name,
+			sizeof(fmt->description));
+
 	return 0;
 }

diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index 9a596c8d894a..22656755a801 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -264,6 +264,8 @@ struct uvc_format {
 	u32 fcc;
 	u32 flags;

+	char name[32];
+
 	unsigned int nframes;
 	struct uvc_frame *frame;
 };

Re: [PATCH] media: usb: uvc: fill in description for unknown pixelformats

[Ricardo Ribalda]

Hi Hans

Thanks for the patch.

I believe the user can fetch this info from lsusb, so this is kind of
duplicated info, and this is why it was removed.

Is there an app that uses this unknown format code ? Or the only
complaint is that WARN() is too loud for the user?

Regards!

On Wed, 29 Mar 2023 at 14:39, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> If the fcc is 0 (indicating an unknown GUID format), then fill in the
> description field in ENUM_FMT. Otherwise the V4L2 core will WARN.
>
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Fixes: 50459f103edf ("media: uvcvideo: Remove format descriptions")
> ---
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index 7aefa76a42b3..2f1ced1212cd 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -256,6 +256,9 @@ static int uvc_parse_format(struct uvc_device *dev,
>                 } else {
>                         dev_info(&streaming->intf->dev,
>                                  "Unknown video format %pUl\n", &buffer[5]);
> +                       snprintf(format->name, sizeof(format->name), "%pUl\n",
> +                                &buffer[5]);
Don't we need at least 38 chars for this?

https://docs.kernel.org/core-api/printk-formats.html#uuid-guid-addresses


> +
>                         format->fcc = 0;
>                 }
>
> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> index 35453f81c1d9..fc6f9e7d8506 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -713,6 +713,10 @@ static int uvc_ioctl_enum_fmt(struct uvc_streaming *stream,
>         if (format->flags & UVC_FMT_FLAG_COMPRESSED)
>                 fmt->flags |= V4L2_FMT_FLAG_COMPRESSED;
>         fmt->pixelformat = format->fcc;
> +       if (format->name[0])
> +               strscpy(fmt->description, format->name,
> +                       sizeof(fmt->description));
> +
>         return 0;
>  }
>
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index 9a596c8d894a..22656755a801 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -264,6 +264,8 @@ struct uvc_format {
>         u32 fcc;
>         u32 flags;
>
> +       char name[32];
> +
>         unsigned int nframes;
>         struct uvc_frame *frame;
>  };
>


--
Ricardo Ribalda

Re: [PATCH] media: usb: uvc: fill in description for unknown pixelformats

[Hans Verkuil]

On 29/03/2023 18:05, Ricardo Ribalda wrote:
> Hi Hans
> 
> Thanks for the patch.
> 
> I believe the user can fetch this info from lsusb, so this is kind of
> duplicated info, and this is why it was removed.

You got to set some description, so using the GUID this seems best.

> Is there an app that uses this unknown format code ? Or the only
> complaint is that WARN() is too loud for the user?

Normally drivers do not pass on unknown formats, but if a driver does,
then I want a WARN. If a driver does this legitimately (and I understand
that's the case for UVC), then the driver should fill in the description
to avoid this WARN.

> 
> Regards!
> 
> On Wed, 29 Mar 2023 at 14:39, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>
>> If the fcc is 0 (indicating an unknown GUID format), then fill in the
>> description field in ENUM_FMT. Otherwise the V4L2 core will WARN.
>>
>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>> Fixes: 50459f103edf ("media: uvcvideo: Remove format descriptions")
>> ---
>> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
>> index 7aefa76a42b3..2f1ced1212cd 100644
>> --- a/drivers/media/usb/uvc/uvc_driver.c
>> +++ b/drivers/media/usb/uvc/uvc_driver.c
>> @@ -256,6 +256,9 @@ static int uvc_parse_format(struct uvc_device *dev,
>>                 } else {
>>                         dev_info(&streaming->intf->dev,
>>                                  "Unknown video format %pUl\n", &buffer[5]);
>> +                       snprintf(format->name, sizeof(format->name), "%pUl\n",
>> +                                &buffer[5]);
> Don't we need at least 38 chars for this?

Yes. But all we have is 31 chars, so we take what we can :-)

This is what uvc did before this was removed.

Regards,

	Hans

> 
> https://docs.kernel.org/core-api/printk-formats.html#uuid-guid-addresses
> 
> 
>> +
>>                         format->fcc = 0;
>>                 }
>>
>> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
>> index 35453f81c1d9..fc6f9e7d8506 100644
>> --- a/drivers/media/usb/uvc/uvc_v4l2.c
>> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
>> @@ -713,6 +713,10 @@ static int uvc_ioctl_enum_fmt(struct uvc_streaming *stream,
>>         if (format->flags & UVC_FMT_FLAG_COMPRESSED)
>>                 fmt->flags |= V4L2_FMT_FLAG_COMPRESSED;
>>         fmt->pixelformat = format->fcc;
>> +       if (format->name[0])
>> +               strscpy(fmt->description, format->name,
>> +                       sizeof(fmt->description));
>> +
>>         return 0;
>>  }
>>
>> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
>> index 9a596c8d894a..22656755a801 100644
>> --- a/drivers/media/usb/uvc/uvcvideo.h
>> +++ b/drivers/media/usb/uvc/uvcvideo.h
>> @@ -264,6 +264,8 @@ struct uvc_format {
>>         u32 fcc;
>>         u32 flags;
>>
>> +       char name[32];
>> +
>>         unsigned int nframes;
>>         struct uvc_frame *frame;
>>  };
>>
> 
> 
> --
> Ricardo Ribalda

Re: [PATCH] media: usb: uvc: fill in description for unknown pixelformats

[Hans de Goede]

Hi,

On 3/29/23 14:28, Hans Verkuil wrote:
> If the fcc is 0 (indicating an unknown GUID format), then fill in the
> description field in ENUM_FMT. Otherwise the V4L2 core will WARN.
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Fixes: 50459f103edf ("media: uvcvideo: Remove format descriptions")

Thanks, patch looks good to me:

FWIW:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans

> ---
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index 7aefa76a42b3..2f1ced1212cd 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -256,6 +256,9 @@ static int uvc_parse_format(struct uvc_device *dev,
>  		} else {
>  			dev_info(&streaming->intf->dev,
>  				 "Unknown video format %pUl\n", &buffer[5]);
> +			snprintf(format->name, sizeof(format->name), "%pUl\n",
> +				 &buffer[5]);
> +
>  			format->fcc = 0;
>  		}
> 
> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> index 35453f81c1d9..fc6f9e7d8506 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -713,6 +713,10 @@ static int uvc_ioctl_enum_fmt(struct uvc_streaming *stream,
>  	if (format->flags & UVC_FMT_FLAG_COMPRESSED)
>  		fmt->flags |= V4L2_FMT_FLAG_COMPRESSED;
>  	fmt->pixelformat = format->fcc;
> +	if (format->name[0])
> +		strscpy(fmt->description, format->name,
> +			sizeof(fmt->description));
> +
>  	return 0;
>  }
> 
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index 9a596c8d894a..22656755a801 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -264,6 +264,8 @@ struct uvc_format {
>  	u32 fcc;
>  	u32 flags;
> 
> +	char name[32];
> +
>  	unsigned int nframes;
>  	struct uvc_frame *frame;
>  };
> 

Re: [PATCH] media: usb: uvc: fill in description for unknown pixelformats

[Laurent Pinchart]

Hello,

On Wed, Mar 29, 2023 at 07:20:59PM +0200, Hans Verkuil wrote:
> On 29/03/2023 18:05, Ricardo Ribalda wrote:
> > Hi Hans
> > 
> > Thanks for the patch.
> > 
> > I believe the user can fetch this info from lsusb, so this is kind of
> > duplicated info, and this is why it was removed.
> 
> You got to set some description, so using the GUID this seems best.
> 
> > Is there an app that uses this unknown format code ? Or the only
> > complaint is that WARN() is too loud for the user?
> 
> Normally drivers do not pass on unknown formats, but if a driver does,
> then I want a WARN. If a driver does this legitimately (and I understand
> that's the case for UVC), then the driver should fill in the description
> to avoid this WARN.

In hindsight we shouldn't have added a text description to formats :-)

> > On Wed, 29 Mar 2023 at 14:39, Hans Verkuil wrote:
> >>
> >> If the fcc is 0 (indicating an unknown GUID format), then fill in the
> >> description field in ENUM_FMT. Otherwise the V4L2 core will WARN.
> >>
> >> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> >> Fixes: 50459f103edf ("media: uvcvideo: Remove format descriptions")
> >> ---
> >> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> >> index 7aefa76a42b3..2f1ced1212cd 100644
> >> --- a/drivers/media/usb/uvc/uvc_driver.c
> >> +++ b/drivers/media/usb/uvc/uvc_driver.c
> >> @@ -256,6 +256,9 @@ static int uvc_parse_format(struct uvc_device *dev,
> >>                 } else {
> >>                         dev_info(&streaming->intf->dev,
> >>                                  "Unknown video format %pUl\n", &buffer[5]);
> >> +                       snprintf(format->name, sizeof(format->name), "%pUl\n",
> >> +                                &buffer[5]);
> > Don't we need at least 38 chars for this?
> 
> Yes. But all we have is 31 chars, so we take what we can :-)
> 
> This is what uvc did before this was removed.
> 
> Regards,
> 
> 	Hans
> 
> > https://docs.kernel.org/core-api/printk-formats.html#uuid-guid-addresses
> > 
> >> +
> >>                         format->fcc = 0;
> >>                 }
> >>
> >> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> >> index 35453f81c1d9..fc6f9e7d8506 100644
> >> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> >> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> >> @@ -713,6 +713,10 @@ static int uvc_ioctl_enum_fmt(struct uvc_streaming *stream,
> >>         if (format->flags & UVC_FMT_FLAG_COMPRESSED)
> >>                 fmt->flags |= V4L2_FMT_FLAG_COMPRESSED;
> >>         fmt->pixelformat = format->fcc;
> >> +       if (format->name[0])
> >> +               strscpy(fmt->description, format->name,
> >> +                       sizeof(fmt->description));
> >> +
> >>         return 0;
> >>  }
> >>
> >> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> >> index 9a596c8d894a..22656755a801 100644
> >> --- a/drivers/media/usb/uvc/uvcvideo.h
> >> +++ b/drivers/media/usb/uvc/uvcvideo.h
> >> @@ -264,6 +264,8 @@ struct uvc_format {
> >>         u32 fcc;
> >>         u32 flags;
> >>
> >> +       char name[32];
> >> +

I'd not really nice to have to store the name for every format, when we
know it will very rarely be used.

One alternative option would be to store the GUID, which would halve the
amount of memory. Another option would be to stop reporting those
formats to userspace in uvc_ioctl_enum_fmt(). They can't be selected
anyway, they have no unique 4CC.

> >>         unsigned int nframes;
> >>         struct uvc_frame *frame;
> >>  };

-- 
Regards,

Laurent Pinchart

Re: [PATCH] media: usb: uvc: fill in description for unknown pixelformats

[Thorsten Leemhuis]

On 29.03.23 14:28, Hans Verkuil wrote:
> If the fcc is 0 (indicating an unknown GUID format), then fill in the
> description field in ENUM_FMT. Otherwise the V4L2 core will WARN.
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Fixes: 50459f103edf ("media: uvcvideo: Remove format descriptions")

Thx for working on this.

Would be good to have Reported-by and Link tags for any reports about
the issue; I'm aware of the following two, maybe there were more:

https://bugzilla.kernel.org/show_bug.cgi?id=217252
https://bugzilla.redhat.com/show_bug.cgi?id=2180107

And a Cc: <stable@vger.kernel.org> # 5.15.x would likely be good as
well, as the culprit was backported.

But I write for a different reason: how urgent is this fix? Is this
"just" fixing a kernel warning, or do users notice this as some apps
crash? The bugzilla.redhat.com ticket's subject indicates it's the
latter; and I think I saw someone else mentioning that this leads to
crashes, but maybe I'm mixing things up.

Because if this fixes a crash, it afaics would be good to get this fixed
rather sooner than later in mainline, so that it can be fixed in stable
as well.

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
If I did something stupid, please tell me, as explained on that page.


> ---
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index 7aefa76a42b3..2f1ced1212cd 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -256,6 +256,9 @@ static int uvc_parse_format(struct uvc_device *dev,
>  		} else {
>  			dev_info(&streaming->intf->dev,
>  				 "Unknown video format %pUl\n", &buffer[5]);
> +			snprintf(format->name, sizeof(format->name), "%pUl\n",
> +				 &buffer[5]);
> +
>  			format->fcc = 0;
>  		}
> 
> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> index 35453f81c1d9..fc6f9e7d8506 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -713,6 +713,10 @@ static int uvc_ioctl_enum_fmt(struct uvc_streaming *stream,
>  	if (format->flags & UVC_FMT_FLAG_COMPRESSED)
>  		fmt->flags |= V4L2_FMT_FLAG_COMPRESSED;
>  	fmt->pixelformat = format->fcc;
> +	if (format->name[0])
> +		strscpy(fmt->description, format->name,
> +			sizeof(fmt->description));
> +
>  	return 0;
>  }
> 
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index 9a596c8d894a..22656755a801 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -264,6 +264,8 @@ struct uvc_format {
>  	u32 fcc;
>  	u32 flags;
> 
> +	char name[32];
> +
>  	unsigned int nframes;
>  	struct uvc_frame *frame;
>  };
> 
> 
> 

Re: [PATCH] media: usb: uvc: fill in description for unknown pixelformats

[Laurent Pinchart]

Hi Thorsten,

On Tue, Apr 11, 2023 at 01:49:03PM +0200, Thorsten Leemhuis wrote:
> On 29.03.23 14:28, Hans Verkuil wrote:
> > If the fcc is 0 (indicating an unknown GUID format), then fill in the
> > description field in ENUM_FMT. Otherwise the V4L2 core will WARN.
> > 
> > Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> > Fixes: 50459f103edf ("media: uvcvideo: Remove format descriptions")
> 
> Thx for working on this.
> 
> Would be good to have Reported-by and Link tags for any reports about
> the issue; I'm aware of the following two, maybe there were more:
> 
> https://bugzilla.kernel.org/show_bug.cgi?id=217252
> https://bugzilla.redhat.com/show_bug.cgi?id=2180107
> 
> And a Cc: <stable@vger.kernel.org> # 5.15.x would likely be good as
> well, as the culprit was backported.
> 
> But I write for a different reason: how urgent is this fix? Is this
> "just" fixing a kernel warning, or do users notice this as some apps
> crash? The bugzilla.redhat.com ticket's subject indicates it's the
> latter; and I think I saw someone else mentioning that this leads to
> crashes, but maybe I'm mixing things up.
> 
> Because if this fixes a crash, it afaics would be good to get this fixed
> rather sooner than later in mainline, so that it can be fixed in stable
> as well.

As far as I understand, it fixes a WARN_ON(). That's technically not a
crash, but it looks like a big scary one to anyone watching the kernel
log.

> Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
> --
> Everything you wanna know about Linux kernel regression tracking:
> https://linux-regtracking.leemhuis.info/about/#tldr
> If I did something stupid, please tell me, as explained on that page.
> 
> > ---
> > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > index 7aefa76a42b3..2f1ced1212cd 100644
> > --- a/drivers/media/usb/uvc/uvc_driver.c
> > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > @@ -256,6 +256,9 @@ static int uvc_parse_format(struct uvc_device *dev,
> >  		} else {
> >  			dev_info(&streaming->intf->dev,
> >  				 "Unknown video format %pUl\n", &buffer[5]);
> > +			snprintf(format->name, sizeof(format->name), "%pUl\n",
> > +				 &buffer[5]);
> > +
> >  			format->fcc = 0;
> >  		}
> > 
> > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> > index 35453f81c1d9..fc6f9e7d8506 100644
> > --- a/drivers/media/usb/uvc/uvc_v4l2.c
> > +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> > @@ -713,6 +713,10 @@ static int uvc_ioctl_enum_fmt(struct uvc_streaming *stream,
> >  	if (format->flags & UVC_FMT_FLAG_COMPRESSED)
> >  		fmt->flags |= V4L2_FMT_FLAG_COMPRESSED;
> >  	fmt->pixelformat = format->fcc;
> > +	if (format->name[0])
> > +		strscpy(fmt->description, format->name,
> > +			sizeof(fmt->description));
> > +
> >  	return 0;
> >  }
> > 
> > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > index 9a596c8d894a..22656755a801 100644
> > --- a/drivers/media/usb/uvc/uvcvideo.h
> > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > @@ -264,6 +264,8 @@ struct uvc_format {
> >  	u32 fcc;
> >  	u32 flags;
> > 
> > +	char name[32];
> > +
> >  	unsigned int nframes;
> >  	struct uvc_frame *frame;
> >  };

-- 
Regards,

Laurent Pinchart

Re: [PATCH] media: usb: uvc: fill in description for unknown pixelformats

[Ricardo Ribalda]

Hi Thorsten

On Tue, 11 Apr 2023 at 13:51, Thorsten Leemhuis
<regressions@leemhuis.info> wrote:
>
> On 29.03.23 14:28, Hans Verkuil wrote:
> > If the fcc is 0 (indicating an unknown GUID format), then fill in the
> > description field in ENUM_FMT. Otherwise the V4L2 core will WARN.
> >
> > Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> > Fixes: 50459f103edf ("media: uvcvideo: Remove format descriptions")
>
> Thx for working on this.
>
> Would be good to have Reported-by and Link tags for any reports about
> the issue; I'm aware of the following two, maybe there were more:
>
> https://bugzilla.kernel.org/show_bug.cgi?id=217252
> https://bugzilla.redhat.com/show_bug.cgi?id=2180107
>
> And a Cc: <stable@vger.kernel.org> # 5.15.x would likely be good as
> well, as the culprit was backported.
>
> But I write for a different reason: how urgent is this fix? Is this
> "just" fixing a kernel warning, or do users notice this as some apps
> crash? The bugzilla.redhat.com ticket's subject indicates it's the
> latter; and I think I saw someone else mentioning that this leads to
> crashes, but maybe I'm mixing things up.

I think it only a crash if panic_on_warn is enabled.

>
> Because if this fixes a crash, it afaics would be good to get this fixed
> rather sooner than later in mainline, so that it can be fixed in stable
> as well.
>
> Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
> --
> Everything you wanna know about Linux kernel regression tracking:
> https://linux-regtracking.leemhuis.info/about/#tldr
> If I did something stupid, please tell me, as explained on that page.
>
>
> > ---
> > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > index 7aefa76a42b3..2f1ced1212cd 100644
> > --- a/drivers/media/usb/uvc/uvc_driver.c
> > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > @@ -256,6 +256,9 @@ static int uvc_parse_format(struct uvc_device *dev,
> >               } else {
> >                       dev_info(&streaming->intf->dev,
> >                                "Unknown video format %pUl\n", &buffer[5]);
> > +                     snprintf(format->name, sizeof(format->name), "%pUl\n",
> > +                              &buffer[5]);
> > +
> >                       format->fcc = 0;
> >               }
> >
> > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> > index 35453f81c1d9..fc6f9e7d8506 100644
> > --- a/drivers/media/usb/uvc/uvc_v4l2.c
> > +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> > @@ -713,6 +713,10 @@ static int uvc_ioctl_enum_fmt(struct uvc_streaming *stream,
> >       if (format->flags & UVC_FMT_FLAG_COMPRESSED)
> >               fmt->flags |= V4L2_FMT_FLAG_COMPRESSED;
> >       fmt->pixelformat = format->fcc;
> > +     if (format->name[0])
> > +             strscpy(fmt->description, format->name,
> > +                     sizeof(fmt->description));
> > +
> >       return 0;
> >  }
> >
> > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > index 9a596c8d894a..22656755a801 100644
> > --- a/drivers/media/usb/uvc/uvcvideo.h
> > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > @@ -264,6 +264,8 @@ struct uvc_format {
> >       u32 fcc;
> >       u32 flags;
> >
> > +     char name[32];
> > +
> >       unsigned int nframes;
> >       struct uvc_frame *frame;
> >  };
> >
> >
> >



-- 
Ricardo Ribalda

Re: [PATCH] media: usb: uvc: fill in description for unknown pixelformats

[Linux regression tracking (Thorsten Leemhuis)]

On 11.04.23 14:06, Laurent Pinchart wrote:
> Hi Thorsten,
> 
> On Tue, Apr 11, 2023 at 01:49:03PM +0200, Thorsten Leemhuis wrote:
>> On 29.03.23 14:28, Hans Verkuil wrote:
>>> If the fcc is 0 (indicating an unknown GUID format), then fill in the
>>> description field in ENUM_FMT. Otherwise the V4L2 core will WARN.
>>>
>>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>>> Fixes: 50459f103edf ("media: uvcvideo: Remove format descriptions")
>>
>> Thx for working on this.
>>
>> Would be good to have Reported-by and Link tags for any reports about
>> the issue; I'm aware of the following two, maybe there were more:
>>
>> https://bugzilla.kernel.org/show_bug.cgi?id=217252
>> https://bugzilla.redhat.com/show_bug.cgi?id=2180107
>>
>> And a Cc: <stable@vger.kernel.org> # 5.15.x would likely be good as
>> well, as the culprit was backported.
>>
>> But I write for a different reason: how urgent is this fix? Is this
>> "just" fixing a kernel warning, or do users notice this as some apps
>> crash? The bugzilla.redhat.com ticket's subject indicates it's the
>> latter; and I think I saw someone else mentioning that this leads to
>> crashes, but maybe I'm mixing things up.
>>
>> Because if this fixes a crash, it afaics would be good to get this fixed
>> rather sooner than later in mainline, so that it can be fixed in stable
>> as well.
> 
> As far as I understand, it fixes a WARN_ON(). That's technically not a
> crash, but it looks like a big scary one to anyone watching the kernel
> log.

Thx for clarifying. I know what WARN_ON() does, but for some reason was
under the strong impression that one of the reports mentioned the video
app was crashing in parallel to the kernel warning. Well, guess I mixed
this up with some other regression. Sorry for the noise!

Ciao, Thorsten

>>> ---
>>> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
>>> index 7aefa76a42b3..2f1ced1212cd 100644
>>> --- a/drivers/media/usb/uvc/uvc_driver.c
>>> +++ b/drivers/media/usb/uvc/uvc_driver.c
>>> @@ -256,6 +256,9 @@ static int uvc_parse_format(struct uvc_device *dev,
>>>  		} else {
>>>  			dev_info(&streaming->intf->dev,
>>>  				 "Unknown video format %pUl\n", &buffer[5]);
>>> +			snprintf(format->name, sizeof(format->name), "%pUl\n",
>>> +				 &buffer[5]);
>>> +
>>>  			format->fcc = 0;
>>>  		}
>>>
>>> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
>>> index 35453f81c1d9..fc6f9e7d8506 100644
>>> --- a/drivers/media/usb/uvc/uvc_v4l2.c
>>> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
>>> @@ -713,6 +713,10 @@ static int uvc_ioctl_enum_fmt(struct uvc_streaming *stream,
>>>  	if (format->flags & UVC_FMT_FLAG_COMPRESSED)
>>>  		fmt->flags |= V4L2_FMT_FLAG_COMPRESSED;
>>>  	fmt->pixelformat = format->fcc;
>>> +	if (format->name[0])
>>> +		strscpy(fmt->description, format->name,
>>> +			sizeof(fmt->description));
>>> +
>>>  	return 0;
>>>  }
>>>
>>> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
>>> index 9a596c8d894a..22656755a801 100644
>>> --- a/drivers/media/usb/uvc/uvcvideo.h
>>> +++ b/drivers/media/usb/uvc/uvcvideo.h
>>> @@ -264,6 +264,8 @@ struct uvc_format {
>>>  	u32 fcc;
>>>  	u32 flags;
>>>
>>> +	char name[32];
>>> +
>>>  	unsigned int nframes;
>>>  	struct uvc_frame *frame;
>>>  };
> 

Re: [PATCH] media: usb: uvc: fill in description for unknown pixelformats

[Laurent Pinchart]

Hi Hans,

On Wed, Apr 05, 2023 at 09:40:20AM +0300, Laurent Pinchart wrote:
> On Wed, Mar 29, 2023 at 07:20:59PM +0200, Hans Verkuil wrote:
> > On 29/03/2023 18:05, Ricardo Ribalda wrote:
> > > Hi Hans
> > > 
> > > Thanks for the patch.
> > > 
> > > I believe the user can fetch this info from lsusb, so this is kind of
> > > duplicated info, and this is why it was removed.
> > 
> > You got to set some description, so using the GUID this seems best.
> > 
> > > Is there an app that uses this unknown format code ? Or the only
> > > complaint is that WARN() is too loud for the user?
> > 
> > Normally drivers do not pass on unknown formats, but if a driver does,
> > then I want a WARN. If a driver does this legitimately (and I understand
> > that's the case for UVC), then the driver should fill in the description
> > to avoid this WARN.
> 
> In hindsight we shouldn't have added a text description to formats :-)
> 
> > > On Wed, 29 Mar 2023 at 14:39, Hans Verkuil wrote:
> > >>
> > >> If the fcc is 0 (indicating an unknown GUID format), then fill in the
> > >> description field in ENUM_FMT. Otherwise the V4L2 core will WARN.
> > >>
> > >> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> > >> Fixes: 50459f103edf ("media: uvcvideo: Remove format descriptions")
> > >> ---
> > >> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > >> index 7aefa76a42b3..2f1ced1212cd 100644
> > >> --- a/drivers/media/usb/uvc/uvc_driver.c
> > >> +++ b/drivers/media/usb/uvc/uvc_driver.c
> > >> @@ -256,6 +256,9 @@ static int uvc_parse_format(struct uvc_device *dev,
> > >>                 } else {
> > >>                         dev_info(&streaming->intf->dev,
> > >>                                  "Unknown video format %pUl\n", &buffer[5]);
> > >> +                       snprintf(format->name, sizeof(format->name), "%pUl\n",
> > >> +                                &buffer[5]);
> > > Don't we need at least 38 chars for this?
> > 
> > Yes. But all we have is 31 chars, so we take what we can :-)
> > 
> > This is what uvc did before this was removed.
> > 
> > Regards,
> > 
> > 	Hans
> > 
> > > https://docs.kernel.org/core-api/printk-formats.html#uuid-guid-addresses
> > > 
> > >> +
> > >>                         format->fcc = 0;
> > >>                 }
> > >>
> > >> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> > >> index 35453f81c1d9..fc6f9e7d8506 100644
> > >> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> > >> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> > >> @@ -713,6 +713,10 @@ static int uvc_ioctl_enum_fmt(struct uvc_streaming *stream,
> > >>         if (format->flags & UVC_FMT_FLAG_COMPRESSED)
> > >>                 fmt->flags |= V4L2_FMT_FLAG_COMPRESSED;
> > >>         fmt->pixelformat = format->fcc;
> > >> +       if (format->name[0])
> > >> +               strscpy(fmt->description, format->name,
> > >> +                       sizeof(fmt->description));
> > >> +
> > >>         return 0;
> > >>  }
> > >>
> > >> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > >> index 9a596c8d894a..22656755a801 100644
> > >> --- a/drivers/media/usb/uvc/uvcvideo.h
> > >> +++ b/drivers/media/usb/uvc/uvcvideo.h
> > >> @@ -264,6 +264,8 @@ struct uvc_format {
> > >>         u32 fcc;
> > >>         u32 flags;
> > >>
> > >> +       char name[32];
> > >> +
> 
> I'd not really nice to have to store the name for every format, when we
> know it will very rarely be used.
> 
> One alternative option would be to store the GUID, which would halve the
> amount of memory. Another option would be to stop reporting those
> formats to userspace in uvc_ioctl_enum_fmt(). They can't be selected
> anyway, they have no unique 4CC.

Any opinion on this ? I'm increasingly tempted by not reporting
unsupported formats to userspace.

> > >>         unsigned int nframes;
> > >>         struct uvc_frame *frame;
> > >>  };

-- 
Regards,

Laurent Pinchart

Re: [PATCH] media: usb: uvc: fill in description for unknown pixelformats

[Hans Verkuil]

On 19/04/2023 08:23, Laurent Pinchart wrote:
> Hi Hans,
> 
> On Wed, Apr 05, 2023 at 09:40:20AM +0300, Laurent Pinchart wrote:
>> On Wed, Mar 29, 2023 at 07:20:59PM +0200, Hans Verkuil wrote:
>>> On 29/03/2023 18:05, Ricardo Ribalda wrote:
>>>> Hi Hans
>>>>
>>>> Thanks for the patch.
>>>>
>>>> I believe the user can fetch this info from lsusb, so this is kind of
>>>> duplicated info, and this is why it was removed.
>>>
>>> You got to set some description, so using the GUID this seems best.
>>>
>>>> Is there an app that uses this unknown format code ? Or the only
>>>> complaint is that WARN() is too loud for the user?
>>>
>>> Normally drivers do not pass on unknown formats, but if a driver does,
>>> then I want a WARN. If a driver does this legitimately (and I understand
>>> that's the case for UVC), then the driver should fill in the description
>>> to avoid this WARN.
>>
>> In hindsight we shouldn't have added a text description to formats :-)
>>
>>>> On Wed, 29 Mar 2023 at 14:39, Hans Verkuil wrote:
>>>>>
>>>>> If the fcc is 0 (indicating an unknown GUID format), then fill in the
>>>>> description field in ENUM_FMT. Otherwise the V4L2 core will WARN.
>>>>>
>>>>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>>>>> Fixes: 50459f103edf ("media: uvcvideo: Remove format descriptions")
>>>>> ---
>>>>> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
>>>>> index 7aefa76a42b3..2f1ced1212cd 100644
>>>>> --- a/drivers/media/usb/uvc/uvc_driver.c
>>>>> +++ b/drivers/media/usb/uvc/uvc_driver.c
>>>>> @@ -256,6 +256,9 @@ static int uvc_parse_format(struct uvc_device *dev,
>>>>>                 } else {
>>>>>                         dev_info(&streaming->intf->dev,
>>>>>                                  "Unknown video format %pUl\n", &buffer[5]);
>>>>> +                       snprintf(format->name, sizeof(format->name), "%pUl\n",
>>>>> +                                &buffer[5]);
>>>> Don't we need at least 38 chars for this?
>>>
>>> Yes. But all we have is 31 chars, so we take what we can :-)
>>>
>>> This is what uvc did before this was removed.
>>>
>>> Regards,
>>>
>>> 	Hans
>>>
>>>> https://docs.kernel.org/core-api/printk-formats.html#uuid-guid-addresses
>>>>
>>>>> +
>>>>>                         format->fcc = 0;
>>>>>                 }
>>>>>
>>>>> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
>>>>> index 35453f81c1d9..fc6f9e7d8506 100644
>>>>> --- a/drivers/media/usb/uvc/uvc_v4l2.c
>>>>> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
>>>>> @@ -713,6 +713,10 @@ static int uvc_ioctl_enum_fmt(struct uvc_streaming *stream,
>>>>>         if (format->flags & UVC_FMT_FLAG_COMPRESSED)
>>>>>                 fmt->flags |= V4L2_FMT_FLAG_COMPRESSED;
>>>>>         fmt->pixelformat = format->fcc;
>>>>> +       if (format->name[0])
>>>>> +               strscpy(fmt->description, format->name,
>>>>> +                       sizeof(fmt->description));
>>>>> +
>>>>>         return 0;
>>>>>  }
>>>>>
>>>>> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
>>>>> index 9a596c8d894a..22656755a801 100644
>>>>> --- a/drivers/media/usb/uvc/uvcvideo.h
>>>>> +++ b/drivers/media/usb/uvc/uvcvideo.h
>>>>> @@ -264,6 +264,8 @@ struct uvc_format {
>>>>>         u32 fcc;
>>>>>         u32 flags;
>>>>>
>>>>> +       char name[32];
>>>>> +
>>
>> I'd not really nice to have to store the name for every format, when we
>> know it will very rarely be used.
>>
>> One alternative option would be to store the GUID, which would halve the
>> amount of memory. Another option would be to stop reporting those
>> formats to userspace in uvc_ioctl_enum_fmt(). They can't be selected
>> anyway, they have no unique 4CC.
> 
> Any opinion on this ? I'm increasingly tempted by not reporting
> unsupported formats to userspace.

Since they cannot be selected, I think that is the best approach.
Perhaps only show them in the kernel log if debugging is enabled.

Regards,

	Hans

> 
>>>>>         unsigned int nframes;
>>>>>         struct uvc_frame *frame;
>>>>>  };
> 

Re: [PATCH] media: usb: uvc: fill in description for unknown pixelformats

[Linux regression tracking (Thorsten Leemhuis)]

On 29.03.23 14:28, Hans Verkuil wrote:
> If the fcc is 0 (indicating an unknown GUID format), then fill in the
> description field in ENUM_FMT. Otherwise the V4L2 core will WARN.

What happened to this? It seems this fall through the cracks.

BTW:

> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>

Afaics it might be good to have these in here:

Link: https://bugzilla.kernel.org/show_bug.cgi?id=217252
Link: https://bugzilla.redhat.com/show_bug.cgi?id=2180107

A comment in the former is what brought me here.

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
If I did something stupid, please tell me, as explained on that page.

#regzbot ^backmonitor:
https://lore.kernel.org/lkml/dc8e5276-ef88-648f-9f0d-10151ea62c90@leemhuis.info/
#regzbot poke

> Fixes: 50459f103edf ("media: uvcvideo: Remove format descriptions")
> ---
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index 7aefa76a42b3..2f1ced1212cd 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -256,6 +256,9 @@ static int uvc_parse_format(struct uvc_device *dev,
>  		} else {
>  			dev_info(&streaming->intf->dev,
>  				 "Unknown video format %pUl\n", &buffer[5]);
> +			snprintf(format->name, sizeof(format->name), "%pUl\n",
> +				 &buffer[5]);
> +
>  			format->fcc = 0;
>  		}
> 
> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> index 35453f81c1d9..fc6f9e7d8506 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -713,6 +713,10 @@ static int uvc_ioctl_enum_fmt(struct uvc_streaming *stream,
>  	if (format->flags & UVC_FMT_FLAG_COMPRESSED)
>  		fmt->flags |= V4L2_FMT_FLAG_COMPRESSED;
>  	fmt->pixelformat = format->fcc;
> +	if (format->name[0])
> +		strscpy(fmt->description, format->name,
> +			sizeof(fmt->description));
> +
>  	return 0;
>  }
> 
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index 9a596c8d894a..22656755a801 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -264,6 +264,8 @@ struct uvc_format {
>  	u32 fcc;
>  	u32 flags;
> 
> +	char name[32];
> +
>  	unsigned int nframes;
>  	struct uvc_frame *frame;
>  };
> 

Re: [PATCH] media: usb: uvc: fill in description for unknown pixelformats

[Laurent Pinchart]

Hi Thorsten,

On Wed, May 31, 2023 at 01:48:51PM +0200, Linux regression tracking (Thorsten Leemhuis) wrote:
> On 29.03.23 14:28, Hans Verkuil wrote:
> > If the fcc is 0 (indicating an unknown GUID format), then fill in the
> > description field in ENUM_FMT. Otherwise the V4L2 core will WARN.
> 
> What happened to this? It seems this fall through the cracks.

I've posted a better fix, see
https://lore.kernel.org/linux-media/20230506065809.24645-1-laurent.pinchart@ideasonboard.com/.
Of course, fate had it that it got reviewed exactly on the day when I started by holidays :-S I'm now back, and will send a pull request.

> BTW:
> 
> > Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> 
> Afaics it might be good to have these in here:
> 
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=217252
> Link: https://bugzilla.redhat.com/show_bug.cgi?id=2180107

I'll add those two links.

> A comment in the former is what brought me here.
> 
> Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
> --
> Everything you wanna know about Linux kernel regression tracking:
> https://linux-regtracking.leemhuis.info/about/#tldr
> If I did something stupid, please tell me, as explained on that page.
> 
> #regzbot ^backmonitor:
> https://lore.kernel.org/lkml/dc8e5276-ef88-648f-9f0d-10151ea62c90@leemhuis.info/
> #regzbot poke
> 
> > Fixes: 50459f103edf ("media: uvcvideo: Remove format descriptions")
> > ---
> > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > index 7aefa76a42b3..2f1ced1212cd 100644
> > --- a/drivers/media/usb/uvc/uvc_driver.c
> > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > @@ -256,6 +256,9 @@ static int uvc_parse_format(struct uvc_device *dev,
> >  		} else {
> >  			dev_info(&streaming->intf->dev,
> >  				 "Unknown video format %pUl\n", &buffer[5]);
> > +			snprintf(format->name, sizeof(format->name), "%pUl\n",
> > +				 &buffer[5]);
> > +
> >  			format->fcc = 0;
> >  		}
> > 
> > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> > index 35453f81c1d9..fc6f9e7d8506 100644
> > --- a/drivers/media/usb/uvc/uvc_v4l2.c
> > +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> > @@ -713,6 +713,10 @@ static int uvc_ioctl_enum_fmt(struct uvc_streaming *stream,
> >  	if (format->flags & UVC_FMT_FLAG_COMPRESSED)
> >  		fmt->flags |= V4L2_FMT_FLAG_COMPRESSED;
> >  	fmt->pixelformat = format->fcc;
> > +	if (format->name[0])
> > +		strscpy(fmt->description, format->name,
> > +			sizeof(fmt->description));
> > +
> >  	return 0;
> >  }
> > 
> > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > index 9a596c8d894a..22656755a801 100644
> > --- a/drivers/media/usb/uvc/uvcvideo.h
> > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > @@ -264,6 +264,8 @@ struct uvc_format {
> >  	u32 fcc;
> >  	u32 flags;
> > 
> > +	char name[32];
> > +
> >  	unsigned int nframes;
> >  	struct uvc_frame *frame;
> >  };

-- 
Regards,

Laurent Pinchart

Re: [PATCH] media: usb: uvc: fill in description for unknown pixelformats

[Linux regression tracking (Thorsten Leemhuis)]

On 31.05.23 14:44, Laurent Pinchart wrote:
> Hi Thorsten,
> 
> On Wed, May 31, 2023 at 01:48:51PM +0200, Linux regression tracking (Thorsten Leemhuis) wrote:
>> On 29.03.23 14:28, Hans Verkuil wrote:
>>> If the fcc is 0 (indicating an unknown GUID format), then fill in the
>>> description field in ENUM_FMT. Otherwise the V4L2 core will WARN.
>>
>> What happened to this? It seems this fall through the cracks.
> 
> I've posted a better fix,

You mean an even better fix, as this one already was a better fix for a
earlier fix ;)

> see
> https://lore.kernel.org/linux-media/20230506065809.24645-1-laurent.pinchart@ideasonboard.com/.
> Of course, fate had it that it got reviewed exactly on the day when I started by holidays :-S

:-D

> I'm now back, and will send a pull request.

In that case: welcome back.

>> BTW:
>>
>>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>>
>> Afaics it might be good to have these in here:
>>
>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=217252
>> Link: https://bugzilla.redhat.com/show_bug.cgi?id=2180107
> 
> I'll add those two links.

Great, many thx!

Ciao, Thorsten

>> A comment in the former is what brought me here.
>>
>> Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
>> --
>> Everything you wanna know about Linux kernel regression tracking:
>> https://linux-regtracking.leemhuis.info/about/#tldr
>> If I did something stupid, please tell me, as explained on that page.
>>
>> #regzbot ^backmonitor:
>> https://lore.kernel.org/lkml/dc8e5276-ef88-648f-9f0d-10151ea62c90@leemhuis.info/
>> #regzbot poke
>>
>>> Fixes: 50459f103edf ("media: uvcvideo: Remove format descriptions")
>>> ---
>>> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
>>> index 7aefa76a42b3..2f1ced1212cd 100644
>>> --- a/drivers/media/usb/uvc/uvc_driver.c
>>> +++ b/drivers/media/usb/uvc/uvc_driver.c
>>> @@ -256,6 +256,9 @@ static int uvc_parse_format(struct uvc_device *dev,
>>>  		} else {
>>>  			dev_info(&streaming->intf->dev,
>>>  				 "Unknown video format %pUl\n", &buffer[5]);
>>> +			snprintf(format->name, sizeof(format->name), "%pUl\n",
>>> +				 &buffer[5]);
>>> +
>>>  			format->fcc = 0;
>>>  		}
>>>
>>> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
>>> index 35453f81c1d9..fc6f9e7d8506 100644
>>> --- a/drivers/media/usb/uvc/uvc_v4l2.c
>>> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
>>> @@ -713,6 +713,10 @@ static int uvc_ioctl_enum_fmt(struct uvc_streaming *stream,
>>>  	if (format->flags & UVC_FMT_FLAG_COMPRESSED)
>>>  		fmt->flags |= V4L2_FMT_FLAG_COMPRESSED;
>>>  	fmt->pixelformat = format->fcc;
>>> +	if (format->name[0])
>>> +		strscpy(fmt->description, format->name,
>>> +			sizeof(fmt->description));
>>> +
>>>  	return 0;
>>>  }
>>>
>>> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
>>> index 9a596c8d894a..22656755a801 100644
>>> --- a/drivers/media/usb/uvc/uvcvideo.h
>>> +++ b/drivers/media/usb/uvc/uvcvideo.h
>>> @@ -264,6 +264,8 @@ struct uvc_format {
>>>  	u32 fcc;
>>>  	u32 flags;
>>>
>>> +	char name[32];
>>> +
>>>  	unsigned int nframes;
>>>  	struct uvc_frame *frame;
>>>  };
>