Re: [PATCH 5/5] hwmon: (cros_ec) Allow modification of fan curves

Am 04.06.26 um 11:04 schrieb Thomas Weißschuh:
> On 2026-05-30 18:37:32+0200, Armin Wolf wrote:
>> Am 29.05.26 um 22:31 schrieb Thomas Weißschuh:
> 
> (...)
> 
>>> +static ssize_t temp_auto_point_temp_store(struct device *dev, struct device_attribute *attr,
>>> +					  const char *buf, size_t size)
>>> +{
>>> +	struct sensor_device_attribute_2 *sattr = to_sensor_dev_attr_2(attr);
>>> +	struct cros_ec_hwmon_priv *priv = dev_get_drvdata(dev);
>>> +	struct ec_thermal_config config;
>>> +	u32 *temp_field;
>>> +	s64 temp;
>>> +	int ret;
>>> +
>>> +	ret = kstrtos64(buf, 10, &temp);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	temp = cros_ec_hwmon_millicelsius_to_kelvin(temp);
>>> +
>>> +	if (overflows_type(temp, config.temp_fan_off))
>>> +		return -ERANGE;
>>> +
>>> +	guard(hwmon_lock)(dev);
>>> +
>>> +	ret = cros_ec_hwmon_get_thermal_config(priv->cros_ec, sattr->index, &config);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	if (cros_ec_hwmon_attr_is_temp_fan_off(sattr))
>>> +		temp_field = &config.temp_fan_off;
>>> +	else /* temp_fan_max */
>>> +		temp_field = &config.temp_fan_max;
>>> +
>>> +	/* Only allow values which are more aggressive than the current ones */
>>> +	if (temp > *temp_field)
>>> +		return -EINVAL;
>>
>> i think it would be more practical for users to increase and later decrease the fan curve values.
>> Could the driver copy the original fan curve configuration and use that instead? This would also
>> require to restore the original fan curve during shutdown and removal.
> 
> That would be possible. We would would have to expose these limits
> through a new UAPI as otherwise the user has no way to know about them.
> Restoring the original on shutdown shouldn't be necessary, as the EC
> will reset the curves at shutdown anyways.

(And what about kexec?)

Ok, i myself would also interested in having a UAPI for communicating 
fan curve constraints to userspace as i am planning to add a similar 
feature to the uniwill-laptop driver.

I can think of two approaches:

1. Clamp the values into the supported range, userspace will have to 
read back the written value to know the current setting.

2. Introducing a new tempX_auto_pointY_temp_min attribute to communicate 
the constraint to userspace.

Guenter, do you have a preference for one of the approaches? Personally
i would prefer approach number 2.

> 
> I am not so sure that it would be generally useful though. Let's hear
> what other people think about it.

The uniwill-laptop driver will (likely) gain support for a similar 
feature in the future, so having such a UAPI would be beneficial.

Thanks,
Armin Wolf

>> Thanks,
>> Armin Wolf
>>
>>> +
>>> +	*temp_field = temp;
>>> +
>>> +	if (config.temp_fan_off > config.temp_fan_max)
>>> +		return -EINVAL;
>>> +
>>> +	ret = cros_ec_hwmon_set_thermal_config(priv->cros_ec, sattr->index, &config);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	return size;
>>> +}
> 
> (...)
>