linux-kernel - Re: [PATCH 2/5] asus-wmi: Add support for TUF laptop keyboard RGB mode control

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: <90W6GR.9Z5CYBT7NOHJ1@ljones.dev>
Date:   Sat, 06 Aug 2022 22:33:45 +1200
From:   Luke Jones <luke@...nes.dev>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     Hans de Goede <hdegoede@...hat.com>,
        Mark Gross <markgross@...nel.org>,
        Platform Driver <platform-driver-x86@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/5] asus-wmi: Add support for TUF laptop keyboard RGB
 mode control

Hi Andy,

On Sat, Aug 6 2022 at 11:56:58 +0200, Andy Shevchenko 
<andy.shevchenko@...il.com> wrote:
> On Fri, Aug 5, 2022 at 10:20 AM Luke D. Jones <luke@...nes.dev> wrote:
>> 
>>  Adds support for TUF laptop RGB mode control.
>> 
>>  Two paths are added:
>>  - /sys/devices/platform/asus-nb-wmi/kernel_rgb_mode
>>  - /sys/devices/platform/asus-nb-wmi/kernel_rgb_mode_index
> 
> ...
> 
>>  +static int keyboard_rgb_mode_check_present(struct asus_wmi *asus)
>>  +{
>>  +       u32 result;
>>  +       int err;
>>  +
>>  +       asus->keyboard_rgb_mode_available = false;
>>  +
>>  +       err = asus_wmi_get_devstate(asus, 
>> ASUS_WMI_DEVID_TUF_RGB_MODE, &result);
>>  +       if (err) {
>>  +               if (err == -ENODEV)
>>  +                       return 0;
>>  +               return err;
>>  +       }
> 
>>  +       if (result & ASUS_WMI_DSTS_PRESENCE_BIT) {
>>  +               asus->keyboard_rgb_mode_available = true;
>>  +       }
> 
> {} are not needed (except if they will be utilized in the next patches
> in the series).

I've usually been pretty good at catching these. I must not have run 
the patch check script on this one.

Fixed.

> 
>>  +       return 0;
>>  +}
> 
> ...
> 
>>  +       if (sscanf(buf, "%hhd %hhd %hhd", &save, &mode, &speed) != 
>> 3)
>>  +               return -EINVAL;
> 
> Usually we have three separate nodes for that, but they are kinda
> hidden in one driver, so I don't care much.

I don't really understand what you mean sorry.

> 
> ...
> 
>>  +       asus->keyboard_rgb_mode.save = save > 0 ? 1 : 0;
> 
> So, it's actually boolean.
> 
> You may write it as
> 
>     ...save = !!save;

Err okay. Done.

> 
>>  +       /* These are the known usable modes across all TUF/ROG */
>>  +       asus->keyboard_rgb_mode.mode = mode < 12 && mode != 9 ? 
>> mode : 0x0a;
>>  +
>>  +       if (speed == 0)
>>  +               asus->keyboard_rgb_mode.speed = 0xe1;
>>  +       else if (speed == 1)
>>  +               asus->keyboard_rgb_mode.speed = 0xeb;
>>  +       else if (speed == 2)
>>  +               asus->keyboard_rgb_mode.speed = 0xf5;
> 
>>  +       else
>>  +               asus->keyboard_rgb_mode.speed = 0xeb;
> 
> So the 1 is default then, why not use switch-case to show this 
> explicitly?
> 
> switch (speed) {
>   case 0:
>     ...
>   break;
>   case 1:
>   default:
>     ...
>   break;
>   case 2:
>     ...
>   break;
> }
> 
> Yes, it's longer, but I think it's cleaner.

Agreed. Done.

> 
>>  +       err = tuf_rgb_brightness_set(cdev, cdev->brightness);
>>  +       if (err)
>>  +               return err;
>>  +       return 0;
> 
> return tuf_rgb_brightness_set(...);

This causes a hang (waiting for return somewhere?) if I don't return 
count. Especially true if the return is 0.

> 
>>  +}
> 
> --
> With Best Regards,
> Andy Shevchenko