| Message ID | 20230606171150.12875-1-laurent.pinchart@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: uvcvideo: Fix menu count handling for userspace XU mappings | expand |
On 06.06.23 19:11, Laurent Pinchart wrote: > This is untested. Poncho, would you be able to test this patch to see if > it fixes your issue ? Yes, your patch on top of 6.3.6 does fix my issue.
Hi Laurent On Tue, 6 Jun 2023 at 19:11, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > When commit 716c330433e3 ("media: uvcvideo: Use standard names for > menus") reworked the handling of menu controls, it inadvertently > replaced a GENMASK(n - 1, 0) with a BIT_MASK(n). The latter isn't > equivalent to the former, which broke adding XU mappings from userspace. > Fix it. Ups, that was wrong :), sorry about that Thanks for the quick fix. > > Reported-by: Poncho <poncho@spahan.ch> > Link: https://lore.kernel.org/linux-media/468a36ec-c3ac-cb47-e12f-5906239ae3cd@spahan.ch/ > Fixes: 716c330433e3 ("media: uvcvideo: Use standard names for menus") > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Ricardo Ribalda <ribalda@chromium.org> > --- > This is untested. Poncho, would you be able to test this patch to see if > it fixes your issue ? > --- > drivers/media/usb/uvc/uvc_v4l2.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c > index 5ac2a424b13d..f4988f03640a 100644 > --- a/drivers/media/usb/uvc/uvc_v4l2.c > +++ b/drivers/media/usb/uvc/uvc_v4l2.c > @@ -45,7 +45,7 @@ static int uvc_control_add_xu_mapping(struct uvc_video_chain *chain, > map->menu_names = NULL; > map->menu_mapping = NULL; > > - map->menu_mask = BIT_MASK(xmap->menu_count); > + map->menu_mask = GENMASK(xmap->menu_count - 1, 0); > > size = xmap->menu_count * sizeof(*map->menu_mapping); > map->menu_mapping = kzalloc(size, GFP_KERNEL); > -- > Regards, > > Laurent Pinchart >
On 06.06.23 19:11, Laurent Pinchart wrote: > When commit 716c330433e3 ("media: uvcvideo: Use standard names for > menus") reworked the handling of menu controls, it inadvertently > replaced a GENMASK(n - 1, 0) with a BIT_MASK(n). The latter isn't > equivalent to the former, which broke adding XU mappings from userspace. > Fix it. > > Reported-by: Poncho <poncho@spahan.ch> > Link: https://lore.kernel.org/linux-media/468a36ec-c3ac-cb47-e12f-5906239ae3cd@spahan.ch/ > Fixes: 716c330433e3 ("media: uvcvideo: Use standard names for menus") Many thx for looking into this. Please consider adding Cc: stable@vger.kernel.org to get this fixed in 6.3.y, as a fixes tag alone is not enough to guarantee a backport attempt by the stable team. Ciao, Thorsten > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > This is untested. Poncho, would you be able to test this patch to see if > it fixes your issue ? > --- > drivers/media/usb/uvc/uvc_v4l2.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c > index 5ac2a424b13d..f4988f03640a 100644 > --- a/drivers/media/usb/uvc/uvc_v4l2.c > +++ b/drivers/media/usb/uvc/uvc_v4l2.c > @@ -45,7 +45,7 @@ static int uvc_control_add_xu_mapping(struct uvc_video_chain *chain, > map->menu_names = NULL; > map->menu_mapping = NULL; > > - map->menu_mask = BIT_MASK(xmap->menu_count); > + map->menu_mask = GENMASK(xmap->menu_count - 1, 0); > > size = xmap->menu_count * sizeof(*map->menu_mapping); > map->menu_mapping = kzalloc(size, GFP_KERNEL);
Hi Laurent Friendly ping. I think this patch was forgotten. Best regards, Poncho On 06.06.23 19:11, Laurent Pinchart wrote: > When commit 716c330433e3 ("media: uvcvideo: Use standard names for > menus") reworked the handling of menu controls, it inadvertently > replaced a GENMASK(n - 1, 0) with a BIT_MASK(n). The latter isn't > equivalent to the former, which broke adding XU mappings from userspace. > Fix it. > > Reported-by: Poncho <poncho@spahan.ch> > Link: https://lore.kernel.org/linux-media/468a36ec-c3ac-cb47-e12f-5906239ae3cd@spahan.ch/ > Fixes: 716c330433e3 ("media: uvcvideo: Use standard names for menus") > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > This is untested. Poncho, would you be able to test this patch to see if > it fixes your issue ? > --- > drivers/media/usb/uvc/uvc_v4l2.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c > index 5ac2a424b13d..f4988f03640a 100644 > --- a/drivers/media/usb/uvc/uvc_v4l2.c > +++ b/drivers/media/usb/uvc/uvc_v4l2.c > @@ -45,7 +45,7 @@ static int uvc_control_add_xu_mapping(struct uvc_video_chain *chain, > map->menu_names = NULL; > map->menu_mapping = NULL; > > - map->menu_mask = BIT_MASK(xmap->menu_count); > + map->menu_mask = GENMASK(xmap->menu_count - 1, 0); > > size = xmap->menu_count * sizeof(*map->menu_mapping); > map->menu_mapping = kzalloc(size, GFP_KERNEL);
Hello, On Wed, Jun 21, 2023 at 05:26:39PM +0200, Poncho wrote: > Hi Laurent > > Friendly ping. I think this patch was forgotten. Thanks for the reminder. I'll send a pull request for v6.6. > On 06.06.23 19:11, Laurent Pinchart wrote: > > When commit 716c330433e3 ("media: uvcvideo: Use standard names for > > menus") reworked the handling of menu controls, it inadvertently > > replaced a GENMASK(n - 1, 0) with a BIT_MASK(n). The latter isn't > > equivalent to the former, which broke adding XU mappings from userspace. > > Fix it. > > > > Reported-by: Poncho <poncho@spahan.ch> > > Link: https://lore.kernel.org/linux-media/468a36ec-c3ac-cb47-e12f-5906239ae3cd@spahan.ch/ > > Fixes: 716c330433e3 ("media: uvcvideo: Use standard names for menus") > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > This is untested. Poncho, would you be able to test this patch to see if > > it fixes your issue ? > > --- > > drivers/media/usb/uvc/uvc_v4l2.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c > > index 5ac2a424b13d..f4988f03640a 100644 > > --- a/drivers/media/usb/uvc/uvc_v4l2.c > > +++ b/drivers/media/usb/uvc/uvc_v4l2.c > > @@ -45,7 +45,7 @@ static int uvc_control_add_xu_mapping(struct uvc_video_chain *chain, > > map->menu_names = NULL; > > map->menu_mapping = NULL; > > > > - map->menu_mask = BIT_MASK(xmap->menu_count); > > + map->menu_mask = GENMASK(xmap->menu_count - 1, 0); > > > > size = xmap->menu_count * sizeof(*map->menu_mapping); > > map->menu_mapping = kzalloc(size, GFP_KERNEL);
On 24.06.23 02:25, Laurent Pinchart wrote: > > On Wed, Jun 21, 2023 at 05:26:39PM +0200, Poncho wrote: >> Hi Laurent >> >> Friendly ping. I think this patch was forgotten. > Thanks for the reminder. I'll send a pull request for v6.6. Why 6.6? If this would be a feature I'd fully understand. But this afaics fixes a regression and thus should be handled as one, even if it's a regression from a earlier release. For details see this recent mail from Linus: https://lore.kernel.org/all/CAHk-=wis_qQy4oDNynNKi5b7Qhosmxtoj1jxo5wmB6SRUwQUBQ@mail.gmail.com/ Side note: as it's a regression it would be good if this commit had a "CC: <stable..." tag 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. >> On 06.06.23 19:11, Laurent Pinchart wrote: >>> When commit 716c330433e3 ("media: uvcvideo: Use standard names for >>> menus") reworked the handling of menu controls, it inadvertently >>> replaced a GENMASK(n - 1, 0) with a BIT_MASK(n). The latter isn't >>> equivalent to the former, which broke adding XU mappings from userspace. >>> Fix it. >>> >>> Reported-by: Poncho <poncho@spahan.ch> >>> Link: https://lore.kernel.org/linux-media/468a36ec-c3ac-cb47-e12f-5906239ae3cd@spahan.ch/ >>> Fixes: 716c330433e3 ("media: uvcvideo: Use standard names for menus") >>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>> --- >>> This is untested. Poncho, would you be able to test this patch to see if >>> it fixes your issue ? >>> --- >>> drivers/media/usb/uvc/uvc_v4l2.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c >>> index 5ac2a424b13d..f4988f03640a 100644 >>> --- a/drivers/media/usb/uvc/uvc_v4l2.c >>> +++ b/drivers/media/usb/uvc/uvc_v4l2.c >>> @@ -45,7 +45,7 @@ static int uvc_control_add_xu_mapping(struct uvc_video_chain *chain, >>> map->menu_names = NULL; >>> map->menu_mapping = NULL; >>> >>> - map->menu_mask = BIT_MASK(xmap->menu_count); >>> + map->menu_mask = GENMASK(xmap->menu_count - 1, 0); >>> >>> size = xmap->menu_count * sizeof(*map->menu_mapping); >>> map->menu_mapping = kzalloc(size, GFP_KERNEL); >
Hi Thorsten, On Sat, Jun 24, 2023 at 11:07:14AM +0200, Thorsten Leemhuis wrote: > On 24.06.23 02:25, Laurent Pinchart wrote: > > On Wed, Jun 21, 2023 at 05:26:39PM +0200, Poncho wrote: > >> Hi Laurent > >> > >> Friendly ping. I think this patch was forgotten. > > Thanks for the reminder. I'll send a pull request for v6.6. > > Why 6.6? If this would be a feature I'd fully understand. But this > afaics fixes a regression and thus should be handled as one, even if > it's a regression from a earlier release. For details see this recent > mail from Linus: > > https://lore.kernel.org/all/CAHk-=wis_qQy4oDNynNKi5b7Qhosmxtoj1jxo5wmB6SRUwQUBQ@mail.gmail.com/ Linus has decided to not pull from the media tree for the next kernel release due to a set of mishaps, see https://lore.kernel.org/linux-media/CAHk-=wgzU8_dGn0Yg+DyX7ammTkDUCyEJ4C=NvnHRhxKWC7Wpw@mail.gmail.com/. Re-reading the mail, fixes should be fine even if the issue has been there for several releases. I'll send send a pull request for v6.5. > Side note: as it's a regression it would be good if this commit had a > "CC: <stable..." tag as well. I've already added that to my tree. > 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. > > >> On 06.06.23 19:11, Laurent Pinchart wrote: > >>> When commit 716c330433e3 ("media: uvcvideo: Use standard names for > >>> menus") reworked the handling of menu controls, it inadvertently > >>> replaced a GENMASK(n - 1, 0) with a BIT_MASK(n). The latter isn't > >>> equivalent to the former, which broke adding XU mappings from userspace. > >>> Fix it. > >>> > >>> Reported-by: Poncho <poncho@spahan.ch> > >>> Link: https://lore.kernel.org/linux-media/468a36ec-c3ac-cb47-e12f-5906239ae3cd@spahan.ch/ > >>> Fixes: 716c330433e3 ("media: uvcvideo: Use standard names for menus") > >>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >>> --- > >>> This is untested. Poncho, would you be able to test this patch to see if > >>> it fixes your issue ? > >>> --- > >>> drivers/media/usb/uvc/uvc_v4l2.c | 2 +- > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>> > >>> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c > >>> index 5ac2a424b13d..f4988f03640a 100644 > >>> --- a/drivers/media/usb/uvc/uvc_v4l2.c > >>> +++ b/drivers/media/usb/uvc/uvc_v4l2.c > >>> @@ -45,7 +45,7 @@ static int uvc_control_add_xu_mapping(struct uvc_video_chain *chain, > >>> map->menu_names = NULL; > >>> map->menu_mapping = NULL; > >>> > >>> - map->menu_mask = BIT_MASK(xmap->menu_count); > >>> + map->menu_mask = GENMASK(xmap->menu_count - 1, 0); > >>> > >>> size = xmap->menu_count * sizeof(*map->menu_mapping); > >>> map->menu_mapping = kzalloc(size, GFP_KERNEL);
On 24.06.23 12:40, Laurent Pinchart wrote: > On Sat, Jun 24, 2023 at 11:07:14AM +0200, Thorsten Leemhuis wrote: >> On 24.06.23 02:25, Laurent Pinchart wrote: >>> On Wed, Jun 21, 2023 at 05:26:39PM +0200, Poncho wrote: >>>> Friendly ping. I think this patch was forgotten. >>> Thanks for the reminder. I'll send a pull request for v6.6. >> >> Why 6.6? If this would be a feature I'd fully understand. But this >> afaics fixes a regression and thus should be handled as one, even if >> it's a regression from a earlier release. For details see this recent >> mail from Linus: >> >> https://lore.kernel.org/all/CAHk-=wis_qQy4oDNynNKi5b7Qhosmxtoj1jxo5wmB6SRUwQUBQ@mail.gmail.com/ > > Linus has decided to not pull from the media tree for the next kernel > release due to a set of mishaps, see > https://lore.kernel.org/linux-media/CAHk-=wgzU8_dGn0Yg+DyX7ammTkDUCyEJ4C=NvnHRhxKWC7Wpw@mail.gmail.com/. Yeah, I remember. > Re-reading the mail, fixes should be fine Yup, he even pulled some fixes during this cycle. > even if the issue has been > there for several releases. I'll send send a pull request for v6.5. Great, thx! >> Side note: as it's a regression it would be good if this commit had a >> "CC: <stable..." tag as well. > I've already added that to my tree. Thx, too! Ciao, Thorsten >> 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. >> >>>> On 06.06.23 19:11, Laurent Pinchart wrote: >>>>> When commit 716c330433e3 ("media: uvcvideo: Use standard names for >>>>> menus") reworked the handling of menu controls, it inadvertently >>>>> replaced a GENMASK(n - 1, 0) with a BIT_MASK(n). The latter isn't >>>>> equivalent to the former, which broke adding XU mappings from userspace. >>>>> Fix it. >>>>> >>>>> Reported-by: Poncho <poncho@spahan.ch> >>>>> Link: https://lore.kernel.org/linux-media/468a36ec-c3ac-cb47-e12f-5906239ae3cd@spahan.ch/ >>>>> Fixes: 716c330433e3 ("media: uvcvideo: Use standard names for menus") >>>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>>>> --- >>>>> This is untested. Poncho, would you be able to test this patch to see if >>>>> it fixes your issue ? >>>>> --- >>>>> drivers/media/usb/uvc/uvc_v4l2.c | 2 +- >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c >>>>> index 5ac2a424b13d..f4988f03640a 100644 >>>>> --- a/drivers/media/usb/uvc/uvc_v4l2.c >>>>> +++ b/drivers/media/usb/uvc/uvc_v4l2.c >>>>> @@ -45,7 +45,7 @@ static int uvc_control_add_xu_mapping(struct uvc_video_chain *chain, >>>>> map->menu_names = NULL; >>>>> map->menu_mapping = NULL; >>>>> >>>>> - map->menu_mask = BIT_MASK(xmap->menu_count); >>>>> + map->menu_mask = GENMASK(xmap->menu_count - 1, 0); >>>>> >>>>> size = xmap->menu_count * sizeof(*map->menu_mapping); >>>>> map->menu_mapping = kzalloc(size, GFP_KERNEL); >
diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c index 5ac2a424b13d..f4988f03640a 100644 --- a/drivers/media/usb/uvc/uvc_v4l2.c +++ b/drivers/media/usb/uvc/uvc_v4l2.c @@ -45,7 +45,7 @@ static int uvc_control_add_xu_mapping(struct uvc_video_chain *chain, map->menu_names = NULL; map->menu_mapping = NULL; - map->menu_mask = BIT_MASK(xmap->menu_count); + map->menu_mask = GENMASK(xmap->menu_count - 1, 0); size = xmap->menu_count * sizeof(*map->menu_mapping); map->menu_mapping = kzalloc(size, GFP_KERNEL);
When commit 716c330433e3 ("media: uvcvideo: Use standard names for menus") reworked the handling of menu controls, it inadvertently replaced a GENMASK(n - 1, 0) with a BIT_MASK(n). The latter isn't equivalent to the former, which broke adding XU mappings from userspace. Fix it. Reported-by: Poncho <poncho@spahan.ch> Link: https://lore.kernel.org/linux-media/468a36ec-c3ac-cb47-e12f-5906239ae3cd@spahan.ch/ Fixes: 716c330433e3 ("media: uvcvideo: Use standard names for menus") Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- This is untested. Poncho, would you be able to test this patch to see if it fixes your issue ? --- drivers/media/usb/uvc/uvc_v4l2.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)