| 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: <5dd6d08b-33eb-f74d-e3b1-d1380ad25565@linux.intel.com>
Date: Tue, 11 Feb 2025 16:00:31 +0200 (EET)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Jackie Dong <xy-jackie@....com>
cc: Hans de Goede <hdegoede@...hat.com>, LKML <linux-kernel@...r.kernel.org>,
platform-driver-x86@...r.kernel.org, dongeg1@...ovo.com,
Mark Pearson <mpearson-lenovo@...ebb.ca>
Subject: Re: [PATCH v3] platform/x86: lenovo-super-hotkey-wmi.c: Support
formic and audio mute LEDs
On Tue, 11 Feb 2025, Jackie Dong wrote:
> On 2/10/25 18:45, Ilpo Järvinen wrote:
> > On Sat, 8 Feb 2025, Jackie Dong wrote:
> >
> > > Implement Lenovo utility data WMI calls needed to make LEDs
> > > work on Ideapads that support this GUID.
> > > This enables the mic and audio LEDs to be updated correctly.
> > >
> > > Tested on below samples.
> > > ThinkBook 13X Gen4 IMH
> > > ThinkBook 14 G6 ABP
> > > ThinkBook 16p Gen4-21J8
> > > ThinkBook 16p Gen8-IRL
> > > ThinkBook 16p G7+ ASP
> > >
> > > Signed-off-by: Jackie Dong <xy-jackie@....com>
> > > Suggested-by: Mark Pearson <mpearson-lenovo@...ebb.ca>
> > > ---
> > > Changes in v3:
> > > - Changed the name of the Kconfig entry to LENOVO_SUPER_HOTKEY_WMI
> > > - Renamed everything in this driver which contains the name "ideapad"
> > > to instead contain the name of this driver.
> > > - Moved struct wmi_device *led_wdev in lenovo_super_hotkey_wmi_private,
> > > and use container_of() to the led_wdev pointer.
> > > - Replaced sizeof(struct wmi_led_args) by sizeof(led_arg)
> > > - Added condtions checking for obj && obj->type == ACPI_TYPE_INTEGER
> > > and free the ACPI object after get the required value.
> > > - Removed led_classdev_unregister() after led_reg_failed label, but
> > > add lenovo_super_hotkey_wmi_remove(struct wmi_device *wdev) to free
> > > resource.
> > > - Removed IDEAPAD_WMI_EVENT_FN_KEYS/IDEAPAD_WMI_EVENT_LUD_KEYS related
> > > source codes and only keep LUD_WMI_METHOD_GUID.
> > >
> > > Changes in v2:
> > > - Update code layout and formatting as recommended in review
> > > - Improved error handling in ideapad_wmi_led_init
> > > - Separated a WMI driver named lenovo-super-hotkey-wmi.c from
> > > ideapad-lap.c, it's only for Lenovo Super Hotkey WMI devices.
> > >
> > > drivers/platform/x86/Kconfig | 9 +
> > > drivers/platform/x86/Makefile | 1 +
> > > .../platform/x86/lenovo-super-hotkey-wmi.c | 248 ++++++++++++++++++
> > > 3 files changed, 258 insertions(+)
> > > create mode 100644 drivers/platform/x86/lenovo-super-hotkey-wmi.c
> > >
> > > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> > > index 0258dd879d64..c1792e8f04e1 100644
> > > --- a/drivers/platform/x86/Kconfig
> > > +++ b/drivers/platform/x86/Kconfig
> > > @@ -475,6 +475,15 @@ config IDEAPAD_LAPTOP
> > > This is a driver for Lenovo IdeaPad netbooks contains drivers for
> > > rfkill switch, hotkey, fan control and backlight control.
> > > +config LENOVO_SUPER_HOTKEY_WMI
> > > + tristate "Lenovo Super Hotkey Utility WMI extras driver"
> > > + depends on ACPI_WMI
> > > + depends on IDEAPAD_LAPTOP
> > > + help
> > > + This driver provides WMI support for Lenovo customized hotkeys
> > > function
> > > + of Lenovo NoteBooks which are for Consumer and SMB customers, such
> > > as
> > > + Ideapad/YOGA/XiaoXin/Gaming/ThinkBook and so on.
> > > +
> > > config LENOVO_YMC
> > > tristate "Lenovo Yoga Tablet Mode Control"
> > > depends on ACPI_WMI
> > > diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> > > index e1b142947067..598335da9f55 100644
> > > --- a/drivers/platform/x86/Makefile
> > > +++ b/drivers/platform/x86/Makefile
> > > @@ -61,6 +61,7 @@ obj-$(CONFIG_UV_SYSFS) += uv_sysfs.o
> > > # IBM Thinkpad and Lenovo
> > > obj-$(CONFIG_IBM_RTL) += ibm_rtl.o
> > > obj-$(CONFIG_IDEAPAD_LAPTOP) += ideapad-laptop.o
> > > +obj-$(CONFIG_LENOVO_SUPER_HOTKEY_WMI) += lenovo-super-hotkey-wmi.o
> > > obj-$(CONFIG_LENOVO_YMC) += lenovo-ymc.o
> > > obj-$(CONFIG_SENSORS_HDAPS) += hdaps.o
> > > obj-$(CONFIG_THINKPAD_ACPI) += thinkpad_acpi.o
> > > diff --git a/drivers/platform/x86/lenovo-super-hotkey-wmi.c
> > > b/drivers/platform/x86/lenovo-super-hotkey-wmi.c
> > > new file mode 100644
> > > index 000000000000..e677bd11fa2f
> > > --- /dev/null
> > > +++ b/drivers/platform/x86/lenovo-super-hotkey-wmi.c
> > > @@ -0,0 +1,248 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Lenovo Super Hotkey Utility WMI extras driver for Ideapad laptop
> > > + *
> > > + * Copyright (C) 2025 Lenovo
> > > + */
> > > +
> > > +#include <linux/leds.h>
> > > +#include <linux/wmi.h>
> > > +#include "ideapad-laptop.h"
> >
> > Add an empty line before the local includes.
> Hi Ilpo,
> Thanks for your review and comments.
> I'll update it in next version patch.
> >
> > > +
> > > +/* Lenovo Super Hotkey WMI GUIDs */
> > > +#define LUD_WMI_METHOD_GUID "CE6C0974-0407-4F50-88BA-4FC3B6559AD8"
> > > +
> > > +/* Lenovo Utility Data WMI method_id */
> > > +#define WMI_LUD_GET_SUPPORT 1
> > > +#define WMI_LUD_SET_FEATURE 2
> > > +
> > > +#define WMI_LUD_GET_MICMUTE_LED_VER 20
> > > +#define WMI_LUD_GET_AUDIOMUTE_LED_VER 26
> > > +
> > > +#define WMI_LUD_SUPPORT_MICMUTE_LED_VER 25
> > > +#define WMI_LUD_SUPPORT_AUDIOMUTE_LED_VER 27
> > > +
> > > +/* Input parameters to mute/unmute audio LED and Mic LED */
> > > +struct wmi_led_args {
> > > + u8 id;
> > > + u8 subid;
> > > + u16 value;
> > > +};
> > > +
> > > +/* Values of input parameters to SetFeature of audio LED and Mic LED */
> > > +enum hotkey_set_feature {
> > > + MIC_MUTE_LED_ON = 1,
> > > + MIC_MUTE_LED_OFF,
> > > + AUDIO_MUTE_LED_ON = 4,
> > > + AUDIO_MUTE_LED_OFF,
> > > +};
> > > +
> > > +#define LSH_ACPI_LED_MAX 2
> > > +
> > > +enum lenovo_super_hotkey_wmi_event_type {
> > > + LSH_WMI_EVENT_LUD_KEYS = 2,
> > > +};
> > > +
> > > +struct lenovo_super_hotkey_wmi_private {
> > > + enum lenovo_super_hotkey_wmi_event_type event;
> > > + struct led_classdev cdev[LSH_ACPI_LED_MAX];
> > > + struct wmi_device *led_wdev;
> > > +};
> > > +
> > > +enum mute_led_type {
> > > + MIC_MUTE,
> > > + AUDIO_MUTE,
> > > +};
> > > +
> > > +static int lsh_wmi_mute_led_set(enum mute_led_type led_type, struct
> > > led_classdev *led_cdev,
> > > + enum led_brightness brightness)
> > > +
> > > +{
> > > + struct lenovo_super_hotkey_wmi_private *wpriv = container_of(led_cdev,
> > > + struct lenovo_super_hotkey_wmi_private,
> > > cdev[led_type]);
> > > + struct wmi_led_args led_arg = {0, 0, 0};
> > > + struct acpi_buffer input;
> > > + acpi_status status;
> > > +
> > > + switch (led_type) {
> > > + case MIC_MUTE:
> > > + led_arg.id = brightness == LED_ON ? MIC_MUTE_LED_ON :
> > > MIC_MUTE_LED_OFF;
> > > + break;
> > > + case AUDIO_MUTE:
> > > + led_arg.id = brightness == LED_ON ? AUDIO_MUTE_LED_ON :
> > > AUDIO_MUTE_LED_OFF;
> > > + break;
> > > + default:
> > > + return -EINVAL;
> > > + }
> > > +
> > > + input.length = sizeof(led_arg);
> > > + input.pointer = &led_arg;
> > > + status = wmidev_evaluate_method(wpriv->led_wdev, 0,
> > > WMI_LUD_SET_FEATURE, &input, NULL);
> > > + if (ACPI_FAILURE(status))
> > > + return -EIO;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int lsh_wmi_audiomute_led_set(struct led_classdev *led_cdev,
> > > + enum led_brightness brightness)
> > > +
> > > +{
> > > + return lsh_wmi_mute_led_set(AUDIO_MUTE, led_cdev, brightness);
> > > +}
> > > +
> > > +static int lsh_wmi_micmute_led_set(struct led_classdev *led_cdev,
> > > + enum led_brightness brightness)
> > > +{
> > > + return lsh_wmi_mute_led_set(MIC_MUTE, led_cdev, brightness);
> > > +}
> > > +
> > > +static int lenovo_super_hotkey_wmi_led_init(enum mute_led_type led_type,
> > > struct device *dev)
> > > +{
> > > + struct lenovo_super_hotkey_wmi_private *wpriv = dev_get_drvdata(dev);
> > > + struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
> > > + struct acpi_buffer input;
> > > + union acpi_object *obj;
> > > + int led_version, err = 0;
> > > + unsigned int wmiarg;
> > > + acpi_status status;
> > > +
> > > + switch (led_type) {
> > > + case MIC_MUTE:
> > > + wmiarg = WMI_LUD_GET_MICMUTE_LED_VER;
> > > + break;
> > > + case AUDIO_MUTE:
> > > + wmiarg = WMI_LUD_GET_AUDIOMUTE_LED_VER;
> > > + break;
> > > + default:
> > > + return -EINVAL;
> > > + }
> > > +
> > > + input.length = sizeof(wmiarg);
> > > + input.pointer = &wmiarg;
> > > + status = wmidev_evaluate_method(wpriv->led_wdev, 0,
> > > WMI_LUD_GET_SUPPORT, &input, &output);
> > > + if (ACPI_FAILURE(status))
> > > + return -EIO;
> > > +
> > > + obj = output.pointer;
> > > + if (obj && obj->type == ACPI_TYPE_INTEGER) {
> > > + led_version = obj->integer.value;
> > > + kfree(output.pointer);
> > > + } else {
> > > + err = -EIO;
> > > + goto led_error;
> > > + }
> > > +
> > > + wpriv->cdev[led_type].max_brightness = LED_ON;
> > > + wpriv->cdev[led_type].dev = dev;
> > > + wpriv->cdev[led_type].flags = LED_CORE_SUSPENDRESUME;
> > > +
> > > + switch (led_type) {
> > > + case MIC_MUTE:
> > > + if (led_version != WMI_LUD_SUPPORT_MICMUTE_LED_VER) {
> > > + err = -EIO;
> > > + goto led_error;
> > > + }
> > > + wpriv->cdev[led_type].name = "platform::micmute";
> > > + wpriv->cdev[led_type].brightness_set_blocking =
> > > &lsh_wmi_micmute_led_set;
> > > + wpriv->cdev[led_type].default_trigger = "audio-micmute";
> > > +
> > > + err = devm_led_classdev_register(dev, &wpriv->cdev[led_type]);
> > > + if (err < 0)
> > > + goto led_reg_failed;
> > > +
> > > + break;
> > > + case AUDIO_MUTE:
> > > + if (led_version != WMI_LUD_SUPPORT_AUDIOMUTE_LED_VER) {
> > > + err = -EIO;
> > > + goto led_error;
> > > + }
> > > + wpriv->cdev[led_type].name = "platform::mute";
> > > + wpriv->cdev[led_type].brightness_set_blocking =
> > > &lsh_wmi_audiomute_led_set;
> > > + wpriv->cdev[led_type].default_trigger = "audio-mute";
> > > +
> > > + err = devm_led_classdev_register(dev, &wpriv->cdev[led_type]);
> > > + if (err < 0)
> > > + goto led_reg_failed;
> > > +
> > > + break;
> > > + default:
> > > + err = -EINVAL;
> > > + dev_err(dev, "Unknown LED type %d\n", led_type);
> >
> > Add include.
> For this comment, I understand that it need to add include for dev_err(). If
> so, my comments is below. If you think that it still need to add the include
> include/linux/dev_printk.h, pls comfirm it again, I'll add it in next version
> patch.
> dev_err() is defined in include/linux/dev_printk.h which is included by
> include/linux/device.h which is included by include/linux/wmi.h which is
> included by this file as below.
Hi,
Even if there's an indirect include path through other headers, you should
include linux/dev_printk.h also here. There's nothing fundamendal that
requires device.h to include dev_printk.h even if it currently does so
relying on these indirect includes is fragile (and hopefully somebody
eventually cleans up the entire include mess).
Same applies to the other case I marked below so please add those as
well.
There are some guaranteed include paths through another header and
I normally won't flag those, e.g., wmi.h will always have to include
device.h because struct device resides within struct wmi_device (it won't
build at all if device.h include is removed from wmi.h unless something
else by chance included device.h before wmi.h).
--
i.
> >> @@ -0,0 +1,248 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * Lenovo Super Hotkey Utility WMI extras driver for Ideapad laptop
> >> + *
> >> + * Copyright (C) 2025 Lenovo
> >> + */
> >> +
> >> +#include <linux/leds.h>
> >> +#include <linux/wmi.h>
>
> >
> > > + goto led_error;
> > > + }
> > > +
> > > + return 0;
> > > +
> > > +led_reg_failed:
> > > + dev_err(dev, "Could not register mute LED %d : %d\n", led_type, err);
> > > +
> > > +led_error:
> > > + kfree(obj);
> >
> > Add include.
> For this comment, I understand that it need to add include for kfree(). If so,
> my comments is below. If you think that it still need to add the include
> include/linux/slab.h, pls comfirm it again, I'll add it in next version patch
> or change to another function.
> kfree() is defined in include/linux/slab.h and the include chain is below.
> include/linux/slab.h -> include/acpi/platform/aclinux.h ->
> include/acpi/platform/acenv.h -> include/linux/acpi.h ->
> include/linux/wmi.h
> and wmi.h is included by this file as below.
>
> >> @@ -0,0 +1,248 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * Lenovo Super Hotkey Utility WMI extras driver for Ideapad laptop
> >> + *
> >> + * Copyright (C) 2025 Lenovo
> >> + */
> >> +
> >> +#include <linux/leds.h>
> >> +#include <linux/wmi.h>
>
> >
> > > + return err;
> > > +}
> > > +
> > > +static void lenovo_super_hotkey_wmi_leds_setup(struct device *dev)
> > > +{
> > > + lenovo_super_hotkey_wmi_led_init(MIC_MUTE, dev);
> > > + lenovo_super_hotkey_wmi_led_init(AUDIO_MUTE, dev);
> > > +}
> > > +
> > > +static int lenovo_super_hotkey_wmi_probe(struct wmi_device *wdev, const
> > > void *context)
> > > +{
> > > + struct lenovo_super_hotkey_wmi_private *wpriv;
> > > +
> > > + wpriv = devm_kzalloc(&wdev->dev, sizeof(*wpriv), GFP_KERNEL);
> > > + if (!wpriv)
> > > + return -ENOMEM;
> > > +
> > > + *wpriv = *(const struct lenovo_super_hotkey_wmi_private *)context;
> > > +
> > > + dev_set_drvdata(&wdev->dev, wpriv);
> > > +
> > > + if (wpriv->event == LSH_WMI_EVENT_LUD_KEYS) {
> > > + wpriv->led_wdev = wdev;
> > > + lenovo_super_hotkey_wmi_leds_setup(&wdev->dev);
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static void lenovo_super_hotkey_wmi_remove(struct wmi_device *wdev)
> > > +{
> > > + struct lenovo_super_hotkey_wmi_private *wpriv =
> > > dev_get_drvdata(&wdev->dev);
> > > + unsigned int i;
> > > +
> > > + for (i = 0; i < LSH_ACPI_LED_MAX; i++)
> > > + led_classdev_unregister(&wpriv->cdev[i]);
> > > +
> > > + kfree(wpriv);
> > > +}
> > > +static const struct lenovo_super_hotkey_wmi_private
> > > lsk_wmi_context_lud_keys = {
> > > + .event = LSH_WMI_EVENT_LUD_KEYS
> > > +};
> > > +
> > > +static const struct wmi_device_id lenovo_super_hotkey_wmi_id_table[] = {
> > > + { LUD_WMI_METHOD_GUID, &lsk_wmi_context_lud_keys }, /* Utility data */
> > > + { }
> > > +};
> > > +
> > > +MODULE_DEVICE_TABLE(wmi, lenovo_super_hotkey_wmi_id_table);
> >
> > Add include.
> For this comment, I understand that it need to add include for
> MODULE_DEVICE_TABLE(). If so, my comment is below. If you think that it still
> need to add the include/linux/module.h, pls comfirm it again, I'll add it in
> next version patch.
> MODULE_DEVICE_TABLE() is defined in include/linux/module.h which is included
> by include/linux/acpi.h which is included by include/linux/wmi.h which is
> included by this file as below.
>
> >> @@ -0,0 +1,248 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * Lenovo Super Hotkey Utility WMI extras driver for Ideapad laptop
> >> + *
> >> + * Copyright (C) 2025 Lenovo
> >> + */
> >> +
> >> +#include <linux/leds.h>
> >> +#include <linux/wmi.h>
>
>
> >
> > > +static struct wmi_driver lenovo_super_hotkey_wmi_driver = {
> > > + .driver = {
> > > + .name = "lenovo_super_hotkey_wmi",
> > > + .probe_type = PROBE_PREFER_ASYNCHRONOUS
> > > + },
> > > + .id_table = lenovo_super_hotkey_wmi_id_table,
> > > + .probe = lenovo_super_hotkey_wmi_probe,
> > > + .remove = lenovo_super_hotkey_wmi_remove,
> > > + .no_singleton = true,
> > > +};
> > > +
> > > +module_wmi_driver(lenovo_super_hotkey_wmi_driver);
> > > +
> > > +MODULE_INFO(depends, "wmi,ideapad-laptop");
> > > +MODULE_AUTHOR("Jackie Dong <dongeg1@...ovo.com>");
> > > +MODULE_DESCRIPTION("Lenovo Super Hotkey Utility WMI extras driver");
> > > +MODULE_LICENSE("GPL");
> > >
> >
>
>
Powered by blists - more mailing lists