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: 
 <CAGwozwEWNkUDCzSq7-Lei1yBAjpQjyZUtW7+8n_Cpn9xd4aR3A@mail.gmail.com>
Date: Sun, 29 Dec 2024 23:41:26 +0100
From: Antheas Kapenekakis <lkml@...heas.dev>
To: Armin Wolf <W_Armin@....de>
Cc: Derek John Clark <derekjohn.clark@...il.com>, 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

Hi Armin,
indeed you covered everything.

I am a bit hesitant about binding sppt, fppt, and spl into those
interfaces as they need to be set in a very specific ordering and
rules. E.g., spl < sppt < fppt after setting tdp and before the fan
curve and after sleep maybe depending on device, after reboot maybe
after keybind (Legion L + Y) as well. Which is not what's expected by
the userspace programs consuming this interface. In addition, this
would expose them to perusing users where they might be confused. I
also know that its difficult by looking at a patch series to
understand the nature of these values. However, given my previous
email, you now have the full context you need to make a decision.
If you think it is appropriate, it is fine by me.

I'd personally stick them next to platform_profile with a /name
discoverability mechanism similar to hwmon, where tuning
software can find them (something similar to Mario's RFC
that I linked above). Other settings such as the bios light that
interface is perfectly good for.

As for the hardware limits. You are absolutely right, the ACPI eforces
none, incl. for Lenovo. And the quality is as you expect. For the
Legion Go, they are quite creative. They added a battery 80%
capacity limit by re-using the key value for booting from AC [1-2].
They also used a weird ABI for the lighting interface to turn off
the suspend light for a good half of the BIOSes, then they fixed it
when they allowed to turn off the suspend light during sleep as well,
which caused that option to break in Legion Space for I want to say
two months. Nevertheless, nobody has broken a Legion Go yet
messing with those settings by e.g., overclocking. It also brings
into view that while the Legion Go uses a derived Legion bios it
has started diverging a bit as it has its own vendor software.

So I would say that it is good that the other function has a discovery
mechanism and that gamezone has some bitmasks for that purpose as
well. It means that if we tap on them during probe, at least for
Legion laptops from the last 3 years, we can get pretty good support
from the get go. Before that, it is a mix of EC + WMI (see [3]).

In regards to firmware limits, it is something I would not include in
the first patch series as it will just make this harder to merge, esp.
if there are laptops with wrong limits. Then there are issues with
overrides etc. I would advertise the limits through _min, _max so we
can figure this out later and I would not do a runtime WMI check, as
we have to run the check during probe anyway to populate sysfs, where
it is natural to cache the limits.

FInally, if indeed the gamezone function is Legion specific, and the
key-value pairs of the Other function are legion specific, from a
stylistic perspective I would tend towards making the ABI of the
driver Legion specific and abstract away its WMI details. E.g., I'd
use the name legion-wmi for a combined driver instead of
lenovo-gamezone-wmi which would then not be useful if lenovo moves
past gamezone. And I'd make sure it only loads on legion laptops. I'm
not up to date on my WMI driver conventions, so this is just a
suggestion.

Best,
Antheas

[1] https://github.com/BartoszCichecki/LenovoLegionToolkit/blob/21c0e8ca8b98181a2dedbec1e436d695932a4b0f/LenovoLegionToolkit.Lib/Enums.cs#L72
[2] https://github.com/hhd-dev/adjustor/blob/188ef6c3e4d7020f2110dd29df6d78847026d41e/src/adjustor/core/lenovo.py#L241
[3] https://github.com/johnfanv2/LenovoLegionLinux