| 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
| ||
|
Message-Id: <GN28GR.5DRC7JSLXRDT3@ljones.dev>
Date: Sun, 07 Aug 2022 13:54:52 +1200
From: Luke Jones <luke@...nes.dev>
To: Barnabás Pőcze <pobrn@...tonmail.com>
Cc: hdegoede@...hat.com, markgross@...nel.org,
platform-driver-x86@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/5] asus-wmi: Add basic support for TUF laptop keyboard
RGB
Hi Barnabás,
Thank you for taking the time to review
On Sat, Aug 6 2022 at 17:30:04 +0000, Barnabás Pőcze
<pobrn@...tonmail.com> wrote:
> Hi
>
>
> 2022. augusztus 5., péntek 10:19 keltezéssel, Luke D. Jones
> <luke@...nes.dev> írta:
>
>> Adds support for TUF laptop RGB control via the multicolor LED API.
>>
>> As this is the base essentials for adjusting the RGB, it sets the
>> default mode of the keyboard to static. This overwrites the booted
>> state of the keyboard.
>>
>> Signed-off-by: Luke D. Jones <luke@...nes.dev>
>> ---
>> drivers/platform/x86/asus-wmi.c | 89
>> ++++++++++++++++++++++
>> include/linux/platform_data/x86/asus-wmi.h | 3 +
>> 2 files changed, 92 insertions(+)
>>
>> diff --git a/drivers/platform/x86/asus-wmi.c
>> b/drivers/platform/x86/asus-wmi.c
>> index 0e7fbed8a50d..33384e3321bb 100644
>> --- a/drivers/platform/x86/asus-wmi.c
>> +++ b/drivers/platform/x86/asus-wmi.c
>> @@ -25,6 +25,7 @@
>> #include <linux/input/sparse-keymap.h>
>> #include <linux/kernel.h>
>> #include <linux/leds.h>
>> +#include <linux/led-class-multicolor.h>
>> #include <linux/module.h>
>> #include <linux/pci.h>
>> #include <linux/pci_hotplug.h>
>> @@ -190,6 +191,11 @@ struct fan_curve_data {
>> u8 percents[FAN_CURVE_POINTS];
>> };
>>
>> +struct keyboard_rgb_led {
>> + struct led_classdev_mc dev;
>> + struct mc_subled subled_info[3]; /* r g b */
>> +};
>> +
>> struct asus_wmi {
>> int dsts_id;
>> int spec;
>> @@ -234,6 +240,9 @@ struct asus_wmi {
>> bool dgpu_disable_available;
>> bool dgpu_disable;
>>
>> + bool keyboard_rgb_mode_available;
>
> I think this variable could be introduced in the next patch, it is
> not used in this one.
>
Thank you for spotting this. I must have missed it when I split the
patches.
>
>> + struct keyboard_rgb_led keyboard_rgb_mode;
>> +
>> bool throttle_thermal_policy_available;
>> u8 throttle_thermal_policy_mode;
>>
>> @@ -1028,6 +1037,35 @@ static enum led_brightness
>> lightbar_led_get(struct led_classdev *led_cdev)
>> return result & ASUS_WMI_DSTS_LIGHTBAR_MASK;
>> }
>>
>> +static int tuf_rgb_brightness_set(struct led_classdev *cdev,
>> + enum led_brightness brightness)
>> +{
>> + u8 r, g, b;
>> + int err;
>> + u32 ret;
>> +
>> + struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(cdev);
>> +
>> + led_mc_calc_color_components(mc_cdev, brightness);
>> + r = mc_cdev->subled_info[0].brightness;
>> + g = mc_cdev->subled_info[1].brightness;
>> + b = mc_cdev->subled_info[2].brightness;
>> +
>> + /* Writing out requires some defaults. This will overwrite boot
>> mode */
>> + err = asus_wmi_evaluate_method3(ASUS_WMI_METHODID_DEVS,
>> ASUS_WMI_DEVID_TUF_RGB_MODE,
>> + 1 | 0 | (r << 16) | (g << 24), (b) | 0, &ret);
>> + if (err) {
>> + pr_err("Unable to set TUF RGB data?\n");
>> + return err;
>> + }
>> + return 0;
>> +}
>> +
>> +static enum led_brightness tuf_rgb_brightness_get(struct
>> led_classdev *cdev)
>> +{
>> + return cdev->brightness;
>> +}
>
> If you can't query the brightness from the hardware, I think you can
> leave
> `led_classdev::brightness_get` to be `NULL`. This callback is only
> used from
> `led_update_brightness()` as far as I can see.
>
I did wonder about this. Thanks, I've nulled it.
>
>> +
>> static void asus_wmi_led_exit(struct asus_wmi *asus)
>> {
>> led_classdev_unregister(&asus->kbd_led);
>> @@ -1105,6 +1143,57 @@ static int asus_wmi_led_init(struct asus_wmi
>> *asus)
>> &asus->lightbar_led);
>> }
>>
>> + if (asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_TUF_RGB_MODE)) {
>> + struct led_classdev_mc *mc_cdev;
>> + struct mc_subled *mc_led_info;
>> + u8 brightness = 127;
>> +
>> + mc_cdev = &asus->keyboard_rgb_mode.dev;
>> +
>> + /*
>> + * asus::kbd_backlight still controls a base 3-level backlight
>> and when
>> + * it is on 0, the RGB is not visible at all. RGB should be
>> treated as
>> + * an additional step.
>> + */
>> + mc_cdev->led_cdev.name = "asus::multicolour::kbd_backlight";
>> + mc_cdev->led_cdev.flags = LED_CORE_SUSPENDRESUME |
>> LED_RETAIN_AT_SHUTDOWN;
>> + mc_cdev->led_cdev.brightness_set_blocking =
>> tuf_rgb_brightness_set;
>> + mc_cdev->led_cdev.brightness_get = tuf_rgb_brightness_get;
>> +
>> + /* Let the multicolour LED own the info */
>> + mc_led_info = devm_kmalloc_array(
>> + &asus->platform_device->dev,
>> + 3,
>> + sizeof(*mc_led_info),
>> + GFP_KERNEL | __GFP_ZERO);
>> +
>
> I am a bit confused as to why dynamic allocation is needed here.
> Haven't you
> already "allocated" the storage in `keyboard_rgb_led::subled_info`?
Honestly, I write rust 90% of the time which is quite clear on these
things, so here I wasn't very sure about what to do - I read the
hid-playstation.c and it had something similar so I used it and
completely forgot about the struct allocation.
I've updated to `struct mc_subled *mc_led_info =
asus->keyboard_rgb_mode.subled_info;` and removed the dynamic alloc.
>
>
>> + if (!mc_led_info)
>> + return -ENOMEM;
>> +
>> + mc_led_info[0].color_index = LED_COLOR_ID_RED;
>> + mc_led_info[1].color_index = LED_COLOR_ID_GREEN;
>> + mc_led_info[2].color_index = LED_COLOR_ID_BLUE;
>> +
>> + /*
>> + * It's not possible to get last set data from device so set
>> defaults
>> + * to make it safe for a user to change either RGB or modes. We
>> don't
>> + * write these defaults to the device because they will
>> overwrite a
>> + * users last saved boot setting (in NVRAM).
>> + */
>> + mc_cdev->led_cdev.brightness = brightness;
>> + mc_cdev->led_cdev.max_brightness = brightness;
>> + mc_led_info[0].intensity = brightness;
>> + mc_led_info[0].brightness = mc_cdev->led_cdev.brightness;
>> + mc_led_info[1].brightness = mc_cdev->led_cdev.brightness;
>> + mc_led_info[2].brightness = mc_cdev->led_cdev.brightness;
>> + led_mc_calc_color_components(mc_cdev, brightness);
>> +
>> + mc_cdev->subled_info = mc_led_info;
>> + mc_cdev->num_colors = 3;
>
> `led_mc_calc_color_components()` uses `led_classdev_mc::num_colors`,
> so I think
> it needs to be set before calling it. But that function sets the
> subled brightness
> based on the intensity, so it will overwrite the brightness values
> that have just
> been set.
Ah thanks, I don't think I understood that before. I've removed
mc_led_info[i].brightness setting now.
>
>
>> +
>> + rv =
>> led_classdev_multicolor_register(&asus->platform_device->dev,
>> mc_cdev);
>> + }
>> +
>> error:
>> if (rv)
>> asus_wmi_led_exit(asus);
>> diff --git a/include/linux/platform_data/x86/asus-wmi.h
>> b/include/linux/platform_data/x86/asus-wmi.h
>> index a571b47ff362..d63c9945a17d 100644
>> --- a/include/linux/platform_data/x86/asus-wmi.h
>> +++ b/include/linux/platform_data/x86/asus-wmi.h
>> @@ -98,6 +98,9 @@
>> /* dgpu on/off */
>> #define ASUS_WMI_DEVID_DGPU 0x00090020
>>
>> +/* TUF laptop RGB control */
>> +#define ASUS_WMI_DEVID_TUF_RGB_MODE 0x00100056
>> +
>> /* DSTS masks */
>> #define ASUS_WMI_DSTS_STATUS_BIT 0x00000001
>> #define ASUS_WMI_DSTS_UNKNOWN_BIT 0x00000002
>> --
>> 2.37.1
>>
>>
>
>
> Regards,
> Barnabás Pőcze
Powered by blists - more mailing lists