Re: [PATCH v13 2/3] hwmon: ltc4283: Add support for the LTC4283 Swap Controller

["Nuno Sá" <noname.nuno@gmail.com>]

On Sat, May 02, 2026 at 09:21:15AM -0700, Guenter Roeck wrote:
> On 5/2/26 03:43, sashiko-bot@kernel.org wrote:
> > Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> > - [Medium] Read-modify-write operations on hardware alarm and fault log registers will cause concurrent events to be lost.
> > --
> > 
> > commit 768d24f774b79f50e1444b93b756d7bda5873159
> > Author: Nuno Sá <nuno.sa@analog.com>
> > 
> > hwmon: ltc4283: Add support for the LTC4283 Swap Controller
> > 
> > This commit introduces driver support for the Analog Devices LTC4283
> > Negative Voltage Hot Swap Controller. It adds functionality to monitor
> > voltage, current, power, and various hardware faults via an I2C interface.
> > 
> > > diff --git a/drivers/hwmon/ltc4283.c b/drivers/hwmon/ltc4283.c
> > > new file mode 100644
> > > index 000000000000..d8931c9a4685
> > > --- /dev/null
> > > +++ b/drivers/hwmon/ltc4283.c
> > 
> > [ ... ]
> > 
> > > +static int ltc4283_read_alarm(struct ltc4283_hwmon *st, u32 reg,
> > > +			      u32 mask, long *val)
> > > +{
> > > +	u32 alarm;
> > > +	int ret;
> > > +
> > > +	ret = regmap_read(st->map, reg, &alarm);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	*val = !!(alarm & mask);
> > > +
> > > +	/* If not status/fault logs, clear the alarm after reading it. */
> > > +	if (reg != LTC4283_FAULT_STATUS && reg != LTC4283_FAULT_LOG)
> > > +		return regmap_write(st->map, reg, alarm & ~mask);
> > 
> > Could this read-modify-write sequence cause concurrent hardware events to be
> > lost?
> > 
> > If these alarm registers use write-0-to-clear semantics, writing back
> > alarm & ~mask will inadvertently write a 0 to any new alarm bits that were
> > asserted by the hardware between the regmap_read() and regmap_write() calls.
> > Usually, a direct atomic write of ~mask without reading first is preferred
> > to avoid clearing unrelated active alarms.
> > 
> 
> Sashiko comes back to this one. Looking this up in the datasheet, it says
> that alarm bits are reset by writing 0 into the alarm bit. That isn't
> what is done here, though. It will write 0 into any alarm bits which were
> not set when the register was read.
> 
> Should this be something like the following instead ?
> 
> 	alarm &= mask;
> 	*val = !!alarm;
> 
> 	if (reg != LTC4283_FAULT_STATUS && reg != LTC4283_FAULT_LOG && alarm)
> 		return regmap_write(st->map, reg, ~alarm & 0xff);
> 
> That would ensure that only the checked bit is cleared, and that it is only
> cleared if the bit was set when it was read.

I'll have to check the above. I gave it a bit more thought and as it is, it
should not be a problem. Because even if we (in the edge case) end up
clearing an alarm that came after that regmap_read(), if the condition
persists in the next conversion, we'll have it set again. Yes, we still
loose one historical alarm but not something will be forever lost at
least.

Having said the above I do agree with your proposed change, but I need
to see how the HW "behaves". I'm afraid that if we just write 1's into all
of the alarms we're not checking we create a window where reading the
register again will, falsely, indicate the alarm is present. If the
read happens before a new conversion was done (note that if all the
channels are enabled we actually have seconds for a new conversion to
go though all channels).

> 
> > [ ... ]
> > 
> > > +static int ltc4283_reset_power_hist(struct ltc4283_hwmon *st)
> > > +{
> > 
> > [ ... ]
> > 
> > > +	/* Clear possible power faults. */
> > > +	return regmap_clear_bits(st->map, LTC4283_FAULT_LOG,
> > > +				 LTC4283_PWR_FAIL_FAULT_MASK | LTC4283_PGI_FAULT_MASK);
> > > +}
> > 
> > Will using regmap_clear_bits() here and in other history reset functions
> > also risk losing concurrent fault logs?
> > 
> > Since regmap_clear_bits() performs a read-modify-write under the hood, any
> > new faults asserting between the read and write phases might be cleared
> > silently. Direct writes using regmap_write() might be safer if the register
> > behaves as write-0-to-clear.
> > 
> Again, looking into the datasheet, I agree. It would be safer to explicitly
> write something like
> 	~(LTC4283_PWR_FAIL_FAULT_MASK | LTC4283_PGI_FAULT_MASK) & 0xff.
> This would avoid accidentally clearing bits which were set between the read
> and the write operation.
>

The above is the same situation as the alarm. So let's see how that
settles.

- Nuno Sá

> Thanks,
> Guenter
>