| 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: <-beJEK1G1H0OjntnmAkk16TlRylvhrHnS5CoJ3QVsPUDLlPuObskKTCR5eE5O-OqylRxXET5hA1f1WVaOJXNgdOCY3eEUHdhg5jempoWQdQ=@protonmail.com>
Date: Sun, 24 Oct 2021 16:50:48 +0000
From: Barnabás Pőcze <pobrn@...tonmail.com>
To: Enver Balalic <balalic.enver@...il.com>
Cc: hdegoede@...hat.com, mgross@...ux.intel.com, pavel@....cz,
platform-driver-x86@...r.kernel.org, linux-leds@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] platform/x86: hp-wmi: add support for hp fourzone lighting
Hi
2021. október 24., vasárnap 17:58 keltezéssel, Enver Balalic írta:
> This patch adds support for HP Fourzone lighting.
> It exposes 2 things:
> - General keyboard backlight On/Off control
> - 4 RGB keyboard backlight zones as multicolor LED's
>
> This patch has been tested on a 2020 HP Omen 15 (AMD) 15-en0023dx.
>
> Signed-off-by: Enver Balalic <balalic.enver@...il.com>
> ---
> There are a couple of things I'm unsure about with this patch:
> - I exposed the 4 RGB keyboard backlight zones with names
> "platform::kbd_backlight-1" up to 4, and the one general toggle
> is "platform::kbd_backlight" is this the proper naming
> scheme ?
>
> - If the general keyboard backlight toggle is set to off, then
> no matter what values are written into any of the zones, the
> lights will be off, should we override the general backlight
> toggle to On if you write a value into one of the zones ?
>
> - If the general keyboard backlight toggle is set to On,
> i set the zones brightness to 255, since it doesn't make sense
> for that to be 0 while the backlight is on. I didn't find
> an example of a multicolor led device having a brightness_get
> function registered, so I don't know if this is the proper way
> or if I should register a brightness_get function for each
> of the zones and somehow figure out the brightness based on
> the rgb value that the firmware reports ?
>
> - The windows omen command center detects if you write zeros
> to all 4 of the zones (black) but you set the general keyboard
> backlight to On (as that might confuse someone into thinking
> that their backlight is not working when in fact it's just
> set to black) and overrides the black values to default values
> where each of the zones is set to a different color. Should we
> do the same in the kernel or leave that to some userspace tool ?
>
>
> drivers/platform/x86/Kconfig | 3 +
> drivers/platform/x86/hp-wmi.c | 342 ++++++++++++++++++++++++++++++++++
> 2 files changed, 345 insertions(+)
>
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 1ebce7b775f2..07411fcd0d4b 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -427,6 +427,9 @@ config HP_WMI
> select INPUT_SPARSEKMAP
> select ACPI_PLATFORM_PROFILE
> select HWMON
> + select LEDS_CLASS
> + select NEW_LEDS
> + select LEDS_CLASS_MULTICOLOR
> help
> Say Y here if you want to support WMI-based hotkeys on HP laptops and
> to read data from WMI such as docking or ambient light sensor state.
> diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c
> index 9c4c9f5095ae..87b4724d6b9f 100644
> --- a/drivers/platform/x86/hp-wmi.c
> +++ b/drivers/platform/x86/hp-wmi.c
> @@ -27,6 +27,8 @@
> #include <linux/rfkill.h>
> #include <linux/string.h>
> #include <linux/dmi.h>
> +#include <linux/leds.h>
> +#include <linux/led-class-multicolor.h>
>
> MODULE_AUTHOR("Matthew Garrett <mjg59@...f.ucam.org>");
> MODULE_DESCRIPTION("HP laptop WMI hotkeys driver");
> @@ -43,6 +45,14 @@ MODULE_PARM_DESC(enable_tablet_mode_sw, "Enable SW_TABLET_MODE reporting (-1=aut
> #define HPWMI_BIOS_GUID "5FB7F034-2C63-45e9-BE91-3D44E2C707E4"
> #define HP_OMEN_EC_THERMAL_PROFILE_OFFSET 0x95
>
> +#define HP_FOURZONE_N_ZONES 4
> +#define HP_FOURZONE_LED_NUM_CHANNELS 3
> +#define HP_FOURZONE_OFFSET_COLORS 25
> +#define HP_FOURZONE_BUF_LEN 37
> +#define HP_FOURZONE_OFFSET_BRIGHTNESS 0
> +#define HP_FOURZONE_KBD_BL_ON 128
> +#define HP_FOURZONE_KBD_BL_OFF 0
> +
> /* DMI board names of devices that should use the omen specific path for
> * thermal profiles.
> * This was obtained by taking a look in the windows omen command center
> @@ -61,6 +71,19 @@ static const char * const omen_thermal_profile_boards[] = {
> "8917", "8918", "8949", "894A", "89EB"
> };
>
> +/* DMI Board names of devices that have fourzone support.
> + * A device supports fourzone if the "Feature" array in the json file
> + * in windows omen command center contains "FourZone".
> + */
> +static const char * const fourzone_boards[] = {
> + "8466", "8467", "8468", "8469", "846A", "846B", "84DA", "84DB", "84DC",
> + "8574", "8575", "860A", "87B5", "8600", "8601", "8602", "8605", "8606",
> + "8607", "8746", "8747", "8749", "874A", "8603", "8604", "8748", "878A",
> + "878B", "878C", "88C8", "88CB", "8786", "8787", "8788", "88D1", "88D2",
> + "88F4", "88FD", "88F5", "88F6", "88F7", "88FE", "88FF", "8900", "8901",
> + "8902", "8912"
> +};
> +
> enum hp_wmi_radio {
> HPWMI_WIFI = 0x0,
> HPWMI_BLUETOOTH = 0x1,
> @@ -117,11 +140,19 @@ enum hp_wmi_gm_commandtype {
> HPWMI_FAN_SPEED_MAX_SET_QUERY = 0x27,
> };
>
> +enum hp_wmi_lm_commandtype {
> + HPWMI_FOURZONE_ZONES_GET = 0x02,
> + HPWMI_FOURZONE_ZONES_SET = 0x03,
> + HPWMI_FOURZONE_KB_BACKLIGHT_GET = 0x04,
> + HPWMI_FOURZONE_KB_BACKLIGHT_SET = 0x05,
> +};
> +
> enum hp_wmi_command {
> HPWMI_READ = 0x01,
> HPWMI_WRITE = 0x02,
> HPWMI_ODM = 0x03,
> HPWMI_GM = 0x20008,
> + HPWMI_LM = 0x20009,
> };
>
> enum hp_wmi_hardware_mask {
> @@ -200,6 +231,32 @@ static const struct key_entry hp_wmi_keymap[] = {
> { KE_END, 0 }
> };
>
> +struct hp_wmi_fourzone_data {
> + struct {
> + unsigned int r;
> + unsigned int g;
> + unsigned int b;
> + } zone[HP_FOURZONE_N_ZONES];
> +};
> +
> +struct hp_fourzone_zone {
> + bool initialized;
> + struct led_classdev_mc mc_cdev;
> + struct mc_subled subled_info[HP_FOURZONE_LED_NUM_CHANNELS];
> +};
> +
> +struct hp_wmi_leds {
> + struct {
> + bool initialized;
> + struct led_classdev led;
> + unsigned int last_brightness;
> + } fourzone_kbd_bl;
> +
> + struct {
> + struct hp_fourzone_zone zones[HP_FOURZONE_N_ZONES];
> + } fourzone;
> +};
> +
> static struct input_dev *hp_wmi_input_dev;
> static struct platform_device *hp_wmi_platform_dev;
> static struct platform_profile_handler platform_profile_handler;
> @@ -208,6 +265,7 @@ static bool platform_profile_support;
> static struct rfkill *wifi_rfkill;
> static struct rfkill *bluetooth_rfkill;
> static struct rfkill *wwan_rfkill;
> +static struct hp_wmi_leds *hp_leds;
>
> struct rfkill2_device {
> u8 id;
> @@ -373,6 +431,18 @@ static int omen_thermal_profile_set(int mode)
> return mode;
> }
>
> +static bool is_fourzone_supported(void)
> +{
> + const char *board_name = dmi_get_system_info(DMI_BOARD_NAME);
> +
> + if (!board_name)
> + return false;
> +
> + return match_string(fourzone_boards,
> + ARRAY_SIZE(fourzone_boards),
> + board_name) >= 0;
> +}
> +
> static bool is_omen_thermal_profile(void)
> {
> const char *board_name = dmi_get_system_info(DMI_BOARD_NAME);
> @@ -653,6 +723,8 @@ static struct attribute *hp_wmi_attrs[] = {
> };
> ATTRIBUTE_GROUPS(hp_wmi);
>
> +static void hp_wmi_fourzone_kbd_bl_notify(void);
> +
> static void hp_wmi_notify(u32 value, void *context)
> {
> struct acpi_buffer response = { ACPI_ALLOCATE_BUFFER, NULL };
> @@ -754,6 +826,7 @@ static void hp_wmi_notify(u32 value, void *context)
> case HPWMI_PROXIMITY_SENSOR:
> break;
> case HPWMI_BACKLIT_KB_BRIGHTNESS:
> + hp_wmi_fourzone_kbd_bl_notify();
> break;
> case HPWMI_PEAKSHIFT_PERIOD:
> break;
> @@ -1114,6 +1187,269 @@ static int platform_profile_set(struct platform_profile_handler *pprof,
> return 0;
> }
>
> +static int hp_wmi_fourzone_get_zones_data(struct hp_wmi_fourzone_data *data)
> +{
> + int ret, i, zone;
> + unsigned char buf[HP_FOURZONE_BUF_LEN];
> +
> + ret = hp_wmi_perform_query(HPWMI_FOURZONE_ZONES_GET, HPWMI_LM,
> + &buf, sizeof(buf),
> + sizeof(buf));
> +
> + if (ret < 0)
> + return ret;
Only a return value of 0 means success. See the comment above `hp_wmi_perform_query()`.
> +
> + for (i = 0; i < HP_FOURZONE_N_ZONES; i++) {
> + zone = HP_FOURZONE_OFFSET_COLORS + (i * 3);
> +
> + data->zone[i].r = buf[zone];
> + data->zone[i].g = buf[zone + 1];
> + data->zone[i].b = buf[zone + 2];
> + }
> +
> + return 0;
> +}
> +
> +static int hp_wmi_fourzone_set_zones_data(struct hp_wmi_fourzone_data *data)
> +{
> + int i, zone;
> + unsigned char buf[HP_FOURZONE_BUF_LEN];
> +
> + for (i = 0; i < HP_FOURZONE_N_ZONES; i++) {
> + zone = HP_FOURZONE_OFFSET_COLORS + (i * 3);
Isn't it a problem that the first 25 bytes potentially contain random values?
I think it would be better to explicitly clear them:
unsigned char buf[HP_FOURZONE_BUF_LEN] = {0};
Maybe even in `hp_wmi_fourzone_get_zones_data()` as well.
> +
> + buf[zone] = data->zone[i].r;
> + buf[zone + 1] = data->zone[i].g;
> + buf[zone + 2] = data->zone[i].b;
> + }
> +
> + return hp_wmi_perform_query(HPWMI_FOURZONE_ZONES_SET, HPWMI_LM,
> + &buf, sizeof(buf),
> + sizeof(buf));
> +}
> +
> +static int hp_wmi_fourzone_set_zone_rgb(int zone, unsigned int r,
> + unsigned int g, unsigned int b)
> +{
> + int err;
> + struct hp_wmi_fourzone_data data;
> +
> + err = hp_wmi_fourzone_get_zones_data(&data);
> + if (err < 0)
> + return err;
> +
> + data.zone[zone].r = r;
> + data.zone[zone].g = g;
> + data.zone[zone].b = b;
> +
> + return hp_wmi_fourzone_set_zones_data(&data);
> +}
> +
> +static int hp_wmi_fourzone_cdev_set_zone(int zone, struct led_classdev *cdev,
> + enum led_brightness brightness)
> +{
> + struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(cdev);
> +
> + led_mc_calc_color_components(mc_cdev, brightness);
> +
> + return hp_wmi_fourzone_set_zone_rgb(zone,
> + mc_cdev->subled_info[0].brightness,
> + mc_cdev->subled_info[1].brightness,
> + mc_cdev->subled_info[2].brightness);
> +}
> +
> +static int hp_wmi_cdev_fourzone_led_set_brightness_zone0(struct led_classdev *cdev,
> + enum led_brightness brightness)
> +{
> + return hp_wmi_fourzone_cdev_set_zone(0, cdev, brightness);
> +}
> +
> +static int hp_wmi_cdev_fourzone_led_set_brightness_zone1(struct led_classdev *cdev,
> + enum led_brightness brightness)
> +{
> + return hp_wmi_fourzone_cdev_set_zone(1, cdev, brightness);
> +}
> +
> +static int hp_wmi_cdev_fourzone_led_set_brightness_zone2(struct led_classdev *cdev,
> + enum led_brightness brightness)
> +{
> + return hp_wmi_fourzone_cdev_set_zone(2, cdev, brightness);
> +}
> +
> +static int hp_wmi_cdev_fourzone_led_set_brightness_zone3(struct led_classdev *cdev,
> + enum led_brightness brightness)
> +{
> + return hp_wmi_fourzone_cdev_set_zone(3, cdev, brightness);
> +}
> +
> +static int hp_wmi_fourzone_kbd_bl_get(void)
> +{
> + int val = 0, ret;
> +
> + ret = hp_wmi_perform_query(HPWMI_FOURZONE_KB_BACKLIGHT_GET, HPWMI_LM,
> + &val, sizeof(val), sizeof(val));
> +
> + if (ret)
> + return ret < 0 ? ret : -EINVAL;
> +
> + return val == HP_FOURZONE_KBD_BL_ON ? 1 : 0;
> +}
> +
> +static int hp_wmi_fourzone_kbd_bl_set(unsigned int brightness)
> +{
> + char kbd_bl_data[1] = { brightness ? HP_FOURZONE_KBD_BL_ON : HP_FOURZONE_KBD_BL_OFF };
> +
> + int ret = hp_wmi_perform_query(HPWMI_FOURZONE_KB_BACKLIGHT_SET, HPWMI_LM,
> + &kbd_bl_data, sizeof(kbd_bl_data),
> + sizeof(kbd_bl_data));
> +
> + if (ret != 0)
> + return -EINVAL;
The comment above `hp_wmi_perform_query()` says
* returns zero on success
* an HP WMI query specific error code (which is positive)
* -EINVAL if the query was not successful at all
* -EINVAL if the output buffer size exceeds buffersize
So I think you should return `ret` as is if it's negative, and return e.g. -EIO
if it's positive. I am not sure `-EINVAL` is the best error code here.
> +
> + hp_leds->fourzone_kbd_bl.last_brightness = brightness;
> +
> + return brightness;
> +}
> +
> +static enum led_brightness hp_wmi_cdev_fourzone_kbd_bl_get(struct led_classdev *led_cdev)
> +{
> + return hp_wmi_fourzone_kbd_bl_get();
> +}
> +
> +static int hp_wmi_cdev_fourzone_kbd_bl_set(struct led_classdev *led_cdev,
> + enum led_brightness brightness)
> +{
> + return hp_wmi_fourzone_kbd_bl_set(brightness);
> +}
> +
> +static int (*fourzone_cdev_zone_set[HP_FOURZONE_N_ZONES])(struct led_classdev*,
> + enum led_brightness) = {
> + &hp_wmi_cdev_fourzone_led_set_brightness_zone0,
> + &hp_wmi_cdev_fourzone_led_set_brightness_zone1,
> + &hp_wmi_cdev_fourzone_led_set_brightness_zone2,
> + &hp_wmi_cdev_fourzone_led_set_brightness_zone3,
> +};
I think you can use `mc_led = lcdev_to_mccdev(...)` to get the multicolor LED,
then use `zone = container_of(mc_led, struct hp_fourzone_zone, mc_cdev)`
to get the `hp_fourzone_zone` struct, then `zone - hp_leds->fourzone.zones`
to get the index. There is no need for these four functions as far as I can see.
> +
> +static void hp_wmi_fourzone_kbd_bl_notify(void)
> +{
> + int brightness;
> +
If the keyboard backlight is not supported, then `hp_leds` remains NULL, right?
Can this function be called in that case?
> + if (!hp_leds->fourzone_kbd_bl.initialized)
> + return;
> +
> + brightness = hp_wmi_fourzone_kbd_bl_get();
> + if (brightness < 0)
> + return;
> +
> + if (brightness == hp_leds->fourzone_kbd_bl.last_brightness)
> + return;
> +
> + hp_leds->fourzone_kbd_bl.last_brightness = brightness;
> +
> + led_classdev_notify_brightness_hw_changed(&hp_leds->fourzone_kbd_bl.led, brightness);
> +}
> +
> +static void fourzone_leds_exit(struct platform_device *device)
> +{
> + int i;
> + struct hp_fourzone_zone *zone;
> +
> + if (hp_leds->fourzone_kbd_bl.initialized) {
> + hp_leds->fourzone_kbd_bl.initialized = false;
> + led_classdev_unregister(&hp_leds->fourzone_kbd_bl.led);
> + }
> +
> + for (i = 0; i < HP_FOURZONE_N_ZONES; i++) {
> + zone = &hp_leds->fourzone.zones[i];
> + if (zone->initialized)
> + devm_led_classdev_multicolor_unregister(&device->dev,
> + &zone->mc_cdev);
> + }
> +}
> +
> +static int fourzone_leds_setup(struct platform_device *device)
> +{
> + int err, brightness, i, zones_brightness;
> + struct led_classdev *cdev;
> + struct hp_wmi_fourzone_data data;
> + struct hp_fourzone_zone *zone;
> + char name[64];
> +
> + hp_leds = devm_kzalloc(&device->dev, sizeof(*hp_leds), GFP_KERNEL);
> + if (!hp_leds)
> + return -ENOMEM;
Have you considered not dynamically allocating this struct?
> +
> + brightness = hp_wmi_fourzone_kbd_bl_get();
> + if (brightness < 0)
> + return brightness;
> +
> + hp_leds->fourzone_kbd_bl.last_brightness = brightness;
> +
> + hp_leds->fourzone_kbd_bl.led.name = "platform::" LED_FUNCTION_KBD_BACKLIGHT;
> + hp_leds->fourzone_kbd_bl.led.brightness = brightness;
> + hp_leds->fourzone_kbd_bl.led.max_brightness = 1;
> + hp_leds->fourzone_kbd_bl.led.brightness_get = hp_wmi_cdev_fourzone_kbd_bl_get;
> + hp_leds->fourzone_kbd_bl.led.brightness_set_blocking = hp_wmi_cdev_fourzone_kbd_bl_set;
> + hp_leds->fourzone_kbd_bl.led.flags = LED_BRIGHT_HW_CHANGED | LED_RETAIN_AT_SHUTDOWN;
> +
> + err = led_classdev_register(&device->dev, &hp_leds->fourzone_kbd_bl.led);
> + if (err)
> + return err;
> +
> + hp_leds->fourzone_kbd_bl.initialized = true;
> +
> + err = hp_wmi_fourzone_get_zones_data(&data);
> + if (err)
> + goto fail_fourzone;
> +
> + zones_brightness = brightness == 1 ? 255 : 0;
> +
> + for (i = 0; i < HP_FOURZONE_N_ZONES; i++) {
> + zone = &hp_leds->fourzone.zones[i];
> +
> + zone->subled_info[0].color_index = LED_COLOR_ID_RED;
> + zone->subled_info[0].channel = 0;
> + zone->subled_info[0].intensity = data.zone[i].r;
> +
> + zone->subled_info[1].color_index = LED_COLOR_ID_GREEN;
> + zone->subled_info[1].channel = 1;
> + zone->subled_info[1].intensity = data.zone[i].g;
> +
> + zone->subled_info[2].color_index = LED_COLOR_ID_BLUE;
> + zone->subled_info[2].channel = 2;
> + zone->subled_info[2].intensity = data.zone[i].b;
> +
> + zone->mc_cdev.subled_info = zone->subled_info;
> + zone->mc_cdev.num_colors = HP_FOURZONE_LED_NUM_CHANNELS;
> +
> + cdev = &zone->mc_cdev.led_cdev;
> + cdev->flags = LED_RETAIN_AT_SHUTDOWN;
> + cdev->brightness = zones_brightness;
> + cdev->max_brightness = 255;
> + zone->mc_cdev.led_cdev.brightness_set_blocking = fourzone_cdev_zone_set[i];
> + snprintf(name, sizeof(name), "platform::%s-%i", LED_FUNCTION_KBD_BACKLIGHT, i + 1);
> + cdev->name = name;
> +
> + err = devm_led_classdev_multicolor_register_ext(&device->dev,
> + &zone->mc_cdev,
> + NULL);
> +
> + if (err) {
> + dev_err(&device->dev, "Cannot register fourzone zone %i LED: %i\n", i, err);
> + goto fail_fourzone;
> + }
> +
> + zone->initialized = true;
> + }
> +
> + return 0;
> +
> +fail_fourzone:
> + fourzone_leds_exit(device);
> +
> + return err;
> +}
> +
> static int thermal_profile_setup(void)
> {
> int err, tp;
> @@ -1186,6 +1522,9 @@ static int __init hp_wmi_bios_setup(struct platform_device *device)
>
> thermal_profile_setup();
>
> + if (is_fourzone_supported())
> + fourzone_leds_setup(device);
> +
> return 0;
> }
>
> @@ -1211,6 +1550,9 @@ static int __exit hp_wmi_bios_remove(struct platform_device *device)
> rfkill_destroy(wwan_rfkill);
> }
>
> + if (hp_leds)
> + fourzone_leds_exit(device);
> +
> if (platform_profile_support)
> platform_profile_remove();
>
> --
> 2.33.1
>
Regards,
Barnabás Pőcze
Powered by blists - more mailing lists