Am 28.12.24 um 12:50 schrieb Antheas Kapenekakis:
I'll let them weigh in on this again if they want to, but I think it
was clear from those threads that this is a new way to use the class.
Armin's comment was related to the fan curve setting John was
discussing, which is specifically covered by the hwmon subsystem.
Hwmon does not cover platform profiles or PPT.
I quote the following from Armin:
The firmware-attribute class interface is only intended for attributes which are persistent
and cannot be exposed over other subsystem interfaces.
The former part is not met here.
If Ilpo and Hans agree to extend the scope of the firmware-attribute class to also cover non-persistent
firmware settings then i will not resist.
To rephrase, your ABI style is not intuitive, because it contains
implementation details such as "gamezone", "capdata01", and "Other
Method", in addition to the ABI being hardcoded to the WMI structure
lenovo uses. The documentation uses those keywords as well.
Yeah, it's a driver for those interfaces... If you want an agnostic
BMOF driver then make one. This isn't that.
It's a driver for Legion Go and Legion laptops. _Not_ those
interfaces. Which only exist in gen 7+ if I recall from John's driver.
That was my comment.
Establishing an ABI that works with older laptops and laptops that
supersede those interfaces would be beneficial I'd say.
Excuse me for asking a stupid question here, but what WMI interfaces exactly are we currently arguing about?
If I understand correctly your last sentence, Armin suggested much of
the same (ie combine and merge).
You don't seem to, no. The suggestion was to use the component driver
API to aggregate the Other Method driver with the capability data
driver so that the firmware-attributes class is only loaded when both
are present. That is decidedly different from breaking the kernel's
WMI driver requirements and merging two GUID's into one driver.
GUID tables loading != drivers loading also, I would not pin that on
the kernel.
What exactly do you think the following does?
+MODULE_DEVICE_TABLE(wmi, gamezone_wmi_id_table);
Call the probe function that can -ENODEV
I do not understand what "I hard code the page to custom" means.
If you mean the capability data does not change you are right, they
are hardcoded in the decompiled ACPI I am pretty sure (it has been
close to a year now so I might be forgetting).
The capability data interface has a data block instance for every
attribute in every fan mode. SPL has one for quiet, balanced,
performance, and custom. The method for getting that data block (page)
is the same as calling get/set in Other Method (0x01030100 -
0x0103FF00). Every page produces different values for each attribute,
but I am only ever retrieving the instance for custom (0x0103FF00) as
that's the only one where setting that method ID in Other Method
changes the values on the Legion Go. It is the only relevant data for
userspace. Other Gaming Series laptops might treat this differently,
where every fan mode has an applicable range. I'll need to do more
testing on other hardware to confirm that. In any case, this isn't
relevant as I'm dropping the gamezone check (as I've stated multiple
times in this discussion) and always setting/getting the custom method
ID for a given attribute.
Hm, for some reason I missed the capability block when doing my RE
[1]. Feel free to reference when making the driver.
You should also provision for the fact some legion laptops have an
extreme mode which is stubbed on the Legion Go
Ok,
let's wrap up this discussion and put a bow on it.
I currently have two issues that block me from committing to your
driver: novel use of kernel APIs/design and performance
degradation/instability from unnecessary calls and checks, as those
are (i) slower (ii) could error out (iii) could have incorrect data.
The former can leave me with tech debt if your proposed ABI is vetoed
and the latter would result in a degraded experience for my users; I
would be putting in work to go backwards. I do not mention missing
features, as that is something I could have also worked on if I
committed to your driver.
Therefore, I'm left in a situation where I have to wait for buy-in
from kernel maintainers and for your V2, hoping it fixes the latter
issue which you said it will only do partly.
Regarding the firmware-attributes: if Ilpo and Hans give their OK, then i see no problems in using the firmware-attribute class for
non-persistent firmware settings. I for my part would be OK with such a change.
Regarding the enforcement of firmware limits: i believe that caching those limits during probing would solve the performance problem.
If users want to override those limits when we can add a module param (marked as unsafe to taint the kernel if used) later which tells
the driver to ignore those limits when writing firmware settings.
Any important points which i missed?
Thanks,
Armin Wolf
Best,
Antheas
[1] https://github.com/hhd-dev/hwinfo/tree/master/devices/legion_go#get-feature-command
