Message-ID: <fd250291-4102-4dcd-8448-d878c3a013bf@gmx.de>
Date: Sat, 11 Jan 2025 01:25:04 +0100
From: Armin Wolf <W_Armin@....de>
To: Derek John Clark <derekjohn.clark@...il.com>
Cc: Mario Limonciello <superm1@...nel.org>,
 Hans de Goede <hdegoede@...hat.com>,
 Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
 Jonathan Corbet <corbet@....net>, Luke Jones <luke@...nes.dev>,
 Xino Ni <nijs1@...ovo.com>, Zhixin Zhang <zhangzx36@...ovo.com>,
 Mia Shao <shaohz1@...ovo.com>, Mark Pearson <mpearson-lenovo@...ebb.ca>,
 "Pierre-Loup A . Griffais" <pgriffais@...vesoftware.com>,
 "Cody T . -H . Chiu" <codyit@...il.com>, John Martens <johnfanv2@...il.com>,
 platform-driver-x86@...r.kernel.org, linux-doc@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 0/4] platform/x86: Add Lenovo Gaming Series WMI Drivers

Am 10.01.25 um 22:52 schrieb Derek John Clark:

> On Thu, Jan 9, 2025 at 3:20 PM Armin Wolf <W_Armin@....de> wrote:
>> Am 02.01.25 um 19:27 schrieb Derek John Clark:
>>
>>> On Wed, Jan 1, 2025 at 8:01 PM Mario Limonciello <superm1@...nel.org> 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@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 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@mail.gmail.com/T/#t
>>>>>
>>>>> Suggested-by: Mario Limonciello <superm1@...nel.org>
>>>>> Signed-off-by: Derek J. Clark <derekjohn.clark@...il.com>
>>>>>
>>>>> 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
>>>>>

Powered by blists - more mailing lists