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

Em seg., 27 de jul. de 2026 às 00:50, <[email protected]> escreveu:
>
> 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;

The modulo logic isn't the issue here; rather, ADT7470_PWM3_AUTO_MASK
and ADT7470_PWM4_AUTO_MASK are incorrectly defined.

This is already addressed later in this series in patch 4/6. While
fixing the definitions is mostly cosmetic, it will prevent Sashiko-bot
from repeatedly flagging this false positive in the future.

Regards,

Luiz