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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [High] The result of `clamp_val()` is ignored, leaving user inputs unclamped and bypassing bounds checks.
- [High] The `VPWR`, `ADIO12`, and `ADIO34` channels read from and incorrectly clear the alarm logs of the unrelated `DRAIN` channel.
- [High] Fatal logic error in property matching aborts device probe when a valid `adi,overcurrent-retries` or `adi,fet-bad-retries` property is provided.
- [Medium] The logic for disabling the redundant ADC2 channel based on the `adi,vpower-drns-enable` property is swapped, disabling the actively monitored channel instead.
- [Medium] Calculation of `st->power_max` in `ltc4283_set_max_limits()` overflows 32-bit arithmetic for small sense resistors.
- [Medium] Software read-modify-write on volatile hardware fault and alarm registers can silently drop concurrently asserted hardware events.
- [Medium] Integer overflow when multiplying user input by `MILLI` prior to bounds clamping leads to unintended limit inversion.
- [Low] The `%*ph` format specifier is incorrectly used to print an array of 32-bit integers, yielding an unintelligible error message.
--

--- Patch [2]: [PATCH v7 2/3] hwmon: ltc4283: Add support for the LTC4283 Swap Controller ---
commit 636c5e8a9699ab80f86857069a188d6f2e25172d
Author: Nuno Sá <nuno.sa@analog.com>
Date:   Sat Mar 14 10:52:20 2026 +0000

    hwmon: ltc4283: Add support for the LTC4283 Swap Controller
    
    This commit adds support for the Analog Devices LTC4283 Hot Swap Controller.
    It features an I2C interface and ADC for monitoring board current, voltage,
    power, energy, and fault status. The driver implements hardware monitoring
    interfaces and configures the chip properties based on device tree inputs.

> diff --git a/drivers/hwmon/ltc4283.c b/drivers/hwmon/ltc4283.c
> new file mode 100644
> index 0000000000000..2066ff5d568fb
> --- /dev/null
> +++ b/drivers/hwmon/ltc4283.c
[ ... ]
> +static int ltc4283_read_alarm(struct ltc4283_hwmon *st, u32 reg,
> +			      u32 mask, long *val)
> +{
[ ... ]
> +	*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_clear_bits(st->map, reg, mask);

[Severity: Medium]
Does this read-modify-write operation on volatile hardware fault and alarm 
registers risk silently dropping concurrently asserted hardware events?

The regmap_clear_bits function reads the current state and writes it back 
with target bits cleared. For Write-0-to-Clear volatile registers, writing a 
0 clears the bit. If a new hardware fault occurs between the read and write, 
the software will write a 0 to the newly asserted bit, inadvertently clearing 
it before it is processed. Should a direct write of the inverted mask be 
used instead?

> +
> +	return 0;
> +}
> +
> +static int ltc4283_read_in_alarm(struct ltc4283_hwmon *st, u32 channel,
> +				 bool max_alm, long *val)
> +{
> +	if (channel == LTC4283_VPWR)

[Severity: High]
Can this result in false alarm readings and silently erased DRAIN fault logs?

This compares the channel ID to a register address macro (LTC4283_VPWR, 
which is 0x44) instead of the channel enum LTC4283_CHAN_VPWR, so it will 
always fail.

> +		return ltc4283_read_alarm(st, LTC4283_ADC_ALM_LOG_1,
> +					  BIT(2 + max_alm), val);
[ ... ]
> +	if (channel >= LTC4283_CHAN_ADIN12 && channel <= LTC4283_CHAN_ADIN34) {
> +		u32 bit = (channel - LTC4283_CHAN_ADIN12) * 2;
> +
> +		return ltc4283_read_alarm(st, LTC4283_ADC_ALM_LOG_5,
> +					  BIT(7 - bit - !max_alm), val);
> +	}

[Severity: High]
Does this bounds check omit the LTC4283_CHAN_ADIO12 and LTC4283_CHAN_ADIO34 
differential channels? 

> +
> +	if (channel == LTC4283_CHAN_DRNS)
> +		return ltc4283_read_alarm(st, LTC4283_ADC_ALM_LOG_4,
> +					  BIT(6 + max_alm), val);
> +
> +	return ltc4283_read_alarm(st, LTC4283_ADC_ALM_LOG_4, BIT(4 + max_alm),
> +				  val);
> +}

