The INT33FD Crystal Cove driver enables the parent IRQ as a wake source
after registering its regmap IRQ chip.  When that succeeds, a later
mfd_add_devices() failure or driver removal leaves the wake enable
unbalanced.  Shutdown only disables IRQ handling, leaving IRQ wake enabled.

Register a managed action only after enable_irq_wake() succeeds.  Since
the action is registered after the managed regmap IRQ chip, reverse devres
order disables IRQ wake before tearing down the IRQ chip.  Keep warning and
continuing when enable_irq_wake() itself fails.

Shutdown does not release managed resources, so explicitly release the
wake-disable action there if it was registered.  This also removes the
action, preventing a second disable during subsequent managed cleanup.

This is limited to the Bay Trail and Cherry Trail Crystal Cove PMIC
variants using the INT33FD ACPI ID.

This issue was identified during our ongoing static-analysis research while
reviewing kernel code.

Fixes: 516523846006 ("mfd: intel_soc_pmic: Core driver")
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:
- Capitalize the subject description.
- Use Assisted-by: LLM as requested.
- Keep devm_add_action_or_reset() on a single line.
- Release the registered wake-disable action at shutdown, skipping it if
  enabling IRQ wake failed and removing it to avoid duplicate cleanup.

 drivers/mfd/intel_soc_pmic_crc.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/mfd/intel_soc_pmic_crc.c b/drivers/mfd/intel_soc_pmic_crc.c
index 627a89334908116a7e1924f9d78121284d1316b0..c082d7aded92eb418c0619031641a614e4989290
100644
--- a/drivers/mfd/intel_soc_pmic_crc.c
+++ b/drivers/mfd/intel_soc_pmic_crc.c
@@ -164,6 +164,13 @@ static const struct crystal_cove_config
crystal_cove_config_cht_crc = {
 	.irq_chip = &crystal_cove_irq_chip,
 };

+static void crystal_cove_disable_irq_wake(void *data)
+{
+	struct intel_soc_pmic *pmic = data;
+
+	disable_irq_wake(pmic->irq);
+}
+
 static int crystal_cove_i2c_probe(struct i2c_client *i2c)
 {
 	const struct crystal_cove_config *config;
@@ -195,8 +202,13 @@ static int crystal_cove_i2c_probe(struct i2c_client *i2c)
 		return ret;

 	ret = enable_irq_wake(pmic->irq);
-	if (ret)
+	if (ret) {
 		dev_warn(dev, "Can't enable IRQ as wake source: %d\n", ret);
+	} else {
+		ret = devm_add_action_or_reset(dev, crystal_cove_disable_irq_wake, pmic);
+		if (ret)
+			return ret;
+	}

 	/* Add lookup table for crc-pwm */
 	pwm_add_table(crc_pwm_lookup, ARRAY_SIZE(crc_pwm_lookup));
@@ -226,6 +238,9 @@ static void crystal_cove_shutdown(struct i2c_client *i2c)
 {
 	struct intel_soc_pmic *pmic = i2c_get_clientdata(i2c);

+	if (devm_is_action_added(&i2c->dev, crystal_cove_disable_irq_wake, pmic))
+		devm_release_action(&i2c->dev, crystal_cove_disable_irq_wake, pmic);
+
 	disable_irq(pmic->irq);

 	return;

On Mon, Sep 14, 2026 at 03:18:51PM -0700, Myeonghun Pak wrote:
> The INT33FD Crystal Cove driver enables the parent IRQ as a wake source
> after registering its regmap IRQ chip.  When that succeeds, a later
> mfd_add_devices() failure or driver removal leaves the wake enable
> unbalanced.  Shutdown only disables IRQ handling, leaving IRQ wake enabled.
> 
> Register a managed action only after enable_irq_wake() succeeds.  Since
> the action is registered after the managed regmap IRQ chip, reverse devres
> order disables IRQ wake before tearing down the IRQ chip.  Keep warning and
> continuing when enable_irq_wake() itself fails.

> Shutdown does not release managed resources, so explicitly release the
> wake-disable action there if it was registered.  This also removes the
> action, preventing a second disable during subsequent managed cleanup.

Why? If it goes to shutdown, cleaning resources makes a little sense, no?

> This is limited to the Bay Trail and Cherry Trail Crystal Cove PMIC
> variants using the INT33FD ACPI ID.
> 
> This issue was identified during our ongoing static-analysis research while
> reviewing kernel code.

...

> static void crystal_cove_shutdown(struct i2c_client *i2c)
>  {
>  	struct intel_soc_pmic *pmic = i2c_get_clientdata(i2c);
> 
> +	if (devm_is_action_added(&i2c->dev, crystal_cove_disable_irq_wake, pmic))
> +		devm_release_action(&i2c->dev, crystal_cove_disable_irq_wake, pmic);

Is it legit to call devm at this point? When is .shutdown() called and what are
the assumptions before and after for a certain device?

>  	disable_irq(pmic->irq);

This looks a bit ugly. Can we start with no-devm? This will solve the above
question as well.

-- 
With Best Regards,
Andy Shevchenko

Hello Andy,

Thanks for the feedback. I’ll use non-devm wake handling and drop the
shutdown changes in v3.

Best regards,
Myeonghun Pak

2026년 9월 15일 (화) 오전 4:16, Andy Shevchenko <andriy.shevchenko@intel.com>님이 작성:
>
> On Mon, Sep 14, 2026 at 03:18:51PM -0700, Myeonghun Pak wrote:
> > The INT33FD Crystal Cove driver enables the parent IRQ as a wake source
> > after registering its regmap IRQ chip.  When that succeeds, a later
> > mfd_add_devices() failure or driver removal leaves the wake enable
> > unbalanced.  Shutdown only disables IRQ handling, leaving IRQ wake enabled.
> >
> > Register a managed action only after enable_irq_wake() succeeds.  Since
> > the action is registered after the managed regmap IRQ chip, reverse devres
> > order disables IRQ wake before tearing down the IRQ chip.  Keep warning and
> > continuing when enable_irq_wake() itself fails.
>
> > Shutdown does not release managed resources, so explicitly release the
> > wake-disable action there if it was registered.  This also removes the
> > action, preventing a second disable during subsequent managed cleanup.
>
> Why? If it goes to shutdown, cleaning resources makes a little sense, no?
>
> > This is limited to the Bay Trail and Cherry Trail Crystal Cove PMIC
> > variants using the INT33FD ACPI ID.
> >
> > This issue was identified during our ongoing static-analysis research while
> > reviewing kernel code.
>
> ...
>
> > static void crystal_cove_shutdown(struct i2c_client *i2c)
> >  {
> >       struct intel_soc_pmic *pmic = i2c_get_clientdata(i2c);
> >
> > +     if (devm_is_action_added(&i2c->dev, crystal_cove_disable_irq_wake, pmic))
> > +             devm_release_action(&i2c->dev, crystal_cove_disable_irq_wake, pmic);
>
> Is it legit to call devm at this point? When is .shutdown() called and what are
> the assumptions before and after for a certain device?
>
> >       disable_irq(pmic->irq);
>
> This looks a bit ugly. Can we start with no-devm? This will solve the above
> question as well.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>