| Message ID | 20200724114823.108237-1-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: uvcvideo: Fix uvc_ctrl_fixup_xu_info() not having any effect | expand |
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?
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; > } > }
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; >> } >> } >
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; } }
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(-)