Re: [PATCH v6 2/5] pinctrl: mediatek: free EINT resources on unbind

On 7/13/26 10:37, Justin Yeh wrote:
> mtk_eint_do_init() creates an IRQ domain, populates it with a mapping for
> every EINT line and installs a chained handler on the parent interrupt,
> but none of these are ever released. This was harmless while the drivers
> were built-in, but now that they can be built as modules and
> unbound/rmmod'd it leaves behind a dangling IRQ domain, interrupt mappings
> whose chip data points at freed memory, and a chained handler that keeps
> firing into that freed data.
> 
> The plain allocations in mtk_eint_do_init() already use the device-managed
> devm_*() helpers, so tear the remaining resources down the same way:
> register a devm action that detaches the chained handler, disposes of the
> per-line mappings and removes the IRQ domain. This mirrors the
> device-managed lifecycle adopted for the GPIO chip and keeps the whole
> EINT setup self-cleaning on unbind.
> 
> Fixes: e46df235b4e6 ("pinctrl: mediatek: refactor EINT related code for all MediaTek pinctrl can fit")
> Signed-off-by: Justin Yeh <[email protected]>
> ---
>   drivers/pinctrl/mediatek/mtk-eint.c | 21 ++++++++++++++++++++-
>   1 file changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pinctrl/mediatek/mtk-eint.c b/drivers/pinctrl/mediatek/mtk-eint.c
> index 47ac92ea98c2..7cd4c4a9fc77 100644
> --- a/drivers/pinctrl/mediatek/mtk-eint.c
> +++ b/drivers/pinctrl/mediatek/mtk-eint.c
> @@ -12,6 +12,7 @@
>    */
>   
>   #include <linux/delay.h>
> +#include <linux/device.h>
>   #include <linux/err.h>
>   #include <linux/gpio/driver.h>
>   #include <linux/io.h>
> @@ -509,6 +510,24 @@ int mtk_eint_find_irq(struct mtk_eint *eint, unsigned long eint_n)
>   }
>   EXPORT_SYMBOL_GPL(mtk_eint_find_irq);
>   
> +static void mtk_eint_teardown(void *data)
> +{
> +	struct mtk_eint *eint = data;
> +	unsigned int i, virq;
> +
> +	/* Detach the demux handler so it can no longer reference freed data. */
> +	irq_set_chained_handler_and_data(eint->irq, NULL, NULL);

add here:

synchronize_irq(eint->irq);

...so that we don't risk race conditions when tearing down.

after which:
Reviewed-by: AngeloGioacchino Del Regno <[email protected]>

> +
> +	/* Dispose of all child mappings before the domain is removed. */
> +	for (i = 0; i < eint->hw->ap_num; i++) {
> +		virq = irq_find_mapping(eint->domain, i);
> +		if (virq)
> +			irq_dispose_mapping(virq);
> +	}
> +
> +	irq_domain_remove(eint->domain);
> +}
> +
>   int mtk_eint_do_init(struct mtk_eint *eint, struct mtk_eint_pin *eint_pin)
>   {
>   	unsigned int size, i, port, virq, inst = 0;
> @@ -601,7 +620,7 @@ int mtk_eint_do_init(struct mtk_eint *eint, struct mtk_eint_pin *eint_pin)
>   	irq_set_chained_handler_and_data(eint->irq, mtk_eint_irq_handler,
>   					 eint);
>   
> -	return 0;
> +	return devm_add_action_or_reset(eint->dev, mtk_eint_teardown, eint);
>   
>   err_eint:
>   	for (i = 0; i < eint->nbase; i++) {