Message-ID: 
 <CAGwozwF79xYrWkCSKpBaLSiXNEZz-5tmayWMbkw-of4zB=LPUQ@mail.gmail.com>
Date: Sat, 28 Dec 2024 12:50:34 +0100
From: Antheas Kapenekakis <lkml@...heas.dev>
To: 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, W_Armin@....de
Subject: Re: [PATCH 0/1] platform/x86: Add Lenovo Legion WMI Drivers

> 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.

> > 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.

> > 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.

Best,
Antheas

[1] https://github.com/hhd-dev/hwinfo/tree/master/devices/legion_go#get-feature-command

Powered by blists - more mailing lists