Re: [PATCH v12 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 leaked and will trigger a Use-After-Free if `__devm_auxiliary_device_create` fails during probe.
- [Medium] Read-modify-write (RMW) operations on hardware fault/alarm log registers can accidentally clear and permanently lose concurrent hardware events.
- [Low] The debugfs attributes for FET faults are registered with incorrect names (`in0_...` instead of `in11_...`).
- [Low] Invalid string values in device tree properties are silently ignored despite comments indicating they should be caught.
--

commit d9fab3c87e5bd2f43c5dc67392ad85d9104dbf0b
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 provides monitoring of board current, voltage, power, energy, and fault
status via an I2C interface.

> diff --git a/drivers/hwmon/ltc4283.c b/drivers/hwmon/ltc4283.c
> new file mode 100644
> index 0000000000000..d66815bfb1341
> --- /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 operation accidentally clear concurrent hardware
events? If a new hardware event asserts immediately between the read and the
write, the mask inversion might write 0 to that newly asserted bit, causing
the event to be cleared before the software ever processes it.

[ ... ]

> +static void ltc4283_debugfs_init(struct ltc4283_hwmon *st, struct i2c_client *i2c)
> +{
> +	debugfs_create_file_unsafe("in0_crit_fault_log", 0400, i2c->debugfs, st,
> +				   &ltc4283_in0_crit_fault_log);
> +	debugfs_create_file_unsafe("in0_lcrit_fault_log", 0400, i2c->debugfs, st,
> +				   &ltc4283_in0_lcrit_fault_log);
> +	debugfs_create_file_unsafe("in0_fet_bad_fault_log", 0400, i2c->debugfs, st,
> +				   &ltc4283_fet_bad_fault_log);
> +	debugfs_create_file_unsafe("in0_fet_short_fault_log", 0400, i2c->debugfs, st,
> +				   &ltc4283_fet_short_fault_log);

Should these debugfs attributes for the FET faults be named starting with
in11_ instead of in0_? The MOSFET DRAIN channel is in11, and the
documentation in ltc4283.rst lists these attributes as
in11_fet_short_fault_log and in11_fet_bad_fault_log.

[ ... ]

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

Does this condition silently ignore invalid string values?
device_property_match_property_string() returns -EINVAL both when a
property is missing and when the string does not match any entry in the
provided array.

[ ... ]

> +static int ltc4283_probe(struct i2c_client *client)
> +{
[ ... ]
> +	hwmon = devm_hwmon_device_register_with_info(dev, "ltc4283", st,
> +						     &ltc4283_chip_info, NULL);
> +
> +	if (IS_ERR(hwmon))
> +		return PTR_ERR(hwmon);
> +
> +	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",
> +					      &st->gpio_mask, id);
> +	if (!adev)
> +		return dev_err_probe(dev, -ENODEV, "Failed to add GPIO device\n");

Can this error path lead to a use-after-free?

According to the I2C subsystem rules, debugfs entries attached to the
client debugfs directory are only cleaned up during the client device
removal function on driver unbind.

If __devm_auxiliary_device_create() fails, the probe function aborts and
the devres subsystem frees the st pointer. However, the debugfs files
persist and retain operations that operate on the freed st pointer.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260430-ltc4283-support-v12-0-5dc9901f2567@analog.com?part=2