linux-kernel - Re: [PATCH v2] 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: <aJ5Gm5AaLI6iJ4le@ewhac.org>
Date: Thu, 14 Aug 2025 13:27:07 -0700
From: "Leo L. Schwab" <ewhac@...ac.org>
To: Kate Hsuan <hpa@...hat.com>
Cc: Hans de Goede <hansg@...nel.org>, Jiri Kosina <jikos@...nel.org>,
	Benjamin Tissoires <bentiss@...nel.org>,
	linux-input@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] HID: lg-g15 - Add support for Logitech G13.

On Thu, Aug 14, 2025 at 05:09:09PM +0800, Kate Hsuan wrote:
> Thank you for your work.
>
	Thank you for your feedback.  And thank you for collecting all your
comments into one post.

> On Tue, Aug 12, 2025 at 2:57 PM Leo L. Schwab <ewhac@...ac.org> wrote:
> The comment should in C comments, for example
>  struct input_dev *input_js;  /*joystick device for G13*/
>
	Will sweep all those up.

> > +static int lg_g13_kbd_led_set(struct led_classdev *led_cdev, enum led_brightness brightness)
> > +{
> > +       struct led_classdev_mc *mc = lcdev_to_mccdev(led_cdev);
> > +       struct lg_g15_led *g15_led =
> > +               container_of(mc, struct lg_g15_led, mcdev);
> > +       struct lg_g15_data *g15 = dev_get_drvdata(led_cdev->dev->parent);
> > +
> > +       /* Ignore LED off on unregister / keyboard unplug */
> > +       if (led_cdev->flags & LED_UNREGISTERING)
> > +               return 0;
> > +
> > +       guard(mutex)(&g15->mutex);
> guard() can be moved to lg_g13_kbd_led_write() to ensure the code is
> protected by a mutex lock when lg_g13_kbd_led_write() is called.
>
	I was mimicking the existing structure of the G15 and G510 code,
which I assumed was set up that way for a reason.  Will move this.

> > +static int lg_g13_event(struct lg_g15_data *g15, u8 const *data)
> > +{
> > +       struct g13_input_report const * const rep = (struct g13_input_report *) data;
> > +       int i, val;
> > +       bool hw_brightness_changed;
> Remove unused variable.
>
	I will be slightly restructuring this.

> >         switch (g15->model) {
> > +       case LG_G13:
> > +               /*
> > +                * Some usermode libraries tend to ignore devices that don't
> > +                * "look like" a joystick.  Create additional input device
> > +                * dedicated as joystick.
> > +                */
> Nit.
> Improve the comment and describe the hardware and the variable
> settings below in brief.

	I'll wordsmith this.  It'll get a bit wordier, though...

> Some style and comment style issues are pointed out, and I'll start to
> test this work after I receive my G13.
>
	If anything explodes, please let me know right away.

					Schwab