linux-kernel - Re: [PATCH] HID: logitech-hidpp: support Color LED feature (8071).

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: <CAO-hwJLW=UT6APsKKZaRHBvKn5GOe5xg+bLQH7TGy-PH8N4yUQ@mail.gmail.com>
Date:   Thu, 24 Mar 2022 20:54:29 +0100
From:   Benjamin Tissoires <benjamin.tissoires@...hat.com>
To:     Manuel Schönlaub <manuel.schoenlaub@...il.com>
Cc:     Filipe Laíns <lains@...eup.net>,
        Jiri Kosina <jikos@...nel.org>,
        "open list:HID CORE LAYER" <linux-input@...r.kernel.org>,
        lkml <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] HID: logitech-hidpp: support Color LED feature (8071).

On Thu, Mar 24, 2022 at 4:34 AM Manuel Schönlaub
<manuel.schoenlaub@...il.com> wrote:
>
> On Wed, Mar 23, 2022 at 09:22:49PM +0000, Filipe Laíns wrote:
> > On Tue, 2022-03-08 at 16:50 -0700, Manuel Schönlaub wrote:
> > > The HID++ protocol allows to set multicolor (RGB) to a static color.
> > > Multiple of such LED zones per device are supported.
> > > This patch exports said LEDs so that they can be set from userspace.
> > >
> > > Signed-off-by: Manuel Schönlaub <manuel.schoenlaub@...il.com>
> > > ---
> > >  drivers/hid/hid-logitech-hidpp.c | 188 +++++++++++++++++++++++++++++++
> > >  1 file changed, 188 insertions(+)
> >
> > *snip*
> >
> > Hi Manuel,
> >
> > Thanks for putting this forward, although I am not sure if this is the best way
> > to handle this.
> >
> > Before anything, could you elaborate a bit on what lead to you wanting this?
> >
> > There are a couple of reasons why merging this in the kernel might be
> > problematic.
> >
> > 1) I don't think we will ever support the full capabilities of the devices, so
> > configuration via userspace apps will always be required, and here we are
> > introducing a weird line between the two.
> >
> > 2) There is already an ecosystem of userspace configuration apps, with which
> > this would conflict. They might not be in the best maintenance state due to lack
> > of time from the maintainers, but moving this functionality to the kernel, which
> > is harder change, and harder to ship to users, will only make that worse.
> >
> > Cheers,
> > Filipe Laíns
>
> Hi Filipe,
>
> sure.
>
> While I realize that there is e.g. ratbagd which supports a great deal of the
> HIDPP features and should allow you to control LEDs, unfortunately for my G305
> it does not support the LED (and as far as I remember my G403 does not
> work at all with it).
>
> Then I figured that actually having the LEDs in kernel would allow led triggers
> to work with them, so you could do fancy stuff like showing disk or CPU activity
> or free physical memory... and here we are now.

The one thing that concerns me with those gaming LEDs, is that there
is much more than just color/intensity.
Those LEDs have effects that you can enable (breathing, pulse, color
changing, etc...) and I am not sure how much you are going to be able
to sync with the simple LED class.

>
> As for supporting the full capabilities of these devices: The patch just adds
> RGB leds, which is something already quite standardized in the linux kernel for
> a variety of devices.
> Some roccat mice even have support for changing the actual DPI in their kernel
> driver, which arguably is a whole different story though and not scope of this patch.
> There are also other features (like on-board profiles) which I would definitely
> see being better off in user space, especially as long as there is no additional
> benefit in having them in the kernel.
>
> Regarding the conflict in userspace: ratbagd currently seems to always write
> LED state in RAM and the on-board profiles at the same time, so I would
> argue that the use case here is different: The user space tools want to
> set the LED color in a persistent way, while here we want to have interaction with
> LED triggers and a more transient way. E.g. the driver would overwrite
> only the transient LED color, not the onboard-profiles.
>
> If that is already too much, what about a module option that allows a user to
> deactivate the feature?

Please no. I am tired of having way too many options that nobody uses
except for a couple of people and we can not remove/change them
because of those 2 persons.

Either you manage to sync the LED class state somehow (in a sensible
manner), or I don't think having such LEDs in the kernel is a good
thing because we are going to fight against userspace.

Cheers,
Benjamin

>
> Best Regards,
>
> Manuel
>