Re: [hwmon PATCH v2 8/8] hwmon: (adt7470) Fix PWM auto temp state array and bounds check

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

Pre-existing issues:
- [High] The driver violates HWMON subsystem guidelines by using a private mutex (`data->lock`) instead of the required `hwmon_lock()` API to serialize sysfs attributes registered outside the `info` structure.
--

--- Patch [8]: [hwmon PATCH v2 8/8] hwmon: (adt7470) Fix PWM auto temp state array and bounds check ---
commit 3702a967e9e4da59d546cf6a3b0fc208ec5ff2bf
Author: Luiz Angelo Daros de Luca <[email protected]>

hwmon: (adt7470) Fix PWM auto temp state array and bounds check

This commit fixes a bounds checking issue in pwm_auto_temp_store() where
values > 0xF could overflow into adjacent channel bits. It also corrects
the array assignment so values are written to pwm_auto_temp instead of
pwm_automatic after a successful I2C write.

Closes: https://lore.kernel.org/all/[email protected]/#t

> diff --git a/drivers/hwmon/adt7470.c b/drivers/hwmon/adt7470.c
> index 772d2a409bb5c..c45b984c02e6b 100644
> --- a/drivers/hwmon/adt7470.c
> +++ b/drivers/hwmon/adt7470.c
> @@ -1049,8 +1049,10 @@ static ssize_t pwm_auto_temp_store(struct device *dev,
>  	if (temp < 0)
>  		return temp;
>  
> +	if (temp > 0xF)
> +		return -EINVAL;
> +
>  	mutex_lock(&data->lock);

[Severity: High]
This is a pre-existing issue, but does this driver violate the hwmon
subsystem locking guidelines by using a private mutex here?

According to the hardware monitoring API documentation, drivers must
implement locking required for interrupt handling and for attributes
registered outside the standard info structure using hwmon_lock() and
hwmon_unlock(). The core relies on this to serialize thermal subsystem
and sysfs operations.

By using the private data->lock in pwm_auto_temp_store(), could this
bypass the central hwmon locking mechanism and cause concurrency issues
with the thermal subsystem?

Would it make sense to update this to use hwmon_lock(dev)?

> -	data->pwm_automatic[attr->index] = temp;
>  
>  	if (!(attr->index % 2)) {
>  		mask = 0xF0;

[ ... ]

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