Re: [hwmon PATCH v2 3/8] hwmon: (adt7470) Fix busy-loop and I2C flooding in update thread

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

Pre-existing issues:
- [High] Setting `num_temp_sensors` to a non-negative value before temperatures have been probed permanently breaks temperature reporting and stalls sysfs reads.
--

--- Patch [3]: [hwmon PATCH v2 3/8] hwmon: (adt7470) Fix busy-loop and I2C flooding in update thread ---
commit 34430e742760a7d00b9b258ac39d10dadd63f0d0
Author: Luiz Angelo Daros de Luca <[email protected]>

hwmon: (adt7470) Fix busy-loop and I2C flooding in update thread

This commit fixes a busy-loop vulnerability in the background thread.
It clamps the minimum auto_update_interval to 500ms to guarantee a
reasonable minimum sleep window between sensor updates.

Reported-by: [email protected]
Closes: https://lore.kernel.org/r/[email protected]
Fixes: 89fac11cb3e7 ("adt7470: make automatic fan control really work")

> diff --git a/drivers/hwmon/adt7470.c b/drivers/hwmon/adt7470.c
> index 62ec68ea0a406..0b19b0925d1c7 100644
> --- a/drivers/hwmon/adt7470.c
> +++ b/drivers/hwmon/adt7470.c
> @@ -509,7 +509,7 @@ static ssize_t auto_update_interval_store(struct device *dev,
>  	if (kstrtol(buf, 10, &temp))
>  		return -EINVAL;
>  
> -	temp = clamp_val(temp, 0, 60000);
> +	temp = clamp_val(temp, 500, 60000);
>  
>  	mutex_lock(&data->lock);
>  	data->auto_update_interval = temp;

[Severity: High]
This is a pre-existing issue, but does setting num_temp_sensors before
temperatures are probed prevent temperature reporting and stall sysfs reads?

If userspace writes a non-negative value to the num_temp_sensors sysfs node
before data->temperatures_probed is set to 1, data->temperatures_probed
remains 0:

drivers/hwmon/adt7470.c:num_temp_sensors_store() {
	...
	data->num_temp_sensors = temp;
	if (temp < 0)
		data->temperatures_probed = 0;
	...
}

Because temperatures_probed is 0, adt7470_update_sensors() will always call
adt7470_read_temperatures() rather than a bulk read. Inside
adt7470_read_temperatures(), it checks the sensor count:

drivers/hwmon/adt7470.c:adt7470_read_temperatures() {
	...
	/* Only count fans if we have to */
	if (data->num_temp_sensors >= 0)
		return 0;

	err = regmap_bulk_read(data->regmap, ADT7470_TEMP_REG(0),
			       &data->temp[0], ADT7470_TEMP_COUNT);
	...
	data->temperatures_probed = 1;
	return 0;
}

Since num_temp_sensors >= 0, it returns early and never actually reads the
temperature registers or sets data->temperatures_probed = 1. This appears to
cause every subsequent sensor update to stall the driver with the probe sleep
time while never actually updating data->temp.

Could num_temp_sensors_store() also set data->temperatures_probed = 1 when
temp >= 0, or should adt7470_read_temperatures() read the registers
regardless of the num_temp_sensors value?

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