[PATCH v2 0/4] power: supply: Fix probe time race against driver teardown and battery parsing

[PATCH v2 0/4] power: supply: Fix probe time race against driver teardown and battery parsing

[Alexey Charkov]

If a bus holding the battery-equipped power supply goes down right after
it registered, it leads to massive fireworks as the driver starts to tear
down its resources while callers from other contexts are already eager to
process events using this driver's callbacks (and their private data).

In current power supply core it's further aggravated by the fact that the
battery parsing code expects a psy struct from the caller, which is only
available once the device has been registered, so the device gets
registered with its hardware constraints (e.g. charging current) not yet
fully filled in.

Fix the former by preventing the unregistration of the psy device before
its callbacks complete, and the latter by providing a battery parsing
helper which doesn't require a psy struct from the caller (as it doesn't
strictly need one).

While here, fix up these issues in bq257xx where all of it was discovered.

Signed-off-by: Alexey Charkov <alchark@flipper.net>
---
Changes in v2:
- Use psy->removing as the predicate for detecting unregistration instead
  of use_cnt, because a leaking caller can leave use_cnt non-zero even
  after the device is being removed, and ->removing already exists and is
  used by the removal function anyway (Sashiko)
- Don't rearrange the existing short-circuit check for use_cnt and the
  availability of the callback in the psy struct to avoid breakage (Sashiko)
- Drop the patch making the bq257xx_external_power_changed() return early
  upon I2C read failures, otherwise a transient I2C failure in updating
  cached status fields could lead to missed updates to charging current or
  input current, which is worse (Sashiko). Those status fields are re-read
  upon every property update anyway
- Amend patch description of patch 3 (formerly 4) to explicitly state the
  potential use of unallocated memory, and add a Fixes: tag accordingly
- Link to v1: https://patch.msgid.link/20260907-bq257xx-init-v1-0-1a08b29e1a68@flipper.net

To: Sebastian Reichel <sre@kernel.org>
To: Pavel Machek <pavel@ucw.cz>
To: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
To: Krzysztof Kozlowski <krzk@kernel.org>
To: Lee Jones <lee@kernel.org>
To: Chris Morgan <macromorgan@hotmail.com>
Cc: linux-pm@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: Sebastian Reichel <sebastian.reichel@collabora.com>

---
Alexey Charkov (4):
      power: supply: core: prevent unregistering a power supply while a callback runs
      power: supply: core: Allow getting battery info before psy is registered
      power: supply: bq257xx: Use psy directly instead of driver data
      power: supply: bq257xx: Parse battery info before registering power supply

 drivers/power/supply/bq257xx_charger.c   |  52 +++++++------
 drivers/power/supply/power_supply_core.c | 128 +++++++++++++++++++++++--------
 include/linux/power_supply.h             |  14 ++++
 3 files changed, 142 insertions(+), 52 deletions(-)
---
base-commit: c68a982815dcce5464e3bf2a31ac94f5146c04ca
change-id: 20260907-bq257xx-init-ff03beb07c36

Best regards,
--  
Alexey Charkov <alchark@flipper.net>

[PATCH v2 1/4] power: supply: core: prevent unregistering a power supply while a callback runs

[Alexey Charkov]

Once a power supply is registered, its callbacks can immediately start
firing from other contexts, such as external_power_changed() triggered by
the TCPM stack. If a power supply is unregistered while the callback is
still running, the driver data can already be freed when the callback
tries to access it, leading to a use-after-free.

This happens e.g. when the hardware bus carrying the power supply device
malfunctions (e.g. I2C is hogged down by another malfunctioning device)
immediately after the power supply is registered, and thus the core is
still processing the callbacks which were queued up when the driver
starts the removal, leading in some cases to a kernel crash, e.g.:

