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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20250314182021.3f6024c5@bootlin.com>
Date: Fri, 14 Mar 2025 18:20:21 +0100
From: Herve Codina <herve.codina@...tlin.com>
To: Thomas Gleixner <tglx@...utronix.de>
Cc: LKML <linux-kernel@...r.kernel.org>, Jiri Slaby <jirislaby@...nel.org>,
 Talel Shenhar <talel@...zon.com>, Nicolas Ferre
 <nicolas.ferre@...rochip.com>, Alexandre Belloni
 <alexandre.belloni@...tlin.com>, Claudiu Beznea <claudiu.beznea@...on.dev>,
 Florian Fainelli <florian.fainelli@...adcom.com>, Guo Ren
 <guoren@...nel.org>, Huacai Chen <chenhuacai@...nel.org>, Jiaxun Yang
 <jiaxun.yang@...goat.com>, Maxime Coquelin <mcoquelin.stm32@...il.com>,
 Alexandre Torgue <alexandre.torgue@...s.st.com>, Chen-Yu Tsai
 <wens@...e.org>, Jernej Skrabec <jernej.skrabec@...il.com>, Samuel Holland
 <samuel@...lland.org>, Andrew Lunn <andrew@...n.ch>, Sebastian Hesselbarth
 <sebastian.hesselbarth@...il.com>, Gregory Clement
 <gregory.clement@...tlin.com>, Uwe Kleine-König
 <ukleinek@...nel.org>, Linus Walleij <linus.walleij@...aro.org>, Bartosz
 Golaszewski <brgl@...ev.pl>
Subject: Re: [patch 6/7] irqchip: Convert generic irqchip locking to guards

Hi Thomas,

On Thu, 13 Mar 2025 15:31:27 +0100 (CET)
Thomas Gleixner <tglx@...utronix.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@...utronix.de>
> Cc: Talel Shenhar <talel@...zon.com>
> Cc: Nicolas Ferre <nicolas.ferre@...rochip.com>
> Cc: Alexandre Belloni <alexandre.belloni@...tlin.com>
> Cc: Claudiu Beznea <claudiu.beznea@...on.dev>
> Cc: Florian Fainelli <florian.fainelli@...adcom.com>
> Cc: Guo Ren <guoren@...nel.org>
> Cc: Herve Codina <herve.codina@...tlin.com>
> Cc: Huacai Chen <chenhuacai@...nel.org>
> Cc: Jiaxun Yang <jiaxun.yang@...goat.com>
> Cc: Maxime Coquelin <mcoquelin.stm32@...il.com>
> Cc: Alexandre Torgue <alexandre.torgue@...s.st.com>
> Cc: Chen-Yu Tsai <wens@...e.org>
> Cc: Jernej Skrabec <jernej.skrabec@...il.com>
> Cc: Samuel Holland <samuel@...lland.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é