Re: [PATCH v2 2/2] power: supply: add LT8491 battery charger driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] The local variable `ret` in `lt8491_get_property` is declared as `s16`, which will cause valid 16-bit I2C reads >= 32768 to be incorrectly treated as negative error codes.
- [High] The driver registers its power supply without devres and lacks a `.remove` callback, leading to a resource leak and use-after-free on device unbind.
- [Medium] The driver calls `i2c_smbus_write_word_data` but fails to check for `I2C_FUNC_SMBUS_WRITE_WORD_DATA` during probe.
- [Medium] Missing polling or delay after triggering a telemetry update command.
- [Medium] Missing bounds check for temperature limits in `lt8491_set_property()`.
- [Medium] Missing bounds check for resistor configuration values leading to silent truncation.
--

commit 3b2b2398d72c0229f6f2624e687f0338dc39a93b
Author: John Erasmus Mari Geronimo <johnerasmusmari.geronimo@analog.com>
    
power: supply: add LT8491 battery charger driver
    
This commit adds initial support for the LT8491 High Voltage Buck-Boost
Battery Charge Controller via I2C. The driver exposes voltage, current,
power, and temperature telemetry through the power supply subsystem.

diff --git a/drivers/power/supply/lt8491_charger.c b/drivers/power/supply/lt8491_charger.c
--- /dev/null
+++ b/drivers/power/supply/lt8491_charger.c
@@ -0,0 +1,412 @@

[ ... ]

> static int lt8491_get_property(struct power_supply *psy,
> 				enum power_supply_property psp,
> 				union power_supply_propval *val)
> {
> 	struct lt8491_info *info = power_supply_get_drvdata(psy);
> 	s16 ret;

[Severity: High]
Will the ret variable incorrectly wrap to a negative value when reading large
16-bit register values? Since i2c_smbus_read_word_data() returns an s32,
reading a negative temperature (where bit 15 is set) will cause ret to become
negative. Does this prematurely trigger the if (ret < 0) error path and
prevent userspace from reading negative temperatures?

[ ... ]

> 	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
> 		scoped_guard(mutex, &info->lock) {
> 			ret = i2c_smbus_read_byte_data(info->client,
> 						LT8491_STAT_CHARGER_REG);
> 			if (ret < 0)
> 				return ret;
>
> 			if (!FIELD_GET(LT8491_TELEM_ACTIVE_MASK, ret)) {
> 				ret = i2c_smbus_write_byte_data(info->client,
> 						LT8491_CTRL_UPDATE_TELEM_REG,
> 						LT8491_UPDATE_TELEM_CMD);
> 				if (ret)
> 					return ret;
> 			}
>
> 			ret = i2c_smbus_read_word_data(info->client,
> 						LT8491_TELE_VBAT_REG);

[Severity: Medium]
Is a delay or completion check required after triggering the telemetry update?
The code writes the update command to the telemetry register but immediately
reads the voltage data. Could this result in reading uninitialized or stale
data if the hardware ADC requires time to process the conversion?

[ ... ]

> static int lt8491_set_property(struct power_supply *psy,
> 				enum power_supply_property psp,
> 				const union power_supply_propval *val)
> {
> 	struct lt8491_info *info = power_supply_get_drvdata(psy);
>
> 	switch (psp) {
> 	case POWER_SUPPLY_PROP_TEMP_ALERT_MIN:
> 		return i2c_smbus_write_byte_data(info->client,
> 						 LT8491_CFG_TBAT_MIN_REG,
> 						 val->intval / 10);

[Severity: Medium]
Should the bounds of val->intval be validated before writing it? If userspace
writes an extremely large or deeply negative value, could it be silently
truncated when cast to the 8-bit parameter of i2c_smbus_write_byte_data(),
configuring incorrect temperature hardware limits?

[ ... ]

> static int lt8491_configure_resistor(struct lt8491_info *info,
> 				     const char *propname, int divider,
> 				     unsigned int reg)
> {
> 	struct device *dev = &info->client->dev;
> 	int ret;
> 	u32 val;
>
> 	ret = device_property_read_u32(dev, propname, &val);
> 	if (ret < 0)
> 		return dev_err_probe(dev, ret, "Missing %s property.\n",
> 				     propname);
>
> 	return i2c_smbus_write_word_data(info->client, reg, val / divider);

[Severity: Medium]
Does this division risk silent truncation? If the device tree contains
very large resistor values, could the result of val / divider exceed the
maximum 16-bit unsigned integer (65535) and get silently truncated by the
u16 parameter in i2c_smbus_write_word_data()?

[ ... ]

> static int lt8491_probe(struct i2c_client *client)
> {
> 	struct device *dev = &client->dev;
> 	struct lt8491_info *info;
> 	struct power_supply_config psy_cfg = {};
> 	int ret;
>
> 	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_BYTE_DATA |
> 				     I2C_FUNC_SMBUS_READ_WORD_DATA))
> 		return -EOPNOTSUPP;

[Severity: Medium]
Since lt8491_configure_resistor() later calls i2c_smbus_write_word_data(),
should this capability check also require I2C_FUNC_SMBUS_WRITE_WORD_DATA
to avoid unexpected probe failures on adapters that lack word write support?

[ ... ]

> 	info->psp = power_supply_register(dev, &lt8491_desc, &psy_cfg);
> 	if (IS_ERR(info->psp))
> 		return dev_err_probe(dev, PTR_ERR(info->psp),
> 				     "Failed to register power supply.\n");
>
> 	return 0;
> }

[ ... ]

> static struct i2c_driver lt8491_driver = {
> 	.driver = {
> 		.name = "lt8491",
> 		.of_match_table = lt8491_of_match,
> 	},
> 	.probe = lt8491_probe,
> 	.id_table = lt8491_id,
> };

[Severity: High]
Will this leave the power supply device registered indefinitely if the driver
is unbound? The probe function uses power_supply_register() instead of the
devres equivalent, and the driver struct entirely omits a remove callback.
Could this lead to a use-after-free when the underlying info struct is
freed via devres but the power supply sysfs nodes remain active?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1788432152.git.johnerasmusmari.geronimo@analog.com?part=2