[Severity: High]
Due to the above two issues, queries for VPWR, ADIO12, and ADIO34 will 
bypass their intended logic and execute this fallback. The fallback targets 
and clears LTC4283_ADC_ALM_LOG_4 (the DRAIN alarm log). This logic error 
appears to erase critical DRAIN fault logs when reading unrelated channels.

[ ... ]
> +static int ltc4283_write_power_byte(const struct ltc4283_hwmon *st, u32 reg,
> +				    long val)
> +{
> +	u64 temp = (u64)LTC4283_ADC1_FS_uV * LTC4283_ADC2_FS_mV * DECA * MILLI;
> +	u32 __raw;
> +
> +	clamp_val(val, 0, st->power_max);

[Severity: High]
Is the result of clamp_val ignored here?

The macro evaluates to the clamped value but doesn't modify the argument in 
place. Without assigning the return value back to val, out-of-bounds user 
inputs will bypass the bounds check. This can cause subsequent arithmetic to 
overflow or produce large values that are silently truncated when written to 
the 8-bit registers.

> +	__raw = DIV64_U64_ROUND_CLOSEST(val * BIT_ULL(8) * st->rsense, temp);
> +
> +	return regmap_write(st->map, reg, __raw);
> +}
[ ... ]
> +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);
> +}

[Severity: Medium]
Similar to ltc4283_read_alarm, does this regmap_clear_bits call risk a 
read-modify-write operation erasing newly asserted power fault events?

[ ... ]
> +static int ltc4283_reset_in_hist(struct ltc4283_hwmon *st, u32 channel)
> +{
[ ... ]
> +	/*
> +	 * Make sure to clear possible under/over voltage faults. Otherwise the
> +	 * chip won't latch on again.
> +	 */
> +	if (channel == LTC4283_CHAN_VIN)
> +		return regmap_clear_bits(st->map, LTC4283_FAULT_LOG,
> +					 LTC4283_OV_FAULT_MASK | LTC4283_UV_FAULT_MASK);
[ ... ]
> +	/* Then, let's also clear possible fet faults. Same as above. */
> +	return regmap_clear_bits(st->map, LTC4283_FAULT_LOG,
> +				 LTC4283_FET_BAD_FAULT_MASK | LTC4283_FET_SHORT_FAULT_MASK);
> +}

[Severity: Medium]
Are these regmap_clear_bits calls also at risk of erasing newly asserted 
voltage or FET fault events via read-modify-write?

[ ... ]
> +static int ltc4283_write_minmax(struct ltc4283_hwmon *st, long val,
> +				u32 channel, bool is_max)
> +{
[ ... ]
> +	if (is_max) {
> +		reg = LTC4283_ADC_2_MAX_TH_DIFF(channel - LTC4283_CHAN_ADIN12);
> +		return ltc4283_write_in_byte(st, reg, LTC4283_ADC1_FS_uV,
> +					     val * MILLI);
> +	}
> +
> +	reg = LTC4283_ADC_2_MIN_TH_DIFF(channel - LTC4283_CHAN_ADIN12);
> +	return ltc4283_write_in_byte(st, reg, LTC4283_ADC1_FS_uV, val * MILLI);
> +}

[Severity: Medium]
Could this multiplication by MILLI cause a signed 32-bit integer overflow?

If a highly permissive positive threshold is passed, val * MILLI can wrap 
around the signed 32-bit integer boundary and become negative. A downstream 
clamp_val() will then catch this negative value and clamp it to 0, which 
would inadvertently set the threshold to 0 and trigger persistent false 
hardware alarms.

