Re: [hwmon PATCH v3 2/6] hwmon: (adt7470) Fix cache updated before hardware write on I2C error

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

Pre-existing issues:
- [High] The PWM enable mask logic swaps the bitmasks for channels 2 and 3 (PWM3 and PWM4).
--

--- Patch [2]: [hwmon PATCH v3 2/6] hwmon: (adt7470) Fix cache updated before hardware write on I2C error ---
commit 6776a671fbb0d95bfa3d1f5ca2bae943732d5533
Author: Luiz Angelo Daros de Luca <[email protected]>

hwmon: (adt7470) Fix cache updated before hardware write on I2C error

This commit ensures the driver's cached values are only updated after a
successful regmap_write. By checking the return value, the cache accurately
reflects the actual hardware state even if an I2C transaction fails.

> diff --git a/drivers/hwmon/adt7470.c b/drivers/hwmon/adt7470.c
> index 481d51617f4be..62ec68ea0a406 100644
> --- a/drivers/hwmon/adt7470.c
> +++ b/drivers/hwmon/adt7470.c

[ ... ]

> @@ -847,10 +850,11 @@ static int adt7470_pwm_write(struct device *dev, u32 attr, int channel, long val
>  		val--;
>  
>  		mutex_lock(&data->lock);
> -		data->pwm_automatic[channel] = val;
>  		err = regmap_update_bits(data->regmap, ADT7470_REG_PWM_CFG(channel),
>  					 pwm_auto_reg_mask,

[Severity: High]
This is a pre-existing issue, but does this modulo logic for the PWM enable 
mask swap the bitmasks for channels 2 and 3 (PWM3 and PWM4)?

Earlier in adt7470_pwm_write(), the mask is assigned using modulo arithmetic:

    case hwmon_pwm_enable:
        if (channel % 2)
            pwm_auto_reg_mask = ADT7470_PWM2_AUTO_MASK;
        else
            pwm_auto_reg_mask = ADT7470_PWM1_AUTO_MASK;

This assigns the mask for PWM4 (0x80) to channel 2 (PWM3) and the mask for 
PWM3 (0x40) to channel 3 (PWM4) during sysfs write. As a result, operations 
intended for PWM3 modify the hardware state of PWM4, and vice versa.

The same pattern is used during the periodic sensor state update in 
adt7470_update_sensors():

    if (i % 2)
        mask = ADT7470_PWM2_AUTO_MASK;
    else
        mask = ADT7470_PWM1_AUTO_MASK;

Could this cause fans to fail to enter the commanded automatic or manual mode, 
potentially leading to improper cooling?

>  					 val ? pwm_auto_reg_mask : 0);
> +		if (!err)
> +			data->pwm_automatic[channel] = val;
>  		mutex_unlock(&data->lock);
>  		break;

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