On Tue, Feb 05, 2019 at 12:47:47PM +0200, Jarkko Sakkinen wrote:
On Tue, Feb 05, 2019 at 12:44:06PM +0200, Jarkko Sakkinen wrote:
On Mon, Feb 04, 2019 at 12:17:43PM +0000, David Laight wrote:
From: Jarkko Sakkinen
Sent: 01 February 2019 11:20 The current approach to read first 6 bytes from the response and then tail of the response, can cause the 2nd memcpy_fromio() to do an unaligned read (e.g. read 32-bit word from address aligned to a 16-bits), depending on how memcpy_fromio() is implemented. If this happens, the read will fail and the memory controller will fill the read with 1's.
To my mind memcpy_to/fromio() should only be used on IO addresses that are adequately like memory, and should be implemented in a way that that won't generate invalid bus cycles. Also memcpy_fromio() should also be allowed to do 'aligned' accesses that go beyond the ends of the required memory area.
...
- memcpy_fromio(buf, priv->rsp, 6);
- memcpy_fromio(buf, priv->rsp, 8); expected = be32_to_cpup((__be32 *) &buf[2]);
- if (expected > count || expected < 6)
- if (expected > count || expected < 8) return -EIO;
- memcpy_fromio(&buf[6], &priv->rsp[6], expected - 6);
- memcpy_fromio(&buf[8], &priv->rsp[8], expected - 8);
Why not just use readl() or readq() ?
Bound to generate better code.
For the first read can be done. The second read is of variable length.
Neither can be done to the first one, because readq() does le64_to_cpu(). Shoud not do any conversions, only raw read. So I'll just stick it to what we have.
ATM tmp_crb is only used in LE architectures (x86 and ARM64), but I still hate to have hidden extra byte order conversions, even if they get compiled as nil ops. It is more wrong than to use memcpy_fromio().
/Jarkko