Adds support for the Lenovo Legion series of laptop hardware to use WMI
interfaces that control various power settings. There are multiple WMI
interfaces that work in concert to provide getting and setting values as
well as validation of input. Currently only the "GameZone", "Other
Method", and "LENOVO_CAPABILITY_DATA_01" interfaces are implemented, but
I attempted to structure the driver so that adding the "Custom Mode",
"Lighting", and the other CAPABILITY_DATA interfaces would be trivial if
desired in a later patch.

This driver is distinct from, but should be considered a replacement for
this patch:
https://lore.kernel.org/all/20241118100503.14228-1-jonmail@163.com/

This driver attempts to standardize the exposed sysfs by mirroring the
asus-armoury driver currently under review. As such, a lot of
inspiration has been drawn from that driver.
https://lore.kernel.org/all/20240930000046.51388-1-luke@ljones.dev/

The driver has been tested by me on the Lenovo Legion Go.

Suggested-by: Mario Limonciello <superm1@kernel.org>
Reviewed-by: Luke Jones <luke@ljones.dev>
Signed-off-by: Derek J. Clark <derekjohn.clark@gmail.com>

Derek J. Clark (1):
  Add lenovo-legion-wmi drivers

 .../wmi/devices/lenovo-legion-wmi.rst         |  79 ++++
 MAINTAINERS                                   |   9 +
 drivers/platform/x86/Kconfig                  |  35 ++
 drivers/platform/x86/Makefile                 |  21 +-
 .../x86/lenovo-legion-wmi-capdata01.c         | 103 +++++
 .../platform/x86/lenovo-legion-wmi-gamezone.c | 233 +++++++++++
 .../platform/x86/lenovo-legion-wmi-other.c    | 377 ++++++++++++++++++
 drivers/platform/x86/lenovo-legion-wmi.h      | 271 +++++++++++++
 8 files changed, 1119 insertions(+), 9 deletions(-)
 create mode 100644 Documentation/wmi/devices/lenovo-legion-wmi.rst
 create mode 100644 drivers/platform/x86/lenovo-legion-wmi-capdata01.c
 create mode 100644 drivers/platform/x86/lenovo-legion-wmi-gamezone.c
 create mode 100644 drivers/platform/x86/lenovo-legion-wmi-other.c
 create mode 100644 drivers/platform/x86/lenovo-legion-wmi.h

-- 
2.47.0

> Adds support for the Lenovo Legion series of laptop hardware to use WMI
> interfaces that control various power settings. 

Note that there already is a driver for Lenovo Legion laptops that I 
wanted to merge upstream.

https://github.com/johnfanv2/LenovoLegionLinux

Compared to the proposed patch, it has the following
advantages:
1. already popular and tested by thousands of users
    - many stars and discussions on github
    - patched into multiple kernels of gaming-related distros
    - packaged as dkms module for almost all relevant Linux 
      distributions including Debian by other developers
2. supports many different Lenovo Legion models starting from 2020/2021
3. supports a lot of more functions, including fan control, which is the
  most requested feature
4. supports the many changes between different in the WMI/ACPI method
5. actually shares some credtis with persons who revere engineered it :)
6. support by GUI tool to configure it all

On the other hand, my driver has the following disadvantages:
1. The version of master on github is the most recent one and contains
   a lot of debug output that has to be removed (often indicated by TODO)
2. It is all in one large c file instead of organizing it neatly into
   multiple files.
3. It was modeled after the ideapad driver instead of the newer ASUS
   driver.

A few notes regarding the many changes of the WMI methods that I tried 
to deal with in my driver: note that in almost every new model a new 
WMI method is used to control the same functionality (e.g. fan control
 or powermode). Additionally, often the constants or the unit changes
: e.g. percent or rpm for fan speed.

> The driver has been tested by me on the Lenovo Legion Go.

The driver on github has been tested by thousands of users.

