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

[Armin Wolf]

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;
> > > +}
>
> (...)