Custom control mapping introduced a bug, where the filter function was applied to every single control.
Fix it so it is only applied to the matching controls.
Reported-by: Paul Menzen pmenzel@molgen.mpg.de Closes: https://lore.kernel.org/linux-media/518cd6b4-68a8-4895-b8fc-97d4dae1ddc4@mol... Cc: stable@vger.kernel.org Fixes: 8f4362a8d42b ("media: uvcvideo: Allow custom control mapping") Signed-off-by: Ricardo Ribalda ribalda@chromium.org --- Paul, could you check if this fixes your issue, thanks! --- drivers/media/usb/uvc/uvc_ctrl.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c index 0136df5732ba..06fede57bf36 100644 --- a/drivers/media/usb/uvc/uvc_ctrl.c +++ b/drivers/media/usb/uvc/uvc_ctrl.c @@ -2680,6 +2680,10 @@ static void uvc_ctrl_init_ctrl(struct uvc_video_chain *chain, for (i = 0; i < ARRAY_SIZE(uvc_ctrl_mappings); ++i) { const struct uvc_control_mapping *mapping = &uvc_ctrl_mappings[i];
+ if (!(uvc_entity_match_guid(ctrl->entity, mapping->entity) && + ctrl->info.selector == mapping->selector)) + continue; + /* Let the device provide a custom mapping. */ if (mapping->filter_mapping) { mapping = mapping->filter_mapping(chain, ctrl); @@ -2687,9 +2691,7 @@ static void uvc_ctrl_init_ctrl(struct uvc_video_chain *chain, continue; }
- if (uvc_entity_match_guid(ctrl->entity, mapping->entity) && - ctrl->info.selector == mapping->selector) - __uvc_ctrl_add_mapping(chain, ctrl, mapping); + __uvc_ctrl_add_mapping(chain, ctrl, mapping); } }
--- base-commit: 68a72104cbcf38ad16500216e213fa4eb21c4be2 change-id: 20240722-fix-filter-mapping-18477dc69048
Best regards,
Dear Ricardo,
Thank you very much for your patch.
Am 22.07.24 um 09:59 schrieb Ricardo Ribalda:
Custom control mapping introduced a bug, where the filter function was applied to every single control.
I’d paste the error messages, so the commit can be easily found.
Fix it so it is only applied to the matching controls.
Reported-by: Paul Menzen pmenzel@molgen.mpg.de Closes: https://lore.kernel.org/linux-media/518cd6b4-68a8-4895-b8fc-97d4dae1ddc4@mol... Cc: stable@vger.kernel.org Fixes: 8f4362a8d42b ("media: uvcvideo: Allow custom control mapping") Signed-off-by: Ricardo Ribalda ribalda@chromium.org
Paul, could you check if this fixes your issue, thanks!
$ b4 shazam https://lore.kernel.org/linux-media/20240722-fix-filter-mapping-v1-1-07cc9c6... $ git log --oneline -2 1391c45f04fb (HEAD -> master) media: uvcvideo: Fix custom control mapping probing 933069701c1b (origin/master, origin/HEAD) Merge tag '6.11-rc-smb3-server-fixes' of git://git.samba.org/ksmbd
Yes, it does fix it on the Dell XPS 13 9360:
$ sudo dmesg --level alert,crit,err,warn [ 0.293264] hpet_acpi_add: no address or irqs in _CRS [ 0.340614] i8042: Warning: Keylock active [ 0.352146] ENERGY_PERF_BIAS: Set to 'normal', was 'performance' [ 1.680610] device-mapper: core: CONFIG_IMA_DISABLE_HTABLE is disabled. Duplicate IMA measurements will not be recorded in the IMA log. [ 9.012306] wmi_bus wmi_bus-PNP0C14:01: [Firmware Bug]: WQBC data block query control method not found [ 9.955744] Bluetooth: hci0: HCI Enhanced Setup Synchronous Connection command is advertised, but not supported.
drivers/media/usb/uvc/uvc_ctrl.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c index 0136df5732ba..06fede57bf36 100644 --- a/drivers/media/usb/uvc/uvc_ctrl.c +++ b/drivers/media/usb/uvc/uvc_ctrl.c @@ -2680,6 +2680,10 @@ static void uvc_ctrl_init_ctrl(struct uvc_video_chain *chain, for (i = 0; i < ARRAY_SIZE(uvc_ctrl_mappings); ++i) { const struct uvc_control_mapping *mapping = &uvc_ctrl_mappings[i];
if (!(uvc_entity_match_guid(ctrl->entity, mapping->entity) &&ctrl->info.selector == mapping->selector))continue;- /* Let the device provide a custom mapping. */ if (mapping->filter_mapping) { mapping = mapping->filter_mapping(chain, ctrl);
@@ -2687,9 +2691,7 @@ static void uvc_ctrl_init_ctrl(struct uvc_video_chain *chain, continue; }
if (uvc_entity_match_guid(ctrl->entity, mapping->entity) &&ctrl->info.selector == mapping->selector)__uvc_ctrl_add_mapping(chain, ctrl, mapping);
__uvc_ctrl_add_mapping(chain, ctrl, mapping);
So the check was just too late. Thank you for fixing it.
Tested-by: Paul Menzel pmenzel@molgen.mpg.de
Kind regards,
Paul
Hi Ricardo,
Thank you for the patch.
On Mon, Jul 22, 2024 at 07:59:43AM +0000, Ricardo Ribalda wrote:
Custom control mapping introduced a bug, where the filter function was applied to every single control.
Fix it so it is only applied to the matching controls.
Reported-by: Paul Menzen pmenzel@molgen.mpg.de Closes: https://lore.kernel.org/linux-media/518cd6b4-68a8-4895-b8fc-97d4dae1ddc4@mol... Cc: stable@vger.kernel.org Fixes: 8f4362a8d42b ("media: uvcvideo: Allow custom control mapping") Signed-off-by: Ricardo Ribalda ribalda@chromium.org
Paul, could you check if this fixes your issue, thanks!
drivers/media/usb/uvc/uvc_ctrl.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c index 0136df5732ba..06fede57bf36 100644 --- a/drivers/media/usb/uvc/uvc_ctrl.c +++ b/drivers/media/usb/uvc/uvc_ctrl.c @@ -2680,6 +2680,10 @@ static void uvc_ctrl_init_ctrl(struct uvc_video_chain *chain, for (i = 0; i < ARRAY_SIZE(uvc_ctrl_mappings); ++i) { const struct uvc_control_mapping *mapping = &uvc_ctrl_mappings[i];
if (!(uvc_entity_match_guid(ctrl->entity, mapping->entity) &&ctrl->info.selector == mapping->selector))continue;
I have a slight preference for
if (!uvc_entity_match_guid(ctrl->entity, mapping->entity) || ctrl->info.selector != mapping->selector) continue;
If that's fine with you, I can handle that when applying the patch.
This change means that the entity and selector test will use the original mapping, not the mapping returned by the filtering function. I think that's fine, both mappings should have the same entity and selector, only the menu mask is meant to change.
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
- /* Let the device provide a custom mapping. */ if (mapping->filter_mapping) { mapping = mapping->filter_mapping(chain, ctrl);
@@ -2687,9 +2691,7 @@ static void uvc_ctrl_init_ctrl(struct uvc_video_chain *chain, continue; }
if (uvc_entity_match_guid(ctrl->entity, mapping->entity) &&ctrl->info.selector == mapping->selector)__uvc_ctrl_add_mapping(chain, ctrl, mapping);
__uvc_ctrl_add_mapping(chain, ctrl, mapping);}
base-commit: 68a72104cbcf38ad16500216e213fa4eb21c4be2 change-id: 20240722-fix-filter-mapping-18477dc69048
Hi Laurent
On Mon, 22 Jul 2024 at 13:39, Laurent Pinchart laurent.pinchart@ideasonboard.com wrote:
Hi Ricardo,
Thank you for the patch.
On Mon, Jul 22, 2024 at 07:59:43AM +0000, Ricardo Ribalda wrote:
Custom control mapping introduced a bug, where the filter function was applied to every single control.
Fix it so it is only applied to the matching controls.
Reported-by: Paul Menzen pmenzel@molgen.mpg.de Closes: https://lore.kernel.org/linux-media/518cd6b4-68a8-4895-b8fc-97d4dae1ddc4@mol... Cc: stable@vger.kernel.org Fixes: 8f4362a8d42b ("media: uvcvideo: Allow custom control mapping") Signed-off-by: Ricardo Ribalda ribalda@chromium.org
Paul, could you check if this fixes your issue, thanks!
drivers/media/usb/uvc/uvc_ctrl.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c index 0136df5732ba..06fede57bf36 100644 --- a/drivers/media/usb/uvc/uvc_ctrl.c +++ b/drivers/media/usb/uvc/uvc_ctrl.c @@ -2680,6 +2680,10 @@ static void uvc_ctrl_init_ctrl(struct uvc_video_chain *chain, for (i = 0; i < ARRAY_SIZE(uvc_ctrl_mappings); ++i) { const struct uvc_control_mapping *mapping = &uvc_ctrl_mappings[i];
if (!(uvc_entity_match_guid(ctrl->entity, mapping->entity) &&ctrl->info.selector == mapping->selector))continue;I have a slight preference for
if (!uvc_entity_match_guid(ctrl->entity, mapping->entity) || ctrl->info.selector != mapping->selector) continue;If that's fine with you, I can handle that when applying the patch.
That looks also good. I can send a v2 if you prefer. I would also add the error messages to the commit message. Let me know what do you prefer, I have time today
Thanks!
This change means that the entity and selector test will use the original mapping, not the mapping returned by the filtering function. I think that's fine, both mappings should have the same entity and selector, only the menu mask is meant to change.
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
/* Let the device provide a custom mapping. */ if (mapping->filter_mapping) { mapping = mapping->filter_mapping(chain, ctrl);@@ -2687,9 +2691,7 @@ static void uvc_ctrl_init_ctrl(struct uvc_video_chain *chain, continue; }
if (uvc_entity_match_guid(ctrl->entity, mapping->entity) &&ctrl->info.selector == mapping->selector)__uvc_ctrl_add_mapping(chain, ctrl, mapping);
__uvc_ctrl_add_mapping(chain, ctrl, mapping); }}
base-commit: 68a72104cbcf38ad16500216e213fa4eb21c4be2 change-id: 20240722-fix-filter-mapping-18477dc69048
-- Regards,
Laurent Pinchart
On Mon, Jul 22, 2024 at 01:41:14PM +0200, Ricardo Ribalda wrote:
On Mon, 22 Jul 2024 at 13:39, Laurent Pinchart wrote:
On Mon, Jul 22, 2024 at 07:59:43AM +0000, Ricardo Ribalda wrote:
Custom control mapping introduced a bug, where the filter function was applied to every single control.
Fix it so it is only applied to the matching controls.
Reported-by: Paul Menzen pmenzel@molgen.mpg.de Closes: https://lore.kernel.org/linux-media/518cd6b4-68a8-4895-b8fc-97d4dae1ddc4@mol... Cc: stable@vger.kernel.org Fixes: 8f4362a8d42b ("media: uvcvideo: Allow custom control mapping") Signed-off-by: Ricardo Ribalda ribalda@chromium.org
Paul, could you check if this fixes your issue, thanks!
drivers/media/usb/uvc/uvc_ctrl.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c index 0136df5732ba..06fede57bf36 100644 --- a/drivers/media/usb/uvc/uvc_ctrl.c +++ b/drivers/media/usb/uvc/uvc_ctrl.c @@ -2680,6 +2680,10 @@ static void uvc_ctrl_init_ctrl(struct uvc_video_chain *chain, for (i = 0; i < ARRAY_SIZE(uvc_ctrl_mappings); ++i) { const struct uvc_control_mapping *mapping = &uvc_ctrl_mappings[i];
if (!(uvc_entity_match_guid(ctrl->entity, mapping->entity) &&ctrl->info.selector == mapping->selector))continue;I have a slight preference for
if (!uvc_entity_match_guid(ctrl->entity, mapping->entity) || ctrl->info.selector != mapping->selector) continue;If that's fine with you, I can handle that when applying the patch.
That looks also good. I can send a v2 if you prefer. I would also add the error messages to the commit message. Let me know what do you prefer, I have time today
If you can send a v2 with an improved commit message that would be nice. I'll apply it right away.
This change means that the entity and selector test will use the original mapping, not the mapping returned by the filtering function. I think that's fine, both mappings should have the same entity and selector, only the menu mask is meant to change.
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
/* Let the device provide a custom mapping. */ if (mapping->filter_mapping) { mapping = mapping->filter_mapping(chain, ctrl);@@ -2687,9 +2691,7 @@ static void uvc_ctrl_init_ctrl(struct uvc_video_chain *chain, continue; }
if (uvc_entity_match_guid(ctrl->entity, mapping->entity) &&ctrl->info.selector == mapping->selector)__uvc_ctrl_add_mapping(chain, ctrl, mapping);
__uvc_ctrl_add_mapping(chain, ctrl, mapping); }}
base-commit: 68a72104cbcf38ad16500216e213fa4eb21c4be2 change-id: 20240722-fix-filter-mapping-18477dc69048
linux-stable-mirror@lists.linaro.org
-
Laurent Pinchart -
Paul Menzel -
Ricardo Ribalda