Re: [PATCH V8 2/4] power: supply: sgm41542: Add SG Micro sgm41542 charger

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Lockless update of cached device state in `get_property` causes lost IRQ edge detection and data tearing.
- [High] The hardware charger watchdog is permanently disabled after the first system suspend/resume cycle.
- [Medium] The power supply is registered and exposed to userspace before its initialization data is populated, causing a race condition on sysfs property accesses.
- [Low] Missing dependency on REGULATOR in Kconfig causes build failures.
--

--- Patch [2]: [PATCH V8 2/4] power: supply: sgm41542: Add SG Micro sgm41542 charger ---
commit 3abcd77069baeaee08cbb10755bbd687ee39d41c
Author: Chris Morgan <macromorgan@hotmail.com>
Date:   Tue Jul 21 17:09:05 2026 +0000

    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.
    
    [1] https://github.com/rockchip-linux/kernel/blob/develop-6.6/drivers/power/supply/sgm41542_charger.c
    [2] https://www.sg-micro.de/rect/assets/1e8de70b-657e-4156-be68-a64fdbe8e418/SGM41541_SGM41542.pdf

> diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
> --- a/drivers/power/supply/Kconfig
> +++ b/drivers/power/supply/Kconfig
> @@ -866,6 +866,14 @@ config CHARGER_S2M
>  	  devices provide USB power supply information and also required
>  	  for USB OTG role switching.
>  
> +config CHARGER_SGM41542
> +	tristate "SGM41542 charger driver"
> +	depends on I2C
> +	depends on GPIOLIB || COMPILE_TEST
> +	select REGMAP_I2C

[Severity: Low]
Since this driver provides a regulator via devm_regulator_register(), should
this also depend on REGULATOR to prevent build failures when the regulator
subsystem is disabled?

[ ... ]

> 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_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: High]
Could this lockless update of sgm->state interfere with the edge detection in
the IRQ handler?

If a sysfs read calls this function concurrently with an interrupt, it updates
sgm->state before the IRQ handler can read the old state:

sysfs read thread:
    sgm4154x_get_state(sgm, &state);
    sgm->state = state; // updated to the new state

IRQ thread (sgm4154x_irq_handler_thread):
    struct sgm4154x_state oldstate = sgm4154x->state; // reads newly updated state
    ...
    if (state.vbus_gd && !oldstate.vbus_gd) { // fails because oldstate already has vbus_gd

This could cause the driver to miss edge transitions, like failing to apply
the input current limit on cable insertion. Additionally, since the state
struct is multi-byte, can this assignment cause data tearing if readers are
accessing it concurrently?

[ ... ]

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

[Severity: Medium]
Is there a race condition here if userspace accesses sysfs properties
immediately after power supply registration?

devm_power_supply_register() (called via sgm4154x_power_supply_init) exposes
the device before sgm4154x_hw_init() populates the safety limits in
sgm->init_data.

If a udev rule or userspace polling attempts to configure charge limits
during this window, could it bypass the intended hardware bounds because the
limit variables are still zeroed?

[ ... ]

> +static int __maybe_unused sgm4154x_suspend(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct sgm4154x_device *sgm = i2c_get_clientdata(client);
> +
> +	/*
> +	 * Disable watchdog during suspend and stop delayed work. When
> +	 * delayed work is restarted after resume watchdog will be
> +	 * re-enabled if it was previously enabled.
> +	 */
> +	sgm4154x_set_watchdog_timer(sgm, SGM4154X_WDT_TIMER_DISABLE);
> +	cancel_delayed_work_sync(&sgm->sgm_delay_work);
> +	return 0;
> +}
> +
> +static int __maybe_unused sgm4154x_resume(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct sgm4154x_device *sgm = i2c_get_clientdata(client);
> +
> +	queue_delayed_work(sgm->sgm_monitor_wq,
> +			   &sgm->sgm_delay_work, 0);
> +	return 0;
> +}

[Severity: High]
Will the charger watchdog be permanently disabled after the first suspend
and resume cycle?

When sgm4154x_suspend() calls sgm4154x_set_watchdog_timer() with 
SGM4154X_WDT_TIMER_DISABLE, it destructively clears sgm->watchdog_enable
to false.

On resume, queue_delayed_work() schedules the monitor work, but
sgm_charger_work() checks this flag:

static void sgm_charger_work(struct work_struct *work)
{
    ...
    if (sgm->watchdog_enable)
        queue_delayed_work(...)
}

Because the flag was wiped during suspend, the timer is never requeued or 
re-enabled, potentially leaving the hardware unprotected against overcharging
if the system were to crash later.

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