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

Herve Codina
2025-03-14T17:47:20Z

Hi Thomas,

On Thu, 13 Mar 2025 15:31:27 +0100 (CET)
Thomas Gleixner <tglx@linutronix.de> 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@linutronix.de>
> Cc: Talel Shenhar <talel@amazon.com>
> Cc: Nicolas Ferre <nicolas.ferre@microchip.com>
> Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
> Cc: Claudiu Beznea <claudiu.beznea@tuxon.dev>
> Cc: Florian Fainelli <florian.fainelli@broadcom.com>
> Cc: Guo Ren <guoren@kernel.org>
> Cc: Herve Codina <herve.codina@bootlin.com>
> Cc: Huacai Chen <chenhuacai@kernel.org>
> Cc: Jiaxun Yang <jiaxun.yang@flygoat.com>
> Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com>
> Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>
> Cc: Chen-Yu Tsai <wens@csie.org>
> Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
> Cc: Samuel Holland <samuel@sholland.org>
> ---
> 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é