Message-ID: <CAFqHKT=Y66KNo-e+o+n76tmPEcqL4EBSUQNDXJcoP8B9NXguew@mail.gmail.com>
Date: Fri, 27 Dec 2024 15:22:35 -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 10:48 AM Antheas Kapenekakis <lkml@...heas.dev> wrote:
Subject: Re: [PATCH 1/1] platform/x86: Add lenovo-legion-wmi drivers

>Hi Derek,

Seasons Greetings, Antheas.

>Good job on the driver. I spent a bit of time reviewing it today, and I
>have a few thoughts. Hopefully you can go through them fast, and we can
>have a solid base of understanding moving forward so that I can e.g.,
>merge your driver on Bazzite ahead of the kernel, so you can get some
>valuable testing.
>
>// Firmware Attributes
>Right now, I have one major concern I'd like to be addressed before
>moving into the details of the driver. This is mirrored in the Asus
>driver you referenced [1] as well and I do not think I have seen solid
>argumentation for it yet.
>
>Essentially, you are using the firmware attributes interface for the
>ephemeral attributes SPL, SPPT, and FPPT which is also a concern mirrored
>in the thread with Armin (+cc) and John (+cc) [2]. The question here
>becomes, if exposing fan curve attributes via the firmware interface is
>fickle, why is it correct for SPL, SPPT, and FPPT?

See [1] and [2] below. Hans de Goede and Ilpo Järvinen signed off on using it
this way for the asus-armoury driver, as proposed by Mario. This class is the
"most" correct one currently present in the kernel. If you think an alternative
solution would make more sense then I would encourage you to create that
interface and submit it. In the meantime I am going to move forward with what
has already been discussed and, if you manage to get in in before this series
is approved, then I will adjust. Otherwise, please CC me on the patch that
changes this driver to include it.

>For context, when it comes to Asus (see [1]), these tunables reset during
>reboots, after sleep, and when changing the TDP mode. Specifically, TDP
>mode, SPL, SPPT, and FPPT, and the fan curve are not preserved between
>reboots, with only TDP mode being preserved between sleep. Following,
>setting TDP mode resets SPL, SPP, and FPPT, and setting any one of those
>resets the fan curve.
>
>For Lenovo and the Legion Go (1st Gen) it actually depends on the BIOS
>version. In early BIOSes, the custom tunings used to be preserved until
>you reset the BIOS. However, I suspect that this caused used confusion, as
>if you uninstalled Legion Space, the tunings to custom mode would still be
>applied which would be unintuitive. In new BIOS versions, the tunings reset
>when changing TDP modes, which can be done with the combination Legion
>L + Y. I am unsure if they are still preserved between reboots if you
>remain in custom mode. Fan curve resets as well. But the point is that they
>are not persisted, at least to the extent expected by the firmware
>attributes class and writing to them in other modes is undefined.
>
>Lastly, if another driver were to be developed e.g., for AMD out-of-tree
>to control devices without a vendor interface, we'd have the same
>issue. As this driver or [1] merging would set a precedent for using
>firmware attributes for these tunings, I think it is important to reach
>agreement before moving forward. For AMD w/o vendor, more details can
>be found in [3]. In [3], Mario makes an RFC for an alt interface for SPL,
>SPPT, and FPPT, through amd_pmf which does not have this peculiar issue.
>
>There are settings where it does make sense however, such as VRAM, boot
>sound (Asus), and the suspend light/barrel plug RGB (Legion). These
>settings are typically persisted in BIOS and there is no ambiguity for
>fitness in those.
>

>// Driver Details
>Ok, as for the rest of the driver, I have (i) some stylistic comments and
>(ii) will mirror similar concerns to John in unnecessary accesses.
>
>(i) Stylistic Comments
>I would personally be a fan of adding WMI support to the kernel, such that
>userspace can access WMI attributes without the use of a driver, as it is
>done in Windows. However, as I have discussed with Mario, such a thing is
>not an easy task, as it would require adding a BMOF parser to the kernel
>which is a monumental effort. In any case, the current kernel requires
>us custom write drivers for the WMI interface.
>
>The reason for this preface is that I think that your driver style is a bit
>"too close" to what such a thing would look. E.g., in the driver I can see
>snippets such as `LENOVO_CAPABILITY_DATA_01 WMI data block driver.` and
>`MODULE_IMPORT_NS("CAPDATA_WMI");`. Since you are going through the effort
>of writing a custom driver, I would be more opinionated in how I'd design
>the driver, so that it's more intuitive from a user's perspective. I think
>John's driver (which has its own issues) and the asus-wmi drivers strike a
>bit better balance, where they "translate" the WMI calls into an ABI that
>can be documented and then parsed by a developer.

I'm not sure what you are talking about exactly. Can you provide some sort of
example? The scope is quite specific as it is, and will be even more specific
once I include the requested changes. There is no ambiguity for the user on
what /sys/firmware/acpi/platform_profile or /sys/class/firmware-attributes/
lenovo-wmi-gamezone/attributes/ppt_pl1_spl are. As Armin suggested and I ack'd,
capdata01 and Other Method drivers will be linked as components in v2.

