linux-kernel - Re: [PATCH v3] platform/x86: samsung-galaxybook: Add samsung-galaxybook driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <a0fd8c4a-598a-4fec-a8c9-c1435f880f7a@gmx.de>
Date: Wed, 18 Dec 2024 19:37:10 +0100
From: Armin Wolf <W_Armin@....de>
To: Hans de Goede <hdegoede@...hat.com>,
 Joshua Grisham <josh@...huagrisham.com>, ilpo.jarvinen@...ux.intel.com,
 platform-driver-x86@...r.kernel.org, corbet@....net,
 linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
 rostedt@...dmis.org, mhiramat@...nel.org, mathieu.desnoyers@...icios.com,
 linux-trace-kernel@...r.kernel.org
Subject: Re: [PATCH v3] platform/x86: samsung-galaxybook: Add
 samsung-galaxybook driver

Am 18.12.24 um 15:24 schrieb Hans de Goede:

> Hi Armin,
>
> Thank you for reviewing this new driver.
>
> On 17-Dec-24 10:31 PM, Armin Wolf wrote:
>> Am 16.12.24 um 11:38 schrieb Joshua Grisham:
> <snip>
>
>>> +/*
>>> + * Hotkey work and filters
>>> + */
>>> +
>>> +static void galaxybook_kbd_backlight_hotkey_work(struct work_struct *work)
>>> +{
>>> +    struct samsung_galaxybook *galaxybook =
>>> +        container_of(work, struct samsung_galaxybook, kbd_backlight_hotkey_work);
>>> +
>>> +    if (galaxybook->kbd_backlight.brightness < galaxybook->kbd_backlight.max_brightness)
>>> +        kbd_backlight_acpi_set(galaxybook, galaxybook->kbd_backlight.brightness + 1);
>>> +    else
>>> +        kbd_backlight_acpi_set(galaxybook, 0);
>>> +
>>> +    led_classdev_notify_brightness_hw_changed(
>>> +        &galaxybook->kbd_backlight,
>>> +        galaxybook->kbd_backlight.brightness);
>> This is the exact reason why i think this should be done in userspace. You can replace this code
>> with a simple input event submission using the KBDILLUM* key codes. This would also allow you to
>> avoid any special locking in this case.
>>
>> This would also allow userspace to configure the step with of the brightness changes.
> As discussed in an earlier thread, there is really no good way to let
> userspace handle this atm. KEY_KBDILLUMTOGGLE gets mapped to XF86KbdLightOnOff
> while we really want Cycle, as we have in e.g. XF86MonBrightnessCycle.
>
> I just checked gnome-settings-daemon and it does handle XF86KbdLightOnOff
> but it only toggles on/off. In most laptops the cycle key is handled by
> the embedded controller and this simply emulates that common setup to
> match what userspace currently expects.
>
> Handling this in userspace would require adding a new KEY_KBDILLUMCYCLE
> and then adding support for that to xkb and maybe also upower and
> gnome-settings-daemon and KDE and then wait for that all to land before
> things start to work.
>
> As for your argument about allowing to set the percentage to step,
> note that this in kernel handling only increaeses the level by 1 on
> the hotkey press that make sense because typically these kbds backlights
> only have 2 - 4 levels.
>
> So IMHO handling this in the kernel is an acceptable compromise,
> (yes I do realize that this is a compromise).
>
> If some need arises later to do move this to userspace we can always
> add a module parameter to completely disable the in kernel handling and
> instead send out a new KEY_KBDILLUMCYCLE key-code when this now
> module parameter is set.
>
> Regards,
>
> Hans

I see, in this case we can indeed handle this inside the kernel.

However we should really use led_set_brightness_sync() here instead of manually updating the brightness.
Also we need a mutex here so that the whole read-modify-write brightness update is atomic.

Thanks,
Armin Wolf