I suggest that we maybe combine the two drivers before merging them,
since Derek seems to have more kernel patching knowledge and I seem
to have more worked on all the Legion laptops.

Am 22.12.24 um 09:42 schrieb John Martens:

>> Adds support for the Lenovo Legion series of laptop hardware to use WMI
>> interfaces that control various power settings.
> Note that there already is a driver for Lenovo Legion laptops that I
> wanted to merge upstream.
>
> https://github.com/johnfanv2/LenovoLegionLinux
>
> Compared to the proposed patch, it has the following
> advantages:
> 1. already popular and tested by thousands of users
>      - many stars and discussions on github
>      - patched into multiple kernels of gaming-related distros
>      - packaged as dkms module for almost all relevant Linux
>        distributions including Debian by other developers
> 2. supports many different Lenovo Legion models starting from 2020/2021
> 3. supports a lot of more functions, including fan control, which is the
>    most requested feature
> 4. supports the many changes between different in the WMI/ACPI method
> 5. actually shares some credtis with persons who revere engineered it :)
> 6. support by GUI tool to configure it all
>
> On the other hand, my driver has the following disadvantages:
> 1. The version of master on github is the most recent one and contains
>     a lot of debug output that has to be removed (often indicated by TODO)
> 2. It is all in one large c file instead of organizing it neatly into
>     multiple files.
> 3. It was modeled after the ideapad driver instead of the newer ASUS
>     driver.
>
> A few notes regarding the many changes of the WMI methods that I tried
> to deal with in my driver: note that in almost every new model a new
> WMI method is used to control the same functionality (e.g. fan control
>   or powermode). Additionally, often the constants or the unit changes
> : e.g. percent or rpm for fan speed.
>
>> The driver has been tested by me on the Lenovo Legion Go.
> The driver on github has been tested by thousands of users.
>
> I suggest that we maybe combine the two drivers before merging them,
> since Derek seems to have more kernel patching knowledge and I seem
> to have more worked on all the Legion laptops.
>
I agree in combining both drivers.

I suggest that we first upstream the bare-minimum for supporting the gamezone GUID
(no fancy features, just the basics) so that:

1. More features can be added later.

2. People can work on the other-guid drivers.

Sadly your WMI driver needs some work too since it uses the deprecated GUID-based WMI interface.
Because of this i suggest that we first upstream Dereks driver for the gamezone GUID, which can
then be extended by you step-by-step.

Thanks,
Armin Wolf

I guess the most important task is to get following points right because
they are hard to fix later.

1. Should there be a unifrom sysfs interface for different access methods?
Depending on the model different methods must be used to control the 
same feature, e.g. the powermode, fan table, dust-cleaning-mode. 
The access methods could be a different WMI method (newer model), 
direct ACPI without WMI, or port mapped IO (outb/inb). I suggest that 
regardless of the access methods it should be produce the same sysfs entry. 

Example: there is a fan-fullspeed-methods/dustcleaning-mode that 
sets the fans to the maximal possible speed. I suggest that regardless of 
the used access method there should be the one file:

/sys/class/firmware-attributes/*/attributes/fanfullspeed/current_value

Alternatively, one could use the less elegant approach:

/sys/class/firmware-attributes/*/attributes/wmi-other-fanfullspeed/current_value
/sys/class/firmware-attributes/*/attributes/wmi-gamezone-fanfullspeed/current_value
/sys/class/firmware-attributes/*/attributes/acpi1-fanfullspeed/current_value

2. Naming and file structure: As mentioned above, there different methods - 
including non-WMI methods - are used. Hence, it might not be optimal name
the driver "legion-wmi". One idea would be to name the folder/driver "legion"
and then seperate into multiple files by access methods (WMI by GUID, ACPI, 
port mapped IO). 

3. Driver Structure, selection of access method and probing: The right access
method (WMI, ACPI, ...) has to be chosen for each model. Some of them can
be automatically probed, some of them have to be hard coded (c.f. also Window
tools) by the letter-only prefix of the BIOS version. 

