linux-kernel - Re: [PATCH] genirq: use relaxed access by default for irq_reg

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: <87ttmvvxon.wl-maz@kernel.org>
Date: Tue, 30 Jan 2024 11:59:52 +0000
From: Marc Zyngier <maz@...nel.org>
To: Lucas Stach <l.stach@...gutronix.de>
Cc: Thomas Gleixner <tglx@...utronix.de>,
	Anup Patel <apatel@...tanamicro.com>,
	James Gowans <jgowans@...zon.com>,
	Koichiro Den <den@...inux.co.jp>,
	linux-kernel@...r.kernel.org,
	kernel@...gutronix.de,
	patchwork-lst@...gutronix.de
Subject: Re: [PATCH] genirq: use relaxed access by default for irq_reg_{readl,writel}

On Mon, 29 Jan 2024 14:45:02 +0000,
Lucas Stach <l.stach@...gutronix.de> wrote:
> 
> irqchip access does not require any memory ordering between other
> memory transactions and the IRQ controller peripheral access.
> As all architectures now implement the relaxed MMIO accessors we
> can switch the irq_reg_{readl,writel} helpers to use them, in
> order to avoid potentially costly barriers in the IRQ handling
> hotpath.
> 
> Signed-off-by: Lucas Stach <l.stach@...gutronix.de>
> ---
>  include/linux/irq.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/irq.h b/include/linux/irq.h
> index 90081afa10ce..fa1597db7887 100644
> --- a/include/linux/irq.h
> +++ b/include/linux/irq.h
> @@ -1218,7 +1218,7 @@ static inline void irq_reg_writel(struct irq_chip_generic *gc,
>  	if (gc->reg_writel)
>  		gc->reg_writel(val, gc->reg_base + reg_offset);
>  	else
> -		writel(val, gc->reg_base + reg_offset);
> +		writel_relaxed(val, gc->reg_base + reg_offset);
>  }
>  
>  static inline u32 irq_reg_readl(struct irq_chip_generic *gc,
> @@ -1227,7 +1227,7 @@ static inline u32 irq_reg_readl(struct irq_chip_generic *gc,
>  	if (gc->reg_readl)
>  		return gc->reg_readl(gc->reg_base + reg_offset);
>  	else
> -		return readl(gc->reg_base + reg_offset);
> +		return readl_relaxed(gc->reg_base + reg_offset);
>  }
>  

If this relaxation is introduced, it really should be documented and
require a buy-in, because unsuspecting drivers may implicitly depend
on the stronger ordering.

I'm a strong advocate of the relaxed ordering, but changing this
wholesale is potentially dangerous.

	M.

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