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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Module unload hang due to missed wakeup in kthread loop
- [High] Temperature alarm bitmask logic error causes false negatives and incorrect states
- [High] Unfreezable polling kthread can crash or hang the I2C bus during system suspend
- [High] Subsystem guideline violation: Driver uses private lock instead of hwmon_lock()
--

--- Patch [3]: [PATCH v2 3/6] hwmon: (adt7470) Fix busy-loop and I2C flooding in update thread ---
commit e6d87eed20a630165f01d0513e17b82247a7f775
Author: Luiz Angelo Daros de Luca <luizluca@gmail.com>

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

This commit prevents a busy-loop and I2C flooding in the background kthread
when userspace configures auto_update_interval and num_temp_sensors to 0.
It resolves the issue by raising the lower limit of the clamp_val in
auto_update_interval_store() from 0 to 500 milliseconds.

Closes: https://lore.kernel.org/r/20260716213252.EACA71F000E9@smtp.kernel.org

> diff --git a/drivers/hwmon/adt7470.c b/drivers/hwmon/adt7470.c
> index 481d51617f4be..77e7be6927906 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);

[Severity: High]
This isn't a bug introduced by this patch, but does the use of a private
mutex for an extra sysfs attribute violate the HWMON subsystem directive
to use the central hwmon_lock() and hwmon_unlock() for this purpose?

>  	data->auto_update_interval = temp;

I would also like to ask about some other pre-existing issues in this file:

drivers/hwmon/adt7470.c:adt7470_update_thread() {
    [ ... ]
	while (!kthread_should_stop()) {
		mutex_lock(&data->lock);
		adt7470_read_temperatures(data);
		mutex_unlock(&data->lock);

		if (kthread_should_stop())
			break;

		schedule_timeout_interruptible(msecs_to_jiffies(data->auto_update_interval));
	}
    [ ... ]
}

[Severity: High]
This is a pre-existing issue, but does checking the stop condition while
the thread is in TASK_RUNNING without preparing to sleep cause missed
wakeups?

If the unbind process calls kthread_stop(), it sets the stop flag and
wakes the thread. Because the thread is still in TASK_RUNNING, the wake
is a no-op, and the thread will then call schedule_timeout_interruptible()
and go to sleep, hanging module unload for up to 60 seconds.

[Severity: High]
This is also a pre-existing issue, but does this kthread lack
set_freezable() and try_to_freeze()? It appears to unconditionally attempt
I2C access while the bus might be suspended. Can this crash or hang the
I2C bus during system suspend and resume transitions?

drivers/hwmon/adt7470.c:adt7470_temp_read() {
    [ ... ]
	case hwmon_temp_alarm:
		*val = !!(data->alarm & channel);
		break;
    [ ... ]
}

[Severity: High]
This is a pre-existing issue, but does this logic incorrectly use the
integer index channel as a bitwise AND mask instead of a bit shift?
Could this yield completely incorrect hardware alarm states, preventing
userspace from properly monitoring temperature emergencies?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260717-adt7470_thermalzone-v2-0-a55147958fad@gmail.com?part=3