[   11.645942] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000005
[   11.646751] Mem abort info:
[   11.647006]   ESR = 0x0000000096000004
[   11.647338]   EC = 0x25: DABT (current EL), IL = 32 bits
[   11.647806]   SET = 0, FnV = 0
[   11.648077]   EA = 0, S1PTW = 0
[   11.648356]   FSC = 0x04: level 0 translation fault
[   11.648785] Data abort info:
[   11.649041]   ISV = 0, ISS = 0x00000004, ISS2 = 0x00000000
[   11.649524]   CM = 0, WnR = 0, TnD = 0, TagAccess = 0
[   11.649981]   GCS = 0, Overlay = 0, DirtyBit = 0
[   11.650390] [0000000000000005] user address but active_mm is swapper
[   11.650955] Internal error: Oops: 0000000096000004 [#1]  SMP
[   11.651460] Modules linked in:
[   11.651742] CPU: 1 UID: 0 PID: 144 Comm: kworker/1:2 Not tainted 7.2.0-rc6-g62a9297af2cd #1 PREEMPT
[   11.652553] Hardware name: Flipper One rev. F0B1C2 (DT)
[   11.653024] Workqueue: events power_supply_changed_work
[   11.653511] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[   11.654135] pc : __power_supply_is_supplied_by+0x18/0x100
[   11.654624] lr : __power_supply_am_i_supplied+0x40/0xb8
[   11.655098] sp : ffff80008192bb30
[   11.655399] x29: ffff80008192bb30 x28: 0000000000000000 x27: 0000000000000000
[   11.656049] x26: 0000000000000000 x25: 0000000000000000 x24: 0000000000000000
[   11.656695] x23: ffff0000c19f4200 x22: ffffdb2232ba4ea8 x21: ffff80008192bc28
[   11.657344] x20: ffff0000c1eef000 x19: ffff80008192bc18 x18: 00000000a0886e62
[   11.657991] x17: 000000040044ffff x16: 04500072b5503510 x15: 0000000000000000
[   11.658639] x14: 0000000000000000 x13: 0000000000000220 x12: 0000000000000000
[   11.659286] x11: 0000000000000000 x10: ffff0000c1fdb2b0 x9 : ffffdb2232ba5590
[   11.659934] x8 : 00000000e5b906e6 x7 : ffff0000c2502778 x6 : ffffdb22339793d0
[   11.660581] x5 : ffff80008192bc18 x4 : ffff0000c19cbca0 x3 : 0000000000000000
[   11.661228] x2 : ffff0000c1fdaf40 x1 : ffffffffffffffed x0 : ffff0000c1eef000
[   11.661878] Call trace:
[   11.662103]  __power_supply_is_supplied_by+0x18/0x100 (P)
[   11.662596]  __power_supply_am_i_supplied+0x40/0xb8
[   11.663040]  psy_for_each_psy_cb+0x20/0x40
[   11.663416]  class_for_each_device+0x110/0x150
[   11.663825]  power_supply_am_i_supplied+0x68/0x100
[   11.664262]  bq257xx_external_power_changed+0x58/0x140
[   11.664733]  __power_supply_changed_work+0x60/0x80
[   11.665170]  psy_for_each_psy_cb+0x20/0x40
[   11.665545]  class_for_each_device+0x110/0x150
[   11.665953]  power_supply_changed_work+0x98/0x1b8
[   11.666382]  process_one_work+0x164/0x4c0
[   11.666758]  worker_thread+0x19c/0x320
[   11.667104]  kthread+0x138/0x150
[   11.667408]  ret_from_fork+0x10/0x20
[   11.667744] Code: d503233f a9bd7bfd 910003fd a90153f3 (f9400c34)
[   11.668294] ---[ end trace 0000000000000000 ]---

Add a read-write semaphore between external_power_changed() and
power_supply_unregister() to prevent the latter from returning (and thus
the driver from freeing its data) while the callback is still running.

Fixes: bc1540561c9e ("power_supply: Add API for safe access of power supply function attrs")
Signed-off-by: Alexey Charkov <alchark@flipper.net>
---
 drivers/power/supply/power_supply_core.c | 26 +++++++++++++++++++++++++-
 include/linux/power_supply.h             | 10 ++++++++++
 2 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
index 47e307709e5e..1279785645fb 100644
--- a/drivers/power/supply/power_supply_core.c
+++ b/drivers/power/supply/power_supply_core.c
@@ -1530,6 +1530,19 @@ void power_supply_external_power_changed(struct power_supply *psy)
 			!psy->desc->external_power_changed)
 		return;
 
+	/*
+	 * Keep power_supply_unregister() from returning, and thus from letting
+	 * the driver's data be freed, while the callback is running. The
+	 * ->removing check has to happen under the lock: on its own, just like
+	 * the use_cnt check above, it only tells us the supply was still there
+	 * when we looked, not that it still is by the time the callback
+	 * dereferences its driver data.
+	 */
+	guard(rwsem_read)(&psy->epc_sem);
+
+	if (psy->removing)
+		return;
+
 	psy->desc->external_power_changed(psy);
 }
 EXPORT_SYMBOL_GPL(power_supply_external_power_changed);
@@ -1774,6 +1787,7 @@ __power_supply_register(struct device *parent,
 	}
 
 	spin_lock_init(&psy->changed_lock);
+	init_rwsem(&psy->epc_sem);
 	init_rwsem(&psy->extensions_sem);
 	INIT_LIST_HEAD(&psy->extensions);
 
