linux-kernel - Re: [PATCH 00/18] Cross-architecture definitions of relaxed MMIO accessors

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20140422134345.GD9820@arm.com>
Date:	Tue, 22 Apr 2014 14:43:45 +0100
From:	Will Deacon <will.deacon@....com>
To:	Sam Ravnborg <sam@...nborg.org>
Cc:	"linux-arch@...r.kernel.org" <linux-arch@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"arnd@...db.de" <arnd@...db.de>,
	"monstr@...str.eu" <monstr@...str.eu>,
	"dhowells@...hat.com" <dhowells@...hat.com>,
	"broonie@...aro.org" <broonie@...aro.org>,
	"benh@...nel.crashing.org" <benh@...nel.crashing.org>,
	"peterz@...radead.org" <peterz@...radead.org>,
	"paulmck@...ux.vnet.ibm.com" <paulmck@...ux.vnet.ibm.com>
Subject: Re: [PATCH 00/18] Cross-architecture definitions of relaxed MMIO
 accessors

Hello Sam,

On Thu, Apr 17, 2014 at 08:15:55PM +0100, Sam Ravnborg wrote:
> On Thu, Apr 17, 2014 at 04:47:15PM +0100, Will Deacon wrote:
> > On Thu, Apr 17, 2014 at 04:36:38PM +0100, Sam Ravnborg wrote:
> > > On Thu, Apr 17, 2014 at 02:44:03PM +0100, Will Deacon wrote:
> > > > This RFC series attempts to define a portable (i.e. cross-architecture)
> > > > definition of the {readX,writeX}_relaxed MMIO accessor functions. These
> > > > functions are already in widespread use amongst drivers (mainly those supporting
> > > > devices embedded in ARM SoCs), but lack any well-defined semantics and,
> > > > subsequently, any portable definitions to allow these drivers to be compiled for
> > > > other architectures.
> > > 
> > > Could this be made in such a way that only architectures that need
> > > to provide their own versions actually have to add them?
> > > 
> > > The current patch-set adds the same dummy defines all over,
> > > and will put this burden also on new architectures.
> > 
> > It shouldn't be a burden for new architectures, as they will use
> > asm-generic/io.h and get the definitions from there.
> 
> Why is it then necesary to do this for sparc:
> diff --git a/arch/sparc/include/asm/io.h b/arch/sparc/include/asm/io.h
> index f6902cf3cbe9..493f22c4684f 100644
> --- a/arch/sparc/include/asm/io.h
> +++ b/arch/sparc/include/asm/io.h
> @@ -10,6 +10,15 @@
>   * Defines used for both SPARC32 and SPARC64
>   */
> 
> +/* Relaxed accessors for MMIO */
> +#define readb_relaxed(__addr)          readb(__addr)
> +#define readw_relaxed(__addr)          readw(__addr)
> +#define readl_relaxed(__addr)          readl(__addr)
> +
> +#define writeb_relaxed(__b, __addr)    writeb(__b, __addr)
> +#define writew_relaxed(__w, __addr)    writew(__w, __addr)
> +#define writel_relaxed(__l, __addr)    writel(__l, __addr)
> 
> And similar for several other architectures.

This is because Sparc (and the other architectures I had to modify) don't
make use of asm-generic/io.h. Furthermore, it's not as simple as adding an
include, since you'll pull in the generic definitions of things like readw
and inb as it stands.

We could make a new asm-generic file purely for the relaxed accessors, but
I really don't think it's worth the hassle.

> For asm-generic/io.h:
> +#ifndef readb_relaxed
> +#define readb_relaxed  readb
> +#endif
> 
> This has same effect as the above.
> Only difference is that the implementation in asm-generic lacks the arguments.

I just followed the existing style in asm-generic/io.h.

> The patch also breaks the pattern that the #define foobar foobar is
> on the line just above the static inline that implements the function.
> 
> -#define readw readw
> +#define readw          readw
> 
> +#ifndef readw_relaxed
> +#define readw_relaxed  readw
> +#endif
> Move this blow below the static inline would make this easier to understand.
> 
>  static inline u16 readw(const volatile void __iomem *addr)
>  {
>         return __le16_to_cpu(__raw_readw(addr));
>  }

Sure, I can fix that.

Will
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/