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

[Sebastian Reichel <sebastian.reichel@collabora.com>]

[-- 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 --]