[PATCH 0/1] ACPI: video: Fix acpi_video_handles_brightness_key_presses()

[PATCH 0/1] ACPI: video: Fix acpi_video_handles_brightness_key_presses()

[Hans de Goede]

Hi Rafael,

Commit 3a0cf7ab8df3 ("ACPI: video: Change how we determine if brightness
key-presses are handled") which fixes not sending any brightness
key-presses at all on some Panasonic models, because of
acpi_video_handles_brightness_key_presses() returning true while it
should not, is causing duplicate brightness key presses on some
(older) Dell models.

This patch fixes this. As mentioned in the commit message a down-side
of the fix is that the first brightness key-press on affected Dell models
might still get reported twice. The alternative would be DMI based quirks
and IMHO this generic fix is better.

Since I send the original commit to Linus through the pdx86 tree
as part of a panasonic-laptop driver fixes series, and since users
are reporting regressions because of the original fix now, I plan to
include this fix to the fix in my next fixes pull-req to Linus
this Friday.

If you have any objections against this fix please let me know
before Friday.

Regards,

Hans


Hans de Goede (1):
  ACPI: video: Fix acpi_video_handles_brightness_key_presses()

 drivers/acpi/acpi_video.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

-- 
2.36.0

[PATCH] ACPI: video: Fix acpi_video_handles_brightness_key_presses()

[Hans de Goede]

Commit 3a0cf7ab8df3 ("ACPI: video: Change how we determine if brightness
key-presses are handled") made acpi_video_handles_brightness_key_presses()
report false when none of the ACPI Video Devices support backlight control.

But it turns out that at least on a Dell Inspiron N4010 there is no ACPI
backlight control, yet brightness hotkeys are still reported through
the ACPI Video Bus; and since acpi_video_handles_brightness_key_presses()
now returns false, brightness keypresses are now reported twice.

To fix this rename the has_backlight flag to may_report_brightness_keys and
also set it the first time a brightness key press event is received.

Depending on the delivery of the other ACPI (WMI) event vs the ACPI Video
Bus event this means that the first brightness key press might still get
reported twice, but all further keypresses will be filtered as before.

Note that this relies on other drivers reporting brightness key events
calling acpi_video_handles_brightness_key_presses() when delivering
the events (rather then once during driver probe). This is already
required and documented in include/acpi/video.h:

/*
 * Note: The value returned by acpi_video_handles_brightness_key_presses()
 * may change over time and should not be cached.
 */

Fixes: 3a0cf7ab8df3 ("ACPI: video: Change how we determine if brightness key-presses are handled")
Link: https://lore.kernel.org/regressions/CALF=6jEe5G8+r1Wo0vvz4GjNQQhdkLT5p8uCHn6ZXhg4nsOWow@mail.gmail.com/
Reported-by: Ben Greening <bgreening@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/acpi/acpi_video.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
index 43177c20ce4f..eaea733b368a 100644
--- a/drivers/acpi/acpi_video.c
+++ b/drivers/acpi/acpi_video.c
@@ -73,7 +73,7 @@ module_param(device_id_scheme, bool, 0444);
 static int only_lcd = -1;
 module_param(only_lcd, int, 0444);
 
-static bool has_backlight;
+static bool may_report_brightness_keys;
 static int register_count;
 static DEFINE_MUTEX(register_count_mutex);
 static DEFINE_MUTEX(video_list_lock);
@@ -1224,7 +1224,7 @@ acpi_video_bus_get_one_device(struct acpi_device *device,
 	acpi_video_device_find_cap(data);
 
 	if (data->cap._BCM && data->cap._BCL)
-		has_backlight = true;
+		may_report_brightness_keys = true;
 
 	mutex_lock(&video->device_list_lock);
 	list_add_tail(&data->entry, &video->video_device_list);
@@ -1693,6 +1693,9 @@ static void acpi_video_device_notify(acpi_handle handle, u32 event, void *data)
 		break;
 	}
 
+	if (keycode)
+		may_report_brightness_keys = true;
+
 	acpi_notifier_call_chain(device, event, 0);
 
 	if (keycode && (report_key_events & REPORT_BRIGHTNESS_KEY_EVENTS)) {
@@ -2253,7 +2256,7 @@ void acpi_video_unregister(void)
 	if (register_count) {
 		acpi_bus_unregister_driver(&acpi_video_bus);
 		register_count = 0;
-		has_backlight = false;
+		may_report_brightness_keys = false;
 	}
 	mutex_unlock(&register_count_mutex);
 }
@@ -2275,7 +2278,7 @@ void acpi_video_unregister_backlight(void)
 
 bool acpi_video_handles_brightness_key_presses(void)
 {
-	return has_backlight &&
+	return may_report_brightness_keys &&
 	       (report_key_events & REPORT_BRIGHTNESS_KEY_EVENTS);
 }
 EXPORT_SYMBOL(acpi_video_handles_brightness_key_presses);
-- 
2.36.0

Re: [PATCH] ACPI: video: Fix acpi_video_handles_brightness_key_presses()

[Rafael J. Wysocki]

On Wed, Jul 13, 2022 at 11:11 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Commit 3a0cf7ab8df3 ("ACPI: video: Change how we determine if brightness
> key-presses are handled") made acpi_video_handles_brightness_key_presses()
> report false when none of the ACPI Video Devices support backlight control.
>
> But it turns out that at least on a Dell Inspiron N4010 there is no ACPI
> backlight control, yet brightness hotkeys are still reported through
> the ACPI Video Bus; and since acpi_video_handles_brightness_key_presses()
> now returns false, brightness keypresses are now reported twice.
>
> To fix this rename the has_backlight flag to may_report_brightness_keys and
> also set it the first time a brightness key press event is received.
>
> Depending on the delivery of the other ACPI (WMI) event vs the ACPI Video
> Bus event this means that the first brightness key press might still get
> reported twice, but all further keypresses will be filtered as before.
>
> Note that this relies on other drivers reporting brightness key events
> calling acpi_video_handles_brightness_key_presses() when delivering
> the events (rather then once during driver probe). This is already
> required and documented in include/acpi/video.h:
>
> /*
>  * Note: The value returned by acpi_video_handles_brightness_key_presses()
>  * may change over time and should not be cached.
>  */
>
> Fixes: 3a0cf7ab8df3 ("ACPI: video: Change how we determine if brightness key-presses are handled")
> Link: https://lore.kernel.org/regressions/CALF=6jEe5G8+r1Wo0vvz4GjNQQhdkLT5p8uCHn6ZXhg4nsOWow@mail.gmail.com/
> Reported-by: Ben Greening <bgreening@gmail.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> ---
>  drivers/acpi/acpi_video.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
> index 43177c20ce4f..eaea733b368a 100644
> --- a/drivers/acpi/acpi_video.c
> +++ b/drivers/acpi/acpi_video.c
> @@ -73,7 +73,7 @@ module_param(device_id_scheme, bool, 0444);
>  static int only_lcd = -1;
>  module_param(only_lcd, int, 0444);
>
> -static bool has_backlight;
> +static bool may_report_brightness_keys;
>  static int register_count;
>  static DEFINE_MUTEX(register_count_mutex);
>  static DEFINE_MUTEX(video_list_lock);
> @@ -1224,7 +1224,7 @@ acpi_video_bus_get_one_device(struct acpi_device *device,
>         acpi_video_device_find_cap(data);
>
>         if (data->cap._BCM && data->cap._BCL)
> -               has_backlight = true;
> +               may_report_brightness_keys = true;
>
>         mutex_lock(&video->device_list_lock);
>         list_add_tail(&data->entry, &video->video_device_list);
> @@ -1693,6 +1693,9 @@ static void acpi_video_device_notify(acpi_handle handle, u32 event, void *data)
>                 break;
>         }
>
> +       if (keycode)
> +               may_report_brightness_keys = true;
> +
>         acpi_notifier_call_chain(device, event, 0);
>
>         if (keycode && (report_key_events & REPORT_BRIGHTNESS_KEY_EVENTS)) {
> @@ -2253,7 +2256,7 @@ void acpi_video_unregister(void)
>         if (register_count) {
>                 acpi_bus_unregister_driver(&acpi_video_bus);
>                 register_count = 0;
> -               has_backlight = false;
> +               may_report_brightness_keys = false;
>         }
>         mutex_unlock(&register_count_mutex);
>  }
> @@ -2275,7 +2278,7 @@ void acpi_video_unregister_backlight(void)
>
>  bool acpi_video_handles_brightness_key_presses(void)
>  {
> -       return has_backlight &&
> +       return may_report_brightness_keys &&
>                (report_key_events & REPORT_BRIGHTNESS_KEY_EVENTS);
>  }
>  EXPORT_SYMBOL(acpi_video_handles_brightness_key_presses);
> --
> 2.36.0
>