Re: [PATCH] irqchip/ti-sci-inta: fix processing of masked irqs

[Grygorii Strashko]

On 09/04/2020 12:31, Marc Zyngier wrote:
> On Wed, 8 Apr 2020 22:15:32 +0300
> Grygorii Strashko <grygorii.strashko@xxxxxx> wrote:
>
> > The ti_sci_inta_irq_handler() does not take into account INTA IRQs state
> > (masked/unmasked) as it uses INTA_STATUS_CLEAR_j register to get INTA IRQs
> > status, which provides raw status value.
> > This causes hard IRQ handlers to be called or threaded handlers to be
> > scheduled many times even if corresponding INTA IRQ is masked.
> > Above, first of all, affects the LEVEL interrupts processing and causes
> > unexpected behavior up the system stack or crash.
> >
> > Fix it by using the Interrupt Masked Status INTA_STATUSM_j register which
> > provides masked INTA IRQs status.
> >
> > Fixes: 9f1463b86c13 ("irqchip/ti-sci-inta: Add support for Interrupt Aggregator driver")
> > Signed-off-by: Grygorii Strashko <grygorii.strashko@xxxxxx>
>
> Given the failure mode, doesn't this deserve a Cc stable?

Sorry, was not sure how it works here.
"Fixes" tag now is usually enough to get included in stable.
Any way, I'll track it and if not included will re-send for stable.

> > ---
> >   drivers/irqchip/irq-ti-sci-inta.c | 3 ++-
> >   1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/irqchip/irq-ti-sci-inta.c b/drivers/irqchip/irq-ti-sci-inta.c
> > index 8f6e6b08eadf..7e3ebf6ed2cd 100644
> > --- a/drivers/irqchip/irq-ti-sci-inta.c
> > +++ b/drivers/irqchip/irq-ti-sci-inta.c
> > @@ -37,6 +37,7 @@
> >   #define VINT_ENABLE_SET_OFFSET	0x0
> >   #define VINT_ENABLE_CLR_OFFSET	0x8
> >   #define VINT_STATUS_OFFSET	0x18
> > +#define VINT_STATUS_MASKED_OFFSET	0x20
> >   
> >   /**
> >    * struct ti_sci_inta_event_desc - Description of an event coming to
> > @@ -116,7 +117,7 @@ static void ti_sci_inta_irq_handler(struct irq_desc *desc)
> >   	chained_irq_enter(irq_desc_get_chip(desc), desc);
> >   
> >   	val = readq_relaxed(inta->base + vint_desc->vint_id * 0x1000 +
> > -			    VINT_STATUS_OFFSET);
> > +			    VINT_STATUS_MASKED_OFFSET);
> >   
> >   	for_each_set_bit(bit, &val, MAX_EVENTS_PER_VINT) {
> >   		virq = irq_find_mapping(domain, vint_desc->events[bit].hwirq);
>
>
> Otherwise queued for post -rc1.

Thanks.

--
Best regards,
grygorii