Hi,
Presently, the 64bit IO functions are not very usable in drivers because
they are not universally available in all architectures. This leads to
a bunch of hacks in the kernel to work around this. (See the last 3
patches in this series.) As part of my switchtec_ntb submission which
added another one of these warts, Greg asked me to look into fixing
it[1].
So this patchset attempts to solve this issue by filling in the missing
implementations in iomap.c and io.h. After that, the alpha architecture is
the only one I found that also needed a fix for this. Finally, this
patchset removes the hacks that have accumulated in the kernel,
thus far, for working around this.
This set is based off of v4.12-rc6.
Thanks,
Logan
[1] https://marc.info/?l=linux-kernel&m=149774601910663&w=2
Logan Gunthorpe (7):
drm/tilcdc: don't use volatile with iowrite64
iomap: implement ioread64 and iowrite64
asm-generic/io.h: make ioread64 and iowrite64 universally available
alpha: provide ioread64 and iowrite64 implementations
ntb: ntb_hw_intel: remove ioread64 and iowrite64 hacks
drm/tilcdc: clean up ifdef hacks around iowrite64
crypto: caam: cleanup CONFIG_64BIT ifdefs when using io{read|write}64
arch/alpha/include/asm/io.h | 2 ++
arch/alpha/kernel/io.c | 18 +++++++++++
arch/powerpc/include/asm/io.h | 2 ++
drivers/crypto/caam/regs.h | 29 -----------------
drivers/gpu/drm/tilcdc/tilcdc_regs.h | 8 +----
drivers/ntb/hw/intel/ntb_hw_intel.c | 30 -----------------
include/asm-generic/io.h | 54 ++++++++++++++++++++++++-------
include/asm-generic/iomap.h | 4 ---
lib/iomap.c | 62 ++++++++++++++++++++++++++++++++++++
9 files changed, 127 insertions(+), 82 deletions(-)
--
2.11.0
Now that ioread64 and iowrite64 are available generically we can
remove the hack at the top of ntb_hw_intel.c that patches them in
when they are not available.
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
Cc: Jon Mason <jdmason@kudzu.us>
Cc: Dave Jiang <dave.jiang@intel.com>
Cc: Allen Hubbe <redacted>
---
drivers/ntb/hw/intel/ntb_hw_intel.c | 30 ------------------------------
1 file changed, 30 deletions(-)
Currently, ioread64 and iowrite64 are only available io CONFIG_64BIT=y
and CONFIG_GENERIC_IOMAP=n. Thus, seeing the functions are not
universally available, it makes them unusable for driver developers.
This leads to ugly hacks such as those at the top of
drivers/ntb/hw/intel/ntb_hw_intel.c
This patch adds fallback implementations for when CONFIG_64BIT and
CONFIG_GENERIC_IOMAP are not set. These functions use two io32 based
calls to complete the operation.
Note, we do not use the volatile keyword in these functions like the
others in the same file. It is necessary to avoid a compiler warning
on arm.
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
Cc: Arnd Bergmann <arnd@arndb.de>
---
include/asm-generic/io.h | 54 +++++++++++++++++++++++++++++++++++++-----------
1 file changed, 42 insertions(+), 12 deletions(-)
This is a prep patch for adding a universal iowrite64.
The patch is to prevent compiler warnings when we add iowrite64 that
would occur because there is an unnecessary volatile in this driver.
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
Cc: Jyri Sarha <redacted>
Cc: Tomi Valkeinen <redacted>
Cc: David Airlie <redacted>
---
drivers/gpu/drm/tilcdc/tilcdc_regs.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
Now that ioread64 and iowrite64 are always available we don't
need the ugly ifdefs to change their implementation when they
are not.
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
Cc: "Horia Geantă" <horia.geanta@nxp.com>
Cc: Dan Douglass <redacted>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: "David S. Miller" <davem@davemloft.net>
---
drivers/crypto/caam/regs.h | 29 -----------------------------
1 file changed, 29 deletions(-)
Currently, ioread64 and iowrite64 are not impleminted in the generic
iomap implementation. The prototypes are defined if CONFIG_64BIT is set
but there is no actual implementation.
Seeing the functions are not universally available, they are unusable
for driver developers. This leads to ugly hacks such as those at
the top of
drivers/ntb/hw/intel/ntb_hw_intel.c
This patch adds generic implementations for these functions. We add
the obvious version if readq/writeq are implemented and fall back
to using two io32 calls in cases that don't provide direct 64bit
accesses. Thus making the functions universally available to
configurations with CONFIG_GENERIC_IOMAP=y.
For any pio accesses, the 64bit operations remain unsupported and
simply call bad_io_access in cases readq would be called.
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <redacted>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Suresh Warrier <redacted>
Cc: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/include/asm/io.h | 2 ++
include/asm-generic/iomap.h | 4 ---
lib/iomap.c | 62 +++++++++++++++++++++++++++++++++++++++++++
3 files changed, 64 insertions(+), 4 deletions(-)
Now that we can expect iowrite64 to always exist the hack is no longer
necessary so we just call iowrite64 directly.
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
Cc: Jyri Sarha <redacted>
Cc: Tomi Valkeinen <redacted>
Cc: David Airlie <redacted>
---
drivers/gpu/drm/tilcdc/tilcdc_regs.h | 6 ------
1 file changed, 6 deletions(-)
Alpha implements its own io operation and doesn't use the
common library. Thus to make ioread64 and iowrite64 globally
available we need to add implementations for alpha.
For this, we simply use calls that chain two 32-bit operations.
(mostly because I don't really understand the alpha architecture.)
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
Cc: Richard Henderson <redacted>
Cc: Ivan Kokshaysky <redacted>
Cc: Matt Turner <mattst88@gmail.com>
---
arch/alpha/include/asm/io.h | 2 ++
arch/alpha/kernel/io.c | 18 ++++++++++++++++++
2 files changed, 20 insertions(+)
On Thu, 22 Jun 2017 10:48:14 -0600
Logan Gunthorpe [off-list ref] wrote:
Alpha implements its own io operation and doesn't use the
common library. Thus to make ioread64 and iowrite64 globally
available we need to add implementations for alpha.
For this, we simply use calls that chain two 32-bit operations.
(mostly because I don't really understand the alpha architecture.)
But this does not do the same thing as an ioread64 with regards to
atomicity or side effects on the device. The same is true of the other
hacks. You either have a real 64bit single read/write from MMIO space or
you don't. You can't fake it.
Alan
But this does not do the same thing as an ioread64 with regards to
atomicity or side effects on the device. The same is true of the other
hacks. You either have a real 64bit single read/write from MMIO space or
you don't. You can't fake it.
Yes, I know. But is it not better than having every driver that wants to
use these functions fake it themselves?
Logan
On Thu, 22 Jun 2017 10:48:13 -0600
Logan Gunthorpe [off-list ref] wrote:
Currently, ioread64 and iowrite64 are only available io CONFIG_64BIT=y
and CONFIG_GENERIC_IOMAP=n. Thus, seeing the functions are not
universally available, it makes them unusable for driver developers.
This leads to ugly hacks such as those at the top of
drivers/ntb/hw/intel/ntb_hw_intel.c
This patch adds fallback implementations for when CONFIG_64BIT and
CONFIG_GENERIC_IOMAP are not set. These functions use two io32 based
calls to complete the operation.
Note, we do not use the volatile keyword in these functions like the
others in the same file. It is necessary to avoid a compiler warning
on arm.
This is a really really bad idea as per the Alpha comment.
ioread64 and iowrite64 generate a single 64bit bus transaction. There is
hardware where mmio operations have side effects so simply using a pair
of 32bit operations blindly does not work (consider something as trivial
as reading a 64bit performance counter or incrementing pointer).
If a platform doesn't support 64bit I/O operations from the CPU then you
either need to use some kind of platform/architecture specific interface
if present or accept you don't have one.
It's not safe to split it. Possibly for some use cases you could add an
ioread64_maysplit()
but you cannot blindly break ioread64/write64() and expect it to
magically allow you to use drivers that depend upon it.
What btw is the actual ARM compiler warning ? Is the compiler also trying
to tell you it's a bad idea ?
Alan
If a platform doesn't support 64bit I/O operations from the CPU then you
either need to use some kind of platform/architecture specific interface
if present or accept you don't have one.
Yes, I understand that.
The thing is that every user that's currently using it right now is
patching in their own version that splits it on non-64bit systems.
It's not safe to split it. Possibly for some use cases you could add an
ioread64_maysplit()
I'm open to doing something like that.
What btw is the actual ARM compiler warning ? Is the compiler also trying
to tell you it's a bad idea ?
It's just the compiler noting that you are mixing volatile and
non-volatile pointers. Strangely some io{read|write}XX use volatile but
most do not. But it's nothing crazy.
Logan
On Thu, 22 Jun 2017 14:24:58 -0600
Logan Gunthorpe [off-list ref] wrote:
On 6/22/2017 2:14 PM, Alan Cox wrote:
quoted
If a platform doesn't support 64bit I/O operations from the CPU then you
either need to use some kind of platform/architecture specific interface
if present or accept you don't have one.
Yes, I understand that.
The thing is that every user that's currently using it right now is
patching in their own version that splits it on non-64bit systems.
quoted
It's not safe to split it. Possibly for some use cases you could add an
ioread64_maysplit()
I'm open to doing something like that.
I think that makes sense for the platforms with that problem. I'm not
sure there are many that can't do it for mmio at least. 486SX can't do it
and I guess some ARM32 but I think almost everyone else can including
most 32bit x86.
What's more of a problem is a lot of platforms can do 64bit MMIO via
ioread/write64 but not 64bit port I/O, and it's not clear how you
represent that via an ioread/write API that abstracts it away.
Alan
I think that makes sense for the platforms with that problem. I'm not
sure there are many that can't do it for mmio at least. 486SX can't do it
and I guess some ARM32 but I think almost everyone else can including
most 32bit x86.
What's more of a problem is a lot of platforms can do 64bit MMIO via
ioread/write64 but not 64bit port I/O, and it's not clear how you
represent that via an ioread/write API that abstracts it away.
In Patch 2, we call bad_io_access for anyone trying to do 64bit accesses
on port I/O.
Logan
On Thu, Jun 22, 2017 at 10:09 PM, Logan Gunthorpe [off-list ref] wrote:
On 6/22/2017 2:08 PM, Alan Cox wrote:
quoted
But this does not do the same thing as an ioread64 with regards to
atomicity or side effects on the device. The same is true of the other
hacks. You either have a real 64bit single read/write from MMIO space or
you don't. You can't fake it.
Yes, I know. But is it not better than having every driver that wants to use
these functions fake it themselves?
Drivers that want a non-atomic variant should include either
include/linux/io-64-nonatomic-hi-lo.h or include/linux/io-64-nonatomic-lo-hi.h
depending on what they need. Drivers that require 64-bit I/O should
probably just depend on CONFIG_64BIT and maybe use readq/writeq.
I see that there are exactly two drivers calling ioread64/iowrite64:
drivers/crypto/caam/ is architecture specific and
drivers/ntb/hw/intel/ntb_hw_intel.c already has a workaround that should
make it build on alpha.
Arnd
Drivers that want a non-atomic variant should include either
include/linux/io-64-nonatomic-hi-lo.h or include/linux/io-64-nonatomic-lo-hi.h
depending on what they need. Drivers that require 64-bit I/O should
probably just depend on CONFIG_64BIT and maybe use readq/writeq.
Ok, I will work something like that up.
We'll still need a patch similar to patch 2 (less the non-atomic
versions) seeing even CONFIG_GENERIC_IOMAP arches don't actually have a
working ioread64/iowrite64 implementation.
Thanks,
Logan
From: Richard Henderson <hidden> Date: 2017-06-22 21:20:54
On 06/22/2017 09:48 AM, Logan Gunthorpe wrote:
Alpha implements its own io operation and doesn't use the
common library. Thus to make ioread64 and iowrite64 globally
available we need to add implementations for alpha.
For this, we simply use calls that chain two 32-bit operations.
(mostly because I don't really understand the alpha architecture.)
It's not difficult to provide this interface[*]. I believe the only reason I
didn't do so from the beginning is that it wasn't used.
r~
* At least for systems other than Jensen, which cannot generate 64-bit I/O. On
the other hand, Jensen doesn't have PCI (EISA only), and so won't have any
devices that care.
Now that ioread64 and iowrite64 are always available we don't=0A=
need the ugly ifdefs to change their implementation when they=0A=
are not.=0A=
=0A=
Thanks Logan.=0A=
=0A=
Note however this is not equivalent - it changes the behaviour, since=0A=
CAAM engine on i.MX6S/SL/D/Q platforms is broken in terms of 64-bit=0A=
register endianness - see CONFIG_CRYPTO_DEV_FSL_CAAM_IMX usage in code=0A=
you are removing.=0A=
=0A=
[Yes, current code has its problems, as it does not differentiate b/w=0A=
i.MX platforms with and without the (unofficial) erratum, but this=0A=
should be fixed separately.]=0A=
=0A=
Below is the change that would keep current logic - still forcing i.MX=0A=
to write CAAM 64-bit registers in BE even if the engine is LE (yes, diff=0A=
is doing a poor job).=0A=
=0A=
Horia=0A=
=0A=
Thanks Horia.
I'm inclined to just use your patch verbatim. I can set you as author,
but no matter how I do it, I'll need your Signed-off-by.
Logan
On 23/06/17 12:51 AM, Horia Geantă wrote:
quoted hunk
On 6/22/2017 7:49 PM, Logan Gunthorpe wrote:
quoted
Now that ioread64 and iowrite64 are always available we don't
need the ugly ifdefs to change their implementation when they
are not.
Thanks Logan.
Note however this is not equivalent - it changes the behaviour, since
CAAM engine on i.MX6S/SL/D/Q platforms is broken in terms of 64-bit
register endianness - see CONFIG_CRYPTO_DEV_FSL_CAAM_IMX usage in code
you are removing.
[Yes, current code has its problems, as it does not differentiate b/w
i.MX platforms with and without the (unofficial) erratum, but this
should be fixed separately.]
Below is the change that would keep current logic - still forcing i.MX
to write CAAM 64-bit registers in BE even if the engine is LE (yes, diff
is doing a poor job).
Horia
Now that ioread64 and iowrite64 are always available we don't
need the ugly ifdefs to change their implementation when they
are not.
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
Cc: Horia Geantă <horia.geanta@nxp.com>
Cc: Dan Douglass <redacted>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: "David S. Miller" <davem@davemloft.net>
Updated patch such that behaviour does not change
from i.MX workaround point of view.
Signed-off-by: Horia Geantă <horia.geanta@nxp.com>
---
drivers/crypto/caam/regs.h | 33 ++++-----------------------------
1 file changed, 4 insertions(+), 29 deletions(-)
This is a prep patch for adding a universal iowrite64.
The patch is to prevent compiler warnings when we add iowrite64 that
would occur because there is an unnecessary volatile in this driver.
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
Cc: Jyri Sarha <redacted>
Cc: Tomi Valkeinen <redacted>
Cc: David Airlie <redacted>
Now that we can expect iowrite64 to always exist the hack is no longer
necessary so we just call iowrite64 directly.
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
Cc: Jyri Sarha <redacted>
Cc: Tomi Valkeinen <redacted>
Cc: David Airlie <redacted>
Hi Jyri,
Thanks for the ack. However, I'm reworking this patch set to use the
include/linux/io-64-nonatomic* headers which will explicitly devolve
into two 32-bit transfers. It's not clear whether this is appropriate
for the tilcdc driver as it was never setup to use 32-bit transfers
(unlike the others I had patched).
If you think it's ok, I can still patch this driver to use the
non-atomic headers. Otherwise I can leave it out. Please let me know.
Thanks,
Logan
On 26/06/17 02:55 AM, Jyri Sarha wrote:
This hardcodes the behavior of include/linux/io-64-nonatomic-hi-lo.h, which
I find rather confusing, as only about one in five drivers wants this
behavior.
I'd suggest you don't add it in lib/iomap.c at all for 32-bit architectures,
but rather use the same logic that we have for readq/writeq in
io-64-nonatomic-hi-lo.h and io-64-nonatomic-lo-hi.h, adding
{lo_hi,hi_lo}_{ioread,iowrite}{,be} to the same files, and provide
the {ioread,iowrite}{,be} macros only if they have not been defined
at that point.
Arnd
This hardcodes the behavior of include/linux/io-64-nonatomic-hi-lo.h, which
I find rather confusing, as only about one in five drivers wants this
behavior.
I'd suggest you don't add it in lib/iomap.c at all for 32-bit architectures,
but rather use the same logic that we have for readq/writeq in
io-64-nonatomic-hi-lo.h and io-64-nonatomic-lo-hi.h, adding
{lo_hi,hi_lo}_{ioread,iowrite}{,be} to the same files, and provide
the {ioread,iowrite}{,be} macros only if they have not been defined
at that point.
Thanks Arnd. Yes, I'm already reworking this patchset to do exactly that.
Logan
On Mon, Jun 26, 2017 at 6:26 PM, Logan Gunthorpe [off-list ref] wrote:
Hi Jyri,
Thanks for the ack. However, I'm reworking this patch set to use the
include/linux/io-64-nonatomic* headers which will explicitly devolve
into two 32-bit transfers. It's not clear whether this is appropriate
for the tilcdc driver as it was never setup to use 32-bit transfers
(unlike the others I had patched).
If you think it's ok, I can still patch this driver to use the
non-atomic headers. Otherwise I can leave it out. Please let me know.
You'd have to first figure out whether this device is of the lo-hi
or the hi-lo variant, or doesn't allow the I/O to be split at all.
Note that we could theoretically define ARM to use strd/ldrd
for writeq/readq, but I would expect that to be wrong with many
other devices that can use the existing io-64-nonatomic headers.
The comment in set_scanout() suggests that we actually do rely
on the write64 to be atomic, so we probably don't want to change
this driver.
Arnd