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

[Marc Zyngier]

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.

>  }
> @@ -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?


>  }
> @@ -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.

Thanks,

	M.

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