[PATCH v2] power: supply: act8945a: Disable status work on removal

[PATCH v2] power: supply: act8945a: Disable status work on removal

[Myeonghun Pak]

Clearing init_done does not synchronize with either status interrupt. A
handler can observe init_done set, be delayed, and schedule the status
work after cancel_work_sync() has returned. The work can then access the
power supply after its managed cleanup.

Use disable_work_sync() to reject subsequent queueing and drain any
existing execution. The charger allocation remains valid until both
managed IRQs have been released, so a late handler can safely attempt to
queue the disabled work.

Replace init_done with the work item's disable state. Initialize and
disable the work before act8945a_charger_config(), which registers the
LBO interrupt, and enable it only after probe initialization succeeds.
This keeps both status IRQs from queueing work before the power supply
is ready, including on probe failure.

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 v2:
- Remove init_done and all accesses to it, as requested by Sebastian
  Reichel.
- Initialize the work before either IRQ is requested and keep it disabled
  until probe initialization succeeds, preserving the probe-time gate.

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/
Review: https://lore.kernel.org/r/atPtc_jKLo8RhAMU@venus/

 drivers/power/supply/act8945a_charger.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/power/supply/act8945a_charger.c b/drivers/power/supply/act8945a_charger.c
index 7e4b087..7d47052 100644
--- a/drivers/power/supply/act8945a_charger.c
+++ b/drivers/power/supply/act8945a_charger.c
@@ -14,6 +14,7 @@
 #include <linux/power_supply.h>
 #include <linux/regmap.h>
 #include <linux/gpio/consumer.h>
+#include <linux/workqueue.h>
 
 static const char *act8945a_charger_model = "ACT8945A";
 static const char *act8945a_charger_manufacturer = "Active-semi";
@@ -78,7 +79,6 @@ struct act8945a_charger {
 	struct regmap *regmap;
 	struct work_struct work;
 
-	bool init_done;
 	struct gpio_desc *lbo_gpio;
 	struct gpio_desc *chglev_gpio;
 };
@@ -444,8 +444,7 @@ 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);
+	schedule_work(&charger->work);
 
 	return IRQ_HANDLED;
 }
@@ -587,6 +586,10 @@ static int act8945a_charger_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
+	/* Keep status IRQs from queueing work until probe has completed. */
+	INIT_WORK(&charger->work, act8945a_work);
+	disable_work(&charger->work);
+
 	ret = act8945a_charger_config(&pdev->dev, charger);
 	if (ret)
 		return ret;
@@ -625,13 +628,11 @@ static int act8945a_charger_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, charger);
 
-	INIT_WORK(&charger->work, act8945a_work);
-
 	ret = act8945a_enable_interrupt(charger);
 	if (ret)
 		return -EIO;
 
-	charger->init_done = true;
+	enable_work(&charger->work);
 
 	return 0;
 }
@@ -640,8 +641,7 @@ 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);
+	disable_work_sync(&charger->work);
 }
 
 static struct platform_driver act8945a_charger_driver = {

base-commit: 238650ef6c7c7cca08e032527329424c9fbd70e5
-- 
2.53.0

Re: [PATCH v2] power: supply: act8945a: Disable status work on removal

[Sebastian Reichel]

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

Hi,

On Sat, Oct 17, 2026 at 10:03:57PM -0400, Myeonghun Pak wrote:
> Clearing init_done does not synchronize with either status interrupt. A
> handler can observe init_done set, be delayed, and schedule the status
> work after cancel_work_sync() has returned. The work can then access the
> power supply after its managed cleanup.
> 
> Use disable_work_sync() to reject subsequent queueing and drain any
> existing execution. The charger allocation remains valid until both
> managed IRQs have been released, so a late handler can safely attempt to
> queue the disabled work.
> 
> Replace init_done with the work item's disable state. Initialize and
> disable the work before act8945a_charger_config(), which registers the
> LBO interrupt, and enable it only after probe initialization succeeds.
> This keeps both status IRQs from queueing work before the power supply
> is ready, including on probe failure.
> 
> 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>
> ---

Having a closer look at how the work is being used: Instead of doing
all this complex stuff this should just use a threaded interrupt and
completley get rid of the worker. I.e.:

1. get rid of init_done (done in your version)
2. switch from devm_request_irq to devm_request_threaded_irq
3. move the act8945a_lbo_detect IRQ request after power-supply
   registration to avoid problems with spurious interrupts
4. move act8945a_set_supply_type + power_supply_changed calls from
   act8945a_work into act8945a_status_changed
5. completley remove charger->work
6. drop empty act8945a_charger_remove()

It results in less and simpler code.

Greetings,

-- Sebastian

> Changes in v2:
> - Remove init_done and all accesses to it, as requested by Sebastian
>   Reichel.
> - Initialize the work before either IRQ is requested and keep it disabled
>   until probe initialization succeeds, preserving the probe-time gate.
> 
> 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/
> Review: https://lore.kernel.org/r/atPtc_jKLo8RhAMU@venus/
> 
>  drivers/power/supply/act8945a_charger.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/power/supply/act8945a_charger.c b/drivers/power/supply/act8945a_charger.c
> index 7e4b087..7d47052 100644
> --- a/drivers/power/supply/act8945a_charger.c
> +++ b/drivers/power/supply/act8945a_charger.c
> @@ -14,6 +14,7 @@
>  #include <linux/power_supply.h>
>  #include <linux/regmap.h>
>  #include <linux/gpio/consumer.h>
> +#include <linux/workqueue.h>
>  
>  static const char *act8945a_charger_model = "ACT8945A";
>  static const char *act8945a_charger_manufacturer = "Active-semi";
> @@ -78,7 +79,6 @@ struct act8945a_charger {
>  	struct regmap *regmap;
>  	struct work_struct work;
>  
> -	bool init_done;
>  	struct gpio_desc *lbo_gpio;
>  	struct gpio_desc *chglev_gpio;
>  };
> @@ -444,8 +444,7 @@ 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);
> +	schedule_work(&charger->work);
>  
>  	return IRQ_HANDLED;
>  }
> @@ -587,6 +586,10 @@ static int act8945a_charger_probe(struct platform_device *pdev)
>  		return -EINVAL;
>  	}
>  
> +	/* Keep status IRQs from queueing work until probe has completed. */
> +	INIT_WORK(&charger->work, act8945a_work);
> +	disable_work(&charger->work);
> +
>  	ret = act8945a_charger_config(&pdev->dev, charger);
>  	if (ret)
>  		return ret;
> @@ -625,13 +628,11 @@ static int act8945a_charger_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, charger);
>  
> -	INIT_WORK(&charger->work, act8945a_work);
> -
>  	ret = act8945a_enable_interrupt(charger);
>  	if (ret)
>  		return -EIO;
>  
> -	charger->init_done = true;
> +	enable_work(&charger->work);
>  
>  	return 0;
>  }
> @@ -640,8 +641,7 @@ 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);
> +	disable_work_sync(&charger->work);
>  }
>  
>  static struct platform_driver act8945a_charger_driver = {
> 
> base-commit: 238650ef6c7c7cca08e032527329424c9fbd70e5
> -- 
> 2.53.0

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