* [PATCH] media: uvcvideo: Fix uvc_ctrl_fixup_xu_info() not having any effect @ 2020-07-24 11:48 Hans de Goede 2020-07-27 21:24 ` Sasha Levin 2020-07-27 22:25 ` Laurent Pinchart 0 siblings, 2 replies; 4+ messages in thread From: Hans de Goede @ 2020-07-24 11:48 UTC (permalink / raw) To: Laurent Pinchart, Mauro Carvalho Chehab Cc: Hans de Goede, linux-media, stable uvc_ctrl_add_info() calls uvc_ctrl_get_flags() which will override the fixed-up flags set by uvc_ctrl_fixup_xu_info(). This commit fixes this by adding a is_xu argument to uvc_ctrl_add_info() and skipping the uvc_ctrl_get_flags() call for xu ctrls. Note that the xu path has already called uvc_ctrl_get_flags() from uvc_ctrl_fill_xu_info() before calling uvc_ctrl_add_info(). This fixes the xu motor controls not working properly on a Logitech 046d:08cc, and presumably also on the other Logitech models which have a quirk for this in the uvc_ctrl_fixup_xu_info() function. Cc: stable@vger.kernel.org Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/media/usb/uvc/uvc_ctrl.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c index e399b9fad757..4bdea5814d6a 100644 --- a/drivers/media/usb/uvc/uvc_ctrl.c +++ b/drivers/media/usb/uvc/uvc_ctrl.c @@ -1815,7 +1815,7 @@ static int uvc_ctrl_fill_xu_info(struct uvc_device *dev, } static int uvc_ctrl_add_info(struct uvc_device *dev, struct uvc_control *ctrl, - const struct uvc_control_info *info); + const struct uvc_control_info *info, bool is_xu); static int uvc_ctrl_init_xu_ctrl(struct uvc_device *dev, struct uvc_control *ctrl) @@ -1830,7 +1830,7 @@ static int uvc_ctrl_init_xu_ctrl(struct uvc_device *dev, if (ret < 0) return ret; - ret = uvc_ctrl_add_info(dev, ctrl, &info); + ret = uvc_ctrl_add_info(dev, ctrl, &info, true); if (ret < 0) uvc_trace(UVC_TRACE_CONTROL, "Failed to initialize control " "%pUl/%u on device %s entity %u\n", info.entity, @@ -2009,7 +2009,7 @@ int uvc_ctrl_restore_values(struct uvc_device *dev) * Add control information to a given control. */ static int uvc_ctrl_add_info(struct uvc_device *dev, struct uvc_control *ctrl, - const struct uvc_control_info *info) + const struct uvc_control_info *info, bool is_xu) { int ret = 0; @@ -2029,7 +2029,8 @@ static int uvc_ctrl_add_info(struct uvc_device *dev, struct uvc_control *ctrl, * default flag values from the uvc_ctrl array when the device doesn't * properly implement GET_INFO on standard controls. */ - uvc_ctrl_get_flags(dev, ctrl, &ctrl->info); + if (!is_xu) + uvc_ctrl_get_flags(dev, ctrl, &ctrl->info); ctrl->initialized = 1; @@ -2252,7 +2253,7 @@ static void uvc_ctrl_init_ctrl(struct uvc_device *dev, struct uvc_control *ctrl) for (; info < iend; ++info) { if (uvc_entity_match_guid(ctrl->entity, info->entity) && ctrl->index == info->index) { - uvc_ctrl_add_info(dev, ctrl, info); + uvc_ctrl_add_info(dev, ctrl, info, false); break; } } -- 2.26.2 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] media: uvcvideo: Fix uvc_ctrl_fixup_xu_info() not having any effect 2020-07-24 11:48 [PATCH] media: uvcvideo: Fix uvc_ctrl_fixup_xu_info() not having any effect Hans de Goede @ 2020-07-27 21:24 ` Sasha Levin 2020-07-27 22:25 ` Laurent Pinchart 1 sibling, 0 replies; 4+ messages in thread From: Sasha Levin @ 2020-07-27 21:24 UTC (permalink / raw) To: Sasha Levin, Hans de Goede, Laurent Pinchart Cc: Hans de Goede, linux-media, stable, stable Hi [This is an automated email] This commit has been processed because it contains a -stable tag. The stable tag indicates that it's relevant for the following trees: all The bot has tested the following trees: v5.7.10, v5.4.53, v4.19.134, v4.14.189, v4.9.231, v4.4.231. v5.7.10: Build OK! v5.4.53: Build OK! v4.19.134: Build OK! v4.14.189: Failed to apply! Possible dependencies: 859086ae3636 ("media: uvcvideo: Apply flags from device to actual properties") v4.9.231: Failed to apply! Possible dependencies: 859086ae3636 ("media: uvcvideo: Apply flags from device to actual properties") v4.4.231: Failed to apply! Possible dependencies: 859086ae3636 ("media: uvcvideo: Apply flags from device to actual properties") NOTE: The patch will not be queued to stable trees until it is upstream. How should we proceed with this patch? -- Thanks Sasha ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] media: uvcvideo: Fix uvc_ctrl_fixup_xu_info() not having any effect 2020-07-24 11:48 [PATCH] media: uvcvideo: Fix uvc_ctrl_fixup_xu_info() not having any effect Hans de Goede 2020-07-27 21:24 ` Sasha Levin @ 2020-07-27 22:25 ` Laurent Pinchart 2020-07-28 9:38 ` Hans de Goede 1 sibling, 1 reply; 4+ messages in thread From: Laurent Pinchart @ 2020-07-27 22:25 UTC (permalink / raw) To: Hans de Goede; +Cc: Mauro Carvalho Chehab, linux-media, stable Hi Hans, Thank you for the patch. On Fri, Jul 24, 2020 at 01:48:23PM +0200, Hans de Goede wrote: > uvc_ctrl_add_info() calls uvc_ctrl_get_flags() which will override > the fixed-up flags set by uvc_ctrl_fixup_xu_info(). > > This commit fixes this by adding a is_xu argument to uvc_ctrl_add_info() > and skipping the uvc_ctrl_get_flags() call for xu ctrls. > > Note that the xu path has already called uvc_ctrl_get_flags() from > uvc_ctrl_fill_xu_info() before calling uvc_ctrl_add_info(). > > This fixes the xu motor controls not working properly on a Logitech > 046d:08cc, and presumably also on the other Logitech models which have > a quirk for this in the uvc_ctrl_fixup_xu_info() function. > > Cc: stable@vger.kernel.org > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/media/usb/uvc/uvc_ctrl.c | 11 ++++++----- > 1 file changed, 6 insertions(+), 5 deletions(-) > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c > index e399b9fad757..4bdea5814d6a 100644 > --- a/drivers/media/usb/uvc/uvc_ctrl.c > +++ b/drivers/media/usb/uvc/uvc_ctrl.c > @@ -1815,7 +1815,7 @@ static int uvc_ctrl_fill_xu_info(struct uvc_device *dev, > } > > static int uvc_ctrl_add_info(struct uvc_device *dev, struct uvc_control *ctrl, > - const struct uvc_control_info *info); > + const struct uvc_control_info *info, bool is_xu); > > static int uvc_ctrl_init_xu_ctrl(struct uvc_device *dev, > struct uvc_control *ctrl) > @@ -1830,7 +1830,7 @@ static int uvc_ctrl_init_xu_ctrl(struct uvc_device *dev, > if (ret < 0) > return ret; > > - ret = uvc_ctrl_add_info(dev, ctrl, &info); > + ret = uvc_ctrl_add_info(dev, ctrl, &info, true); > if (ret < 0) > uvc_trace(UVC_TRACE_CONTROL, "Failed to initialize control " > "%pUl/%u on device %s entity %u\n", info.entity, > @@ -2009,7 +2009,7 @@ int uvc_ctrl_restore_values(struct uvc_device *dev) > * Add control information to a given control. > */ > static int uvc_ctrl_add_info(struct uvc_device *dev, struct uvc_control *ctrl, > - const struct uvc_control_info *info) > + const struct uvc_control_info *info, bool is_xu) > { > int ret = 0; > > @@ -2029,7 +2029,8 @@ static int uvc_ctrl_add_info(struct uvc_device *dev, struct uvc_control *ctrl, > * default flag values from the uvc_ctrl array when the device doesn't > * properly implement GET_INFO on standard controls. > */ > - uvc_ctrl_get_flags(dev, ctrl, &ctrl->info); > + if (!is_xu) > + uvc_ctrl_get_flags(dev, ctrl, &ctrl->info); Would it make sense to instead move this line (and the above comment) to uvc_ctrl_init_ctrl(), right after the uvc_ctrl_add_info() call ? If you would prefer keeping it here, I think is_xu should be renamed to update_flags (with the true/false values switched) to make it clearer. It would then also add a comment to uvc_ctrl_init_xu_ctrl() right before the call to uvc_ctrl_add_info() to state that we don't update flags to avoid overwriting the value set by uvc_ctrl_fixup_xu_info() in uvc_ctrl_fill_xu_info(). What's your preference ? > > ctrl->initialized = 1; > > @@ -2252,7 +2253,7 @@ static void uvc_ctrl_init_ctrl(struct uvc_device *dev, struct uvc_control *ctrl) > for (; info < iend; ++info) { > if (uvc_entity_match_guid(ctrl->entity, info->entity) && > ctrl->index == info->index) { > - uvc_ctrl_add_info(dev, ctrl, info); > + uvc_ctrl_add_info(dev, ctrl, info, false); > break; > } > } -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] media: uvcvideo: Fix uvc_ctrl_fixup_xu_info() not having any effect 2020-07-27 22:25 ` Laurent Pinchart @ 2020-07-28 9:38 ` Hans de Goede 0 siblings, 0 replies; 4+ messages in thread From: Hans de Goede @ 2020-07-28 9:38 UTC (permalink / raw) To: Laurent Pinchart; +Cc: Mauro Carvalho Chehab, linux-media, stable Hi, On 7/28/20 12:25 AM, Laurent Pinchart wrote: > Hi Hans, > > Thank you for the patch. > > On Fri, Jul 24, 2020 at 01:48:23PM +0200, Hans de Goede wrote: >> uvc_ctrl_add_info() calls uvc_ctrl_get_flags() which will override >> the fixed-up flags set by uvc_ctrl_fixup_xu_info(). >> >> This commit fixes this by adding a is_xu argument to uvc_ctrl_add_info() >> and skipping the uvc_ctrl_get_flags() call for xu ctrls. >> >> Note that the xu path has already called uvc_ctrl_get_flags() from >> uvc_ctrl_fill_xu_info() before calling uvc_ctrl_add_info(). >> >> This fixes the xu motor controls not working properly on a Logitech >> 046d:08cc, and presumably also on the other Logitech models which have >> a quirk for this in the uvc_ctrl_fixup_xu_info() function. >> >> Cc: stable@vger.kernel.org >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> drivers/media/usb/uvc/uvc_ctrl.c | 11 ++++++----- >> 1 file changed, 6 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c >> index e399b9fad757..4bdea5814d6a 100644 >> --- a/drivers/media/usb/uvc/uvc_ctrl.c >> +++ b/drivers/media/usb/uvc/uvc_ctrl.c >> @@ -1815,7 +1815,7 @@ static int uvc_ctrl_fill_xu_info(struct uvc_device *dev, >> } >> >> static int uvc_ctrl_add_info(struct uvc_device *dev, struct uvc_control *ctrl, >> - const struct uvc_control_info *info); >> + const struct uvc_control_info *info, bool is_xu); >> >> static int uvc_ctrl_init_xu_ctrl(struct uvc_device *dev, >> struct uvc_control *ctrl) >> @@ -1830,7 +1830,7 @@ static int uvc_ctrl_init_xu_ctrl(struct uvc_device *dev, >> if (ret < 0) >> return ret; >> >> - ret = uvc_ctrl_add_info(dev, ctrl, &info); >> + ret = uvc_ctrl_add_info(dev, ctrl, &info, true); >> if (ret < 0) >> uvc_trace(UVC_TRACE_CONTROL, "Failed to initialize control " >> "%pUl/%u on device %s entity %u\n", info.entity, >> @@ -2009,7 +2009,7 @@ int uvc_ctrl_restore_values(struct uvc_device *dev) >> * Add control information to a given control. >> */ >> static int uvc_ctrl_add_info(struct uvc_device *dev, struct uvc_control *ctrl, >> - const struct uvc_control_info *info) >> + const struct uvc_control_info *info, bool is_xu) >> { >> int ret = 0; >> >> @@ -2029,7 +2029,8 @@ static int uvc_ctrl_add_info(struct uvc_device *dev, struct uvc_control *ctrl, >> * default flag values from the uvc_ctrl array when the device doesn't >> * properly implement GET_INFO on standard controls. >> */ >> - uvc_ctrl_get_flags(dev, ctrl, &ctrl->info); >> + if (!is_xu) >> + uvc_ctrl_get_flags(dev, ctrl, &ctrl->info); > > Would it make sense to instead move this line (and the above comment) to > uvc_ctrl_init_ctrl(), right after the uvc_ctrl_add_info() call ? I was thinking about the same lines, since that seems like the obvious / logical fix. I was thinking about doing it before the uvc_ctrl_add_info() call, but that will not work, because info is const before the call. Doing it after the add_info() call, as you suggest, we can pass &ctrl->info to the get_flags call. So yes that will work and is the logical thing to do. I'll spin a v2 with this change. > If you > would prefer keeping it here, I think is_xu should be renamed to > update_flags (with the true/false values switched) to make it clearer. > It would then also add a comment to uvc_ctrl_init_xu_ctrl() right before > the call to uvc_ctrl_add_info() to state that we don't update flags to > avoid overwriting the value set by uvc_ctrl_fixup_xu_info() in > uvc_ctrl_fill_xu_info(). > > What's your preference ? See above. Regards, Hans > >> >> ctrl->initialized = 1; >> >> @@ -2252,7 +2253,7 @@ static void uvc_ctrl_init_ctrl(struct uvc_device *dev, struct uvc_control *ctrl) >> for (; info < iend; ++info) { >> if (uvc_entity_match_guid(ctrl->entity, info->entity) && >> ctrl->index == info->index) { >> - uvc_ctrl_add_info(dev, ctrl, info); >> + uvc_ctrl_add_info(dev, ctrl, info, false); >> break; >> } >> } > ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-07-28 9:38 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-07-24 11:48 [PATCH] media: uvcvideo: Fix uvc_ctrl_fixup_xu_info() not having any effect Hans de Goede 2020-07-27 21:24 ` Sasha Levin 2020-07-27 22:25 ` Laurent Pinchart 2020-07-28 9:38 ` Hans de Goede
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).