[PATCH] Revert "power: supply: qcom_battmgr: Register the power supplies after PDR is up"

[PATCH] Revert "power: supply: qcom_battmgr: Register the power supplies after PDR is up"

[Johan Hovold]

This reverts commit b43f7ddc2b7a5a90447d96cb4d3c6d142dd4a810.

The offending commit deferred power-supply class device registration
until the service-started notification is received.

This triggers a NULL pointer dereference during boot of the Lenovo
ThinkPad X13s and SC8280XP CRD as battery status notifications can be
received before the service-start notification:

	Unable to handle kernel NULL pointer dereference at virtual address 00000000000005c0
	...
	Call trace:
	 _acquire+0x338/0x2064
	 acquire+0x1e8/0x318
	 spin_lock_irqsave+0x60/0x88
	 _supply_changed+0x2c/0xa4
	 battmgr_callback+0x1d4/0x60c [qcom_battmgr]
	 pmic_glink_rpmsg_callback+0x5c/0xa4 [pmic_glink]
	 qcom_glink_native_rx+0x58c/0x7e8
	 qcom_glink_smem_intr+0x14/0x24 [qcom_glink_smem]
	 __handle_irq_event_percpu+0xb0/0x2d4
	 handle_irq_event+0x4c/0xb8

As trying to serialise this is non-trivial and risks missing
notifications, let's revert to registration during probe so that the
driver data is all set up once the service goes live.

The warning message during resume in case the aDSP firmware is not
running that motivated the change can be considered a feature and should
not be suppressed.

Fixes: b43f7ddc2b7a ("power: supply: qcom_battmgr: Register the power supplies after PDR is up")
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/power/supply/qcom_battmgr.c | 109 +++++++++++++---------------
 1 file changed, 49 insertions(+), 60 deletions(-)

