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

[Thomas Weißschuh]

On 2026-06-08 06:34:43-0700, Guenter Roeck wrote:
> On 6/4/26 03:33, Armin Wolf wrote:
> > 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.
> > 
> 
> ... which is widely used in hwmon drivers, so it is not special.
> We don't usually expect userspace to know the valid attribute range.

That works for me.

What should be clamp to, though?
The range active during probe or the currently active one?

> > 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.
> > 
> 
> Again, we don't usually provide constraints for limit attributes
> to userspace. Otherwise we would need separate _min and _max attributes
> for literally every limit attribute. That would add a lot of complexity
> for little if any gain.

One constraint that exists today is the range accepted by kstrtol(),
which can return -ERANGE. Not that I want to argue that this is an
issue and requires change, just pointing it out.

> Worse, almost all attributes do not just have min/max constraints but
> step size constraints as well. Hardware does not typically accept values
> in millisecond/millivolt etc. but have varying step sizes. How would you
> express that ? The hardware won't accept a temperature of, say, 27.123
> degrees C. Hwmon drivers are expected to adjust that to the next supported
> value. That means userspace has to read the value back to know that value
> anyway. Or do you plan to provide the step size to userspace as well ?
> What would you do if the step size is non-linear ?

To me the step size argument is slightly different than the range one.
The difference between the values provided by the user and the one
actually used ones should be fairly small, while for the range it may be
much larger. Not that it makes a difference here.

> On top of that, _min and _max attributes are already associated with
> limits. I would find it confusing if new attributes would redefine that
> naming scheme to supported ranges.

Ack.

(...)