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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] The driver fails to enforce the battery's maximum charge current limit (`max_ichg`) when userspace writes to `CONSTANT_CHARGE_CURRENT`.
- [High] Missing `.remove` callback leads to a Use-After-Free on the delayed worktimer when the device is unbound.
- [High] Missing battery properties evaluate to true and override safety defaults with `-EINVAL`, causing charge limits to be corrupted.
- [High] Registering the power supply before full initialization exposes uninitialized structures to sysfs writes, causing kernel panics or hardware damage.
- [Medium] Unbalanced `disable_irq_wake()` calls due to unconditional error handling, and leaked IRQ wake configuration on unbind.
- [Medium] The error code from `devm_power_supply_register` is squashed to a hardcoded `-EINVAL`.
--

commit 217e7bc6e3aefce7cb6ef93777df1d2955458981
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 and confirmed with vendor datasheet.

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_set_ichrg_curr(struct sgm4154x_device *sgm, int cur_ua)
> +{
> +	int reg_val;
> +	int ret;
> +
> +	cur_ua = clamp(cur_ua, SGM4154X_ICHRG_I_MIN_UA, SGM4154X_ICHRG_I_MAX_UA);
> +	reg_val = cur_ua / SGM4154X_ICHRG_I_STEP_UA;

[Severity: High]
When userspace writes to CONSTANT_CHARGE_CURRENT, will this bypass the
battery-specific sgm->init_data.max_ichg safety limit?

It appears the requested current is only clamped against the hardware's
absolute physical maximum (3.78A) and ignores the battery's maximum
charge current limit.

Can this inadvertently program the hardware to supply maximum current and
bypass battery safety constraints?

[ ... ]

> +static int sgm4154x_set_chrg_volt(struct sgm4154x_device *sgm, int chrg_volt)
> +{
> +	int reg_val;
> +	int ret;
> +
> +	/*
> +	 * Note that the value of 0x01111 represents a "special value"
> +	 * corresponding to 4352000uV instead of the expected 4336000uV,
> +	 * per the datasheet. All other values are as expected. So not
> +	 * only do we need to clamp between max and min values, but
> +	 * also clamp anything below 4352000uv to 4304000uv to prevent
> +	 * overcharging.
> +	 */
> +	if (chrg_volt < 4352000)
> +		chrg_volt = clamp(chrg_volt, SGM4154X_VREG_V_MIN_UV, 4304000);
> +	chrg_volt = clamp(chrg_volt, SGM4154X_VREG_V_MIN_UV, sgm->init_data.max_vreg);
> +	reg_val = (chrg_volt - SGM4154X_VREG_V_MIN_UV) / SGM4154X_VREG_V_STEP_UV;
> +	reg_val = reg_val << 3;

[Severity: High]
If sgm->init_data.max_vreg is negative (e.g., -EINVAL), will this result in
undefined behavior?

The clamp function could produce a negative result here, which is then left
shifted (reg_val << 3).

Could shifting a negative integer invoke undefined behavior and write corrupt
data to the hardware configuration?

[ ... ]

> +static int sgm4154x_power_supply_init(struct sgm4154x_device *sgm,
> +				      struct device *dev)
> +{
> +	struct power_supply_config psy_cfg = { .drv_data = sgm,
> +					       .fwnode = dev->fwnode, };
> +
> +	psy_cfg.supplied_to = sgm4154x_charger_supplied_to;
> +	psy_cfg.num_supplicants = ARRAY_SIZE(sgm4154x_charger_supplied_to);
> +	psy_cfg.fwnode = dev->fwnode;
> +	sgm->charger = devm_power_supply_register(sgm->dev,
> +						  &sgm4154x_power_supply_desc,
> +						  &psy_cfg);
> +	if (IS_ERR(sgm->charger))
> +		return -EINVAL;

[Severity: Medium]
Should this propagate the actual error code instead of returning -EINVAL?

Squashing the error code from devm_power_supply_register to a hardcoded
-EINVAL suppresses the original failure context.

Would this break deferred probing if the power supply subsystem requests
-EPROBE_DEFER?

[ ... ]

> +	ret = power_supply_get_battery_info(sgm->charger, &bat_info);
> +	if (ret)
> +		dev_warn(sgm->dev, "sgm4154x: cannot read battery info\n");
> +	else {
> +		if (bat_info->constant_charge_current_max_ua)
> +			sgm->init_data.max_ichg = bat_info->constant_charge_current_max_ua;
> +		if (bat_info->constant_charge_voltage_max_uv)
> +			sgm->init_data.max_vreg = bat_info->constant_charge_voltage_max_uv;

[Severity: High]
Are these property checks sufficient to validate the battery info?

Missing battery properties are initialized to -EINVAL (-22) by
power_supply_get_battery_info. Because -22 is non-zero, simple truthiness
checks will evaluate to true.

Will this overwrite the safe hardware defaults with negative values,
corrupting charge limits later on?

[ ... ]

> +	sgm->sgm_monitor_wq = devm_alloc_ordered_workqueue(dev, "%s",
> +			WQ_MEM_RECLAIM | WQ_FREEZABLE, "sgm-monitor-wq");
> +	if (!sgm->sgm_monitor_wq)
> +		return -EINVAL;
> +
> +	ret = sgm4154x_power_supply_init(sgm, dev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to register power supply\n");
> +
> +	ret = sgm4154x_hw_init(sgm);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Cannot initialize the chip.\n");

[Severity: High]
Is it safe to register the power supply before the hardware boundaries
and delayed work structures are initialized?

Userspace or udev can immediately write to sysfs attributes like online
as soon as the power supply is registered.

If this happens, could sgm4154x_set_watchdog_timer queue an uninitialized
delayed_work struct, potentially corrupting timer lists and causing a kernel
panic?

[ ... ]

> +	if (client->irq) {
> +		ret = devm_request_threaded_irq(dev, client->irq, NULL,
> +						sgm4154x_irq_handler_thread,
> +						IRQF_TRIGGER_FALLING |
> +						IRQF_ONESHOT,
> +						"sgm41542-irq", sgm);
> +		if (ret)
> +			return ret;
> +
> +		enable_irq_wake(client->irq);
> +	}
> +
> +	INIT_DELAYED_WORK(&sgm->sgm_delay_work, sgm_charger_work);

[Severity: High]
Does this driver safely tear down sgm_delay_work on unbind?

The delayed work is initialized here but there appears to be no .remove
callback or devm_delayed_work_autocancel action to cancel it. When the
device is unbound, devres will automatically destroy the workqueue and free
the sgm structure.

If the 5-second timer fires afterward, could this result in a use-after-free?

> +
> +	ret = sgm4154x_vbus_regulator_register(sgm);
> +	if (ret) {
> +		disable_irq_wake(client->irq);
> +		return dev_err_probe(dev, ret,
> +				     "Unable to register VBUS regulator\n");
> +	}

[Severity: Medium]
Will this result in an unbalanced disable_irq_wake call if client->irq is 0?

The enable_irq_wake call is properly gated behind a check for client->irq.
However, if sgm4154x_vbus_regulator_register fails, the error path
unconditionally calls disable_irq_wake.

Additionally, could the driver leak the IRQ wake depth count since it lacks a
.remove callback to disable the wake IRQ upon successful unbind?

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