Re: [PATCH v3 3/3] mfd: tps65217: Use devres for IRQ domain and wake teardown

[Lee Jones <lee@kernel.org>]

On Fri, 04 Sep 2026, Жамбакиев Радий Рикардинович wrote:

> From: Radiy Zhambakiev <r.zhambakiev@prosoftsystems.ru>
> 
> Removing the parent device used to tear the IRQ domain down in
> tps65217_remove() before the MFD children are unbound: the i2c core
> only releases the parent's devres after the remove callback returns,
> whereas devm_mfd_add_devices() registered its own devres action which
> unbinds the children. Children that requested interrupts from the
> domain (e.g. tps65217-charger) therefore call free_irq() on virtual
> IRQs whose descriptors have already been disposed. With
> CONFIG_SPARSE_IRQ free_irq() then returns early without stopping the
> threaded handler, leaking the irqaction, the IRQ kthread and a module
> reference.
> 
> Register the domain teardown as a devres action right after the domain
> is created instead. Devres actions are released in reverse order of
> registration, so the MFD children release their IRQs first, then the
> parent interrupt is freed and its thread stopped, and only then is the
> domain torn down. The same ordering covers probe failure, which no
> longer needs manual cleanup, and the remove callback can be dropped
> entirely.
> 
> Balance the enable_irq_wake() in tps65217_irq_init() by registering
> disable_irq_wake() as a devres action as well. Only do so when
> enabling actually succeeded, since the parent chip may not support
> setting wake-up.
> 
> Fixes: 6556bdacf646fcaa ("mfd: tps65217: Add support for IRQs")
> Cc: stable@vger.kernel.org
> Signed-off-by: Radiy Zhambakiev <r.zhambakiev@prosoftsystems.ru>
> ---
>  drivers/mfd/tps65217.c | 68 +++++++++++++++++++++++++++++++-----------
>  1 file changed, 50 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/mfd/tps65217.c b/drivers/mfd/tps65217.c
> index 9a1528456ffc..2d799b60c79b 100644
> --- a/drivers/mfd/tps65217.c
> +++ b/drivers/mfd/tps65217.c
> @@ -146,6 +146,36 @@ static const struct irq_domain_ops tps65217_irq_domain_ops = {
>  	.map = tps65217_irq_map,
>  };
>  
> +static void tps65217_irq_cleanup(struct tps65217 *tps)
> +{
> +	unsigned int virq;
> +	int i;
> +
> +	if (!tps->irq_domain)
> +		return;
> +
> +	for (i = 0; i < TPS65217_NUM_IRQ; i++) {

for (int i = 0; ...

> +		virq = irq_find_mapping(tps->irq_domain, i);
> +		if (virq)
> +			irq_dispose_mapping(virq);
> +	}
> +
> +	irq_domain_remove(tps->irq_domain);
> +	tps->irq_domain = NULL;
> +}
> +
> +static void tps65217_domain_release(void *data)
> +{
> +	struct tps65217 *tps = data;
> +
> +	tps65217_irq_cleanup(tps);
> +}

Superfluous abstraction.  Please remove it.

> +
> +static void tps65217_irq_wake_disable(void *data)
> +{
> +	disable_irq_wake((unsigned int)(unsigned long)data);

Introduce a variable, then you can omit the casts.

> +}
> +
>  static int tps65217_irq_init(struct tps65217 *tps, int irq)
>  {
>  	int ret;
> @@ -170,6 +200,16 @@ static int tps65217_irq_init(struct tps65217 *tps, int irq)
>  		return -ENOMEM;
>  	}
>  
> +	/*
> +	 * Devres actions are released in reverse order of registration,
> +	 * so the domain is torn down after the parent interrupt and the
> +	 * MFD children, which are registered later in probe, have
> +	 * released their IRQs.
> +	 */

We don't usually describe how manages resources work when using devm_*().

> +	ret = devm_add_action_or_reset(tps->dev, tps65217_domain_release, tps);
> +	if (ret)
> +		return ret;
> +
>  	ret = devm_request_threaded_irq(tps->dev, irq, NULL,
>  					tps65217_irq_thread, IRQF_ONESHOT,
>  					"tps65217-irq", tps);
> @@ -179,7 +219,16 @@ static int tps65217_irq_init(struct tps65217 *tps, int irq)
>  		return ret;
>  	}
>  
> -	enable_irq_wake(irq);
> +	ret = enable_irq_wake(irq);
> +	if (ret) {
> +		dev_warn(tps->dev, "failed to enable IRQ wake: %d\n", ret);

Return zero here, then omit the else branch.

> +	} else {
> +		ret = devm_add_action_or_reset(tps->dev,
> +				tps65217_irq_wake_disable,
> +				(void *)(unsigned long)irq);
> +		if (ret)
> +			return ret;
> +	}
>  
>  	return 0;
>  }
> @@ -380,22 +429,6 @@ static int tps65217_probe(struct i2c_client *client)
>  	return 0;
>  }
>  
> -static void tps65217_remove(struct i2c_client *client)
> -{
> -	struct tps65217 *tps = i2c_get_clientdata(client);
> -	unsigned int virq;
> -	int i;
> -
> -	for (i = 0; i < TPS65217_NUM_IRQ; i++) {
> -		virq = irq_find_mapping(tps->irq_domain, i);
> -		if (virq)
> -			irq_dispose_mapping(virq);
> -	}
> -
> -	irq_domain_remove(tps->irq_domain);
> -	tps->irq_domain = NULL;
> -}
> -
>  static const struct i2c_device_id tps65217_id_table[] = {
>  	{"tps65217", TPS65217},
>  	{ /* sentinel */ }
> @@ -409,7 +442,6 @@ static struct i2c_driver tps65217_driver = {
>  	},
>  	.id_table	= tps65217_id_table,
>  	.probe		= tps65217_probe,
> -	.remove		= tps65217_remove,
>  };

-- 
Lee Jones