Message-ID: <eb165321-6a52-4464-bb58-11d8f9ff2008@gmx.de>
Date: Fri, 27 Dec 2024 00:19:08 +0100
From: Armin Wolf <W_Armin@....de>
To: John Martens <johnfanv2@...il.com>
Cc: corbet@....net, derekjohn.clark@...il.com, hdegoede@...hat.com,
 ilpo.jarvinen@...ux.intel.com, linux-doc@...r.kernel.org,
 linux-kernel@...r.kernel.org, luke@...nes.dev, mpearson-lenovo@...ebb.ca,
 nijs1@...ovo.com, pgriffais@...vesoftware.com,
 platform-driver-x86@...r.kernel.org, shaohz1@...ovo.com, superm1@...nel.org
Subject: Re: [PATCH 0/1] platform/x86: Add Lenovo Legion WMI Drivers

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.

Powered by blists - more mailing lists