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

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

On Tue, Jun 09, 2026 at 06:07:38AM -0700, Guenter Roeck wrote:
> On 6/9/26 05:50, Nuno Sá wrote:
> > 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.
> > 
> 
> Outch :-(.
> 
> Oh well, that can't be helped. Should I apply the series as-is (v13) ?
> 

If there's no more concerns from you, I would say so. If I eventually
have the time, I should apply some of the outcome from this series to
the ltc4282 driver.

- Nuno Sá

> Thanks,
> Guenter
>