RE: [PATCH 2/3] irqchip: renesas-rzg2l: Mask interrupts for changing interrupt settings

[Biju Das]

Hi Marc Zyngier,

Thanks for the feedback.

> Subject: Re: [PATCH 2/3] irqchip: renesas-rzg2l: Mask interrupts for
> changing interrupt settings
> 
> On Mon, 18 Sep 2023 13:24:10 +0100,
> Biju Das <biju.das.jz@xxxxxxxxxxxxxx> wrote:
> >
> > As per RZ/G2L hardware manual Rev.1.30 section 8.8.3 Precaution when
> > changing interrupt settings,  we need to mask the interrupts for any
> > changes in below settings:
> >
> >  * When changing the noise filter settings.
> >  * When switching the GPIO pins to IRQ or GPIOINT.
> >  * When changing the source of TINT.
> >  * When changing the interrupt detection method.
> >
> > This patch masks the interrupts when there is a change in the
> > interrupt detection method and changing the source of TINT.
> >
> > Fixes: 3fed09559cd8 ("irqchip: Add RZ/G2L IA55 Interrupt Controller
> > driver")
> > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > Tested-by: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
> > ---
> >  drivers/irqchip/irq-renesas-rzg2l.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/irqchip/irq-renesas-rzg2l.c
> > b/drivers/irqchip/irq-renesas-rzg2l.c
> > index 2cee5477be6b..33a22bafedcd 100644
> > --- a/drivers/irqchip/irq-renesas-rzg2l.c
> > +++ b/drivers/irqchip/irq-renesas-rzg2l.c
> > @@ -116,11 +116,13 @@ static void rzg2l_irqc_irq_disable(struct irq_data
> *d)
> >  		u8 tssr_index = TSSR_INDEX(offset);
> >  		u32 reg;
> >
> > +		irq_chip_mask_parent(d);
> >  		raw_spin_lock(&priv->lock);
> >  		reg = readl_relaxed(priv->base + TSSR(tssr_index));
> >  		reg &= ~(TSSEL_MASK << TSSEL_SHIFT(tssr_offset));
> >  		writel_relaxed(reg, priv->base + TSSR(tssr_index));
> >  		raw_spin_unlock(&priv->lock);
> > +		irq_chip_unmask_parent(d);
> 
> What guarantees that the parent irqchip state has been correctly restored?
> Nothing refcounts the nesting of mask/unmask.
> 
> >  	}
> >  	irq_chip_disable_parent(d);
> 
> I'd rather you start by *disabling* the parent, and then none of that
> matters at all.

Agreed. Will do this in next version.

> 
> >  }
> > @@ -137,11 +139,13 @@ static void rzg2l_irqc_irq_enable(struct irq_data
> *d)
> >  		u8 tssr_index = TSSR_INDEX(offset);
> >  		u32 reg;
> >
> > +		irq_chip_mask_parent(d);
> >  		raw_spin_lock(&priv->lock);
> >  		reg = readl_relaxed(priv->base + TSSR(tssr_index));
> >  		reg |= (TIEN | tint) << TSSEL_SHIFT(tssr_offset);
> >  		writel_relaxed(reg, priv->base + TSSR(tssr_index));
> >  		raw_spin_unlock(&priv->lock);
> > +		irq_chip_unmask_parent(d);
> >  	}
> >  	irq_chip_enable_parent(d);
> 
> Same thing: if the parent was disabled, why do we need to do anything?

OK. It is not required.

> 
> 
> >  }
> > @@ -226,10 +230,12 @@ static int rzg2l_irqc_set_type(struct irq_data *d,
> unsigned int type)
> >  	unsigned int hw_irq = irqd_to_hwirq(d);
> >  	int ret = -EINVAL;
> >
> > +	irq_chip_mask_parent(d);
> >  	if (hw_irq >= IRQC_IRQ_START && hw_irq <= IRQC_IRQ_COUNT)
> >  		ret = rzg2l_irq_set_type(d, type);
> >  	else if (hw_irq >= IRQC_TINT_START && hw_irq < IRQC_NUM_IRQ)
> >  		ret = rzg2l_tint_set_edge(d, type);
> > +	irq_chip_unmask_parent(d);
> 
> This one is the only interesting one: why don't you mask the interrupt at
> the local level rather than on the parent? And this should be conditioned
> on the interrupt state itself (enabled or disabled), not done
> unconditionally.

OK. Will do this locally by conditioned on the interrupt state.

Cheers,
Biju