linux-kernel - Re: [PATCH] Revert "regmap-mmio: Use native endianness for read/write"

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 for Android: free password hash cracker in your pocket

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

Message-ID: <6962431.EnoXCqILtZ@wuerfel>
Date:	Tue, 26 Jan 2016 10:09:35 +0100
From:	Arnd Bergmann <arnd@...db.de>
To:	Johannes Berg <johannes@...solutions.net>
Cc:	Mark Brown <broonie@...nel.org>, Simon Arlott <simon@...e.lp0.eu>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Revert "regmap-mmio: Use native endianness for read/write"

On Tuesday 26 January 2016 09:25:36 Johannes Berg wrote:
> On Mon, 2016-01-25 at 23:52 +0000, Mark Brown wrote:
> 
> >  - Make the default for MMIO regmaps be explicitly little endian with
> >    either an ifdef for MIPS to keep it working or an explict native
> >    endianness tag in the DT instead of the straight revert to LE (the
> >    latter seems better).
> 
> This makes sense, and I agree that the latter is better.

+1

> >  - Convert the MMIO regmap to use reg_read() and reg_write() with
> >    implementations using either readX() or ioread_*be() and
> > equivalents for write.  This means the core does no endianness
> > swapping and it's all in the bus.
> 
> I don't think there's ioread64be/iowrite64be, and I'm also not entirely
> sure how that works since it uses __raw_* internally in lib/iomap.c?

Again, it's complicated: 

* We should probably add ioread64be()/iowrite64be() on *64bit* architectures
  for consistency with readq/writeq

* On 32-bit architectures, you generally cannot do 64-bit atomic I/O
  operations, and we have two implementations that do it nonatomically,
  depending on how a device is wired to the bus, see
  include/linux/io-64-nonatomic-{hi-lo,lo-hi}.h.
  I think we should just not go there for regmap unless we absolutely
  have to.

> > Unfortunately that all sounds a bit too big for v4.5...  perhaps a
> > combination of a revert of the implementation and the addition of the
> > native tag to the DT for v4.5 followed by the reworking of the bus
> > for v4.6, I really would rather keep the DT change in v4.5 since
> > specifying LE is just bad and we don't want that to propagate any
> > more than it has to.
> 
> Yes, that makes sense.

Sounds good.

> > From this I also conclude that we need to improve our testing of big
> > endian ARM systems since nobody managed to notice this all the time
> > this was cooking in -next.
> 
> To my knowledge before I did a couple of days ago nobody ever ran i.MX6
> in big endian mode :)

At least nobody ever talked about it in public if they did, because they
would have to do the same patches you wrote.

There is still one open question about the defaults: I think we all agree
that there is no way we can change the default for compatible="syscon"
devices on ARM to from little-endian to cpu-endian, as that would break
everything. Annotating the MIPS dts files as "cpu-endian" and leaving the
rest to default to "little" is probably best here.

However, we have some freedom at the regmap-mmio level, which we can
sanitize in 4.6 if we want to make it more consistent with the rest
of regmap. We have around 50 callers of {devm_,}regmap_init_mmio()
and almost all of them do not specify endianess but expect little-endian
behavior. We can change all existing instances to set REGMAP_ENDIAN_NATIVE
explicitly and change regmap_init_mmio() to return an error if the
caller does not specify a particular endianess (big, little, native).
This is a tradeoff between interface simplicity (defaulting to LE is
convenient for most people) and consistency (all other regmap interfaces
default to native if I understood Mark right).

	Arnd