Re: [PATCH V7 2/6] power: supply: sgm41542: Add SG Micro sgm41542 charger

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Hardware watchdog resets charger configuration during system suspend due to missing PM suspend/resume handlers.
- [High] The IRQ handler unconditionally overwrites the input current limit whenever VBUS is present, silently destroying userspace-configured limits.
- [Medium] Missing cleanup for device_init_wakeup() on driver unbind and probe error paths.
- [Medium] Lack of synchronization in the sysfs property setter for ONLINE state allows hardware/software state desynchronization.
- [Medium] The IRQ handler returns early on an I2C failure, skipping the critical `power_supply_changed()` uevent notification.
--

commit 518a06e67b4771b8e50e9e0705ace20eca6acede
Author: Chris Morgan <macromorgan@hotmail.com>

power: supply: sgm41542: Add SG Micro sgm41542 charger

Add support for the SG Micro SGM41542 charger/boost converter.
Driver was adapted from Rockchip BSP driver [1] and confirmed
with vendor datasheet [2].

Link: https://github.com/rockchip-linux/kernel/blob/develop-6.6/drivers/power/supply/sgm41542_charger.c
Link: https://www.sg-micro.de/rect/assets/1e8de70b-657e-4156-be68-a64fdbe8e418/SGM41541_SGM41542.pdf

> diff --git a/drivers/power/supply/sgm41542_charger.c b/drivers/power/supply/sgm41542_charger.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/power/supply/sgm41542_charger.c

[ ... ]

> +static int sgm4154x_charger_set_property(struct power_supply *psy,
> +					 enum power_supply_property prop,
> +					 const union power_supply_propval *val)
> +{
> +	struct sgm4154x_device *sgm = power_supply_get_drvdata(psy);
> +	int ret = -EINVAL;
> +
> +	switch (prop) {
> +	case POWER_SUPPLY_PROP_ONLINE:
> +		if (val->intval) {
> +			ret = sgm4154x_enable_charger(sgm);
> +			sgm4154x_set_watchdog_timer(sgm, SGM4154X_WDT_TIMER_40S);
> +		} else {
> +			sgm4154x_set_watchdog_timer(sgm, SGM4154X_WDT_TIMER_DISABLE);
> +			ret = sgm4154x_disable_charger(sgm);
> +		}
> +		break;

[Severity: Medium]
Is there a need for a mutex in this sysfs property setter?

When modifying POWER_SUPPLY_PROP_ONLINE, the code performs multiple
operations without holding any locks.

Could concurrent sysfs writes interleave these operations? This might lead
to the hardware charger being disabled while the watchdog timer remains
enabled, causing the software sgm->watchdog_enable flag to become
desynchronized with the actual hardware state.

[ ... ]

> +static irqreturn_t sgm4154x_irq_handler_thread(int irq, void *private)
> +{
> +	struct sgm4154x_device *sgm4154x = private;
> +	struct sgm4154x_state state;
> +	int ret;
> +
> +	ret = sgm4154x_get_state(sgm4154x, &state);
> +	if (ret) {
> +		dev_err(sgm4154x->dev, "get state error!\n");
> +		return IRQ_NONE;
> +	}
> +	sgm4154x->state = state;
> +	if (state.vbus_gd) {
> +		if (sgm4154x->init_data.ilim >= SGM4154X_DEFAULT_INPUT_CUR) {
> +			ret = sgm4154x_set_input_curr_lim(sgm4154x, sgm4154x->init_data.ilim);

[Severity: High]
Will this unconditionally overwrite the input current limit configured
by userspace?

This checks if VBUS is good, but doesn't check if VBUS just transitioned
to being good.

Since it unconditionally applies init_data.ilim, won't any subsequent
interrupt while VBUS is present (such as a battery thermal event) silently
overwrite custom limits previously set via sysfs?

> +			if (ret) {
> +				dev_err(sgm4154x->dev, "set input current error!\n");
> +				/* Reading IRQ clears interrupt, so return handled */
> +				return IRQ_HANDLED;
> +			}
> +		}
> +	}
> +	power_supply_changed(sgm4154x->charger);
> +
> +	return IRQ_HANDLED;
> +}

[Severity: Medium]
Should this early return be avoided so that power_supply_changed()
is still called?

If the I2C write for the input current limit fails, the code immediately
returns.

Will this cause userspace to silently miss important hardware state changes,
like cable insertion or fault conditions, that triggered the interrupt?

[ ... ]

> +static int sgm4154x_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct sgm4154x_device *sgm;
> +	int ret;
> +
> +	sgm = devm_kzalloc(dev, sizeof(*sgm), GFP_KERNEL);
> +	if (!sgm)
> +		return -ENOMEM;
> +
> +	sgm->dev = dev;
> +
> +	sgm->regmap = devm_regmap_init_i2c(client, &sgm4154x_regmap_config);
> +	if (IS_ERR(sgm->regmap))
> +		return dev_err_probe(dev, PTR_ERR(sgm->regmap),
> +				     "Failed to allocate register map\n");
> +
> +	i2c_set_clientdata(client, sgm);
> +
> +	ret = sgm4154x_hw_chipid_detect(sgm);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Unable to read HW ID\n");
> +
> +	device_init_wakeup(dev, 1);

[Severity: Medium]
Is there a missing cleanup step for device_init_wakeup()?

The device is marked as wakeup-capable here, but there doesn't seem to be
a corresponding device_init_wakeup(dev, 0) during probe error paths or in
a driver remove callback.

Could this leave the system PM subsystem in an inconsistent state if the
driver unbinds or the probe fails later?

> +
> +	sgm->sgm_monitor_wq = devm_alloc_ordered_workqueue(dev, "sgm-monitor-wq",
> +			WQ_MEM_RECLAIM | WQ_FREEZABLE);

[Severity: High]
How is the hardware watchdog handled during system suspend?

The workqueue feeding the hardware watchdog is allocated with WQ_FREEZABLE,
meaning it stops feeding the watchdog during suspend.

[ ... ]

> +static struct i2c_driver sgm4154x_driver = {
> +	.driver = {
> +		.name = "sgm4154x-charger",
> +		.of_match_table = sgm4154x_of_match,
> +	},
> +	.probe = sgm4154x_probe,
> +	.id_table = sgm4154x_i2c_ids,
> +};

Because there are no PM suspend/resume handlers defined in the driver, the
hardware watchdog is never disabled before sleep.

If the system sleeps longer than the 40-second watchdog timeout, will the
charger IC expire the watchdog and reset all registers (such as charge
current and voltage limits) to their hardware defaults?

Upon resume, the frozen workqueue thaws and continues resetting the
watchdog, but the custom charging parameters are never re-applied.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260610144407.438846-1-macroalpha82@gmail.com?part=2