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

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

On Mon, May 04, 2026 at 10:51:12AM +0100, Nuno Sá wrote:
> 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).

Hi Guenter,

I got time for this again and apparently my concerns were valid. As it
turns out the chip is not "smart" enough:

root@analog:/sys/class/hwmon/hwmon1# cat in5_max_alarm
0
root@analog:/sys/class/hwmon/hwmon1# cat in5_max_alarm
1

And you can imagine the outcome of running `sensors` :)

So I guess we have to leave this as-is.

- Nuno Sá

> 
> > 
> > > [ ... ]
> > > 
> > > > +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
> >