@@ -1914,7 +1928,17 @@ EXPORT_SYMBOL_GPL(devm_power_supply_register);
 void power_supply_unregister(struct power_supply *psy)
 {
 	WARN_ON(atomic_dec_return(&psy->use_cnt));
-	psy->removing = true;
+
+	/*
+	 * Publish ->removing so that no new ->external_power_changed() call can
+	 * start, and wait via semaphore for one that is already running: it may
+	 * be a supplier's changed_work, which cancel_work_sync() below does not
+	 * cover, and it may still dereference driver data that the caller is
+	 * about to free.
+	 */
+	scoped_guard(rwsem_write, &psy->epc_sem)
+		psy->removing = true;
+
 	cancel_work_sync(&psy->changed_work);
 	cancel_delayed_work_sync(&psy->deferred_register_work);
 	sysfs_remove_link(&psy->dev.kobj, "powers");
diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index e749d2189335..131cafded72f 100644
--- a/include/linux/power_supply.h
+++ b/include/linux/power_supply.h
@@ -351,6 +351,16 @@ struct power_supply {
 	bool removing;
 	atomic_t use_cnt;
 	struct power_supply_battery_info *battery_info;
+	/*
+	 * Held for read while ->external_power_changed() runs, and for write by
+	 * power_supply_unregister() when it publishes @removing, so that it
+	 * waits for an in-flight callback to finish. Without this a driver's
+	 * data, typically devm-allocated on its own device, can be freed while
+	 * the callback is still using it.
+	 * Must not be shared with extensions_sem: callbacks may read their own
+	 * properties, which takes that one for read.
+	 */
+	struct rw_semaphore epc_sem;
 	struct rw_semaphore extensions_sem; /* protects "extensions" */
 	struct list_head extensions;
 #ifdef CONFIG_THERMAL

-- 
2.55.0

[PATCH v2 2/4] power: supply: core: Allow getting battery info before psy is registered

[Alexey Charkov]

Some power supplies, such as battery chargers, may need to program the
device parameters based on what their connected battery allows. Current
API requires registering the power supply to access battery information,
which is problematic because a registered power supply is immediately
available to the rest of the system, but the battery parameters may not
be set yet in the charger.

Given that the battery info helpers really only need a fwnode and a struct
device to hang devres-allocated resourses on, add a pure dev-based get/put
API alongside the existing psy-based one, which can be used to query the
battery information before registering the power supply.

Signed-off-by: Alexey Charkov <alchark@flipper.net>
---
 drivers/power/supply/power_supply_core.c | 102 ++++++++++++++++++++++---------
 include/linux/power_supply.h             |   4 ++
 2 files changed, 77 insertions(+), 29 deletions(-)

diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
index 1279785645fb..09473361772f 100644
--- a/drivers/power/supply/power_supply_core.c
+++ b/drivers/power/supply/power_supply_core.c
@@ -725,21 +725,18 @@ struct power_supply *devm_power_supply_get_by_reference(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(devm_power_supply_get_by_reference);
 
-int power_supply_get_battery_info(struct power_supply *psy,
-				  struct power_supply_battery_info **info_out)
+static int __power_supply_get_battery_info(struct device *dev,
+					   struct fwnode_handle *srcnode,
+					   struct power_supply_battery_info **info_out)
 {
 	struct power_supply_resistance_temp_table *resist_table;
 	struct power_supply_battery_info *info;
-	struct fwnode_handle *srcnode, *fwnode;
+	struct fwnode_handle *fwnode;
 	const char *value;
 	int err, len, index, proplen;
 	u32 *propdata __free(kfree) = NULL;
 	u32 min_max[2];
 
-	srcnode = dev_fwnode(&psy->dev);
-	if (!srcnode && psy->dev.parent)
-		srcnode = dev_fwnode(psy->dev.parent);
-
 	fwnode = fwnode_find_reference(srcnode, "monitored-battery", 0);
 	if (IS_ERR(fwnode))
 		return PTR_ERR(fwnode);
@@ -750,7 +747,7 @@ int power_supply_get_battery_info(struct power_supply *psy,
 
 
 	/* Try static batteries first */
-	err = samsung_sdi_battery_get_info(&psy->dev, value, &info);
+	err = samsung_sdi_battery_get_info(dev, value, &info);
 	if (!err)
 		goto out_ret_pointer;
 	else if (err == -ENODEV)
@@ -765,7 +762,7 @@ int power_supply_get_battery_info(struct power_supply *psy,
 		goto out_put_node;
 	}
 
-	info = devm_kzalloc(&psy->dev, sizeof(*info), GFP_KERNEL);
+	info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
 	if (!info) {
 		err = -ENOMEM;
 		goto out_put_node;
@@ -826,7 +823,7 @@ int power_supply_get_battery_info(struct power_supply *psy,
 		else if (!strcmp("lithium-ion-manganese-oxide", value))
 			info->technology = POWER_SUPPLY_TECHNOLOGY_LiMn;
 		else
-			dev_warn(&psy->dev, "%s unknown battery type\n", value);
+			dev_warn(dev, "%s unknown battery type\n", value);
 	}
 
 	fwnode_property_read_u32(fwnode, "energy-full-design-microwatt-hours",
@@ -877,7 +874,7 @@ int power_supply_get_battery_info(struct power_supply *psy,
 		err = len;
 		goto out_put_node;
 	} else if (len > POWER_SUPPLY_OCV_TEMP_MAX) {
-		dev_err(&psy->dev, "Too many temperature values\n");
+		dev_err(dev, "Too many temperature values\n");
 		err = -EINVAL;
 		goto out_put_node;
 	} else if (len > 0) {
@@ -892,28 +889,28 @@ int power_supply_get_battery_info(struct power_supply *psy,
 		char *propname __free(kfree) = kasprintf(GFP_KERNEL, "ocv-capacity-table-%d",
 							 index);
 		if (!propname) {
-			power_supply_put_battery_info(psy, info);
+			power_supply_put_battery_info_from_dev(dev, info);
 			err = -ENOMEM;
 			goto out_put_node;
 		}
 		proplen = fwnode_property_count_u32(fwnode, propname);
 		if (proplen < 0 || proplen % 2 != 0) {
-			dev_err(&psy->dev, "failed to get %s\n", propname);
-			power_supply_put_battery_info(psy, info);
+			dev_err(dev, "failed to get %s\n", propname);
+			power_supply_put_battery_info_from_dev(dev, info);
 			err = -EINVAL;
 			goto out_put_node;
 		}
 
 		u32 *propdata __free(kfree) = kzalloc_objs(*propdata, proplen);
 		if (!propdata) {
-			power_supply_put_battery_info(psy, info);
+			power_supply_put_battery_info_from_dev(dev, info);
 			err = -EINVAL;
 			goto out_put_node;
 		}
 		err = fwnode_property_read_u32_array(fwnode, propname, propdata, proplen);
 		if (err < 0) {
-			dev_err(&psy->dev, "failed to get %s\n", propname);
-			power_supply_put_battery_info(psy, info);
+			dev_err(dev, "failed to get %s\n", propname);
+			power_supply_put_battery_info_from_dev(dev, info);
 			goto out_put_node;
 		}
 
@@ -921,9 +918,9 @@ int power_supply_get_battery_info(struct power_supply *psy,
 		info->ocv_table_size[index] = tab_len;
 
 		info->ocv_table[index] = table =
-			devm_kcalloc(&psy->dev, tab_len, sizeof(*table), GFP_KERNEL);
+			devm_kcalloc(dev, tab_len, sizeof(*table), GFP_KERNEL);
 		if (!info->ocv_table[index]) {
-			power_supply_put_battery_info(psy, info);
+			power_supply_put_battery_info_from_dev(dev, info);
 			err = -ENOMEM;
 			goto out_put_node;
 		}
@@ -939,14 +936,14 @@ int power_supply_get_battery_info(struct power_supply *psy,
 		err = 0;
 		goto out_ret_pointer;
 	} else if (proplen < 0 || proplen % 2 != 0) {
-		power_supply_put_battery_info(psy, info);
+		power_supply_put_battery_info_from_dev(dev, info);
 		err = (proplen < 0) ? proplen : -EINVAL;
 		goto out_put_node;
 	}
 
 	propdata = kzalloc_objs(*propdata, proplen);
 	if (!propdata) {
-		power_supply_put_battery_info(psy, info);
+		power_supply_put_battery_info_from_dev(dev, info);
 		err = -ENOMEM;
 		goto out_put_node;
 	}
@@ -954,17 +951,17 @@ int power_supply_get_battery_info(struct power_supply *psy,
 	err = fwnode_property_read_u32_array(fwnode, "resistance-temp-table",
 					     propdata, proplen);
 	if (err < 0) {
-		power_supply_put_battery_info(psy, info);
+		power_supply_put_battery_info_from_dev(dev, info);
 		goto out_put_node;
 	}
 
 	info->resist_table_size = proplen / 2;
-	info->resist_table = resist_table = devm_kcalloc(&psy->dev,
+	info->resist_table = resist_table = devm_kcalloc(dev,
 							 info->resist_table_size,
 							 sizeof(*resist_table),
 							 GFP_KERNEL);
 	if (!info->resist_table) {
-		power_supply_put_battery_info(psy, info);
+		power_supply_put_battery_info_from_dev(dev, info);
 		err = -ENOMEM;
 		goto out_put_node;
 	}
@@ -982,22 +979,69 @@ int power_supply_get_battery_info(struct power_supply *psy,
 	fwnode_handle_put(fwnode);
 	return err;
 }
+
+int power_supply_get_battery_info(struct power_supply *psy,
+				  struct power_supply_battery_info **info_out)
+{
+	struct fwnode_handle *srcnode;
+
+	srcnode = dev_fwnode(&psy->dev);
+	if (!srcnode && psy->dev.parent)
+		srcnode = dev_fwnode(psy->dev.parent);
+
+	return __power_supply_get_battery_info(&psy->dev, srcnode, info_out);
+}
 EXPORT_SYMBOL_GPL(power_supply_get_battery_info);
 
-void power_supply_put_battery_info(struct power_supply *psy,
-				   struct power_supply_battery_info *info)
+/**
+ * power_supply_get_battery_info_from_dev() - Get battery info without a supply
+ * @dev: Device holding the "monitored-battery" reference, which also owns the
+ *	 devres allocations made for the returned info
+ * @info_out: Pointer to store the resulting battery info
+ *
+ * Same as power_supply_get_battery_info(), but keyed off a plain device rather
+ * than a registered power supply. Chargers that program hardware limits taken
+ * from the battery node need those values *before* they can safely register
+ * their power supply: registering makes the supply callable, so a later probe
+ * failure would free driver data underneath a running callback.
+ *
+ * Release the result with power_supply_put_battery_info_from_dev().
+ *
+ * Return: 0 on success or an error code on failure.
+ */
+int power_supply_get_battery_info_from_dev(struct device *dev,
+					   struct power_supply_battery_info **info_out)
+{
+	return __power_supply_get_battery_info(dev, dev_fwnode(dev), info_out);
+}
+EXPORT_SYMBOL_GPL(power_supply_get_battery_info_from_dev);
+
+/**
+ * power_supply_put_battery_info_from_dev() - Release battery info
+ * @dev: Device passed to power_supply_get_battery_info_from_dev()
+ * @info: Battery info to release
+ */
+void power_supply_put_battery_info_from_dev(struct device *dev,
+					    struct power_supply_battery_info *info)
 {
 	int i;
 
 	for (i = 0; i < POWER_SUPPLY_OCV_TEMP_MAX; i++) {
 		if (info->ocv_table[i])
-			devm_kfree(&psy->dev, info->ocv_table[i]);
+			devm_kfree(dev, info->ocv_table[i]);
 	}
 
 	if (info->resist_table)
-		devm_kfree(&psy->dev, info->resist_table);
+		devm_kfree(dev, info->resist_table);
+
+	devm_kfree(dev, info);
+}
+EXPORT_SYMBOL_GPL(power_supply_put_battery_info_from_dev);
 
-	devm_kfree(&psy->dev, info);
+void power_supply_put_battery_info(struct power_supply *psy,
+				   struct power_supply_battery_info *info)
+{
+	power_supply_put_battery_info_from_dev(&psy->dev, info);
 }
 EXPORT_SYMBOL_GPL(power_supply_put_battery_info);
 
diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index 131cafded72f..f42ae4e3bf81 100644
--- a/include/linux/power_supply.h
+++ b/include/linux/power_supply.h
@@ -865,6 +865,10 @@ extern int power_supply_get_battery_info(struct power_supply *psy,
 					 struct power_supply_battery_info **info_out);
 extern void power_supply_put_battery_info(struct power_supply *psy,
 					  struct power_supply_battery_info *info);
+extern int power_supply_get_battery_info_from_dev(struct device *dev,
+						  struct power_supply_battery_info **info_out);
+extern void power_supply_put_battery_info_from_dev(struct device *dev,
+						   struct power_supply_battery_info *info);
 extern bool power_supply_battery_info_has_prop(struct power_supply_battery_info *info,
 					       enum power_supply_property psp);
 extern int power_supply_battery_info_get_prop(struct power_supply_battery_info *info,

-- 
2.55.0

[PATCH v2 3/4] power: supply: bq257xx: Use psy directly instead of driver data

[Alexey Charkov]

bq257xx_external_power_changed() is handed the power supply it is called
for, but passes pdata->charger to power_supply_am_i_supplied(). Those are
not equivalent during probe or teardown, and a supplier's changed_work can
reach power_supply_external_power_changed() when pdata->charger doesn't
point anywhere meaningful (e.g. freed memory during teardown).

Use the psy argument, which is valid whenever the callback runs.

Fixes: 1cc017b7f9c7 ("power: supply: bq257xx: Add support for BQ257XX charger")
Signed-off-by: Alexey Charkov <alchark@flipper.net>
---
 drivers/power/supply/bq257xx_charger.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/power/supply/bq257xx_charger.c b/drivers/power/supply/bq257xx_charger.c
index b735bb02a8e0..ab3519454193 100644
--- a/drivers/power/supply/bq257xx_charger.c
+++ b/drivers/power/supply/bq257xx_charger.c
@@ -1055,7 +1055,7 @@ static void bq257xx_external_power_changed(struct power_supply *psy)
 
 	pdata->chip->bq257xx_get_state(pdata);
 
-	pdata->supplied = power_supply_am_i_supplied(pdata->charger);
+	pdata->supplied = power_supply_am_i_supplied(psy);
 	if (pdata->supplied < 0)
 		return;
 

-- 
2.55.0

[PATCH v2 4/4] power: supply: bq257xx: Parse battery info before registering power supply

[Alexey Charkov]

Switch to a dev-based battery get/put interface to parse battery info
before registering the power supply, so that nobody tries to access the
power supply until we finish programming the device parameters.

Signed-off-by: Alexey Charkov <alchark@flipper.net>
---
 drivers/power/supply/bq257xx_charger.c | 50 ++++++++++++++++++++--------------
 1 file changed, 29 insertions(+), 21 deletions(-)

diff --git a/drivers/power/supply/bq257xx_charger.c b/drivers/power/supply/bq257xx_charger.c
index ab3519454193..a02d293c7395 100644
--- a/drivers/power/supply/bq257xx_charger.c
+++ b/drivers/power/supply/bq257xx_charger.c
@@ -1163,38 +1163,38 @@ static const struct bq257xx_chip_info bq25792_chip_info = {
 /**
  * bq257xx_parse_dt() - Parse the device tree for required properties
  * @pdata: driver platform data
- * @psy_cfg: power supply config data
  * @dev: device struct
  *
  * Read the device tree to identify the minimum system voltage, the
  * maximum charge current, the maximum charge voltage, and the maximum
- * input current.
+ * input current. Deliberately keyed off @dev rather than the charger power
+ * supply, so that it can run before the supply is registered.
  *
  * Return: Returns 0 on success or error code on error.
  */
-static int bq257xx_parse_dt(struct bq257xx_chg *pdata,
-		struct power_supply_config *psy_cfg, struct device *dev)
+static int bq257xx_parse_dt(struct bq257xx_chg *pdata, struct device *dev)
 {
 	struct power_supply_battery_info *bat_info;
 	int ret;
 
-	ret = power_supply_get_battery_info(pdata->charger,
-					    &bat_info);
+	ret = power_supply_get_battery_info_from_dev(dev, &bat_info);
 	if (ret)
 		return dev_err_probe(dev, ret,
 				     "Unable to get battery info\n");
 
 	if ((bat_info->voltage_min_design_uv <= 0) ||
 	    (bat_info->constant_charge_voltage_max_uv <= 0) ||
-	    (bat_info->constant_charge_current_max_ua <= 0))
+	    (bat_info->constant_charge_current_max_ua <= 0)) {
+		power_supply_put_battery_info_from_dev(dev, bat_info);
 		return dev_err_probe(dev, -EINVAL,
 				     "Required bat info missing or invalid\n");
+	}
 
 	pdata->vsys_min = bat_info->voltage_min_design_uv;
 	pdata->vbat_max = bat_info->constant_charge_voltage_max_uv;
 	pdata->ichg_max = bat_info->constant_charge_current_max_ua;
 
-	power_supply_put_battery_info(pdata->charger, bat_info);
+	power_supply_put_battery_info_from_dev(dev, bat_info);
 
 	ret = device_property_read_u32(dev,
 				       "input-current-limit-microamp",
@@ -1210,9 +1210,14 @@ static int bq257xx_parse_dt(struct bq257xx_chg *pdata,
  * @pdev: platform device
  *
  * Probe the charger device, allocate driver data structure, select the
- * appropriate chip-specific function pointers, register the power supply,
- * parse device tree properties for battery limits, initialize hardware,
- * and set up the interrupt handler if available.
+ * appropriate chip-specific function pointers, parse device tree properties
+ * for battery limits, initialize hardware, register the power supply, and set
+ * up the interrupt handler if available.
+ *
+ * The power supply is registered only once the hardware is up, because
+ * registering it lets the core call ->external_power_changed() at any time. A
+ * probe failure after that point would have devres free @pdata while such a
+ * callback is still running on it.
  *
  * Return: Returns 0 on success or error code on failure.
  */
@@ -1245,6 +1250,14 @@ static int bq257xx_charger_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, pdata);
 
+	ret = bq257xx_parse_dt(pdata, dev);
+	if (ret)
+		return ret;
+
+	ret = pdata->chip->bq257xx_hw_init(pdata);
+	if (ret)
+		return dev_err_probe(dev, ret, "Cannot initialize the charger\n");
+
 	psy_cfg.drv_data = pdata;
 	psy_cfg.fwnode = dev_fwnode(dev);
 
@@ -1255,16 +1268,11 @@ static int bq257xx_charger_probe(struct platform_device *pdev)
 		return dev_err_probe(dev, PTR_ERR(pdata->charger),
 				     "Power supply register charger failed\n");
 
-	ret = bq257xx_parse_dt(pdata, &psy_cfg, dev);
-	if (ret)
-		return ret;
-
-	ret = pdata->chip->bq257xx_hw_init(pdata);
-	if (ret)
-		return dev_err_probe(dev, ret, "Cannot initialize the charger\n");
-
-	platform_set_drvdata(pdev, pdata);
-
+	/*
+	 * Requested after the supply is registered so that devres tears it down
+	 * first, quiescing the interrupt before the supply it reports on goes
+	 * away.
+	 */
 	if (bq->client->irq) {
 		ret = devm_request_threaded_irq(dev, bq->client->irq, NULL,
 						bq257xx_irq_handler_thread,

-- 
2.55.0

Re: [PATCH v2 2/4] power: supply: core: Allow getting battery info before psy is registered

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 11204 bytes --]

Hello Alexey,

On Wed, Sep 09, 2026 at 07:41:45PM +0400, Alexey Charkov wrote:
> Some power supplies, such as battery chargers, may need to program the
> device parameters based on what their connected battery allows. Current
> API requires registering the power supply to access battery information,
> which is problematic because a registered power supply is immediately
> available to the rest of the system, but the battery parameters may not
> be set yet in the charger.
> 
> Given that the battery info helpers really only need a fwnode and a struct
> device to hang devres-allocated resourses on, add a pure dev-based get/put
> API alongside the existing psy-based one, which can be used to query the
> battery information before registering the power supply.

Use the new init callback for that, which got introduced in the v7.3
cycle:

c1eb5905fdce ("power: supply: Add registration init callback")

See for example fdece8642eca ("power: supply: bq25630: Initialize
hardware before exposing the power supply") for a driver that was
converted to this.

Greetings,

-- Sebastian

> 
> Signed-off-by: Alexey Charkov <alchark@flipper.net>
> ---
>  drivers/power/supply/power_supply_core.c | 102 ++++++++++++++++++++++---------
>  include/linux/power_supply.h             |   4 ++
>  2 files changed, 77 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
> index 1279785645fb..09473361772f 100644
> --- a/drivers/power/supply/power_supply_core.c
> +++ b/drivers/power/supply/power_supply_core.c
> @@ -725,21 +725,18 @@ struct power_supply *devm_power_supply_get_by_reference(struct device *dev,
>  }
>  EXPORT_SYMBOL_GPL(devm_power_supply_get_by_reference);
>  
> -int power_supply_get_battery_info(struct power_supply *psy,
> -				  struct power_supply_battery_info **info_out)
> +static int __power_supply_get_battery_info(struct device *dev,
> +					   struct fwnode_handle *srcnode,
> +					   struct power_supply_battery_info **info_out)
>  {
>  	struct power_supply_resistance_temp_table *resist_table;
>  	struct power_supply_battery_info *info;
> -	struct fwnode_handle *srcnode, *fwnode;
> +	struct fwnode_handle *fwnode;
>  	const char *value;
>  	int err, len, index, proplen;
>  	u32 *propdata __free(kfree) = NULL;
>  	u32 min_max[2];
>  
> -	srcnode = dev_fwnode(&psy->dev);
> -	if (!srcnode && psy->dev.parent)
> -		srcnode = dev_fwnode(psy->dev.parent);
> -
>  	fwnode = fwnode_find_reference(srcnode, "monitored-battery", 0);
>  	if (IS_ERR(fwnode))
>  		return PTR_ERR(fwnode);
> @@ -750,7 +747,7 @@ int power_supply_get_battery_info(struct power_supply *psy,
>  
>  
>  	/* Try static batteries first */
> -	err = samsung_sdi_battery_get_info(&psy->dev, value, &info);
> +	err = samsung_sdi_battery_get_info(dev, value, &info);
>  	if (!err)
>  		goto out_ret_pointer;
>  	else if (err == -ENODEV)
> @@ -765,7 +762,7 @@ int power_supply_get_battery_info(struct power_supply *psy,
>  		goto out_put_node;
>  	}
>  
> -	info = devm_kzalloc(&psy->dev, sizeof(*info), GFP_KERNEL);
> +	info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
>  	if (!info) {
>  		err = -ENOMEM;
>  		goto out_put_node;
> @@ -826,7 +823,7 @@ int power_supply_get_battery_info(struct power_supply *psy,
>  		else if (!strcmp("lithium-ion-manganese-oxide", value))
>  			info->technology = POWER_SUPPLY_TECHNOLOGY_LiMn;
>  		else
> -			dev_warn(&psy->dev, "%s unknown battery type\n", value);
> +			dev_warn(dev, "%s unknown battery type\n", value);
>  	}
>  
>  	fwnode_property_read_u32(fwnode, "energy-full-design-microwatt-hours",
> @@ -877,7 +874,7 @@ int power_supply_get_battery_info(struct power_supply *psy,
>  		err = len;
>  		goto out_put_node;
>  	} else if (len > POWER_SUPPLY_OCV_TEMP_MAX) {
> -		dev_err(&psy->dev, "Too many temperature values\n");
> +		dev_err(dev, "Too many temperature values\n");
>  		err = -EINVAL;
>  		goto out_put_node;
>  	} else if (len > 0) {
> @@ -892,28 +889,28 @@ int power_supply_get_battery_info(struct power_supply *psy,
>  		char *propname __free(kfree) = kasprintf(GFP_KERNEL, "ocv-capacity-table-%d",
>  							 index);
>  		if (!propname) {
> -			power_supply_put_battery_info(psy, info);
> +			power_supply_put_battery_info_from_dev(dev, info);
>  			err = -ENOMEM;
>  			goto out_put_node;
>  		}
>  		proplen = fwnode_property_count_u32(fwnode, propname);
>  		if (proplen < 0 || proplen % 2 != 0) {
> -			dev_err(&psy->dev, "failed to get %s\n", propname);
> -			power_supply_put_battery_info(psy, info);
> +			dev_err(dev, "failed to get %s\n", propname);
> +			power_supply_put_battery_info_from_dev(dev, info);
>  			err = -EINVAL;
>  			goto out_put_node;
>  		}
>  
>  		u32 *propdata __free(kfree) = kzalloc_objs(*propdata, proplen);
>  		if (!propdata) {
> -			power_supply_put_battery_info(psy, info);
> +			power_supply_put_battery_info_from_dev(dev, info);
>  			err = -EINVAL;
>  			goto out_put_node;
>  		}
>  		err = fwnode_property_read_u32_array(fwnode, propname, propdata, proplen);
>  		if (err < 0) {
> -			dev_err(&psy->dev, "failed to get %s\n", propname);
> -			power_supply_put_battery_info(psy, info);
> +			dev_err(dev, "failed to get %s\n", propname);
> +			power_supply_put_battery_info_from_dev(dev, info);
>  			goto out_put_node;
>  		}
>  
> @@ -921,9 +918,9 @@ int power_supply_get_battery_info(struct power_supply *psy,
>  		info->ocv_table_size[index] = tab_len;
>  
>  		info->ocv_table[index] = table =
> -			devm_kcalloc(&psy->dev, tab_len, sizeof(*table), GFP_KERNEL);
> +			devm_kcalloc(dev, tab_len, sizeof(*table), GFP_KERNEL);
>  		if (!info->ocv_table[index]) {
> -			power_supply_put_battery_info(psy, info);
> +			power_supply_put_battery_info_from_dev(dev, info);
>  			err = -ENOMEM;
>  			goto out_put_node;
>  		}
> @@ -939,14 +936,14 @@ int power_supply_get_battery_info(struct power_supply *psy,
>  		err = 0;
>  		goto out_ret_pointer;
>  	} else if (proplen < 0 || proplen % 2 != 0) {
> -		power_supply_put_battery_info(psy, info);
> +		power_supply_put_battery_info_from_dev(dev, info);
>  		err = (proplen < 0) ? proplen : -EINVAL;
>  		goto out_put_node;
>  	}
>  
>  	propdata = kzalloc_objs(*propdata, proplen);
>  	if (!propdata) {
> -		power_supply_put_battery_info(psy, info);
> +		power_supply_put_battery_info_from_dev(dev, info);
>  		err = -ENOMEM;
>  		goto out_put_node;
>  	}
> @@ -954,17 +951,17 @@ int power_supply_get_battery_info(struct power_supply *psy,
>  	err = fwnode_property_read_u32_array(fwnode, "resistance-temp-table",
>  					     propdata, proplen);
>  	if (err < 0) {
> -		power_supply_put_battery_info(psy, info);
> +		power_supply_put_battery_info_from_dev(dev, info);
>  		goto out_put_node;
>  	}
>  
>  	info->resist_table_size = proplen / 2;
> -	info->resist_table = resist_table = devm_kcalloc(&psy->dev,
> +	info->resist_table = resist_table = devm_kcalloc(dev,
>  							 info->resist_table_size,
>  							 sizeof(*resist_table),
>  							 GFP_KERNEL);
>  	if (!info->resist_table) {
> -		power_supply_put_battery_info(psy, info);
> +		power_supply_put_battery_info_from_dev(dev, info);
>  		err = -ENOMEM;
>  		goto out_put_node;
>  	}
> @@ -982,22 +979,69 @@ int power_supply_get_battery_info(struct power_supply *psy,
>  	fwnode_handle_put(fwnode);
>  	return err;
>  }
> +
> +int power_supply_get_battery_info(struct power_supply *psy,
> +				  struct power_supply_battery_info **info_out)
> +{
> +	struct fwnode_handle *srcnode;
> +
> +	srcnode = dev_fwnode(&psy->dev);
> +	if (!srcnode && psy->dev.parent)
> +		srcnode = dev_fwnode(psy->dev.parent);
> +
> +	return __power_supply_get_battery_info(&psy->dev, srcnode, info_out);
> +}
>  EXPORT_SYMBOL_GPL(power_supply_get_battery_info);
>  
> -void power_supply_put_battery_info(struct power_supply *psy,
> -				   struct power_supply_battery_info *info)
> +/**
> + * power_supply_get_battery_info_from_dev() - Get battery info without a supply
> + * @dev: Device holding the "monitored-battery" reference, which also owns the
> + *	 devres allocations made for the returned info
> + * @info_out: Pointer to store the resulting battery info
> + *
> + * Same as power_supply_get_battery_info(), but keyed off a plain device rather
> + * than a registered power supply. Chargers that program hardware limits taken
> + * from the battery node need those values *before* they can safely register
> + * their power supply: registering makes the supply callable, so a later probe
> + * failure would free driver data underneath a running callback.
> + *
> + * Release the result with power_supply_put_battery_info_from_dev().
> + *
> + * Return: 0 on success or an error code on failure.
> + */
> +int power_supply_get_battery_info_from_dev(struct device *dev,
> +					   struct power_supply_battery_info **info_out)
> +{
> +	return __power_supply_get_battery_info(dev, dev_fwnode(dev), info_out);
> +}
> +EXPORT_SYMBOL_GPL(power_supply_get_battery_info_from_dev);
> +
> +/**
> + * power_supply_put_battery_info_from_dev() - Release battery info
> + * @dev: Device passed to power_supply_get_battery_info_from_dev()
> + * @info: Battery info to release
> + */
> +void power_supply_put_battery_info_from_dev(struct device *dev,
> +					    struct power_supply_battery_info *info)
>  {
>  	int i;
>  
>  	for (i = 0; i < POWER_SUPPLY_OCV_TEMP_MAX; i++) {
>  		if (info->ocv_table[i])
> -			devm_kfree(&psy->dev, info->ocv_table[i]);
> +			devm_kfree(dev, info->ocv_table[i]);
>  	}
>  
>  	if (info->resist_table)
> -		devm_kfree(&psy->dev, info->resist_table);
> +		devm_kfree(dev, info->resist_table);
> +
> +	devm_kfree(dev, info);
> +}
> +EXPORT_SYMBOL_GPL(power_supply_put_battery_info_from_dev);
>  
> -	devm_kfree(&psy->dev, info);
> +void power_supply_put_battery_info(struct power_supply *psy,
> +				   struct power_supply_battery_info *info)
> +{
> +	power_supply_put_battery_info_from_dev(&psy->dev, info);
>  }
>  EXPORT_SYMBOL_GPL(power_supply_put_battery_info);
>  
> diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
> index 131cafded72f..f42ae4e3bf81 100644
> --- a/include/linux/power_supply.h
> +++ b/include/linux/power_supply.h
> @@ -865,6 +865,10 @@ extern int power_supply_get_battery_info(struct power_supply *psy,
>  					 struct power_supply_battery_info **info_out);
>  extern void power_supply_put_battery_info(struct power_supply *psy,
>  					  struct power_supply_battery_info *info);
> +extern int power_supply_get_battery_info_from_dev(struct device *dev,
> +						  struct power_supply_battery_info **info_out);
> +extern void power_supply_put_battery_info_from_dev(struct device *dev,
> +						   struct power_supply_battery_info *info);
>  extern bool power_supply_battery_info_has_prop(struct power_supply_battery_info *info,
>  					       enum power_supply_property psp);
>  extern int power_supply_battery_info_get_prop(struct power_supply_battery_info *info,
> 
> -- 
> 2.55.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 2/4] power: supply: core: Allow getting battery info before psy is registered

[Alexey Charkov]

Hello Sebastian,

On Wed, Sep 9, 2026 at 11:52 PM Sebastian Reichel
<sebastian.reichel@collabora.com> wrote:
>
> Hello Alexey,
>
> On Wed, Sep 09, 2026 at 07:41:45PM +0400, Alexey Charkov wrote:
> > Some power supplies, such as battery chargers, may need to program the
> > device parameters based on what their connected battery allows. Current
> > API requires registering the power supply to access battery information,
> > which is problematic because a registered power supply is immediately
> > available to the rest of the system, but the battery parameters may not
> > be set yet in the charger.
> >
> > Given that the battery info helpers really only need a fwnode and a struct
> > device to hang devres-allocated resourses on, add a pure dev-based get/put
> > API alongside the existing psy-based one, which can be used to query the
> > battery information before registering the power supply.
>
> Use the new init callback for that, which got introduced in the v7.3
> cycle:
>
> c1eb5905fdce ("power: supply: Add registration init callback")
>
> See for example fdece8642eca ("power: supply: bq25630: Initialize
> hardware before exposing the power supply") for a driver that was
> converted to this.

Indeed, that should work quite neatly, thank you for pointing it out
(I've missed that addition). I'll then drop patch 2 and rework patches
3 and 4 to use the init callback. I believe they can then form a
separate mini-series only touching the driver but not the core.

Patch 1 still stands on its own, though, as it prevents a
use-after-free during teardown. Would you mind giving that a look?

Thanks a lot,
Alexey