Re: [PATCH v4 1/2] platform/x86: asus-wmi: add TUF keyboard RGB readback support

[Denis Benato]

On 11/9/26 19:02, Ilpo Järvinen wrote:
> On Thu, 29 Oct 2026, Bartu Alev wrote:
>
>> The TUF keyboard RGB attributes kbd_rgb_mode and kbd_rgb_state are
>> write-only, so userspace tools keep their own copy of the lighting
>> configuration and lose track of it whenever the EC changes state on its
>> own: after a firmware reset or a boot into another operating system the
>> tools show a stale configuration until the user rewrites every field.
>>
>> DSTS device 0x0010005B evaluates the DSDT method EC0.KBLS(), which
>> snapshots the EC's host-visible keyboard state into a 16-byte buffer:
>> lighting mode, red, green and blue levels, animation speed, and the
>> four power-state flags.  The first byte is a constant 1 written by the
>> AML, which distinguishes a KBLS reply from any other object the method
>> could return.
>>
>> Add kbd_rgb_read_status() to evaluate and validate that buffer, parse
>> it through a packed struct, and convert both attributes to
>> DEVICE_ATTR_RW.  The read returns the write format with "n/a" in the
>> command position: the command word belongs to the write verb (apply
>> now or save to flash) and both verbs update the EC mirror the same
>> way, so there is no state to report there.  The three EC speed codes
>> map back to the sysfs indices 0 to 2.
>>
>> Readback was verified against the EC firmware: the status bytes mirror
>> EC RAM at 0x0B22 to 0x0B2E, and the speed codes 0xe1, 0xeb and 0xf5
>> select animation step thresholds of 30, 20 and 10 ticks per phase,
>> which matches the slow, normal and fast labels.  Tested on a TUF
>> Gaming A15 (FA507NV).
>>
>> Suggested-by: Denis Benato <denis.benato@xxxxxxxxx>
>> Signed-off-by: Bartu Alev <bartualev@xxxxxxxxx>
>> ---
>>  drivers/platform/x86/asus-wmi.c            | 83 +++++++++++++++++++++++++++++-
>>  include/linux/platform_data/x86/asus-wmi.h |  3 ++
>>  2 files changed, 84 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
>> index b33ebdb21aa2..054d1d684bb0 100644
>> --- a/drivers/platform/x86/asus-wmi.c
>> +++ b/drivers/platform/x86/asus-wmi.c
>> @@ -1062,6 +1062,65 @@ static DEVICE_ATTR_RW(gpu_mux_mode);
>>  #define TUF_KBD_RGB_SPEED_SLOW		0xe1
>>  #define TUF_KBD_RGB_SPEED_NORMAL	0xeb
>>  #define TUF_KBD_RGB_SPEED_FAST		0xf5
>> +/* TUF keyboard RGB status readback *******************************************/
>> +struct asus_wmi_kbd_rgb_status {
>> +	u8 signature;
>> +	u8 mode;
>> +	u8 red;
>> +	u8 green;
>> +	u8 blue;
>> +	u8 speed;
>> +	u8 state;
>> +	u8 unused[9];
>> +} __packed;
>> +
>> +static int kbd_rgb_read_status(struct asus_wmi_kbd_rgb_status *status)
>> +{
>> +	int err;
>> +
>> +	err = asus_wmi_evaluate_method_buf(ASUS_WMI_METHODID_DSTS,
>> +					   ASUS_WMI_DEVID_TUF_RGB_READ_STATUS,
>> +					   0, (u8 *)status, sizeof(*status));
>> +	if (err)
>> +		return err < 0 ? err : -ENODEV;
> What do you (or Denis) think on sashiko's mention about 
> asus_wmi_evaluate_method_buf() casting integer.value to int which might 
> end up aliasing high-bit on return values with errnos?

Hi Ilpo,

I don't see the sashiko response neither in my mail nor in the lore,
but I think I understand the problem.

What I think is that we have been lucky.

ASUS probably chose the presence bit high enough not to cause
troubles when they have to report a status to armoury crate
and they don't use negative numbers, but yes I agree that the
return value is more to be seen as a bitfield (with some-bits
fields as numbers) rather than ints.

I have actually spotted this on a commit from Marco that was not
being sent on the lkml (because he touched that code) and I didn't
like what I saw (I didn't like what was already there).

Those commits were about modernizing the use of mutex
and __free where appropriate and I kinda liked them, will
ask him to revive it...

But proper and coherent error handling will touch basically
a large part of supported hardware and asus-wmi core
parts,

I am a bit hesitant to touch that code because I might break
some old and not widely known device: it's the only reason
I haven't (yet) acted in this regard.

> The other caller seems to also return that value directly out of _store 
> function().

The other caller got lucky exactly as the rest of driver.

setters in wmi don't even report the presence bit so there
is even less "incentive" to actually check it.