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

[Guenter Roeck]

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.

> 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.

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 ?

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.

> > 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.

FWIW, as long as it does not reside in drivers/hwmon/, you can add any attribute
you want, and even violate the documented UAPI. That is one of the "advantages"
of having hwmon drivers reside outside drivers/hwmon/.

Guenter