[ ... ]
> +static int ltc4283_write_curr_byte(const struct ltc4283_hwmon *st,
> +				   u32 reg, long val)
> +{
> +	u32 temp = LTC4283_ADC1_FS_uV * DECA * MILLI;
> +	u32 reg_val, isense_max;
> +
> +	isense_max = DIV_ROUND_CLOSEST(st->vsense_max * MICRO * DECA, st->rsense);
> +	clamp_val(val, 0, isense_max);

[Severity: High]
Like in ltc4283_write_power_byte, the returned value of clamp_val is not 
assigned back to val, leaving user inputs unclamped and bypassing bounds 
checks.

> +	reg_val = DIV_ROUND_CLOSEST_ULL(val * BIT_ULL(8) * st->rsense, temp);
> +
> +	return regmap_write(st->map, reg, reg_val);
> +}
[ ... ]
> +static int ltc4283_set_max_limits(struct ltc4283_hwmon *st, struct device *dev)
> +{
> +	u32 temp = st->vsense_max * DECA * MICRO;
> +	int ret;
> +
> +	ret = ltc4283_write_in_byte(st, LTC4283_SENSE_MAX_TH, LTC4283_ADC1_FS_uV,
> +				    st->vsense_max * MILLI);
> +	if (ret)
> +		return ret;
> +
> +	/* Power is given by ISENSE * Vout. */
> +	st->power_max = DIV_ROUND_CLOSEST(temp, st->rsense) * LTC4283_ADC2_FS_mV;

[Severity: Medium]
Does this multiplication overflow a 32-bit unsigned integer before assignment?

For typical small shunt values (e.g., 10 uOhm or rsense = 100), the product 
of the 32-bit values can exceed the 32-bit maximum, resulting in integer 
wrap-around. This assigns a massively truncated threshold to st->power_max 
and disrupts hardware limit bounds logic.

> +	return ltc4283_write_power_byte(st, LTC4283_POWER_MAX_TH, st->power_max);
> +}
> +
> +static int ltc4283_parse_array_prop(const struct ltc4283_hwmon *st,
> +				    struct device *dev, const char *prop,
> +				    const u32 *vals, u32 n_vals)
> +{
[ ... ]
> +	return dev_err_probe(dev, -EINVAL,
> +			     "Invalid %s property value %u, expected one of: %*ph\n",
> +			     prop, prop_val, n_vals, vals);
> +}

[Severity: Low]
Can this format string yield an unintelligible error message?

The %*ph format specifier expects a u8 * array and prints raw bytes. Using 
it for a const u32 * array prints only the first n_vals bytes of the first 
integer in memory, creating a mangled message.

[ ... ]
> +static int ltc4283_setup(struct ltc4283_hwmon *st, struct device *dev)
> +{
[ ... ]
> +	ret = device_property_match_property_string(dev, "adi,overcurrent-retries",
> +						    ltc4283_oc_fet_retry,
> +						    ARRAY_SIZE(ltc4283_oc_fet_retry));
> +	/* We still want to catch when an invalid string is given. */
> +	if (ret != -EINVAL)
> +		return dev_err_probe(dev, ret,
> +				     "adi,overcurrent-retries invalid value\n");

[Severity: High]
Does this erroneously trigger an error return on a successful property match?

The device_property_match_property_string function returns a non-negative 
index on success. Since a valid positive index does not equal -EINVAL, a 
successfully parsed property causes the driver to interpret it as an error 
and abort the probe.

> +	if (ret >= 0) {
> +		ret = regmap_update_bits(st->map, LTC4283_CONTROL_2,
> +					 LTC4283_OC_RETRY_MASK,
> +					 FIELD_PREP(LTC4283_OC_RETRY_MASK, ret));
> +		if (ret)
> +			return ret;
> +	}
> +
> +	ret = device_property_match_property_string(dev, "adi,fet-bad-retries",
> +						    ltc4283_oc_fet_retry,
> +						    ARRAY_SIZE(ltc4283_oc_fet_retry));
> +	if (ret != -EINVAL)
> +		return dev_err_probe(dev, ret,
> +				     "adi,fet-bad-retries invalid value\n");

[Severity: High]
Similar to above, this aborts the probe if a valid adi,fet-bad-retries 
property is provided.

[ ... ]
> +	if (device_property_read_bool(dev, "adi,vpower-drns-enable")) {
> +		__clear_bit(LTC4283_CHAN_DRAIN, &st->ch_enable_mask);
> +		chan = LTC4283_CHAN_DRAIN - LTC4283_CHAN_ADI_1;
> +		val = 1;
> +	} else {
> +		__clear_bit(LTC4283_CHAN_DRNS, &st->ch_enable_mask);
> +		chan = LTC4283_CHAN_DRNS - LTC4283_CHAN_ADI_1;
> +		val = 0;
> +	}

[Severity: Medium]
Is the logic for disabling the redundant ADC2 channel inverted here?

When adi,vpower-drns-enable is true, VPWR is configured to monitor DRNS, 
making the DRNS ADC2 channel redundant, but the driver disables the DRAIN 
channel. When false, VPWR monitors DRAIN, making DRAIN redundant, but the 
driver disables DRNS. This leaves the redundant channel active while 
disabling the actively monitored channel.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260314-ltc4283-support-v7-0-1cda48e93802@analog.com?part=2