[PATCH v3] power: supply: act8945a: Handle status changes in threaded IRQs

[PATCH v3] power: supply: act8945a: Handle status changes in threaded IRQs

[Myeonghun Pak]

Clearing init_done cannot prevent status IRQs from queueing work after
cancel_work_sync(), risking access to an unregistered power supply.

Use threaded IRQs with IRQF_ONESHOT and a shared mutex to preserve
serialized status updates. Request the LBO IRQ after power supply
registration so managed cleanup drains both IRQs before the supply is
unregistered. Remove the work item, init_done and remove callback.

This issue was identified as part of our ongoing research while
reviewing kernel code.

Fixes: a09209acd6a8 ("power: supply: act8945a_charger: Add status change update support")
Assisted-by: LLM
Co-developed-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Myeonghun Pak <mhun512@gmail.com>
---
Changes in v3:
- Follow Sebastian Reichel's suggestion to handle status changes in
  threaded IRQs, removing the work item, init_done and remove callback.
- Request the LBO IRQ after power supply registration.
- Add IRQF_ONESHOT to both IRQs and preserve their trigger flags.
- Remove the now-unused platform_set_drvdata() call.
- Use a common mutex to preserve status-update serialization across both
  IRQ threads, and initialize it before requesting either IRQ.

Changes in v2:
- Remove init_done and all accesses to it.
- Initialize and disable the work before either IRQ is requested and
  enable it only after probe initialization succeeds.

Compile-tested with GCC 15, W=1 (x86_64), with OF, OF_IRQ and
GPIOLIB enabled. No runtime testing.

v1: https://lore.kernel.org/r/179211805459.3650807.8742309660995961596.G171@gmail.com/

 drivers/power/supply/act8945a_charger.c | 61 ++++++++++++---------------------
 1 file changed, 22 insertions(+), 39 deletions(-)

diff --git a/drivers/power/supply/act8945a_charger.c b/drivers/power/supply/act8945a_charger.c
index 7e4b087f7679..9411e1f049aa 100644
--- a/drivers/power/supply/act8945a_charger.c
+++ b/drivers/power/supply/act8945a_charger.c
@@ -8,6 +8,7 @@
  */
 #include <linux/interrupt.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/of.h>
 #include <linux/of_irq.h>
 #include <linux/platform_device.h>
@@ -76,9 +77,9 @@ struct act8945a_charger {
 	struct power_supply *psy;
 	struct power_supply_desc desc;
 	struct regmap *regmap;
-	struct work_struct work;
+	/* Serialize status updates from both IRQ threads. */
+	struct mutex status_lock;
 
-	bool init_done;
 	struct gpio_desc *lbo_gpio;
 	struct gpio_desc *chglev_gpio;
 };
@@ -430,22 +431,14 @@ static unsigned int act8945a_set_supply_type(struct act8945a_charger *charger,
 	return 0;
 }
 
-static void act8945a_work(struct work_struct *work)
-{
-	struct act8945a_charger *charger =
-			container_of(work, struct act8945a_charger, work);
-
-	act8945a_set_supply_type(charger, &charger->desc.type);
-
-	power_supply_changed(charger->psy);
-}
-
 static irqreturn_t act8945a_status_changed(int irq, void *dev_id)
 {
 	struct act8945a_charger *charger = dev_id;
 
-	if (charger->init_done)
-		schedule_work(&charger->work);
+	mutex_lock(&charger->status_lock);
+	act8945a_set_supply_type(charger, &charger->desc.type);
+	power_supply_changed(charger->psy);
+	mutex_unlock(&charger->status_lock);
 
 	return IRQ_HANDLED;
 }
@@ -490,13 +483,6 @@ static int act8945a_charger_config(struct device *dev,
 		return err;
 	}
 
-	ret = devm_request_irq(dev, gpiod_to_irq(charger->lbo_gpio),
-			       act8945a_status_changed,
-			       (IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING),
-			       "act8945a_lbo_detect", charger);
-	if (ret)
-		dev_info(dev, "failed to request gpio \"lbo\" IRQ\n");
-
 	charger->chglev_gpio = devm_gpiod_get_optional(dev,
 						       "active-semi,chglev",
 						       GPIOD_IN);
@@ -581,6 +567,8 @@ static int act8945a_charger_probe(struct platform_device *pdev)
 	if (!charger)
 		return -ENOMEM;
 
+	mutex_init(&charger->status_lock);
+
 	charger->regmap = dev_get_regmap(pdev->dev.parent, NULL);
 	if (!charger->regmap) {
 		dev_err(&pdev->dev, "Parent did not provide regmap\n");
@@ -617,39 +605,34 @@ static int act8945a_charger_probe(struct platform_device *pdev)
 		return PTR_ERR(charger->psy);
 	}
 
-	ret = devm_request_irq(&pdev->dev, irq, act8945a_status_changed,
-			       IRQF_TRIGGER_FALLING, "act8945a_interrupt",
-			       charger);
+	ret = devm_request_threaded_irq(&pdev->dev,
+					gpiod_to_irq(charger->lbo_gpio), NULL,
+					act8945a_status_changed,
+					IRQF_TRIGGER_FALLING |
+					IRQF_TRIGGER_RISING | IRQF_ONESHOT,
+					"act8945a_lbo_detect", charger);
 	if (ret)
-		return ret;
-
-	platform_set_drvdata(pdev, charger);
+		dev_info(&pdev->dev, "failed to request gpio \"lbo\" IRQ\n");
 
-	INIT_WORK(&charger->work, act8945a_work);
+	ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
+					act8945a_status_changed,
+					IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
+					"act8945a_interrupt", charger);
+	if (ret)
+		return ret;
 
 	ret = act8945a_enable_interrupt(charger);
 	if (ret)
 		return -EIO;
 
-	charger->init_done = true;
-
 	return 0;
 }
 
-static void act8945a_charger_remove(struct platform_device *pdev)
-{
-	struct act8945a_charger *charger = platform_get_drvdata(pdev);
-
-	charger->init_done = false;
-	cancel_work_sync(&charger->work);
-}
-
 static struct platform_driver act8945a_charger_driver = {
 	.driver	= {
 		.name = "act8945a-charger",
 	},
 	.probe	= act8945a_charger_probe,
-	.remove	= act8945a_charger_remove,
 };
 module_platform_driver(act8945a_charger_driver);
 

base-commit: 238650ef6c7c7cca08e032527329424c9fbd70e5
-- 
2.53.0