>Such a design process would then also allow to claim the name legion, as
>you could make sure the legion driver only loads on Legion laptops, where
>now it would randomly load in other laptops as well. Sidenote here is
>that this is something I also found confusing, as the Legion Go does not
>have a keyboard so that driver should definitely not load.

This is a consequence of the WMI design in the kernel as is. WMI GUID tables
only load on devices that implement them. If a device erroneously reports an
unimplemented GUID that is a problem with the BIOS and it needs (preferred) a
BIOS update and/or a DMI quirk to block it if loading it creates an issue. This
will be device specific.

>E.g., you could only load when you detect the Gamezone interface and then
>access the other two as well. The Gamezone interface is only used in
>Legion laptops AFAIK. I think Lenovo also kind of implements all three
>as stubs even if some are not used, to allow extending them with BIOS
>updates, so blindly loading drivers based on the interface might not be
>the best idea in any case. When the Legion Go released, I think only one
>of the three interfaces was used, for example, but all three were present.

If we run into a situation where that check passes on a device because the
interface produces a valid ACPI object that says it is supported even though
the interface isn't fully implemented then we can quirk it. Also, this is
contradictory to your point (ii) where you insist these calls are unnecessary.

>(ii) Unnecessary Accesses
>Even though you went hands off on your design of the driver, at the same
>time you held back when it comes to userspace accesses by globally forcing
>the Other function hints as limits. This creates two issues.
>
>The first one is that those limits might be wrong for certain devices or
>certain users might want to go a bit above them, which would mean that if
>you enforce them you'd need to provide a way to quirk them and a module
>parameter override (I know they are correct for the Legion Go as I have
>looked through them as well). But if it is not necessary in Windows,
>why would we add additional roadblocks in the kernel? If Lenovo feels the
>need to enforce them, they can do so in their firmware and extend that
>protection to Windows as well. Asus does when it comes to their fan curve
>firmware, for example.

Unless you have some concrete examples of this actually occuring I'm inclined
to assume that Lenovo knows how to implement their interface. If we do find a
device that has a broken CAPDATA interface, again we can quirk it.
Notwithstanding that, the data exists and it ensures that the interface isn't
abused to the point that hardware is pushed beyond its design limitations.
Mario or Xino can correct me here, but I expect AMD and Lenovo have an interest
in ensuring the kernel doesn't permit settings that can lead to additional
RMAs. For users who wish to disregard that there are solutions to modify this
data which void the warranty, as you are well aware. I'll also point out that
Lenovo controls the userspace software in Windows and never intended to support
Linux originally, so it is reasonable to assume this was just an oversight in
the design and they didn't expect anyone else would implement another WMI driver
or software when it was designed. In any case, My primary concern is that the
interface is safe and reliable.

>The second is that you are making a lot of necessary calls, which may harm
>performance or potentially cause instability. My workflow for setting TDP
>on the Go is that I first respect the TDP mode the user has set using
>Legion L + Y. If the user wants to change that, then I do the following:
>set TDP mode to custom, set the TDP, set the SPL, SPPT, and FPPT, and then
>set the fan curve. I also add a small delay in-between each of these calls
>as a further precaution. In your current driver, each of these calls would
>make two additional calls to the WMI interface (one for the limits and
>one for the TDP mode), which would more than double the number of calls
>made in a typical scenario (from 5 to 12), where each triplet is made
>back to back.
>
>There is also the issue mentioned by John, where you do cross-interface
>interactions etc. To fix this, you'd have to retrieve the limits from
>firmware on probing and then cache them in the driver after quirking. It
>would be much easier just to skip all of this for now and just use them to
>prepopulate _min and _max values as hints instead, which you already do.

I had already planned on making this change for other reasons, but I want to
dispel some misinformation you have presented here. First, I highly doubt
anyone is making enough calls to this interface that the extra 4ns are going
to even be noticed. I would say they are certainly using the interface
incorrectly if they do. Second, LENOVO_CAPABILITY_DATA_01 is a separate
interface, and it is (currently) only called once per attribute change. For
some background, this was originally added when I was permitting the current
fan profile to determine which capability data page was presented and set. In
the submitted version I hard code the page to custom as testing under quiet,
balanced, and performance showed no change. Technically it is an assumption
that this will be universally true for all Legion hardware, but the change was
made as the final approved version of Mario's patches which added the necessary
custom profile to platform_profile explicitly states in the documentation that
"custom" mode is not user selectable. As Armin pointed out, this only applies to
the legacy interface in /sys/firmware. In any case, you need not worry about
this as v2 will not need this call more than once during probe, and gamezone
will not be called by Other Method ever.

>Those were my comments. You mentioned that you are travelling and might not
>have access to your PC so take your time with the replies.
>
>Happy holidays,
>Antheas


[1] https://lore.kernel.org/all/e9f4fb37-5277-a7f0-2bec-8a6909b4e674@linux.intel.com/
[2] https://lore.kernel.org/all/07fe1bf8-f470-4e40-86ba-0f8d2e61a3e6@amd.com/

Cheers,
Derek

Powered by blists - more mailing lists