Re: [PATCH v3 2/2] power: supply: Add Qualcomm SMB1351 charger driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] power: supply: qcom_smb1351: race on retry_until while
  smb1351_work() runs
- [Low] power: supply: qcom_smb1351: unlocked work scheduling in
  smb1351_probe()
--

commit 51205e0d5cfa98bce2fcb03aa43f9fb231439ca3
Author: Adrian Nguyen <thenguyen1024@gmail.com>

power: supply: Add Qualcomm SMB1351 charger driver

This commit adds a power supply driver for the Qualcomm SMB1351/SMB1350
parallel charger that keeps charging disabled over I2C and reports VBUS
presence and charger status.

> diff --git a/drivers/power/supply/qcom_smb1351.c b/drivers/power/supply/qcom_smb1351.c
> new file mode 100644
> index 0000000000000..33289c767bdb1
> --- /dev/null
> +++ b/drivers/power/supply/qcom_smb1351.c

[ ... ]

> @@ -0,0 +1,439 @@
> +	/*
> +	 * The chip can lose VBUS, and with it its configuration, without the
> +	 * PMIC charger reporting a change. If it has been reset or does not
> +	 * answer, run the work again, unless a run with its own deadline is
> +	 * already pending. Report "Unknown" rather than an error, which would
> +	 * drop the whole uevent.
> +	 */
> +	ret = smb1351_read_status(chip, &stat);
> +	if (ret) {
> +		spin_lock(&chip->lock);
> +		if (!delayed_work_pending(&chip->work)) {
> +			WRITE_ONCE(chip->retry_until, jiffies +
> +				   msecs_to_jiffies(SMB1351_RETRY_TIMEOUT_MS));
> +			queue_delayed_work(system_freezable_wq, &chip->work, 0);
> +		}
> +		spin_unlock(&chip->lock);
> +	}

[Severity: Medium]
Can smb1351_get_property() still overwrite chip->retry_until and bypass the
retry backoff while smb1351_work() is running?

The workqueue core clears WORK_STRUCT_PENDING_BIT before calling
smb1351_work(), so delayed_work_pending(&chip->work) returns false while
smb1351_work() is executing I2C transfers:

CPU0 (kworker)                    CPU1
smb1351_work()
  /* pending bit cleared */
  regmap_read(..., &version)
    /* fails when VBUS is lost */
                                  smb1351_get_property()
                                    smb1351_read_status() -> -ENXIO
                                    spin_lock(&chip->lock)
                                    !delayed_work_pending(&chip->work)
                                    chip->retry_until = jiffies + 2s
                                    queue_delayed_work(..., 0)
                                    spin_unlock(&chip->lock)
  time_before(jiffies, retry_until)
  queue_delayed_work(..., 200ms)  <-- ignored, already queued with delay 0

When userspace reads status or charge_type while smb1351_work() is running,
smb1351_get_property() extends chip->retry_until by another 2 seconds and
queues chip->work with a 0 ms delay.  smb1351_work() then sees the extended
deadline instead of expiring and setting chip->online to false, and its own
queue_delayed_work() with SMB1351_RETRY_MS becomes a no-op because the work
is already pending, re-running smb1351_work() immediately without the 200 ms
backoff.

In addition, because smb1351_get_property() reads chip->online before
smb1351_read_status() and does not recheck it under chip->lock (and
smb1351_work() updates chip->online outside chip->lock), a status read that
races with the final retry in smb1351_work() setting chip->online to false
will find !delayed_work_pending(&chip->work) after smb1351_work() finishes
and start a new 2-second retry loop.

[ ... ]

> +	/* Leave the chip to the work, it does not answer without VBUS */
> +	WRITE_ONCE(chip->retry_until,
> +		   jiffies + msecs_to_jiffies(SMB1351_RETRY_TIMEOUT_MS));
> +	enable_delayed_work(&chip->work);
> +	mod_delayed_work(system_freezable_wq, &chip->work, 0);
> +
> +	return 0;
> +}

[Severity: Low]
Can smb1351_probe() race with smb1351_external_power_changed() when enabling
and scheduling chip->work without holding chip->lock?

chip->psy is already live after devm_power_supply_register(), so
smb1351_external_power_changed() can run on another CPU while probe finishes:

CPU0                              CPU1
smb1351_probe()
  devm_power_supply_register()
  ...
  chip->retry_until = jiffies + 2s
  enable_delayed_work(&chip->work)
                                  smb1351_external_power_changed()
                                    spin_lock(&chip->lock)
                                    chip->retry_until = jiffies + 2.3s
                                    mod_delayed_work(..., 300ms)
                                    spin_unlock(&chip->lock)
  mod_delayed_work(..., 0)        <-- overwrites 300ms settle delay with 0

If smb1351_external_power_changed() runs between enable_delayed_work() and
mod_delayed_work(), smb1351_probe() replaces the 300 ms VBUS settle delay
(SMB1351_SETTLE_MS) with 0, running smb1351_work() before VBUS has settled.
If smb1351_external_power_changed() runs just before enable_delayed_work(),
its mod_delayed_work() is dropped while the work is disabled and
smb1351_probe() overwrites the longer retry_until deadline and schedules the
work without the settle delay.

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