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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] Invalid pointer dereference of I2C client device during IRQ wake teardown.
- [High] Watchdog timer bits are always written as zero, silently permanently disabling the watchdog.
- [High] Hardware watchdog will incorrectly trigger and reset device limits during system suspend.
- [High] TOCTOU race in probe causes integer underflow that applies extreme maximum current limits.
- [High] Ignored return value of `enable_irq_wake()` results in unbalanced decrement during teardown.
- [Medium] Ignored return value from `devm_add_action_or_reset()` leaves device without expected wakeup capabilities.
- [Medium] Unprotected concurrent writes to the unused `sgm->state` field causes a data race.
--

commit a4ca3f3960debe0f5d998a1b64ed909e6c09a1bf
Author: Chris Morgan <macromorgan@hotmail.com>
Subject: power: supply: sgm41542: Add SG Micro sgm41542 charger

This commit adds support for the SG Micro SGM41542 charger and boost
converter. The driver is adapted from the Rockchip BSP and confirmed
with the 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
> index 0000000000000..f6aff2c2335d5
> --- /dev/null
> +++ b/drivers/power/supply/sgm41542_charger.c
> @@ -0,0 +1,1057 @@
[ ... ]
> +static int sgm4154x_set_input_curr_lim(struct sgm4154x_device *sgm, int iindpm)
> +{
> +	int reg_val;
> +	int ret;
> +
> +	if (iindpm < SGM4154X_IINDPM_I_MIN_UA)
> +		return -EINVAL;
> +
> +	/*
> +	 * Per the datasheet, values between 100000uA and 3100000uA work
> +	 * as expected with the register defined as having a step of
> +	 * 100000 and a min/max of 100000 (0x00) through 3100000 (0x1e).
> +	 * The register value of 0x1f however corresponds to 3800000uA not
> +	 * 3200000uA as one would expect.
> +	 */
> +	if ((iindpm > SGM4154X_IINDPM_I_MAX_UA) || (iindpm > sgm->init_data.ilim))
> +		iindpm = min(SGM4154X_IINDPM_I_MAX_UA, sgm->init_data.ilim);
> +
> +	if (iindpm > 3100000 && iindpm < SGM4154X_IINDPM_I_MAX_UA)
> +		iindpm = 3100000;
> +
> +	if (iindpm == SGM4154X_IINDPM_I_MAX_UA)
> +		reg_val = 0x1f;
> +	else
> +		reg_val = (iindpm - SGM4154X_IINDPM_I_MIN_UA) / SGM4154X_IINDPM_STEP_UA;

[Severity: High]
Does this logic underflow if called early?

If sysfs limits are set before sgm4154x_hw_init() runs in probe,
sgm->init_data.ilim will still be 0. This would cause iindpm to be clamped
to 0, and the step calculation above would evaluate to:
reg_val = (0 - 100000) / 100000 = -1

When passed to regmap_update_bits(), the -1 will be masked against 0x1F,
resulting in 0x1F. This would inadvertently apply the maximum 3.8A limit
to the upstream power supply instead of the lowest limit.

> +
> +	ret = regmap_update_bits(sgm->regmap,
> +				 SGM4154X_CHRG_CTRL_0,
> +				 SGM4154X_IINDPM_I_MASK,
> +				 reg_val);
[ ... ]
> +static int sgm4154x_set_watchdog_timer(struct sgm4154x_device *sgm, u8 time)
> +{
> +	int ret;
> +
> +	if (time > 3)
> +		return -EINVAL;
> +
> +	ret = regmap_update_bits(sgm->regmap,
> +				 SGM4154X_CHRG_CTRL_5,
> +				 SGM4154X_WDT_TIMER_MASK,
> +				 time);

[Severity: High]
Since SGM4154X_WDT_TIMER_MASK spans bits 4 and 5 (GENMASK(5, 4)), shouldn't
the time parameter (values 0-3) be shifted left by 4 before updating the
register?

Passing the unshifted value here causes the regmap_update_bits masking logic
to evaluate to (time & 0x30), which is always 0. This effectively disables
the hardware watchdog permanently.

> +
> +	if (ret) {
[ ... ]
> +static int sgm4154x_charger_get_property(struct power_supply *psy,
> +					 enum power_supply_property psp,
> +					 union power_supply_propval *val)
> +{
> +	struct sgm4154x_device *sgm = power_supply_get_drvdata(psy);
> +	struct sgm4154x_state state;
> +	int ret;
> +
> +	ret = sgm4154x_get_state(sgm, &state);
> +	if (ret) {
> +		dev_err(sgm->dev, "get state error!\n");
> +		return ret;
> +	}
> +	sgm->state = state;

[Severity: Medium]
Could this concurrent, unprotected write to sgm->state cause a data race
between sysfs readers and the IRQ handler below?

Since the sgm->state field is completely unused elsewhere in the driver,
can we safely remove it entirely to avoid the regression?

> +
> +	switch (psp) {
[ ... ]
> +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;

[Severity: Medium]
Could this concurrent, unprotected write to sgm4154x->state cause a data race?

> +	if (state.vbus_gd) {
[ ... ]
> +static void sgm4154x_disable_irq_wake(void *data)
> +{
> +	struct sgm4154x_device *sgm = data;
> +	struct i2c_client *client = to_i2c_client(sgm->dev->parent);
> +
> +	disable_irq_wake(client->irq);

[Severity: High]
Is it possible this casts the wrong device pointer?

sgm->dev is already the I2C client's device, so sgm->dev->parent points to
the upstream I2C adapter. Casting the adapter via to_i2c_client() and
accessing client->irq will read arbitrary memory, potentially corrupting a
random IRQ descriptor's wake state when passed to disable_irq_wake().

> +}
[ ... ]
> +static int sgm4154x_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct sgm4154x_device *sgm;
> +	int ret;
[ ... ]
> +	sgm->sgm_monitor_wq = devm_alloc_ordered_workqueue(dev, "sgm-monitor-wq",
> +			WQ_MEM_RECLAIM | WQ_FREEZABLE);

[Severity: High]
Is it safe to use a freezable workqueue here without providing PM suspend
callbacks to disable the hardware watchdog?

If the system suspends while the watchdog is active, the workqueue will pause
and the watchdog will expire, resetting the charger configuration and
clearing the thermal/current limits established during probe.

> +	if (!sgm->sgm_monitor_wq)
> +		return -EINVAL;
> +
> +	ret = devm_delayed_work_autocancel(dev, &sgm->sgm_delay_work,
> +					   sgm_charger_work);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Unable to register delayed work\n");
> +
> +	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);

[Severity: High]
Does registering the power supply earlier expose the sysfs interface to
userspace before sgm4154x_hw_init() has populated the limits in
sgm->init_data?

If userspace writes to the input limit early, the uninitialized zeros will be
used in sgm4154x_set_input_curr_lim() which may trigger the underflow
mentioned above.

> +	if (ret)
> +		return dev_err_probe(dev, ret, "Cannot initialize the chip.\n");
> +
> +	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);

[Severity: High]
If the IRQ controller doesn't support wakeup and enable_irq_wake() fails,
does ignoring the error here result in an unbalanced disable_irq_wake() call
during teardown?

> +		devm_add_action_or_reset(dev, sgm4154x_disable_irq_wake, sgm);

[Severity: Medium]
Also, does ignoring the return value of devm_add_action_or_reset() risk
leaving the device without its teardown handler if the devm allocation fails?

> +	}

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