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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] power: supply: qcom_smb1351: race on retry_until in smb1351_get_property()
--

commit 775470b6736c36c7e8120aec1a8d3f4b8da3feec
Author: Adrian Nguyen <thenguyen1024@gmail.com>

power: supply: Add Qualcomm SMB1351 charger driver

This commit adds a power supply driver for the Qualcomm SMB1351 and SMB1350
switch-mode battery chargers when powered from VBUS alone. The driver keeps
charging disabled via I2C, configures the float voltage from the monitored
battery, and reports online status, charging status, charge type, and float
voltage.

> diff --git a/drivers/power/supply/qcom_smb1351.c b/drivers/power/supply/qcom_smb1351.c
> new file mode 100644
> index 0000000000000..8089051cb44ae
> --- /dev/null
> +++ b/drivers/power/supply/qcom_smb1351.c
> @@ -0,0 +1,426 @@

[ ... ]

> +static int smb1351_get_property(struct power_supply *psy,
> +				enum power_supply_property psp,
> +				union power_supply_propval *val)
> +{
> +	struct smb1351 *chip = power_supply_get_drvdata(psy);
> +	bool online = READ_ONCE(chip->online);
> +	unsigned int stat, vfloat;
> +	int ret;

[ ... ]

> +	/*
> +	 * 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 && !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);
> +	}

[Severity: Medium]
Can smb1351_get_property() race with smb1351_work() or
smb1351_external_power_changed() when updating chip->retry_until and queueing
chip->work?

In process_one_work(), WORK_STRUCT_PENDING_BIT is cleared before invoking the
work callback, so delayed_work_pending(&chip->work) returns false while
smb1351_work() is actively executing its I2C transfers.

When VBUS is lost while chip->online is true, if userspace reads the status
or charge_type sysfs attributes via:

  power_supply_show_property() ->
    power_supply_get_property() ->
      smb1351_get_property()

while smb1351_work() is running, smb1351_read_status() fails and
smb1351_get_property() sees !delayed_work_pending(&chip->work). It then
overwrites chip->retry_until with a fresh timeout and queues chip->work with
a delay of 0.

When the running smb1351_work() subsequently checks chip->retry_until:

	if (ret && time_before(jiffies, READ_ONCE(chip->retry_until))) {
		queue_delayed_work(system_freezable_wq, &chip->work,
				   msecs_to_jiffies(SMB1351_RETRY_MS));
		return;
	}

it observes the extended deadline instead of expiring and setting
chip->online to false, and its queue_delayed_work() call with
SMB1351_RETRY_MS is ignored because WORK_STRUCT_PENDING_BIT was already set
with delay 0, causing immediate back-to-back re-execution without the 200 ms
backoff.

Similarly, because power_supply_get_property() and
power_supply_external_power_changed() both acquire psy->epc_sem for reading,
smb1351_get_property() can also run concurrently with
smb1351_external_power_changed() and overwrite the longer
SMB1351_SETTLE_MS + SMB1351_RETRY_TIMEOUT_MS deadline in chip->retry_until.

> +
> +	if (psp == POWER_SUPPLY_PROP_STATUS)
> +		val->intval = ret ? POWER_SUPPLY_STATUS_UNKNOWN :
> +				    smb1351_status(stat);
> +	else
> +		val->intval = ret ? POWER_SUPPLY_CHARGE_TYPE_UNKNOWN :
> +				    smb1351_charge_type(stat);
> +
> +	return 0;
> +}

[ ... ]

> +static void smb1351_work(struct work_struct *work)
> +{
> +	struct smb1351 *chip = container_of(to_delayed_work(work),
> +					    struct smb1351, work);
> +	unsigned int version;
> +	bool online;
> +	int ret;
> +
> +	/* The chip only answers while it has VBUS */
> +	ret = regmap_read(chip->regmap, SMB1351_VERSION_REG, &version);
> +	online = !ret;
> +
> +	if (online && !chip->identified) {
> +		bool smb1350 = version & SMB1351_VERSION_SMB1350;
> +
> +		dev_info(chip->dev, "found %s (version %#04x)\n",
> +			 smb1350 ? "SMB1350" : "SMB1351", version);
> +		chip->identified = true;
> +	}
> +
> +	if (online)
> +		ret = smb1351_configure(chip);
> +
> +	if (ret && time_before(jiffies, READ_ONCE(chip->retry_until))) {
> +		queue_delayed_work(system_freezable_wq, &chip->work,
> +				   msecs_to_jiffies(SMB1351_RETRY_MS));
> +		return;
> +	}
> +
> +	if (!online) {
> +		/* Without VBUS the chip does not acknowledge its address */
> +		if (ret != -ENXIO)
> +			dev_warn_ratelimited(chip->dev,
> +					     "failed to read version: %pe\n",
> +					     ERR_PTR(ret));
> +	} else if (ret) {
> +		dev_err_ratelimited(chip->dev,
> +				    "failed to disable charging: %pe\n",
> +				    ERR_PTR(ret));
> +	}
> +
> +	if (online != chip->online) {
> +		dev_dbg(chip->dev, "VBUS %s\n", online ? "present" : "absent");
> +		WRITE_ONCE(chip->online, online);
> +		power_supply_changed(chip->psy);
> +	}
> +}
> +
> +static void smb1351_external_power_changed(struct power_supply *psy)
> +{
> +	struct smb1351 *chip = power_supply_get_drvdata(psy);
> +
> +	WRITE_ONCE(chip->retry_until,
> +		   jiffies + msecs_to_jiffies(SMB1351_SETTLE_MS +
> +					      SMB1351_RETRY_TIMEOUT_MS));
> +	mod_delayed_work(system_freezable_wq, &chip->work,
> +			 msecs_to_jiffies(SMB1351_SETTLE_MS));
> +}

[ ... ]

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