Depending on the driver structure there are multiple ideas how to manage this i
nformation:

a: global-access-into-driver-decide-by-enum: initially the driver can store
the method of access (WMI, ACPI, ...) for e.g. modifying fanfullspeed as
an enum/bitfield/... globally. The value can be decided on by probing and
some hard coded rules. There is one "glovbal" c-file that acts as an 
entrypoint into the driver and adds all the show/store functions. When the 
show-function is called it is decided e.g. by a switch statement which 
function in one of the different files (WMI, ACPI, ...) is called. 
The upside of this method is that if there are not warnings in the code, 
then every case is totally covered. The downside is a lot of boilerplate 
code.

b: global-access-into-driver-decide-by-function-pointer: Same as above
in case a, but direclty use function pointers instead of enum/bits. There
is one function pointers for each attribute in a "global" struct. When
the driver is loaded initially, it sets each function pointers to modify 
an attribute the right function for the model. The upside is 
less boilerplate. The downside is that it might get a little
less safe working with the function pointers.

c: independent-access-in-independent-driver-parts: the driver is split
into totally independent parts for each method (WMI, ACPI, ...) and GUID.
Each driver part is responsible for creating the sysfs entries. To
prevent conflicts each part has to use a unique name (see 1)
for the attribute. Alternatively, the choice of access has to be propagated 
down to each part to prevent creating the same sysfs attribute multiple
times. The upside is the elegance and easy extension. The downside
is the weird sysfs user-interface and the weird coupling between
the different driver parts.

d: totally independent drivers: make a totally independent driver
(module) for each access method.


Some more remarks: 
- I would never make one attribute depend on another
attribute, e.g. when changing some power parameters of GPU/CPU it 
should not change the power mode (e.g. going to custom mode). Initially,
I did the same but it turned out to be not a good idea. However,
if one changes some power settings and is not custom-powermode some
sometimes some weird things happen. Sometimes also all changes are 
ignored by the firmware as seen in the ACPI dissassembly. I guess
it is best to just manage this in user space.
- When trying to find out what access method to choose one cannot rely
on the ACPI/WMI interface. From disassembling the ACPI, one can see
that sometimes/often even if the function is not implemented it
will return without error. Moreover, there are some WMI methods
with name "*IsSupported" (or similar) but they often do not tell
the truth.
- Using just one WMI interface is simple — my grandmother could do it. 
However, when juggling and organizing the various access methods, your 
guidance is needed to set the driver on the right path from the beginning.
So I defenitely, appreciate your input on the different options.

On Wed, Dec 25, 2024 at 4:18 PM John Martens <johnfanv2@gmail.com> wrote:

>I guess the most important task is to get following points right because
>they are hard to fix later.
>
>1. Should there be a unifrom sysfs interface for different access methods?
>Depending on the model different methods must be used to control the
>same feature, e.g. the powermode, fan table, dust-cleaning-mode.
>The access methods could be a different WMI method (newer model),
>direct ACPI without WMI, or port mapped IO (outb/inb). I suggest that
>regardless of the access methods it should be produce the same sysfs entry.
>

>Example: there is a fan-fullspeed-methods/dustcleaning-mode that
>sets the fans to the maximal possible speed. I suggest that regardless of
>the used access method there should be the one file:
>
>/sys/class/firmware-attributes/*/attributes/fanfullspeed/current_value
>
>Alternatively, one could use the less elegant approach:
>
>/sys/class/firmware-attributes/*/attributes/wmi-other-fanfullspeed/current_value
>/sys/class/firmware-attributes/*/attributes/wmi-gamezone-fanfullspeed/current_value
>/sys/class/firmware-attributes/*/attributes/acpi1-fanfullspeed/current_value

