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: <aLSntMknSv3lMarZ@ewhac.org>
Date: Sun, 31 Aug 2025 12:51:16 -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 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.

> g13_keys_for_bits_js[] is contiguous so no need
> for this if (g13_keys_for_bits_js[i]) test.
> 
> This will also allow you to drop the {} from the for loop.
>
	Noted.

	I've been mucking around with Rust a bit lately (and badly indented
JavaScript rather more), and am now in the habit putting braces around
single-line `if` clauses.

> Besides from my few small remarks this looks good to me,
> feel free to add:
> 
> Reviewed-by: Hans de Goede <hansg@...nel.org>
> 
> to the next version.
>
	Thank you very kindly, sir!

					Schwab