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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Debugfs entries are not cleaned up on probe failure or driver unbind, leading to Use-After-Free.
- [High] `ltc4283_read_in_alarm()` incorrectly uses the `LTC4283_VPWR` register macro instead of the `LTC4283_CHAN_VPWR` enum when checking the channel.
- [Medium] Time-Of-Check to Time-Of-Use (TOCTOU) race condition when clearing alarms and fault logs causes hardware events to be silently lost.
- [Medium] Invalid string values for device properties are silently ignored instead of returning an error.
--

commit bd4cadd06bf8b8a849bcb4a4612ec0b3c5c91910
Author: Nuno Sá <nuno.sa@analog.com>
Subject: hwmon: ltc4283: Add support for the LTC4283 Swap Controller

This commit adds support for the Analog Devices LTC4283 Hot Swap Controller.
It features programmable current limits, adjustable inrush current, and an
I2C interface for monitoring board current, voltage, power, and energy.

> diff --git a/drivers/hwmon/ltc4283.c b/drivers/hwmon/ltc4283.c
> new file mode 100644
> index 000000000000..9b492a8b2227
> --- /dev/null
> +++ b/drivers/hwmon/ltc4283.c
> @@ -0,0 +1,1795 @@
[ ... ]
> +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);
> +
> +	return 0;
> +}

Could this non-atomic read-modify-write cause hardware events to be lost?

If the hardware asserts a new event bit between the regmap_read() and the
regmap_write(), won't the write overwrite the new bit with its previous 0
state, inadvertently clearing the new event before it is processed?

[ ... ]
> +static int ltc4283_read_in_alarm(struct ltc4283_hwmon *st, u32 channel,
> +				 bool max_alm, long *val)
> +{
> +	if (channel == LTC4283_VPWR)
> +		return ltc4283_read_alarm(st, LTC4283_ADC_ALM_LOG_1,
> +					  BIT(2 + max_alm), val);

Is channel compared to the register address instead of the enum index?

LTC4283_VPWR is defined as 0x44 (68), but channel is an enum value where
LTC4283_CHAN_VPWR is 1. Since 1 will never equal 68, won't this check fail
and cause it to fall through to the DRAIN channel logic, reading the wrong
hardware alarm bits for the VPWR channel?

[ ... ]
> +	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 < 0 && ret != -EINVAL)
> +		return dev_err_probe(dev, ret,
> +				     "adi,overcurrent-retries invalid value\n");

Will this actually catch invalid strings?

device_property_match_property_string() returns -EINVAL both when the
property is missing and when it contains an invalid string. By ignoring
-EINVAL, won't the driver silently accept invalid configurations instead
of warning the user?

[ ... ]
> +static int ltc4283_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev, *hwmon;
> +	struct auxiliary_device *adev;
> +	struct ltc4283_hwmon *st;
> +	int ret, id;
[ ... ]
> +	ltc4283_debugfs_init(st, client);
> +
> +	if (!st->gpio_mask)
> +		return 0;
> +
> +	id = (client->adapter->nr << 10) | client->addr;
> +	adev = __devm_auxiliary_device_create(dev, KBUILD_MODNAME, "gpio",
> +					      NULL, id);
> +	if (!adev)
> +		return dev_err_probe(dev, -ENODEV, "Failed to add GPIO device\n");
> +
> +	return 0;
> +}

Can this cause a use-after-free if the probe fails?

The debugfs files are created in i2c->debugfs with the devres-managed st
pointer as their data. Since i2c->debugfs is cleaned up by the I2C core
only when the client device is removed, if __devm_auxiliary_device_create()
fails, st will be freed by devres, but the debugfs entries will persist.

Would it be better to explicitly tear down these debugfs files using a
devm action so they don't dereference the freed st pointer?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260428-ltc4283-support-v10-0-4f26f46491c3@analog.com?part=2