I agree generally, though there are some limitations. As Armin mentioned, the
firmware-attributes interface will handle this with the pattern
/sys/class/firmware-attributes/<name>. <name> is derived when the class is
initialized. In v1 I used lenovo-legion-wmi but based on the discussion so far
I'll likely drop legion from all patterns and call everything based on its GUID
plain text name like Mark suggested, lenovo-wmi-other for Other Method,
lenovo-wmi-custom for Custom Method, etc. That will ensure two things:
(1) drivers won't conflict in namespace if there is an unused but present in
the bios older interface on a newer device.
(2) Userspace apps can determine the preferred interface for a given device if
more than one happens to be present. I don't think this will be very common,
but it can be handled if it does happen.

The goal of this driver, jointly with asus-armoury, is to standardize the
features that aren't already in the kernel while utilizing those that are. I.E.
AMD PPT doesn't yet have a standard interface, platform-profile does. The names
of the attributes that provide specific functionality will remain the same
between drivers (ppt_pl1_spl for example). As firmware-attributes is a class,
it is easy to use udev in userspace apps to avoid hard coding paths so that any
interface that provides these attributes will be interchangeable or include a
hierarchy table. In the event more than one "competing" driver loads the
userspace app can prioritize a specific interface and, if a genuine conflict
arises between two drivers on a specific device, it will be easy enough to
quirk that device to not load one or the other. The fanfullspeed attribute
should be able to follow this same pattern when implemented.

>2. Naming and file structure: As mentioned above, there different methods -
>including non-WMI methods - are used. Hence, it might not be optimal name
>the driver "legion-wmi". One idea would be to name the folder/driver "legion"
>and then seperate into multiple files by access methods (WMI by GUID, ACPI,
>port mapped IO).

Since each driver will be independent I don't see this being a problem.
lenovo-acpi-tunables or lenovo-i2c-tunables (as example) could coexist.
As above, each firmware-attributes path will be different but the attributes
within them would be standardized.

>3. Driver Structure, selection of access method and probing: The right access
>method (WMI, ACPI, ...) has to be chosen for each model. Some of them can
>be automatically probed, some of them have to be hard coded (c.f. also Window
>tools) by the letter-only prefix of the BIOS version.
>
>Depending on the driver structure there are multiple ideas how to manage this i
>nformation:
>
>a: global-access-into-driver-decide-by-enum: initially the driver can store
>the method of access (WMI, ACPI, ...) for e.g. modifying fanfullspeed as
>an enum/bitfield/... globally. The value can be decided on by probing and
>some hard coded rules. There is one "glovbal" c-file that acts as an
>entrypoint into the driver and adds all the show/store functions. When the
>show-function is called it is decided e.g. by a switch statement which
>function in one of the different files (WMI, ACPI, ...) is called.
>The upside of this method is that if there are not warnings in the code,
>then every case is totally covered. The downside is a lot of boilerplate
>code.
>
>b: global-access-into-driver-decide-by-function-pointer: Same as above
>in case a, but direclty use function pointers instead of enum/bits. There
>is one function pointers for each attribute in a "global" struct. When
>the driver is loaded initially, it sets each function pointers to modify
>an attribute the right function for the model. The upside is
>less boilerplate. The downside is that it might get a little
>less safe working with the function pointers.
>
>c: independent-access-in-independent-driver-parts: the driver is split
>into totally independent parts for each method (WMI, ACPI, ...) and GUID.
>Each driver part is responsible for creating the sysfs entries. To
>prevent conflicts each part has to use a unique name (see 1)
>for the attribute. Alternatively, the choice of access has to be propagated
>down to each part to prevent creating the same sysfs attribute multiple
>times. The upside is the elegance and easy extension. The downside
>is the weird sysfs user-interface and the weird coupling between
>the different driver parts.
>
>d: totally independent drivers: make a totally independent driver
>(module) for each access method.
>
>Some more remarks:
>- I would never make one attribute depend on another
>attribute, e.g. when changing some power parameters of GPU/CPU it
>should not change the power mode (e.g. going to custom mode). Initially,
>I did the same but it turned out to be not a good idea. However,
>if one changes some power settings and is not custom-powermode some
>sometimes some weird things happen. Sometimes also all changes are
>ignored by the firmware as seen in the ACPI dissassembly. I guess
>it is best to just manage this in user space.

