Re: [patch 6/7] irqchip: Convert generic irqchip locking to guards

[Herve Codina]

Hi Thomas,

On Thu, 13 Mar 2025 15:31:27 +0100 (CET)
Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:

> Conversion was done with Coccinelle and a few manual fixups.
> 
> In a few interrupt chip callbacks this changes replaces
> raw_spin_lock_irqsave() with a guard(raw_spinlock). That's intended and
> correct because those interrupt chip callbacks are invoked with the
> interrupt descriptor lock held and interrupts disabled. No point in using
> the irqsave variant.
> 
> No functional change.
> 
> Signed-off-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Cc: Talel Shenhar <talel@xxxxxxxxxx>
> Cc: Nicolas Ferre <nicolas.ferre@xxxxxxxxxxxxx>
> Cc: Alexandre Belloni <alexandre.belloni@xxxxxxxxxxx>
> Cc: Claudiu Beznea <claudiu.beznea@xxxxxxxxx>
> Cc: Florian Fainelli <florian.fainelli@xxxxxxxxxxxx>
> Cc: Guo Ren <guoren@xxxxxxxxxx>
> Cc: Herve Codina <herve.codina@xxxxxxxxxxx>
> Cc: Huacai Chen <chenhuacai@xxxxxxxxxx>
> Cc: Jiaxun Yang <jiaxun.yang@xxxxxxxxxxx>
> Cc: Maxime Coquelin <mcoquelin.stm32@xxxxxxxxx>
> Cc: Alexandre Torgue <alexandre.torgue@xxxxxxxxxxx>
> Cc: Chen-Yu Tsai <wens@xxxxxxxx>
> Cc: Jernej Skrabec <jernej.skrabec@xxxxxxxxx>
> Cc: Samuel Holland <samuel@xxxxxxxxxxxx>
> ---
>  drivers/irqchip/irq-al-fic.c           |   18 +++++-------------
>  drivers/irqchip/irq-atmel-aic.c        |   19 ++++++-------------
>  drivers/irqchip/irq-atmel-aic5.c       |   28 ++++++++--------------------
>  drivers/irqchip/irq-bcm7120-l2.c       |   22 +++++++++-------------
>  drivers/irqchip/irq-brcmstb-l2.c       |    8 ++------
>  drivers/irqchip/irq-csky-apb-intc.c    |    3 +--
>  drivers/irqchip/irq-dw-apb-ictl.c      |    3 +--
>  drivers/irqchip/irq-ingenic-tcu.c      |    9 +++------
>  drivers/irqchip/irq-lan966x-oic.c      |   18 +++++++-----------
>  drivers/irqchip/irq-loongson-liointc.c |    9 ++-------
>  drivers/irqchip/irq-mscc-ocelot.c      |    3 +--
>  drivers/irqchip/irq-stm32-exti.c       |   21 ++++++---------------
>  drivers/irqchip/irq-sunxi-nmi.c        |    9 ++-------
>  drivers/irqchip/irq-tb10x.c            |   13 +++----------
>  14 files changed, 56 insertions(+), 127 deletions(-)
> 

...

> --- a/drivers/irqchip/irq-lan966x-oic.c
> +++ b/drivers/irqchip/irq-lan966x-oic.c
> @@ -71,14 +71,12 @@ static unsigned int lan966x_oic_irq_star
>  	struct lan966x_oic_chip_regs *chip_regs = gc->private;
>  	u32 map;
>  
> -	irq_gc_lock(gc);
> -
> -	/* Map the source interrupt to the destination */
> -	map = irq_reg_readl(gc, chip_regs->reg_off_map);
> -	map |= data->mask;
> -	irq_reg_writel(gc, map, chip_regs->reg_off_map);
> -
> -	irq_gc_unlock(gc);
> +	scoped_guard (raw_spinlock, &gc->lock) {
> +		/* Map the source interrupt to the destination */
> +		map = irq_reg_readl(gc, chip_regs->reg_off_map);
> +		map |= data->mask;
> +		irq_reg_writel(gc, map, chip_regs->reg_off_map);
> +	}
>  
>  	ct->chip.irq_ack(data);
>  	ct->chip.irq_unmask(data);
> @@ -95,14 +93,12 @@ static void lan966x_oic_irq_shutdown(str
>  
>  	ct->chip.irq_mask(data);
>  
> -	irq_gc_lock(gc);
> +	guard(raw_spinlock)(&gc->lock);
>  
>  	/* Unmap the interrupt */
>  	map = irq_reg_readl(gc, chip_regs->reg_off_map);
>  	map &= ~data->mask;
>  	irq_reg_writel(gc, map, chip_regs->reg_off_map);
> -
> -	irq_gc_unlock(gc);
>  }

Here, I would really prefer a scoped_guard() to clearly identify what is
protected such as:

	scoped_guard (raw_spinlock, &gc->lock) {
		/* Unmap the interrupt */
		map = irq_reg_readl(gc, chip_regs->reg_off_map);
		map &= ~data->mask;
 		irq_reg_writel(gc, map, chip_regs->reg_off_map);
	}

IMHO, guard() is nice when it protects an entire function but it can be
missed when it is present in a middle of code (looks too much like a
simple C function call).

Best regards,
Hervé