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

["Rafael J. Wysocki" <rafael@kernel.org>]

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
>