Re: [PATCH 7/7] platform/x86: asus-wmi: show the effective duty for enabled curves

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] platform/x86: asus-wmi: missing fan_curve_notify() on curve disable paths
- [Low] platform/x86: asus-wmi: duplicate fan_curve_notify() on curve disable
--

commit d8679565f78ed87d40eb465628808366a670ad22
Author: Bartu Alev <bartualev@gmail.com>

platform/x86: asus-wmi: show the effective duty for enabled curves

This commit changes fan_curve_show() to return the maximum of the stored duty
and the firmware floor duty for pwmY_auto_pointZ_pwm when a custom fan curve
is enabled, and adds sysfs notifications for pwmY_auto_pointZ_pwm on curve
enable, disable, and floor refresh transitions.

> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index 74476904cb99c..bae5a528ac5d8 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
[ ... ]
> @@ -3754,7 +3754,7 @@ static int fan_curve_load_floors(struct asus_wmi *asus, u32 fan_dev)
>  	return 0;
>  }
>  
> -static void fan_curve_notify_floors(struct asus_wmi *asus)
> +static void fan_curve_notify(struct asus_wmi *asus)
>  {
>  	struct device *hwmon = asus->fan_curve_hwmon;
>  	bool valid[ARRAY_SIZE(asus->custom_fan_curves)];
> @@ -3778,6 +3778,10 @@ static void fan_curve_notify_floors(struct asus_wmi *asus)
>  				 "pwm%d_auto_point%d_floor_pwm",
>  				 fan + 1, point + 1);
>  			sysfs_notify(&hwmon->kobj, NULL, name);
> +			snprintf(name, sizeof(name),
> +				 "pwm%d_auto_point%d_pwm",
> +				 fan + 1, point + 1);
> +			sysfs_notify(&hwmon->kobj, NULL, name);
>  		}
>  	}
>  }
> @@ -3797,7 +3801,7 @@ static void fan_curve_refresh_floors(struct asus_wmi *asus)
>  	if (asus->mid_fan_curve_available)
>  		fan_curve_load_floors(asus, ASUS_WMI_DEVID_MID_FAN_CURVE);
>  
> -	fan_curve_notify_floors(asus);
> +	fan_curve_notify(asus);
>  }
>  
>  /* Check if the capability exists, populate its defaults and read its floor table */
> @@ -3855,15 +3859,28 @@ static ssize_t fan_curve_show(struct device *dev,
>  	struct asus_wmi *asus = dev_get_drvdata(dev);
>  	struct fan_curve_data *data;
>  	int value, pwm, index;
> +	bool enabled;
>  
>  	data = fan_curve_attr_2_select(asus, attr);
>  	pwm = dev_attr->nr & FAN_CURVE_PWM_MASK;
>  	index = dev_attr->index;
>  
> -	if (pwm)
> +	if (pwm) {
> +		enabled = data->enabled;
>  		value = data->percents[index];
> -	else
> +
> +		/*
> +		 * The EC runs the point at the higher of the curve and the
> +		 * firmware floor, so report the duty it applies.  Only the
> +		 * floor table is protected by fan_curve_lock; enabled and
> +		 * percents are written without it, so this is a snapshot.
> +		 */
> +		guard(mutex)(&asus->fan_curve_lock);
> +		if (enabled && data->floors_valid)
> +			value = max(value, (int)data->floors[index]);
> +	} else {
>  		value = data->temps[index];
> +	}
>  
>  	return sysfs_emit(buf, "%d\n", value);
>  }

[Severity: Medium]
Now that fan_curve_show() returns either the floor-clamped duty or the raw
stored duty depending on data->enabled, are sysfs notifications missing on
other paths that clear data->enabled?

Writing to any point via fan_curve_store() disables the curve without calling
fan_curve_notify():

drivers/platform/x86/asus-wmi.c:fan_curve_store() {
	...
	if (pwm)
		data->percents[index] = value;
	else
		data->temps[index] = value;

	data->enabled = false;

	return count;
}

Toggling the main hwmon fan mode in pwm1_enable_store() also disables all
custom fan curves without calling fan_curve_notify():

drivers/platform/x86/asus-wmi.c:pwm1_enable_store() {
	...
	/* Must set to disabled if mode is toggled */
	if (asus->cpu_fan_curve_available)
		asus->custom_fan_curves[FAN_CURVE_DEV_CPU].enabled = false;
	if (asus->gpu_fan_curve_available)
		asus->custom_fan_curves[FAN_CURVE_DEV_GPU].enabled = false;
	if (asus->mid_fan_curve_available)
		asus->custom_fan_curves[FAN_CURVE_DEV_MID].enabled = false;

	return count;
}

Similarly, in fan_curve_enable_store(), if data->enabled was true and gets
set to false before an early error return (such as in fan_curve_validate() or
the reset branches), fan_curve_notify() at the end of the function is
bypassed.

Should these paths also notify pollers when an enabled curve transitions to
disabled so they do not keep stale floor-clamped values?

> @@ -4059,6 +4076,9 @@ static ssize_t fan_curve_enable_store(struct device *dev,
>  			fan_curve_refresh_floors(asus);
>  		}
>  	}
> +
> +	fan_curve_notify(asus);
> +
>  	return count;
>  }

[Severity: Low]
Does calling fan_curve_notify(asus) unconditionally at the end of
fan_curve_enable_store() emit duplicate notifications whenever a curve is
disabled?

When data->enabled is false, all three branches of the else block already
invoke fan_curve_refresh_floors(asus) (either directly or through
throttle_thermal_policy_write(asus) -> fan_curve_refresh_floors(asus)), which
itself calls fan_curve_notify(asus):

drivers/platform/x86/asus-wmi.c:fan_curve_enable_store() {
	...
	if (data->enabled) {
		...
	} else {
		if (asus->throttle_thermal_policy_dev) {
			err = throttle_thermal_policy_write(asus);
			if (err)
				return err;
		} else if (asus->fan_type == FAN_TYPE_SPEC83) {
			err = asus_fan_set_auto(asus);
			if (err)
				return err;
			fan_curve_refresh_floors(asus);
		} else {
			...
			fan_curve_refresh_floors(asus);
		}
	}

	fan_curve_notify(asus);
}

In addition, when enabling a single fan curve, fan_curve_notify(asus) notifies
both pwmY_auto_pointZ_floor_pwm and pwmY_auto_pointZ_pwm across all fans with
valid floors, even though the floor tables were not reloaded and the other
fans did not change state. Could the enable path notify only the affected
fan's pwm attributes, and avoid the second fan_curve_notify(asus) call on the
disable path?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261101-asus-fan-floors-v4-v1-0-b8d1c31a99a9@gmail.com?part=7