Re: [PATCH 1/2] hwmon: (cros_ec) Implement custom kelvin to celsius conversions

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing hwmon core serialization in thermal cooling device callbacks, causing potential race conditions.
--

commit ee3fdfc1d667eaecb682b673010b74478f9bb80e
Author: Thomas Weißschuh <linux@weissschuh.net>

hwmon: (cros_ec) Implement custom kelvin to celsius conversions

The ChromeOS EC APIs use integers representing degrees kelvin for
temperatures. This commit implements a custom conversion which
preserves round values.

> diff --git a/drivers/hwmon/cros_ec_hwmon.c b/drivers/hwmon/cros_ec_hwmon.c
> index ea24056ae6467..44291799dd8e4 100644
> --- a/drivers/hwmon/cros_ec_hwmon.c
> +++ b/drivers/hwmon/cros_ec_hwmon.c

[ ... ]

> @@ -228,7 +236,7 @@ static int cros_ec_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
>  								cros_ec_hwmon_attr_to_thres(attr),
>  								&threshold);
>  			if (ret == 0)
> -				*val = kelvin_to_millicelsius(threshold);
> +				*val = cros_ec_hwmon_kelvin_to_millicelsius(threshold);
>  		}
>  	}
>  

While reviewing this driver, I noticed the following in the same file:

> static int cros_ec_hwmon_cooling_set_cur_state(struct thermal_cooling_device *cdev,
> 					       unsigned long val)
> {
> 	const struct cros_ec_hwmon_cooling_priv *priv = cdev->devdata;
> 
> 	return cros_ec_hwmon_write_pwm_input(priv->hwmon_priv->cros_ec, priv->index, val);
> }

[Severity: High]
This is a pre-existing issue, but does this cooling device callback need to
take the hwmon lock to prevent race conditions?

The thermal subsystem calls cros_ec_hwmon_cooling_set_cur_state()
asynchronously, which bypasses the hwmon core serialization.

Looking at cros_ec_hwmon_write_pwm_input(), it performs a read-modify-write
sequence on the EC PWM state:

cros_ec_hwmon_write_pwm_input()
    cros_ec_hwmon_read_pwm_enable(cros_ec, index, &control_method);
    if (control_method != 1) 
        return -EOPNOTSUPP;
    cros_ec_hwmon_set_fan_pwm_val(cros_ec, index, val);

Can this race with a concurrent sysfs write modifying the fan PWM, leading
to an incorrect fan control state? 

The hardware monitoring subsystem guidelines require drivers to use
hwmon_lock() and hwmon_unlock() for attributes registered outside the core
info parameter, such as these thermal cooling state callbacks.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260630-cros_ec-hwmon-overflow-v1-0-3d2ecd3eb0f2@weissschuh.net?part=1