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: <aMsQ7dBJrwpWbdJk@ewhac.org>
Date: Wed, 17 Sep 2025 12:50:05 -0700
From: "Leo L. Schwab" <ewhac@...ac.org>
To: Hans de Goede <hansg@...nel.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.

On Wed, Sep 17, 2025 at 12:33:36PM +0200, Hans de Goede wrote:
> On 16-Sep-25 00:18, Leo L. Schwab wrote:
> > 	What do you want to happen to `brightness_hw_changed` when
> > `brightness` is changed in sysfs while the backlight is on?  As it stands,
> > the current behavior is:
> > 	* Driver loads and probes; `brightness` and `brightness_hw_changed`
> > 	  both set to 255.
> 
> Ack, except that as mentioned above I would not touch brightness_hw_changed
> and just leave it at -1.
> 
> > 	* sysfs `brightness` changed to 128.  `brightness_hw_changed`
> > 	  remains at 255.
> > 	* Toggle backilght off.  `brightness_hw_changed` changed to 0.
> > 	  `brightness` remains at 128.
> > 	* Toggle backlight back on.  `brightness_hw_changed` gets a copy of
> > 	  `brightness`, and both are now 128.
> 
> Ack this is all correct.
>
	...Oy.

	Okay, I can give you that.

> > 	This seems inconsistent to me.
> 
> This is working as intended / how the API was designed as
> Documentation/ABI/testing/sysfs-class-led says:
> 
>                 Reading this file will return the last brightness level set
>                 by the hardware, this may be different from the current
>                 brightness. Reading this file when no hw brightness change
>                 event has happened will return an ENODATA error.
>
	First: Why isn't this mentioned in Documentation/leds/leds_class.rst?

	And second: This doesn't really clarify anything.  That paragraph
may be legitimately interpreted to mean that `brightness_hw_changed` is
completely independent of `brightness`, as it was in my original
implementation.

> >  Hence my earlier suggestion that
> > `brightness_hw_changed` should track all changes to `brightness`, except
> > when the backlight is toggled off.
> 
> Then it also would be reporting values coming from sysfs writes,
> which it explicitly should not do.
>
	Okay, fair, but having `brightness_hw_changed` read as 255, then
later as 128 after hitting the toggle button a couple of times strikes me as
inconsistent behavior.

> Summarizing in my view the following changes are necessary on v4:
> 
> 1. Add backlight_disabled (or backlight_enabled) flag to struct lg_g15_data
> 2. Init that flag from prope()
> 3. Use that flag on receiving input reports to see if notify()
>    should be called
> 4. Replace the LED_FULL passed to notify() (for off->on)
>    with g15_led->brightness
> 
	Will do; will post v6 shortly.  And someone should update the docs
describing the expected interaction between `brightness` and
`brightness_hw_changed`.

					Schwab