linux-kernel - Re: [PATCH] platform/x86: acer-wmi: Add support for Acer Helios 300 RGB keyboard backlight

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: <20210519085429.GA2025@bug>
Date:   Wed, 19 May 2021 10:54:30 +0200
From:   Pavel Machek <pavel@....cz>
To:     Hans de Goede <hdegoede@...hat.com>
Cc:     Jafar Akhondali <jafar.akhoondali@...il.com>, jlee@...e.com,
        mgross@...ux.intel.com, platform-driver-x86@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] platform/x86: acer-wmi: Add support for Acer Helios 300
 RGB keyboard backlight

Hi!

> > From e65b0ddbf559aa3ca8a7998404e7e67e64b705e9 Mon Sep 17 00:00:00 2001
> > From: JafarAkhondali <jafar.akhoondali@...il.com>
> > Date: Fri, 14 May 2021 08:26:47 +0430
> > Subject: [PATCH] platform/x86: acer-wmi: Add support for Acer Helios
> > 300 rgb keyboard backlight
> > 
> > The Acer helios 300 provides gaming functions WMI that is available in
> > Windows, however this was not implemented in Linux. The process of finding
> > the related method was done by decompiling PredatorSense(official Acer
> > gaming functions software for Predator series) and decompiling WQ
> > buffers. This patch provides a gaming interface which will then expose a
> > character device named "acer-gkbbl". This character device accepts 16
> > bytes long config, which is specific for the backlight method. The
> > meaning of each bytes ordered by bit position is as follows:
> > 
> > Bit 0 -> Backlight modes:
> > 1: Breath
> > 2: Neon
> > 3: Wave
> > 4: Shifting
> > 5: Zoom
> > Bit 1 -> Animation Speed: from 1 to 9 ( 1 is slowest, 9 is fastest)
> > Bit 2 -> Brightness from 0 to 100 ( 0 is no backlight, 100 is brightest)
> > Bit 3 -> Unknown. Wave effect uses 8, other modes must use 0
> > Bit 4 -> Animation Direction:
> > 1: Right-to-Left
> > 2: Left-to-Right
> > Bit 5 -> Red Color Selection
> > Bit 6 -> Green Color Selection
> > Bit 7 -> Blue Color Selection
> > Bit 8 -> Currently unknown, or not used in known model
> > Bit 9 -> Currently unknown, or not used in known model
> > Bit 10 -> Currently unknown, or not used in known model
> > Bit 11 -> Currently unknown, or not used in known model
> > Bit 12 -> Currently unknown, or not used in known model
> > Bit 13 -> Currently unknown, or not used in known model
> > Bit 14 -> Currently unknown, or not used in known model
> > Bit 15 -> Currently unknown, or not used in known model
> > 
> > Filling this config is out of scope for the kernel module, and this module
> > only acts as an interface.
> > 
> > Currently, I'm not sure with the method for communicating with user-space,
> > but since leds.h subsystem wouldn't fit for complex actions such as this
> > complex config, I couldn't find any better method than char dev.
> 
> Thank you for your patch, given that there is no existing kernel
> interface which is a good match for the features exported by this
> keyboard I'm fine with just having a raw interface where userspace
> writes GAMING_KBBL_CONFIG_LEN bytes as you suggest.

Keyboard backlight goes through LED interface (so please cc the mailing list) and
no, passing raw bytes to hardware is not an acceptable interface.

> But lets not use a classdev + chardev for this please, you can
> just add a binary write-only sysfs-atrribute under the wmi-dev for
> this with a name like (for example) gaming_kbd_backlight_config
> and then userspace can write to that without needing a class + char
> dev just for this single write.

NAK. We have existing interfaces for this.

Best regards,
								Pavel