Message-Id: <20241226001828.423658-1-johnfanv2@gmail.com>
Date: Thu, 26 Dec 2024 01:18:28 +0100
From: John Martens <johnfanv2@...il.com>
To: w_armin@....de
Cc: corbet@....net,
	derekjohn.clark@...il.com,
	hdegoede@...hat.com,
	ilpo.jarvinen@...ux.intel.com,
	johnfanv2@...il.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


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.

Powered by blists - more mailing lists