linux-kernel - Re: [PATCH 0/1] platform/x86: Add Lenovo Legion WMI Drivers

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAFqHKTnOA5N-uADQLbdA-b+k-TOMdjZtCPsFsCo9jarMiNioLg@mail.gmail.com>
Date: Fri, 27 Dec 2024 19:30:39 -0800
From: Derek John Clark <derekjohn.clark@...il.com>
To: Antheas Kapenekakis <lkml@...heas.dev>
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

On Fri, Dec 27, 2024 at 5:10 PM Antheas Kapenekakis <lkml@...heas.dev> wrote:

> They need to re-agree given the context below. It is one thing for
> them to agree on it theoretically for settings that they might imagine
> are persistent and another thing when in reality they are not.
>
> You did not address this in your comment here.
>
> The problem I am raising is that SPL, SPPT, and FPPT specifically are
> not persistent enough to meet the guidelines of that interface.
>
> [1] and [2] do not address this either.
>
> I do not have an alternative planned, just noting that I'd like
> everyone to be on the same page before we go ahead with this ABI.

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.

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

> 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);

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

> It's good that you will be fixing that/I hope you will be fixing that.
> It is not clear from your comment. Please try to skip the capability
> call too.

I was pretty clear...

>> v2 will not need this call more than once during probe, and gamezone
> > will not be called by Other Method ever.

Derek