linux-kernel - Re: [PATCH v3] HID: lg-g15 - Add support for Logitech G13.

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: <8ae2cc92-5dfe-466d-95fd-da74309d7244@kernel.org>
Date: Tue, 2 Sep 2025 11:07:09 +0200
From: Hans de Goede <hansg@...nel.org>
To: "Leo L. Schwab" <ewhac@...ac.org>
Cc: Kate Hsuan <hpa@...hat.com>, Jiri Kosina <jikos@...nel.org>,
 Benjamin Tissoires <bentiss@...nel.org>, linux-input@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] HID: lg-g15 - Add support for Logitech G13.

Hi Leo,

On 31-Aug-25 9:51 PM, Leo L. Schwab wrote:
> On Sun, Aug 31, 2025 at 03:01:12PM +0200, Hans de Goede wrote:
>>> +static const u16 g13_keys_for_bits_js[] = {
>>> +	/* Joystick buttons */
>>> +	/* These keybits are at bit indices 33, 34, and 35. */
>>> +	BTN_BASE,	/* Left side */
>>> +	BTN_BASE2,	/* Bottom side */
>>> +	BTN_THUMB,	/* Stick depress */
>>> +};
>>
>> You are using this 33 offset hardcoded below, maybe
>> at a #define for this, e.g. :
>>
>> #define G13_JS_KEYBITS_OFFSET	33
>>
> 	Noted.
> 
>> g13_keys_for_bits_js[] is contiguous so no need
>> for this if (g13_keys_for_bits_js[i]) test.
>>
> 	Noted.
> 
>>> +	if (IS_ENABLED(CONFIG_LEDS_BRIGHTNESS_HW_CHANGED)) {
>>
>> I do not believe that this IS_ENABLED(CONFIG_LEDS_BRIGHTNESS_HW_CHANGED)
>> is necessary, led_classdev_notify_brightness_hw_changed() has a static
>> inline replacement when CONFIG_LEDS_BRIGHTNESS_HW_CHANGED is not set,
>> so you can just call it unconditionally.
>>
>> This is already called unconditionally in other places of the code.
>>
> 	I was actually bit by this in the first two revs by the build bot.
> If CONFIG_LEDS_BRIGHTNESS_HW_CHANGED is not enabled, the field
> `cdev.brightness_hw_changed`, referenced a bit further down, does not exist,
> and causes a build failure.
> 
> 	My first attempt at #ifdef-ing around it led to another build bot
> warning about `hw_brightness_changed` being defined but not used.  Then I
> leanred about `IS_ENABLED()`, which is evidently now preferred over `#ifdef
> CONFIG_`, and nicely isolated the whole block, so I went with that.

Ah I see. Yes if you do need to do a CONFIG check then using IS_ENABLED()
is good.

But I'm afraid that the underlying problem here is the use of
cdev.brightness_hw_changed this is really only meant for led-class.c
internal use.

The idea of cdev.brightness_hw_changed is that it stores the last
value set by the hw.

But in the mean time that value may have been overwritten by software.

I think that you will fail to call led_classdev_notify_brightness_hw_changed()
(you can add a debug print to check) if the following happens:

1. Brightness set to 255 (RGB 255,255,255) through sysfs
2. State toggled to off by backlight control button, brightness is now 0
3. Brightness set to 255 (RGB 255,255,255) through sysfs
4. State toggled to off by backlight control button, brightness is now 0

In this case the second hw-button toggle will not call
led_classdev_notify_brightness_hw_changed(), since cdev.brightness_hw_changed
still has the 0 value from last time.

I also see that you use TEST_BIT(rep->keybits, 23) ? LED_FULL : LED_OFF
for the brightness value send to led_classdev_notify_brightness_hw_changed()
but I would expect the hw to restore the previous brightness on a toggle
from off -> on through the button? So then that should be send.

And you also never update cdev.brightness and use the cached 
struct lg_g15_led.brightness in lg_g13_kbd_led_get(). This means that
after a hw toggle of the backlight reading the brightness from sysfs
will show the wrong (old) value.

I think that instead what you need to do is create a
lg_g13_leds_changed_work() mirroring lg_g15_leds_changed_work()
but then only for the leds[0] instead of using the for loops.

Combined with caching the keybit 23 value (1) and then when
keybit 23 changes value queue the work.

This will also allow you to drop the:

	if (IS_ENABLED(CONFIG_LEDS_BRIGHTNESS_HW_CHANGED))

but that is just a side effect. The most important thing here
is to:

1. Correctly update the cached brightness after hitting the toggle button
2. Compare the new brightness against the previous cached brightness to
   determine if led_classdev_notify_brightness_hw_changed() should be called,
   rather then using the possible stale cdev.brightness_hw_changed
3. Pass the actual new brightness to
   led_classdev_notify_brightness_hw_changed() and not LED_FULL.

Note I see that lg_g13_get_leds_state() does 2 USB transfers,
when running from the work we only need to get the backlight
state and not the macro keys state. So either give it a parameter
whether it should update the macro-keys or not; or split it into
2 functions, calling both functions from 
lg_g15_get_initial_led_brightness() and only calling the one
for the backlight from lg_g13_leds_changed_work().

Regards,

Hans

1) You can just init the keybit23 cache at 1 since I think the bl always
starts on, eating the cost of possibly running the work one time too many
on the first key press if the bl was turned off before the driver probes.