| 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
| ||
|
Message-Id: <D9UF8OXHEJKZ.23PW2J8J7VYSZ@gmail.com>
Date: Mon, 12 May 2025 16:16:12 -0300
From: "Kurt Borja" <kuurtb@...il.com>
To: "Antheas Kapenekakis" <lkml@...heas.dev>,
<platform-driver-x86@...r.kernel.org>
Cc: "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 10/10] platform/x86: msi-wmi-platform: Restore fan
curves on PWM disable and unload
On Sun May 11, 2025 at 5:44 PM -03, Antheas Kapenekakis wrote:
> MSI software is a bit weird in that even when the manual fan curve is
> disabled, the fan speed is still somewhat affected by the curve. So
> we have to restore the fan curves on unload and PWM disable, as it
> is done in Windows.
>
> Suggested-by: Armin Wolf <W_Armin@....de>
> Signed-off-by: Antheas Kapenekakis <lkml@...heas.dev>
> ---
> drivers/platform/x86/msi-wmi-platform.c | 123 +++++++++++++++++++++++-
> 1 file changed, 122 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/platform/x86/msi-wmi-platform.c b/drivers/platform/x86/msi-wmi-platform.c
> index 7dafe17d4d6be..a917db0300c06 100644
> --- a/drivers/platform/x86/msi-wmi-platform.c
> +++ b/drivers/platform/x86/msi-wmi-platform.c
> @@ -123,16 +123,25 @@ struct msi_wmi_platform_quirk {
> bool shift_mode; /* Shift mode is supported */
> bool charge_threshold; /* Charge threshold is supported */
> bool dual_fans; /* For devices with two hwmon fans */
> + bool restore_curves; /* Restore factory curves on unload */
> int pl_min; /* Minimum PLx value */
> int pl1_max; /* Maximum PL1 value */
> int pl2_max; /* Maximum PL2 value */
> };
>
> +struct msi_wmi_platform_factory_curves {
> + u8 cpu_fan_table[32];
> + u8 gpu_fan_table[32];
> + u8 cpu_temp_table[32];
> + u8 gpu_temp_table[32];
> +};
> +
> struct msi_wmi_platform_data {
> struct wmi_device *wdev;
> struct msi_wmi_platform_quirk *quirks;
> struct mutex wmi_lock; /* Necessary when calling WMI methods */
> struct device *ppdev;
> + struct msi_wmi_platform_factory_curves factory_curves;
> struct acpi_battery_hook battery_hook;
> struct device *fw_attrs_dev;
> struct kset *fw_attrs_kset;
> @@ -219,6 +228,7 @@ static struct msi_wmi_platform_quirk quirk_gen1 = {
> .shift_mode = true,
> .charge_threshold = true,
> .dual_fans = true,
> + .restore_curves = true,
> .pl_min = 8,
> .pl1_max = 43,
> .pl2_max = 45
> @@ -227,6 +237,7 @@ static struct msi_wmi_platform_quirk quirk_gen2 = {
> .shift_mode = true,
> .charge_threshold = true,
> .dual_fans = true,
> + .restore_curves = true,
> .pl_min = 8,
> .pl1_max = 30,
> .pl2_max = 37
> @@ -507,6 +518,94 @@ static struct attribute *msi_wmi_platform_hwmon_attrs[] = {
> };
> ATTRIBUTE_GROUPS(msi_wmi_platform_hwmon);
>
> +static int msi_wmi_platform_curves_save(struct msi_wmi_platform_data *data)
> +{
> + int ret;
> +
> + data->factory_curves.cpu_fan_table[0] =
> + MSI_PLATFORM_FAN_SUBFEATURE_CPU_FAN_TABLE;
> + ret = msi_wmi_platform_query_unlocked(
> + data, MSI_PLATFORM_GET_FAN,
> + data->factory_curves.cpu_fan_table,
> + sizeof(data->factory_curves.cpu_fan_table));
> + if (ret < 0)
> + return ret;
> + data->factory_curves.cpu_fan_table[0] =
> + MSI_PLATFORM_FAN_SUBFEATURE_CPU_FAN_TABLE;
Is it necessary to set the subfeature again here (and bellow)?
Also there is a lot of code repetition here, I would suggest a helper
function. It will be optimized/inlined by the compiler anyway.
> +
> + data->factory_curves.gpu_fan_table[0] =
> + MSI_PLATFORM_FAN_SUBFEATURE_GPU_FAN_TABLE;
> + ret = msi_wmi_platform_query_unlocked(
> + data, MSI_PLATFORM_GET_FAN,
> + data->factory_curves.gpu_fan_table,
> + sizeof(data->factory_curves.gpu_fan_table));
> + if (ret < 0)
> + return ret;
> + data->factory_curves.gpu_fan_table[0] =
> + MSI_PLATFORM_FAN_SUBFEATURE_GPU_FAN_TABLE;
> +
> + data->factory_curves.cpu_temp_table[0] =
> + MSI_PLATFORM_FAN_SUBFEATURE_CPU_TEMP_TABLE;
> + ret = msi_wmi_platform_query_unlocked(
> + data, MSI_PLATFORM_GET_TEMPERATURE,
> + data->factory_curves.cpu_temp_table,
> + sizeof(data->factory_curves.cpu_temp_table));
> + if (ret < 0)
> + return ret;
> + data->factory_curves.cpu_temp_table[0] =
> + MSI_PLATFORM_FAN_SUBFEATURE_CPU_TEMP_TABLE;
> +
> + data->factory_curves.gpu_temp_table[0] =
> + MSI_PLATFORM_FAN_SUBFEATURE_GPU_TEMP_TABLE;
> + ret = msi_wmi_platform_query_unlocked(
> + data, MSI_PLATFORM_GET_TEMPERATURE,
> + data->factory_curves.gpu_temp_table,
> + sizeof(data->factory_curves.gpu_temp_table));
> + if (ret < 0)
> + return ret;
> + data->factory_curves.gpu_temp_table[0] =
> + MSI_PLATFORM_FAN_SUBFEATURE_GPU_TEMP_TABLE;
> +
> + return 0;
> +}
> +
> +static int msi_wmi_platform_curves_load(struct msi_wmi_platform_data *data)
> +{
> + u8 buffer[32] = { };
> + int ret;
> +
> + memcpy(buffer, data->factory_curves.cpu_fan_table,
> + sizeof(data->factory_curves.cpu_fan_table));
> + ret = msi_wmi_platform_query_unlocked(data, MSI_PLATFORM_SET_FAN,
> + buffer, sizeof(buffer));
> + if (ret < 0)
> + return ret;
A helper for this operation would be nice too.
> +
> + memcpy(buffer, data->factory_curves.gpu_fan_table,
> + sizeof(data->factory_curves.gpu_fan_table));
> + ret = msi_wmi_platform_query_unlocked(data, MSI_PLATFORM_SET_FAN,
> + buffer, sizeof(buffer));
> + if (ret < 0)
> + return ret;
> +
> + memcpy(buffer, data->factory_curves.cpu_temp_table,
> + sizeof(data->factory_curves.cpu_temp_table));
> + ret = msi_wmi_platform_query_unlocked(
> + data, MSI_PLATFORM_SET_TEMPERATURE, buffer, sizeof(buffer));
> + if (ret < 0)
> + return ret;
> +
> + memcpy(buffer, data->factory_curves.gpu_temp_table,
> + sizeof(data->factory_curves.gpu_temp_table));
> + ret = msi_wmi_platform_query_unlocked(
> + data, MSI_PLATFORM_SET_TEMPERATURE, buffer, sizeof(buffer));
> + if (ret < 0)
> + return ret;
> +
> + return 0;
> +}
> +
> +
> static umode_t msi_wmi_platform_is_visible(const void *drvdata, enum hwmon_sensor_types type,
> u32 attr, int channel)
> {
> @@ -603,9 +702,19 @@ static int msi_wmi_platform_write(struct device *dev, enum hwmon_sensor_types ty
> return -EINVAL;
> }
>
> - return msi_wmi_platform_query_unlocked(
> + ret = msi_wmi_platform_query_unlocked(
> data, MSI_PLATFORM_SET_AP, buffer,
> sizeof(buffer));
> + if (ret < 0)
> + return ret;
> +
> + if (val == 2 && data->quirks->restore_curves) {
> + ret = msi_wmi_platform_curves_load(data);
> + if (ret < 0)
> + return ret;
> + }
> +
> + return 0;
> default:
> return -EOPNOTSUPP;
> }
> @@ -1373,6 +1482,13 @@ static int msi_wmi_platform_probe(struct wmi_device *wdev, const void *context)
>
> msi_wmi_platform_profile_setup(data);
>
> + if (data->quirks->restore_curves) {
> + guard(mutex)(&data->wmi_lock);
We don't need locking here. data->factory_curves is not shared until you
register the hwmon device.
> + ret = msi_wmi_platform_curves_save(data);
> + if (ret < 0)
> + return ret;
> + }
> +
> return msi_wmi_platform_hwmon_init(data);
> }
>
> @@ -1382,6 +1498,11 @@ static void msi_wmi_platform_remove(struct wmi_device *wdev)
>
> if (data->quirks->charge_threshold)
> battery_hook_unregister(&data->battery_hook);
> +
> + if (data->quirks->restore_curves) {
> + guard(mutex)(&data->wmi_lock);
We can avoid locking here by adding a devm action that restores the
curves. devm resources are unloaded in LIFO order.
Please, also check my comments on [Patch 2]. I don't think that patch is
needed.
--
~ Kurt
> + msi_wmi_platform_curves_load(data);
> + }
> }
>
> static const struct wmi_device_id msi_wmi_platform_id_table[] = {
Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)
Powered by blists - more mailing lists