[PATCH] irqchip/imx-irqsteer: Fix irq handling if an error occurs in imx_irqsteer_irq_handler()

[PATCH] irqchip/imx-irqsteer: Fix irq handling if an error occurs in imx_irqsteer_irq_handler()

[Christophe JAILLET]

chained_irq_enter(() should be paired with a corresponding
chained_irq_exit().

Here, if (hwirq < 0), a early return occurs and chained_irq_exit() is not
called.

Add a new label and a goto for fix it.

Fixes: 28528fca4908 ("irqchip/imx-irqsteer: Add multi output interrupts support")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
Compile tested only.

Review with care, irq handling is sometimes tricky...
---
 drivers/irqchip/irq-imx-irqsteer.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/irqchip/irq-imx-irqsteer.c b/drivers/irqchip/irq-imx-irqsteer.c
index 75a0e980ff35..59abe5a8beb8 100644
--- a/drivers/irqchip/irq-imx-irqsteer.c
+++ b/drivers/irqchip/irq-imx-irqsteer.c
@@ -135,7 +135,7 @@ static void imx_irqsteer_irq_handler(struct irq_desc *desc)
 	if (hwirq < 0) {
 		pr_warn("%s: unable to get hwirq base for irq %d\n",
 			__func__, irq);
-		return;
+		goto out;
 	}
 
 	for (i = 0; i < 2; i++, hwirq += 32) {
@@ -153,6 +153,7 @@ static void imx_irqsteer_irq_handler(struct irq_desc *desc)
 			generic_handle_domain_irq(data->domain, pos + hwirq);
 	}
 
+out:
 	chained_irq_exit(irq_desc_get_chip(desc), desc);
 }
 
-- 
2.47.0

Re: [PATCH] irqchip/imx-irqsteer: Fix irq handling if an error occurs in imx_irqsteer_irq_handler()

[Christophe JAILLET]

Le 17/11/2024 à 12:21, Christophe JAILLET a écrit :
> chained_irq_enter(() should be paired with a corresponding
> chained_irq_exit().
> 
> Here, if (hwirq < 0), a early return occurs and chained_irq_exit() is not
> called.

After several month without any feedback, this is a polite ping.
Is this patch correct?

CJ


> 
> Add a new label and a goto for fix it.
> 
> Fixes: 28528fca4908 ("irqchip/imx-irqsteer: Add multi output interrupts support")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> Compile tested only.
> 
> Review with care, irq handling is sometimes tricky...
> ---
>   drivers/irqchip/irq-imx-irqsteer.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/irqchip/irq-imx-irqsteer.c b/drivers/irqchip/irq-imx-irqsteer.c
> index 75a0e980ff35..59abe5a8beb8 100644
> --- a/drivers/irqchip/irq-imx-irqsteer.c
> +++ b/drivers/irqchip/irq-imx-irqsteer.c
> @@ -135,7 +135,7 @@ static void imx_irqsteer_irq_handler(struct irq_desc *desc)
>   	if (hwirq < 0) {
>   		pr_warn("%s: unable to get hwirq base for irq %d\n",
>   			__func__, irq);
> -		return;
> +		goto out;
>   	}
>   
>   	for (i = 0; i < 2; i++, hwirq += 32) {
> @@ -153,6 +153,7 @@ static void imx_irqsteer_irq_handler(struct irq_desc *desc)
>   			generic_handle_domain_irq(data->domain, pos + hwirq);
>   	}
>   
> +out:
>   	chained_irq_exit(irq_desc_get_chip(desc), desc);
>   }
>   

Re: [PATCH] irqchip/imx-irqsteer: Fix irq handling if an error occurs in imx_irqsteer_irq_handler()

[Marc Zyngier]

On Sat, 12 Jul 2025 10:58:35 +0100,
Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote:
> 
> Le 17/11/2024 à 12:21, Christophe JAILLET a écrit :
> > chained_irq_enter(() should be paired with a corresponding
> > chained_irq_exit().
> > 
> > Here, if (hwirq < 0), a early return occurs and chained_irq_exit() is not
> > called.
> 
> After several month without any feedback, this is a polite ping.
> Is this patch correct?

An untested patch is unlikely to make a lot of forward progress, to
be honest.

> 
> CJ
> 
> 
> > 
> > Add a new label and a goto for fix it.
> > 
> > Fixes: 28528fca4908 ("irqchip/imx-irqsteer: Add multi output interrupts support")
> > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> > ---
> > Compile tested only.
> > 
> > Review with care, irq handling is sometimes tricky...
> > ---
> >   drivers/irqchip/irq-imx-irqsteer.c | 3 ++-
> >   1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/irqchip/irq-imx-irqsteer.c b/drivers/irqchip/irq-imx-irqsteer.c
> > index 75a0e980ff35..59abe5a8beb8 100644
> > --- a/drivers/irqchip/irq-imx-irqsteer.c
> > +++ b/drivers/irqchip/irq-imx-irqsteer.c
> > @@ -135,7 +135,7 @@ static void imx_irqsteer_irq_handler(struct irq_desc *desc)
> >   	if (hwirq < 0) {
> >   		pr_warn("%s: unable to get hwirq base for irq %d\n",
> >   			__func__, irq);
> > -		return;
> > +		goto out;
> >   	}
> >     	for (i = 0; i < 2; i++, hwirq += 32) {
> > @@ -153,6 +153,7 @@ static void imx_irqsteer_irq_handler(struct irq_desc *desc)
> >   			generic_handle_domain_irq(data->domain, pos + hwirq);
> >   	}
> >   +out:
> >   	chained_irq_exit(irq_desc_get_chip(desc), desc);
> >   }

The real question is *how* do you end-up in this situation.

To trigger this case, you need a mux interrupt that is handled by
imx_irqsteer_irq_handler(), but for which you haven't got a
translation from DT the first place. Do you see the chicken and egg
problem?

In summary, this driver is checking for conditions that can't possibly
happen, and this check should simply be deleted instead of being
blindly "fixed".

	M.

-- 
Without deviation from the norm, progress is not possible.