Message-Id: <D9UF0KCST7K7.23TWU22S6L09H@gmail.com>
Date: Mon, 12 May 2025 16:05:35 -0300
From: "Kurt Borja" <kuurtb@...il.com>
To: "Antheas Kapenekakis" <lkml@...heas.dev>
Cc: <platform-driver-x86@...r.kernel.org>, "Armin Wolf" <W_Armin@....de>,
 "Jonathan Corbet" <corbet@....net>, "Hans de Goede" <hdegoede@...hat.com>,
 Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>, "Jean
 Delvare" <jdelvare@...e.com>, "Guenter Roeck" <linux@...ck-us.net>,
 <linux-doc@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
 <linux-hwmon@...r.kernel.org>
Subject: Re: [PATCH v1 00/10] platform/x86: msi-wmi-platform: Add fan
 curves/platform profile/tdp/battery limiting

On Mon May 12, 2025 at 7:16 AM -03, Antheas Kapenekakis wrote:
> On Mon, 12 May 2025 at 01:30, Kurt Borja <kuurtb@...il.com> wrote:
>>
>> Hi Antheas,
>>
>> On Sun May 11, 2025 at 5:44 PM -03, Antheas Kapenekakis wrote:
>> > This draft patch series brings into parity the msi-wmi-platform driver with
>> > the MSI Center M Windows application for the MSI Claw (all models).
>> > Unfortunately, MSI Center M and this interface do not have a discovery API,
>> > necessitating the introduction of a quirk system.
>> >
>> > While this patch series is fully functional and tested, there are still
>> > some issues that need to be addressed:
>> >   - Armin notes we need to disable fan curve support by default and quirk
>> >     it as well, as it is not supported on all models. However, the way
>> >     PWM enable ops work, this makes it a bit difficult, so I would like
>> >     some suggestions on how to rework this.
>>
>> If I understood the question correctly, then you should control the
>> visibility of all "curve" related attributes with the quirk.
>
> Yep, this is what I was wondering. I will investigate this. It would
> be good to get some comments on the quirk naming as well.

You can check [1] for an example of the hwmon visibility. It's a similar
problem because some models have 2 fans and others have 4. In the
alienware-wmi driver we also have custom hwmon attributes, see [2] for
how to handle visibility with those.

I would personally just name it fan_curve or has_fan_curve.

>
>> The custom hwmon attribute_group has an is_visible callback, and so do
>> the hwmon_ops.
>>
>> >   - It turns out that to fully disable the fan curve, we have to restore
>> >     the default fan values. This is also what is done on the OEM software.
>> >     For this, the last patch in the series is used, which is a bit dirty.
>>
>> I have a couple questions about this.
>>
>> * What are the default fan curves? Can these be statically defined?
>> * Are user-defined fan curves persistent between reboots?
>>
>> I have some doubts about the approach you took on the last patch, but I
>> want to understand how the platform works first.
>
> So do I. Essentially here is how the Windows software works: when it
> first opens, it saves the current curve in Windows registry. Then,
> when the user sets a fan curve, it applies it in the same way we do
> here and sets a bit in AP. When the custom curve is removed, it unsets
> that bit and restores the original curve in WMI.
>
> The logical reasoning would be that that bit controls the fan curve.
> This is how it is named in the software. However, when setting that
> bit on its own, it seems to only partially affect the fan curve. E.g.,
> when the fan curve is 100% in all points, unsetting that bit makes it
> go down to 50% when no load occurs. When using the default fan curve,
> it goes to 0%. Therefore, it seems like that bit makes the fan curve
> semi-autonomous?
>
> The fan curve seems to be hardware specific and resets after reboots.
> So a straightforward way to get it is to grab it on a fresh boot.

Oh - this is interesting. Then I think it is the right approach. I'll
add a couple more comments.

>
> Antheas
>
>> >
>> > Sleep was tested with all values being preserved during S0iX (platform
>> > profile, fan curve, PL1/PL2), so we do not need suspend/resume hooks, at
>> > least for the Claw devices.
>> >
>> > For PL1/PL2, we use firmware-attributes. So for that I +cc Kurt since if
>> > his new high level interface is merged beforehand, we can use that instead.
>>
>> Hopefully!
>>
>> --
>>  ~ Kurt
>>
>> >
>> > Antheas Kapenekakis (8):
>> >   platform/x86: msi-wmi-platform: Add unlocked msi_wmi_platform_query
>> >   platform/x86: msi-wmi-platform: Add quirk system
>> >   platform/x86: msi-wmi-platform: Add platform profile through shift
>> >     mode
>> >   platform/x86: msi-wmi-platform: Add PL1/PL2 support via firmware
>> >     attributes
>> >   platform/x86: msi-wmi-platform: Add charge_threshold support
>> >   platform/x86: msi-wmi-platform: Drop excess fans in dual fan devices
>> >   platform/x86: msi-wmi-platform: Update header text
>> >   platform/x86: msi-wmi-platform: Restore fan curves on PWM disable and
>> >     unload
>> >
>> > Armin Wolf (2):
>> >   platform/x86: msi-wmi-platform: Use input buffer for returning result
>> >   platform/x86: msi-wmi-platform: Add support for fan control
>> >
>> >  .../wmi/devices/msi-wmi-platform.rst          |   26 +
>> >  drivers/platform/x86/Kconfig                  |    3 +
>> >  drivers/platform/x86/msi-wmi-platform.c       | 1181 ++++++++++++++++-
>> >  3 files changed, 1156 insertions(+), 54 deletions(-)
>> >
>> >
>> > base-commit: 62b1dcf2e7af3dc2879d1a39bf6823c99486a8c2
>>

[1] https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/tree/drivers/platform/x86/dell/alienware-wmi-wmax.c?h=for-next#n742
[2] https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/tree/drivers/platform/x86/dell/alienware-wmi-wmax.c?h=for-next#n942

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists