Re: [PATCH 2/2] power: supply: qcom_battmgr: Add thermal mitigation support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:

New issues:
- [High] Software state properties are updated outside of `battmgr->lock`, creating data races and state desynchronization.
- [High] Thermal mitigation state is lost on firmware restart (PDR) if the charging limit was throttled to 0 uA.
- [High] Calling `devm_thermal_of_cooling_device_register()` from an asynchronous workqueue causes a resource leak and use-after-free race with device unbind.
- [Medium] The SM8550 variant lacks the `CHARGE_CONTROL_LIMIT` and `CHARGE_CONTROL_LIMIT_MAX` sysfs properties.
- [Medium] Reading the charge control limits triggers synchronous, blocking firmware requests whose results are ignored.
- [Medium] Userspace sysfs race allows throttling to 0 uA due to uninitialized `thermal_levels[0]`.

Pre-existing issues:
- [High] `qcom_battmgr_request` is called without acquiring `battmgr->lock` in `qcom_battmgr_enable_worker`.
--

commit c50b550e81a56aef117f049101952557cf7ba5cd
Author: Dhruvin Rajpura <drajpura@qti.qualcomm.com>

power: supply: qcom_battmgr: Add thermal mitigation support

This patch explicitly registers a thermal cooling device for the battery
manager to throttle charging current during thermal stress. It maps discrete
current levels from the qcom,thermal-mitigation DT property to cooling
states. It also exposes CHARGE_CONTROL_LIMIT properties to sysfs to allow
userspace observation and control.

> diff --git a/drivers/power/supply/qcom_battmgr.c b/drivers/power/supply/qcom_battmgr.c
> --- a/drivers/power/supply/qcom_battmgr.c
> +++ b/drivers/power/supply/qcom_battmgr.c
> @@ -449,6 +460,8 @@ static const u8 sm8350_bat_prop_map[] = {
>  	[POWER_SUPPLY_PROP_POWER_NOW] = BATT_POWER_NOW,
>  	[POWER_SUPPLY_PROP_CHARGE_CONTROL_START_THRESHOLD] = BATT_CHG_CTRL_START_THR,
>  	[POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD] = BATT_CHG_CTRL_END_THR,
> +	[POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT] = BATT_CHG_CTRL_LIM,
> +	[POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT_MAX] = BATT_CHG_CTRL_LIM_MAX,
>  };

[Severity: Medium]
Does mapping BATT_CHG_CTRL_LIM trigger a blocking BATTMGR_BAT_PROPERTY_GET
request over GLINK when userspace reads POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT?

Looking at qcom_battmgr_bat_get_property(), the result of this firmware update
is discarded, and the locally cached curr_thermal_level is returned instead.
Could this be avoided to prevent a useless IPC call?

[ ... ]

> @@ -770,6 +789,55 @@ static int qcom_battmgr_bat_is_writeable(struct power_supply *psy,
>  	return 0;
>  }
>  
> +static int __battery_psy_set_charge_current(struct qcom_battmgr *battmgr,
> +					    u32 fcc_ua)
> +{
> +	int ret;
> +
> +	mutex_lock(&battmgr->lock);
> +	ret = qcom_battmgr_request_property(battmgr, BATTMGR_BAT_PROPERTY_SET,
> +					    BATT_CHG_CTRL_LIM, fcc_ua);
> +	mutex_unlock(&battmgr->lock);
> +	if (ret < 0) {
> +		dev_err(battmgr->dev, "Failed to set FCC %u, ret=%d\n", fcc_ua, ret);
> +	} else {
> +		dev_dbg(battmgr->dev, "Set FCC to %u uA\n", fcc_ua);
> +		battmgr->last_fcc_ua = fcc_ua;

[Severity: High]
Can concurrent updates desynchronize the software tracking variables from the
hardware state?

The GLINK request is protected by battmgr->lock, but last_fcc_ua is assigned
here after the lock is released.

> +	}
> +
> +	return ret;
> +}
> +
> +static int battery_psy_set_charge_current(struct qcom_battmgr *battmgr,
> +					  int val)
> +{
> +	u32 fcc_ua, prev_fcc_ua;
> +	int ret;
> +
> +	if (!battmgr->num_thermal_levels)
> +		return 0;
> +
> +	if (battmgr->num_thermal_levels < 0) {
> +		dev_err(battmgr->dev, "Incorrect num_thermal_levels\n");
> +		return -EINVAL;
> +	}
> +
> +	if (val < 0 || val > battmgr->num_thermal_levels)
> +		return -EINVAL;
> +
> +	fcc_ua = battmgr->thermal_levels[val];

[Severity: Medium]
If sysfs writes to CHARGE_CONTROL_LIMIT occur after probe() but before
qcom_battmgr_enable_worker() has initialized thermal_levels[0], could this read
a zero-initialized value and improperly throttle charging to 0 uA?

> +	prev_fcc_ua = battmgr->thermal_fcc_ua;
> +	battmgr->thermal_fcc_ua = fcc_ua;
> +
> +	ret = __battery_psy_set_charge_current(battmgr, fcc_ua);
> +	if (!ret)
> +		battmgr->curr_thermal_level = val;
> +	else
> +		battmgr->thermal_fcc_ua = prev_fcc_ua;

[Severity: High]
Additionally, thermal_fcc_ua and curr_thermal_level are updated here completely
outside of battmgr->lock. If thermal framework and sysfs operations concurrently
set the charge current, could the software state interleave with hardware
updates?

> +
> +	return ret;
> +}

