Message-ID: <CAFqHKTna+kJpHLo5s4Fm1TmHcSSqSTr96JHDm0DJ0dxsZMkixA@mail.gmail.com>
Date: Fri, 27 Dec 2024 15:18:45 -0800
From: Derek John Clark <derekjohn.clark@...il.com>
To: John Martens <johnfanv2@...il.com>
Cc: w_armin@....de, corbet@....net, 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

On Wed, Dec 25, 2024 at 4:18 PM John Martens <johnfanv2@...il.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

Powered by blists - more mailing lists