I think that is the consensus going forward. In v2 each interface will be
independent of the others except when it relies on a WMI data block as part of
its data validation.

>- When trying to find out what access method to choose one cannot rely
>on the ACPI/WMI interface. From disassembling the ACPI, one can see
>that sometimes/often even if the function is not implemented it
>will return without error. Moreover, there are some WMI methods
>with name "*IsSupported" (or similar) but they often do not tell
>the truth.

Generally I prefer to use the path of least intervention and automatic
discovery with explicit deny lists, then explicit allow lists if needed. That
will limit the amount of work needed to support new hardware that implements
the same functionality. WMI interfaces for example should load automatically,
unless there is a restrictive DMI quirk for a specific model. If BIOS revision
is of concern then that driver will quirk an alternative method or deny the
driver overall. If automatic discovery isn't an option, such as EC writes with
inb/outb, then we would explicitly load it using a DMI table in that driver.
From my experience this seems to be the standard for kernel drivers. We will
need to rely on your historical data and Lenovo to get this right.

>- Using just one WMI interface is simple — my grandmother could do it.
>However, when juggling and organizing the various access methods, your
>guidance is needed to set the driver on the right path from the beginning.
>So I defenitely, appreciate your input on the different options.

Thanks John,
Derek

Am 26.12.24 um 01:18 schrieb John Martens:

> I guess the most important task is to get following points right because
> they are hard to fix later.
>
> 1. Should there be a unifrom sysfs interface for different access methods?
> Depending on the model different methods must be used to control the
> same feature, e.g. the powermode, fan table, dust-cleaning-mode.
> The access methods could be a different WMI method (newer model),
> direct ACPI without WMI, or port mapped IO (outb/inb). I suggest that
> regardless of the access methods it should be produce the same sysfs entry.
>
> Example: there is a fan-fullspeed-methods/dustcleaning-mode that
> sets the fans to the maximal possible speed. I suggest that regardless of
> the used access method there should be the one file:
>
> /sys/class/firmware-attributes/*/attributes/fanfullspeed/current_value
>
> Alternatively, one could use the less elegant approach:
>
> /sys/class/firmware-attributes/*/attributes/wmi-other-fanfullspeed/current_value
> /sys/class/firmware-attributes/*/attributes/wmi-gamezone-fanfullspeed/current_value
> /sys/class/firmware-attributes/*/attributes/acpi1-fanfullspeed/current_value

The firmware-attribute class interface is only intended for attributes which are persistent
and cannot be exposed over other subsystem interfaces.

The "current_value" attribute can be exposed using the hwmon subsystem. Also i assume that
the setting is not persistent across reboots (correct me if i am wrong).

Also depending on the driver the path will be:

/sys/class/firmware-attributes/<name>/attributes/<attr name>/current_value

Because of this i think having a separate interface for each driver is better. We can of course harmonize
the naming of the attributes where it makes sense.