[ ... ]

> @@ -892,6 +977,8 @@ static const struct power_supply_desc sm8350_bat_psy_desc = {
>  	.properties = sm8350_bat_props,
>  	.num_properties = ARRAY_SIZE(sm8350_bat_props),
>  	.get_property = qcom_battmgr_bat_get_property,
> +	.set_property = qcom_battmgr_bat_set_property,
> +	.property_is_writeable = qcom_battmgr_prop_is_writeable,
>  };
>  
>  static const enum power_supply_property sm8550_bat_props[] = {

[Severity: Medium]
The commit message states that CHARGE_CONTROL_LIMIT and
CHARGE_CONTROL_LIMIT_MAX are exposed to allow userspace control. Should
these properties also be added to sm8550_bat_props to expose them for the
SM8550 variant?

[ ... ]

> +static int qcom_battmgr_register_thermal_cooling_device(struct qcom_battmgr *battmgr)
> +{
> +	struct thermal_cooling_device *tcd;
> +	u32 prev;
> +	int ret, i;

[ ... ]

> +	battmgr->thermal_levels[0] = battmgr->status.max_charge_ctl_limit;
> +	battmgr->thermal_fcc_ua = battmgr->status.max_charge_ctl_limit;
> +
> +	/* Register Thermal Cooling Device */
> +	tcd = devm_thermal_of_cooling_device_register(battmgr->dev,
> +						      battmgr->dev->of_node,
> +						      (char *)battmgr->bat_psy->desc->name,
> +						      battmgr, &battery_tcd_ops);

[Severity: High]
Is it safe to allocate devm_ resources from an asynchronous workqueue?

If the driver unbinds, devres_release_all() will free the battmgr structure.
If qcom_battmgr_enable_worker() runs concurrently or just after, could it
register a cooling device to a dead device's devres list, leading to a
resource leak and eventual use-after-free when the thermal core polls it?

[ ... ]

> @@ -1596,6 +1834,18 @@ static void qcom_battmgr_enable_worker(struct work_struct *work)
>  	struct qcom_battmgr *battmgr = container_of(work, struct qcom_battmgr, enable_work);
>  	struct qcom_battmgr_enable_request req = {
>  		.hdr.owner = cpu_to_le32(PMIC_GLINK_OWNER_BATTMGR),
>  		.hdr.type = cpu_to_le32(PMIC_GLINK_NOTIFY),
>  		.hdr.opcode = cpu_to_le32(BATTMGR_REQUEST_NOTIFICATION),
>  	};
>  	int ret;
>  
>  	ret = qcom_battmgr_request(battmgr, &req, sizeof(req));

[Severity: High]
This isn't a bug introduced by this patch, but is it intentional that
qcom_battmgr_request() is called here without holding battmgr->lock?

If sysfs operations take the lock simultaneously, could this concurrent
execution mutate the GLINK communication state and corrupt
reinit_completion(&battmgr->ack)?

>  	if (ret)
>  		dev_err(battmgr->dev, "failed to request power notifications\n");
> +
> +	if (!battmgr->thermal_dev_registered) {
> +		ret = qcom_battmgr_register_thermal_cooling_device(battmgr);
> +		if (ret < 0)
> +			dev_err(battmgr->dev, "failed to register thermal cooling device ret:%d\n",
> +				ret);
> +	} else if (battmgr->last_fcc_ua) {

[Severity: High]
Does checking if (battmgr->last_fcc_ua) prevent restoration of the mitigation
limit after a firmware restart?

If the device was legitimately throttled to 0 microamps due to extreme heat,
this check evaluates to false and skips the restoration, which could cause
the firmware to resume charging at the maximum limit.

> +		ret = __battery_psy_set_charge_current(battmgr, battmgr->last_fcc_ua);
> +		if (ret < 0)
> +			dev_err(battmgr->dev, "Failed to set FCC (%u uA), ret:%d\n",
> +				battmgr->last_fcc_ua, ret);
> +	}
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609-cooling_device_reg-v1-0-e15bddcb0086@qti.qualcomm.com?part=2