Re: [PATCH] hwmon: (cros_ec): Hide unconfigured temperature thresholds

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Transient EC communication errors during driver probe permanently hide thermal threshold sysfs attributes.
--

--- Patch [1]: [PATCH] hwmon: (cros_ec): Hide unconfigured temperature thresholds ---
commit de61a2547d7ffb8ebe260dea1df08c0623451745
Author: Daniel Schaefer <[email protected]>

hwmon: (cros_ec): Hide unconfigured temperature thresholds

This commit updates the cros_ec_hwmon driver to hide sysfs temperature
threshold attributes when the EC reports they are unconfigured (0 K). It
achieves this by evaluating the threshold value during the is_visible
callback and returning 0 when the threshold is 0 or if the read fails.

> diff --git a/drivers/hwmon/cros_ec_hwmon.c b/drivers/hwmon/cros_ec_hwmon.c
> index 03bfcc40bb7c3..57158443dafe6 100644
> --- a/drivers/hwmon/cros_ec_hwmon.c
> +++ b/drivers/hwmon/cros_ec_hwmon.c

[ ... ]

> @@ -334,8 +335,17 @@ static umode_t cros_ec_hwmon_is_visible(const void *data, enum hwmon_sensor_type
>  	} else if (type == hwmon_temp) {
>  		if (priv->temp_sensor_names[channel]) {
>  			if (cros_ec_hwmon_attr_is_temp_threshold(attr)) {
> -				if (priv->temp_threshold_supported)
> -					return 0444;
> +				if (!priv->temp_threshold_supported)
> +					return 0;
> +
> +				if (cros_ec_hwmon_read_temp_threshold(priv->cros_ec, channel,
> +								      cros_ec_hwmon_attr_to_thres(attr),
> +								      &threshold) != 0)
> +					return 0;

[Severity: Medium]
Will this permanently hide valid thermal threshold attributes if there
is a transient EC communication error during driver probe?

The hwmon subsystem calls the is_visible callback during registration.
If cros_ec_hwmon_read_temp_threshold() encounters a temporary transport
error (such as -EIO or -ETIMEDOUT), it returns a non-zero error code.

Since this check returns 0 on any error, the attribute is permanently
suppressed. Should this perhaps only return 0 if the error is exactly
-EOPNOTSUPP, similar to how hwmon_fan_target is handled earlier in
this function?

> +				if (threshold == 0)
> +					return 0;
> +
> +				return 0444;
>  			} else {
>  				return 0444;
>  			}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1