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] [day] [month] [year] [list]

Message-ID: <c12adb45-fa6d-4bb8-afd2-a02e3026d646@kernel.org>
Date: Mon, 8 Sep 2025 23:08:29 +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 3-Sep-25 9:39 PM, Leo L. Schwab wrote:
> 	For some reason, your replies aren't making it to me directly -- I
> had to find and scrape your reply off the LKML web site:
> 
> On Tue, 2 Sep 2025 23:05:06 +0200, Hans de Goede wrote:
>> On 2-Sep-25 22:41, Leo L. Schwab wrote:
>>> 	This does not happen.  The G13 accepts and remembers backlight color
>>> settings even when the LEDs have been toggled off locally.
>>> [ ... ]
>>
>> I see, interesting.
>>
>> So what happens if you turn off the backlight with the toggle button on the G13
>> and then write 0 to brightness in sysfs and then press the toggle button again?
>>
> 	It's a little difficult to see, but the backlight turns back on with
> minimal brightness.  To my eye, it looks like it's displaying #000001.

Ok.

>> Right it does seem that using cdev.brightness_hw_changed is valid in
>> this case.
>>
>> But the LED API is supposed to have the brightness attribute present
>> the actual current brightness of the device.
>>
>> I'm not sure how upower will react if the poll() on brightness_hw_changed
>> wakes upower up and then the reported brightness is unchanged...
>>
>> I need to think about this a bit and check the upower code, let me
>> get back to you on this in a day or 2 ...
>>
> 	Certainly.

Thank you for waiting. After looking at the upower code + running some
tests with a G510 I think that your hw_brightness_changed support
is pretty good as is.

There are 2 improvements which I would like to see:

1. When the backlight is turned on through the button, you
should pass g15_led->brightness to the notify() call rather
then LED_FULL. GNOME will show an OSD with the new brightness
value shown as a mini progress bar similar to how it shows
speaker volume when doing mute/unmute. This mini progress
bar should show the actual brightness being restored, not
always full brightness.

2. ATM if the backlight is turned off on the G13 when
the driver loads and then one of the buttons gets pressed
then a notify() will happen because the led_cdev.hw_brightness_changed
value of -1 will be different from the value of 0 in the
input-report. This notify will lead to an unwanted OSD
notification in GNOME, so this needs to be fixed.
IMHO the best fix would be to use:

	hid_hw_raw_request(..., HID_INPUT_REPORT, HID_REQ_GET_REPORT);

at probe to get the input-report so that the driver will
actually now the backlight state at probe() time without
needing to wait for the first time the input-report is send.

You have inspired me to add hw_brightness_changed support
to the G510 code, see the attached patch. This patch can
also be used as an example how to get the input report
on the G13 during probe().

Note this also adds a variable at the driver level to
track the backlight state also fixing the compile issue
you hit without needing to use #ifdef-ery.

I'll wait for your G13 support to land first and then
rebase the G510 patch on top.

Regards,

Hans

View attachment "0001-HID-hid-lg-g15-Add-hw_brightness_changed-support-for.patch" of type "text/x-patch" (3783 bytes)