Am 10.01.25 um 22:52 schrieb Derek John Clark:
On Thu, Jan 9, 2025 at 3:20 PM Armin Wolf <W_Armin@xxxxxx> wrote:
Am 02.01.25 um 19:27 schrieb Derek John Clark:
On Wed, Jan 1, 2025 at 8:01 PM Mario Limonciello <superm1@xxxxxxxxxx> wrote:
On 1/1/25 18:47, Derek J. Clark wrote:
Adds support for the Lenovo "Gaming Series" of laptop hardware that 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
Mode", and "LENOVO_CAPABILITY_DATA_01" interfaces are implemented, but
I attempted to structure the driver so that adding the "Custom Mode",
"Lighting", and other data block interfaces would be trivial in a later
patches.
This driver is distinct from, but should be considered a replacement for
this patch:
https://lore.kernel.org/all/20241118100503.14228-1-jonmail@xxxxxxx/
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@xxxxxxxxxx/
The drivers have been tested by me on the Lenovo Legion Go.
v2:
- Broke up initial patch into a 4 patch series.
- Removed all references to "Legion" in documentation, Kconfig,
driver structs, functions, etc. Everything now refers either to the
interface being used or the Lenovo "Gaming Series" of laptop hardware.
- Fixed all Acked changes requested by Mario and Armin.
- Capability Data is now cached before kset creation for each attribute.
If the lenovo-wmi-capdata01 interface is not present, fails to grab
valid data, doesn't include the requested attribute id page, or the
data block indicates the attribute is not supported, the attribute will
not be created in sysfs.
- The sysfs path for the firmware-attributes class was moved from
lenovo-legion-wmi to lenovo-wmi-other.
- The Other Mode WMI interface no longer relies on gamezone as
discussed. However; this creates a problem that should be discussed
here. The current_value attribute is now only accurate when the
"custom" profile is set on the device. Previously it would report the
value from the Capability Data 01 instance related to the currently
selected profile, which reported an accurate accounting of the current
system state in all cases. I submitted this as-is since we discussed
removing that dependency, but I am not a fan of the current_value
attribute being incorrect for 3 of the 4 available profiles, especially
when the data is available. There is also no way to -ENOTSUPP or
similar when not in custom mode as that would also require us to know
the state of the gamezone interface. What I would prefer to do would be
to make the gamezone interface optional by treating custom as the
default mode in the current_value functions, then only update the mode
if a callback to get the current fan profile is a success. That way the
logic will work with or without the GameZone interface, but it will be
greatly improved if it is present.
I agree there needs to be /some/ sort of dependency.
One thing I was thinking you could do is use:
wmi_has_guid() to tell whether or not the "GZ" interface is even present
from the "Other" driver. Move the GUID for the GZ interface into a
common header both drivers include.
However that only helps in the case of a system that supports custom but
not GZ. I think you still will need some sort of symbol to either get a
pointer to the platform profile class or tell if the profile for the
driver is set to custom.
I personally don't see a problem with a simple symbol like this:
bool lenovo_wmi_gamezone_is_custom(void);
You could then have your logic in all the store and show call a helper
something like this:
static bool lenovo_wmi_custom_mode() {
if (!wmi_has_guid(GZ_GUID)
return true;
if (!IS_REACHABLE(CONFIG_LENOVO_WMI_GAMEZONE))
return true;
return lenovo_wmi_gamezone_is_custom();
}
I agree with checking wmi_has_guid() before calling anything across
interfaces.
Please do not use wmi_has_guid() for this as WMI devices can disappear
at any time.
As far as using a bool to determine if we are in custom,
that seems to me like that would be a half measure. Since we would be
calling across interfaces anyway there is a benefit to getting the
full scope, where knowing only if we are in custom or not would just
add the ability to exit early. What I would prefer is knowing the
specific state of the hardware as it will allow me to call the
specific method ID as related to the current profile. I'll elaborate a
bit on what I mean.
Each attribute ID corresponds to a specific fan profile mode for a
specific attribute. It is used as both the data block ID in
LENOVO_CAPABILITY_DATA_01, and as the first argument when using
GetFeatureValue/SetFeatureValue on the Other Mode interface. I map
these with the lenovo_wmi_attr_id struct. The fan mode value provided
by the gamezone interface corresponds directly to the mode value in
the ID. For example, ID 0x01010100 would provide the capability data
for the CPU device (0x01), SPPT (0x01), in Quiet mode (0x01). There is
no type ID for these attributes (0x00) like there are on some
unimplemented attributes. Balanced mode is 0x02, Performance is 0x03,
Extreme mode (Which the Go doesn't use and there is no analogue for in
the kernel atm) is 0xE0, and custom mode is 0xFF. When the
GetSmartFanMode method ID is called on the gamezone interface it
returns one of these values, corresponding to the current state of the
hardware. This allows us to call GetFeatureValue for the current
profile. Currently we are always calling the custom mode method ID
(0x0101FF00) in GetFeatureValue.
If we want to avoid an additional wmi call in GZ, then grabbing it
from the platform profile and translating it back would maybe suffice.
In that case I would need to implement the
LENOVO_GAMEZONE_SMART_FAN_MODE_EVENT GUID
"D320289E-8FEA-41E0-86F9-611D83151B5F" to ensure that the profile is
updated properly when the hardware is switched profiles using the
physical buttons. This is probably a good idea anyway, but some
guidance on implementing that would be nice as I think it would be an
additional driver and then we have more cross referencing.
I attached a prototype WMI driver for another device which had a similar problem.
The solution was to provide a notifier so other event consumers can be notified
when an WMI event was received.
Example event consumer callback code:
static int uniwill_wmi_notify_call(struct notifier_block *nb, unsigned long action, void *data)
{
if (action != UNIWILL_OSD_PERF_MODE_CHANGED)
return NOTIFY_DONE;
platform_profile_cycle();
return NOTIFY_OK;
}
I would also suggest that you use a notifier for communicating with the gamezone
interface. Then you just have to submit commands (as action values) in the form of events
which will then be processed by the available gamezone drivers (the result can be stored in *data).
Those gamezone drivers can then return NOTIFY_STOP which will ensure that only a single gamezone
driver can successfully process a given command.
All in all the patch series seems to progress nicely. I am confident that we will solve the remaining issues.
Thanks,
Armin Wolf
That's a novel approach. There are some EVENT GUID's for the gamezone
interface I'll need to incorporate to keep everything in sync. These
devices have physical buttons (Fn+Q on laptops, Legion +Y button on
handhelds) to cycle the profiles. I didn't add this previously because
we were always updating it when called. I presume that each GUID will
need a separate driver for this. Any advice or examples on how to use
this to update the pprof in GameZone would be appreciated as I've
never used .notify before.
The WMI driver inside the attachment should be a suitable starting point.
You can also reuse the same driver for many different GUIDs and do the following:
- use the context inside the wmi_device_id to find out which GUID is being probed.
You can use drivers/platform/x86/xiaomi-wmi.c as an example.
- inside the .notify callback parse the event data and the call the notifier.
You can use the action parameter to signal which kind of WMI event was received (SMART_FAN_MODE_EVENT, ...)
and the data parameter to pass the event data.
With this you only need to provide a single WMI driver.
The lenovo-wmi-gamezone driver can then register with this notifier and listen for
platform profile changes:
static int lenovo_gz_notify_call(struct notifier_block *nb, unsigned long action, void *data)
{
if (action != SMART_FAN_MODE_EVENT) // Filter events
return NOTIFY_DONE;
<check *data if necessary>
platform_profile_cycle(); // Cycle platform profile if necessary
return NOTIFY_OK;
}
My expected information flow will be these paths:
Physical Button press -> WMI event GUID notifier driver -> Gamezone
driver update & notify_call -> Other Mode save data to priv for lookup
when current_value is checked and return STOP .
or
platform-profile class write from sysfs -> Gamezone driver update &
notify_call ->Other Mode save data to priv for lookup when
current_value is checked and return STOP .
Thanks,
Derek
Your approach would have a problem: how to communicate the initial platform profile state
when lenovo-wmi-other probes?
I suggest that lenovo-wmi-gamezone stores the current platform profile. This value can then
be retrieved by lenovo-wmi-other by using the special gamezone notifier. This would also allow
lenovo-wmi-other to detect when lenovo-wmi-gamezone is not ready and can thus not provide
platform profile data.
Thanks,
Armin Wolf
The simplest solution IMO would be to do something closer to what I
was doing in v1 just for current_value_show, where we instantiate the
mode variable as SMARTFAN_MODE_CUSTOM (0xFF) then check if the gz
interface is present. If it is, pass the mode variable as a pointer to
GZ where it can call GetSmartFanMode and update the value. Otherwise,
bypass that block and treat it as custom. This does add an additional
WMI call, but only when reading the current_value.
- I did extensive testing of this firmware-attributes interface and its
ability to retain the value set by the user. The SPL, SPPT, FPPT, and
platform profile all retain the users last setting when resuming from
suspend, a full reboot, and a full shutdown. The only time the values
are not preserved is when the user manually selects a new platform
profile using either the pprof interface or the manual selection
button on the device, in which case you would not expect them to be
retained as they were intentionally changed. Based on the previous
discussion it may be the case that older BIOS' will preserve the
settings even after changing profiles, though I haven't confirmed
this.
This is good to hear considering the concerns raised by some others.
But FWIW we have nothing in the firmware attributes API documentation
that mandates what the firmware does for storage of settings across a
power cycle so this is currently up to the platform to decide.
v1:
https://lore.kernel.org/platform-driver-x86/CAFqHKTna+kJpHLo5s4Fm1TmHcSSqSTr96JHDm0DJ0dxsZMkixA@xxxxxxxxxxxxxx/T/#t
Suggested-by: Mario Limonciello <superm1@xxxxxxxxxx>
Signed-off-by: Derek J. Clark <derekjohn.clark@xxxxxxxxx>
Derek J. Clark (4):
platform/x86: Add lenovo-wmi drivers Documentation
platform/x86: Add Lenovo GameZone WMI Driver
platform/x86: Add Lenovo Capability Data 01 WMI Driver
platform/x86: Add Lenovo Other Mode WMI Driver
Documentation/wmi/devices/lenovo-wmi.rst | 104 ++++++
MAINTAINERS | 9 +
drivers/platform/x86/Kconfig | 34 ++
drivers/platform/x86/Makefile | 3 +
drivers/platform/x86/lenovo-wmi-capdata01.c | 131 +++++++
drivers/platform/x86/lenovo-wmi-gamezone.c | 203 +++++++++++
drivers/platform/x86/lenovo-wmi-other.c | 385 ++++++++++++++++++++
drivers/platform/x86/lenovo-wmi.h | 241 ++++++++++++
8 files changed, 1110 insertions(+)
create mode 100644 Documentation/wmi/devices/lenovo-wmi.rst
create mode 100644 drivers/platform/x86/lenovo-wmi-capdata01.c
create mode 100644 drivers/platform/x86/lenovo-wmi-gamezone.c
create mode 100644 drivers/platform/x86/lenovo-wmi-other.c
create mode 100644 drivers/platform/x86/lenovo-wmi.h