diff --git a/drivers/power/supply/qcom_battmgr.c b/drivers/power/supply/qcom_battmgr.c
index a12e2a66d516..ec163d1bcd18 100644
--- a/drivers/power/supply/qcom_battmgr.c
+++ b/drivers/power/supply/qcom_battmgr.c
@@ -282,7 +282,6 @@ struct qcom_battmgr_wireless {
 
 struct qcom_battmgr {
 	struct device *dev;
-	struct auxiliary_device *adev;
 	struct pmic_glink_client *client;
 
 	enum qcom_battmgr_variant variant;
@@ -1294,69 +1293,11 @@ static void qcom_battmgr_enable_worker(struct work_struct *work)
 		dev_err(battmgr->dev, "failed to request power notifications\n");
 }
 
-static char *qcom_battmgr_battery[] = { "battery" };
-
-static void qcom_battmgr_register_psy(struct qcom_battmgr *battmgr)
-{
-	struct power_supply_config psy_cfg_supply = {};
-	struct auxiliary_device *adev = battmgr->adev;
-	struct power_supply_config psy_cfg = {};
-	struct device *dev = &adev->dev;
-
-	psy_cfg.drv_data = battmgr;
-	psy_cfg.of_node = adev->dev.of_node;
-
-	psy_cfg_supply.drv_data = battmgr;
-	psy_cfg_supply.of_node = adev->dev.of_node;
-	psy_cfg_supply.supplied_to = qcom_battmgr_battery;
-	psy_cfg_supply.num_supplicants = 1;
-
-	if (battmgr->variant == QCOM_BATTMGR_SC8280XP) {
-		battmgr->bat_psy = devm_power_supply_register(dev, &sc8280xp_bat_psy_desc, &psy_cfg);
-		if (IS_ERR(battmgr->bat_psy))
-			dev_err(dev, "failed to register battery power supply (%ld)\n",
-				PTR_ERR(battmgr->bat_psy));
-
-		battmgr->ac_psy = devm_power_supply_register(dev, &sc8280xp_ac_psy_desc, &psy_cfg_supply);
-		if (IS_ERR(battmgr->ac_psy))
-			dev_err(dev, "failed to register AC power supply (%ld)\n",
-				PTR_ERR(battmgr->ac_psy));
-
-		battmgr->usb_psy = devm_power_supply_register(dev, &sc8280xp_usb_psy_desc, &psy_cfg_supply);
-		if (IS_ERR(battmgr->usb_psy))
-			dev_err(dev, "failed to register USB power supply (%ld)\n",
-				PTR_ERR(battmgr->usb_psy));
-
-		battmgr->wls_psy = devm_power_supply_register(dev, &sc8280xp_wls_psy_desc, &psy_cfg_supply);
-		if (IS_ERR(battmgr->wls_psy))
-			dev_err(dev, "failed to register wireless charing power supply (%ld)\n",
-				PTR_ERR(battmgr->wls_psy));
-	} else {
-		battmgr->bat_psy = devm_power_supply_register(dev, &sm8350_bat_psy_desc, &psy_cfg);
-		if (IS_ERR(battmgr->bat_psy))
-			dev_err(dev, "failed to register battery power supply (%ld)\n",
-				PTR_ERR(battmgr->bat_psy));
-
-		battmgr->usb_psy = devm_power_supply_register(dev, &sm8350_usb_psy_desc, &psy_cfg_supply);
-		if (IS_ERR(battmgr->usb_psy))
-			dev_err(dev, "failed to register USB power supply (%ld)\n",
-				PTR_ERR(battmgr->usb_psy));
-
-		battmgr->wls_psy = devm_power_supply_register(dev, &sm8350_wls_psy_desc, &psy_cfg_supply);
-		if (IS_ERR(battmgr->wls_psy))
-			dev_err(dev, "failed to register wireless charing power supply (%ld)\n",
-				PTR_ERR(battmgr->wls_psy));
-	}
-}
-
 static void qcom_battmgr_pdr_notify(void *priv, int state)
 {
 	struct qcom_battmgr *battmgr = priv;
 
 	if (state == SERVREG_SERVICE_STATE_UP) {
-		if (!battmgr->bat_psy)
-			qcom_battmgr_register_psy(battmgr);
-
 		battmgr->service_up = true;
 		schedule_work(&battmgr->enable_work);
 	} else {
@@ -1371,9 +1312,13 @@ static const struct of_device_id qcom_battmgr_of_variants[] = {
 	{}
 };
 
+static char *qcom_battmgr_battery[] = { "battery" };
+
 static int qcom_battmgr_probe(struct auxiliary_device *adev,
 			      const struct auxiliary_device_id *id)
 {
+	struct power_supply_config psy_cfg_supply = {};
+	struct power_supply_config psy_cfg = {};
 	const struct of_device_id *match;
 	struct qcom_battmgr *battmgr;
 	struct device *dev = &adev->dev;
@@ -1383,7 +1328,14 @@ static int qcom_battmgr_probe(struct auxiliary_device *adev,
 		return -ENOMEM;
 
 	battmgr->dev = dev;
-	battmgr->adev = adev;
+
+	psy_cfg.drv_data = battmgr;
+	psy_cfg.of_node = adev->dev.of_node;
+
+	psy_cfg_supply.drv_data = battmgr;
+	psy_cfg_supply.of_node = adev->dev.of_node;
+	psy_cfg_supply.supplied_to = qcom_battmgr_battery;
+	psy_cfg_supply.num_supplicants = 1;
 
 	INIT_WORK(&battmgr->enable_work, qcom_battmgr_enable_worker);
 	mutex_init(&battmgr->lock);
@@ -1395,6 +1347,43 @@ static int qcom_battmgr_probe(struct auxiliary_device *adev,
 	else
 		battmgr->variant = QCOM_BATTMGR_SM8350;
 
+	if (battmgr->variant == QCOM_BATTMGR_SC8280XP) {
+		battmgr->bat_psy = devm_power_supply_register(dev, &sc8280xp_bat_psy_desc, &psy_cfg);
+		if (IS_ERR(battmgr->bat_psy))
+			return dev_err_probe(dev, PTR_ERR(battmgr->bat_psy),
+					     "failed to register battery power supply\n");
+
+		battmgr->ac_psy = devm_power_supply_register(dev, &sc8280xp_ac_psy_desc, &psy_cfg_supply);
+		if (IS_ERR(battmgr->ac_psy))
+			return dev_err_probe(dev, PTR_ERR(battmgr->ac_psy),
+					     "failed to register AC power supply\n");
+
+		battmgr->usb_psy = devm_power_supply_register(dev, &sc8280xp_usb_psy_desc, &psy_cfg_supply);
+		if (IS_ERR(battmgr->usb_psy))
+			return dev_err_probe(dev, PTR_ERR(battmgr->usb_psy),
+					     "failed to register USB power supply\n");
+
+		battmgr->wls_psy = devm_power_supply_register(dev, &sc8280xp_wls_psy_desc, &psy_cfg_supply);
+		if (IS_ERR(battmgr->wls_psy))
+			return dev_err_probe(dev, PTR_ERR(battmgr->wls_psy),
+					     "failed to register wireless charing power supply\n");
+	} else {
+		battmgr->bat_psy = devm_power_supply_register(dev, &sm8350_bat_psy_desc, &psy_cfg);
+		if (IS_ERR(battmgr->bat_psy))
+			return dev_err_probe(dev, PTR_ERR(battmgr->bat_psy),
+					     "failed to register battery power supply\n");
+
+		battmgr->usb_psy = devm_power_supply_register(dev, &sm8350_usb_psy_desc, &psy_cfg_supply);
+		if (IS_ERR(battmgr->usb_psy))
+			return dev_err_probe(dev, PTR_ERR(battmgr->usb_psy),
+					     "failed to register USB power supply\n");
+
+		battmgr->wls_psy = devm_power_supply_register(dev, &sm8350_wls_psy_desc, &psy_cfg_supply);
+		if (IS_ERR(battmgr->wls_psy))
+			return dev_err_probe(dev, PTR_ERR(battmgr->wls_psy),
+					     "failed to register wireless charing power supply\n");
+	}
+
 	battmgr->client = devm_pmic_glink_register_client(dev,
 							  PMIC_GLINK_OWNER_BATTMGR,
 							  qcom_battmgr_callback,
-- 
2.43.0

Re: [PATCH] Revert "power: supply: qcom_battmgr: Register the power supplies after PDR is up"

[Konrad Dybcio]

On 1/23/24 17:00, Johan Hovold wrote:
> This reverts commit b43f7ddc2b7a5a90447d96cb4d3c6d142dd4a810.
> 
> The offending commit deferred power-supply class device registration
> until the service-started notification is received.
> 
> This triggers a NULL pointer dereference during boot of the Lenovo
> ThinkPad X13s and SC8280XP CRD as battery status notifications can be
> received before the service-start notification:
> 
> 	Unable to handle kernel NULL pointer dereference at virtual address 00000000000005c0
> 	...
> 	Call trace:
> 	 _acquire+0x338/0x2064
> 	 acquire+0x1e8/0x318
> 	 spin_lock_irqsave+0x60/0x88
> 	 _supply_changed+0x2c/0xa4
> 	 battmgr_callback+0x1d4/0x60c [qcom_battmgr]
> 	 pmic_glink_rpmsg_callback+0x5c/0xa4 [pmic_glink]
> 	 qcom_glink_native_rx+0x58c/0x7e8
> 	 qcom_glink_smem_intr+0x14/0x24 [qcom_glink_smem]
> 	 __handle_irq_event_percpu+0xb0/0x2d4
> 	 handle_irq_event+0x4c/0xb8
> 
> As trying to serialise this is non-trivial and risks missing
> notifications, let's revert to registration during probe so that the
> driver data is all set up once the service goes live.
> 
> The warning message during resume in case the aDSP firmware is not
> running that motivated the change can be considered a feature and should
> not be suppressed.
> 
> Fixes: b43f7ddc2b7a ("power: supply: qcom_battmgr: Register the power supplies after PDR is up")
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---

Johan and I talked off-list and decided reverting this is the best
way to go forward for now, the problem that my original commit
solved (or well, tried to, anyway) is only an issue with a
configuration that's not quite supported (i.e. missing all
pieces of the puzzle for a functional battmgr) and more work would
need to be put into supporting that. I *may* do it in the future,
but for now both of us need to work on different things.

Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad

Re: [PATCH] Revert "power: supply: qcom_battmgr: Register the power supplies after PDR is up"

[Sebastian Reichel]

On Tue, 23 Jan 2024 17:00:53 +0100, Johan Hovold wrote:
> This reverts commit b43f7ddc2b7a5a90447d96cb4d3c6d142dd4a810.
> 
> The offending commit deferred power-supply class device registration
> until the service-started notification is received.
> 
> This triggers a NULL pointer dereference during boot of the Lenovo
> ThinkPad X13s and SC8280XP CRD as battery status notifications can be
> received before the service-start notification:
> 
> [...]

Applied, thanks!

[1/1] Revert "power: supply: qcom_battmgr: Register the power supplies after PDR is up"
      commit: d0266d7ab1618482d58015d67a5220e590333298

Best regards,
-- 
Sebastian Reichel <sebastian.reichel@collabora.com>