>
> 2. Naming and file structure: As mentioned above, there different methods -
> including non-WMI methods - are used. Hence, it might not be optimal name
> the driver "legion-wmi". One idea would be to name the folder/driver "legion"
> and then seperate into multiple files by access methods (WMI by GUID, ACPI,
> port mapped IO).
>
> 3. Driver Structure, selection of access method and probing: The right access
> method (WMI, ACPI, ...) has to be chosen for each model. Some of them can
> be automatically probed, some of them have to be hard coded (c.f. also Window
> tools) by the letter-only prefix of the BIOS version.
>
> Depending on the driver structure there are multiple ideas how to manage this i
> nformation:
>
> a: global-access-into-driver-decide-by-enum: initially the driver can store
> the method of access (WMI, ACPI, ...) for e.g. modifying fanfullspeed as
> an enum/bitfield/... globally. The value can be decided on by probing and
> some hard coded rules. There is one "glovbal" c-file that acts as an
> entrypoint into the driver and adds all the show/store functions. When the
> show-function is called it is decided e.g. by a switch statement which
> function in one of the different files (WMI, ACPI, ...) is called.
> The upside of this method is that if there are not warnings in the code,
> then every case is totally covered. The downside is a lot of boilerplate
> code.
>
> b: global-access-into-driver-decide-by-function-pointer: Same as above
> in case a, but direclty use function pointers instead of enum/bits. There
> is one function pointers for each attribute in a "global" struct. When
> the driver is loaded initially, it sets each function pointers to modify
> an attribute the right function for the model. The upside is
> less boilerplate. The downside is that it might get a little
> less safe working with the function pointers.
>
> c: independent-access-in-independent-driver-parts: the driver is split
> into totally independent parts for each method (WMI, ACPI, ...) and GUID.
> Each driver part is responsible for creating the sysfs entries. To
> prevent conflicts each part has to use a unique name (see 1)
> for the attribute. Alternatively, the choice of access has to be propagated
> down to each part to prevent creating the same sysfs attribute multiple
> times. The upside is the elegance and easy extension. The downside
> is the weird sysfs user-interface and the weird coupling between
> the different driver parts.
>
> d: totally independent drivers: make a totally independent driver
> (module) for each access method.

I would prefer approach d. Ideally each driver would use standard subsystem interfaces (hwmon, backlight, firmware-attributes, platform-profile, etc)
so that userspace software can be written in a driver-agnostic way.

>
>
> Some more remarks:
> - I would never make one attribute depend on another
> attribute, e.g. when changing some power parameters of GPU/CPU it
> should not change the power mode (e.g. going to custom mode). Initially,
> I did the same but it turned out to be not a good idea. However,
> if one changes some power settings and is not custom-powermode some
> sometimes some weird things happen. Sometimes also all changes are
> ignored by the firmware as seen in the ACPI dissassembly. I guess
> it is best to just manage this in user space.

I agree.

> - When trying to find out what access method to choose one cannot rely
> on the ACPI/WMI interface. From disassembling the ACPI, one can see
> that sometimes/often even if the function is not implemented it
> will return without error. Moreover, there are some WMI methods
> with name "*IsSupported" (or similar) but they often do not tell
> the truth.

Oh no.

I hope that we just misinterpret the result of those methods. Otherwise this would indeed be
very frustrating. Maybe some help from Lenovo can solve this issue.

Thanks,
Armin Wolf

> - Using just one WMI interface is simple — my grandmother could do it.
> However, when juggling and organizing the various access methods, your
> guidance is needed to set the driver on the right path from the beginning.
> So I defenitely, appreciate your input on the different options.

On Thu, Dec 26, 2024, at 6:19 PM, Armin Wolf wrote:
> Am 26.12.24 um 01:18 schrieb John Martens:
>
<snip>
>
>> - When trying to find out what access method to choose one cannot rely
>> on the ACPI/WMI interface. From disassembling the ACPI, one can see
>> that sometimes/often even if the function is not implemented it
>> will return without error. Moreover, there are some WMI methods
>> with name "*IsSupported" (or similar) but they often do not tell
>> the truth.
>
> Oh no.
>
> I hope that we just misinterpret the result of those methods. Otherwise 
> this would indeed be
> very frustrating. Maybe some help from Lenovo can solve this issue.
>

Can someone (John?) send me the details off email list and I'll see if the Legion team can help.
(this comes with a caveat that the Legion platforms are not part of the official Linux program yet, so no promises or guarantees)

>
>> - Using just one WMI interface is simple — my grandmother could do it.

You have an awesome grandmother :D 

Mark