Message-ID: <b7089d69-4d7b-42fb-90b3-bd13a27fcf1e@gmx.de>
Date: Sun, 29 Dec 2024 21:45:40 +0100
From: Armin Wolf <W_Armin@....de>
To: Antheas Kapenekakis <lkml@...heas.dev>,
 Derek John Clark <derekjohn.clark@...il.com>
Cc: 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,
 zhangzx36@...ovo.com, johnfanv2@...il.com, codyit@...il.com
Subject: Re: [PATCH 0/1] platform/x86: Add Lenovo Legion WMI Drivers

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

Powered by blists - more mailing lists