Re: [PATCH v1 10/10] platform/x86: msi-wmi-platform: Restore fan curves on PWM disable and unload

["Kurt Borja" <kuurtb@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 7473 bytes --]

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@gmx.de>
> Signed-off-by: Antheas Kapenekakis <lkml@antheas.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[] = {


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]