linux-renesas-soc.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] memory: renesas-rpc-if: Avoid unaligned bus access for HyperFlash
@ 2021-09-22 18:48 Andrew Gabbasov
  2021-09-24 11:12 ` Krzysztof Kozlowski
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Andrew Gabbasov @ 2021-09-22 18:48 UTC (permalink / raw)
  To: linux-renesas-soc, linux-kernel, Krzysztof Kozlowski,
	Sergei Shtylyov, Geert Uytterhoeven

HyperFlash devices in Renesas SoCs use 2-bytes addressing, according
to HW manual paragraph 62.3.3 (which officially describes Serial Flash
access, but seems to be applicable to HyperFlash too). And 1-byte bus
read operations to 2-bytes unaligned addresses in external address space
read mode work incorrectly (returns the other byte from the same word).

Function memcpy_fromio(), used by the driver to read data from the bus,
in ARM64 architecture (to which Renesas cores belong) uses 8-bytes
bus accesses for appropriate aligned addresses, and 1-bytes accesses
for other addresses. This results in incorrect data read from HyperFlash
in unaligned cases.

This issue can be reproduced using something like the following commands
(where mtd1 is a parition on Hyperflash storage, defined properly
in a device tree):

[Correct fragment, read from Hyperflash]

    root@rcar-gen3:~# dd if=/dev/mtd1 of=/tmp/zz bs=32 count=1
    1+0 records in
    1+0 records out
    root@rcar-gen3:~# hexdump -C /tmp/zz
    00000000  f4 03 00 aa f5 03 01 aa  f6 03 02 aa f7 03 03 aa  |................|
    00000010  00 00 80 d2 40 20 18 d5  00 06 81 d2 a0 18 a6 f2  |....@ ..........|
    00000020

[Incorrect read of the same fragment: see the difference at offsets 8-11]

    root@rcar-gen3:~# dd if=/dev/mtd1 of=/tmp/zz bs=12 count=1
    1+0 records in
    1+0 records out
    root@rcar-gen3:~# hexdump -C /tmp/zz
    00000000  f4 03 00 aa f5 03 01 aa  03 03 aa aa              |............|
    0000000c

Fix this issue by creating a local replacement of the copying function,
that performs only properly aligned bus accesses, and is used for reading
from HyperFlash.

Fixes: ca7d8b980b67f ("memory: add Renesas RPC-IF driver")
Signed-off-by: Andrew Gabbasov <andrew_gabbasov@mentor.com>
---
 drivers/memory/renesas-rpc-if.c | 47 ++++++++++++++++++++++++++++++++-
 1 file changed, 46 insertions(+), 1 deletion(-)

diff --git a/drivers/memory/renesas-rpc-if.c b/drivers/memory/renesas-rpc-if.c
index 45eed659b0c6..374c92e57538 100644
--- a/drivers/memory/renesas-rpc-if.c
+++ b/drivers/memory/renesas-rpc-if.c
@@ -502,6 +502,48 @@ int rpcif_manual_xfer(struct rpcif *rpc)
 }
 EXPORT_SYMBOL(rpcif_manual_xfer);
 
+static void memcpy_fromio_readw(void *to,
+				const void __iomem *from,
+				size_t count)
+{
+	const int maxw = (IS_ENABLED(CONFIG_64BIT)) ? 8 : 4;
+	u8 buf[2];
+
+	if (count && ((unsigned long)from & 1)) {
+		*(u16 *)buf = __raw_readw((void __iomem *)((unsigned long)from & ~1));
+		*(u8 *)to = buf[1];
+		from++;
+		to++;
+		count--;
+	}
+	while (count >= 2 && !IS_ALIGNED((unsigned long)from, maxw)) {
+		*(u16 *)to = __raw_readw(from);
+		from += 2;
+		to += 2;
+		count -= 2;
+	}
+	while (count >= maxw) {
+#ifdef CONFIG_64BIT
+		*(u64 *)to = __raw_readq(from);
+#else
+		*(u32 *)to = __raw_readl(from);
+#endif
+		from += maxw;
+		to += maxw;
+		count -= maxw;
+	}
+	while (count >= 2) {
+		*(u16 *)to = __raw_readw(from);
+		from += 2;
+		to += 2;
+		count -= 2;
+	}
+	if (count) {
+		*(u16 *)buf = __raw_readw(from);
+		*(u8 *)to = buf[0];
+	}
+}
+
 ssize_t rpcif_dirmap_read(struct rpcif *rpc, u64 offs, size_t len, void *buf)
 {
 	loff_t from = offs & (RPCIF_DIRMAP_SIZE - 1);
@@ -523,7 +565,10 @@ ssize_t rpcif_dirmap_read(struct rpcif *rpc, u64 offs, size_t len, void *buf)
 	regmap_write(rpc->regmap, RPCIF_DRDMCR, rpc->dummy);
 	regmap_write(rpc->regmap, RPCIF_DRDRENR, rpc->ddr);
 
-	memcpy_fromio(buf, rpc->dirmap + from, len);
+	if (rpc->bus_size == 2)
+		memcpy_fromio_readw(buf, rpc->dirmap + from, len);
+	else
+		memcpy_fromio(buf, rpc->dirmap + from, len);
 
 	pm_runtime_put(rpc->dev);
 
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] memory: renesas-rpc-if: Avoid unaligned bus access for HyperFlash
  2021-09-22 18:48 [PATCH] memory: renesas-rpc-if: Avoid unaligned bus access for HyperFlash Andrew Gabbasov
@ 2021-09-24 11:12 ` Krzysztof Kozlowski
  2021-09-24 12:34   ` Andrew Gabbasov
  2021-09-25  9:54 ` Wolfram Sang
  2021-09-28 10:35 ` Krzysztof Kozlowski
  2 siblings, 1 reply; 8+ messages in thread
From: Krzysztof Kozlowski @ 2021-09-24 11:12 UTC (permalink / raw)
  To: Andrew Gabbasov, linux-renesas-soc, linux-kernel,
	Sergei Shtylyov, Geert Uytterhoeven

On 22/09/2021 20:48, Andrew Gabbasov wrote:
> HyperFlash devices in Renesas SoCs use 2-bytes addressing, according
> to HW manual paragraph 62.3.3 (which officially describes Serial Flash
> access, but seems to be applicable to HyperFlash too). And 1-byte bus
> read operations to 2-bytes unaligned addresses in external address space
> read mode work incorrectly (returns the other byte from the same word).
> 
> Function memcpy_fromio(), used by the driver to read data from the bus,
> in ARM64 architecture (to which Renesas cores belong) uses 8-bytes
> bus accesses for appropriate aligned addresses, and 1-bytes accesses
> for other addresses. This results in incorrect data read from HyperFlash
> in unaligned cases.
> 
> This issue can be reproduced using something like the following commands
> (where mtd1 is a parition on Hyperflash storage, defined properly
> in a device tree):
> 
> [Correct fragment, read from Hyperflash]
> 
>     root@rcar-gen3:~# dd if=/dev/mtd1 of=/tmp/zz bs=32 count=1
>     1+0 records in
>     1+0 records out
>     root@rcar-gen3:~# hexdump -C /tmp/zz
>     00000000  f4 03 00 aa f5 03 01 aa  f6 03 02 aa f7 03 03 aa  |................|
>     00000010  00 00 80 d2 40 20 18 d5  00 06 81 d2 a0 18 a6 f2  |....@ ..........|
>     00000020
> 
> [Incorrect read of the same fragment: see the difference at offsets 8-11]
> 
>     root@rcar-gen3:~# dd if=/dev/mtd1 of=/tmp/zz bs=12 count=1
>     1+0 records in
>     1+0 records out
>     root@rcar-gen3:~# hexdump -C /tmp/zz
>     00000000  f4 03 00 aa f5 03 01 aa  03 03 aa aa              |............|
>     0000000c
> 
> Fix this issue by creating a local replacement of the copying function,
> that performs only properly aligned bus accesses, and is used for reading
> from HyperFlash.
> 
> Fixes: ca7d8b980b67f ("memory: add Renesas RPC-IF driver")
> Signed-off-by: Andrew Gabbasov <andrew_gabbasov@mentor.com>
> ---
>  drivers/memory/renesas-rpc-if.c | 47 ++++++++++++++++++++++++++++++++-
>  1 file changed, 46 insertions(+), 1 deletion(-)
> 

Thanks for the patch.

Please rebase and test on a recent Linux kernel. This looks like work on
something slightly older or stable kernel, since you Cc not the address
from maintainers.

The patch came slightly after Wolfram's and I wonder whether you hit
similar issue:
https://lore.kernel.org/lkml/20210922091007.5516-1-wsa+renesas@sang-engineering.com/


Best regards,
Krzysztof

^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: [PATCH] memory: renesas-rpc-if: Avoid unaligned bus access for HyperFlash
  2021-09-24 11:12 ` Krzysztof Kozlowski
@ 2021-09-24 12:34   ` Andrew Gabbasov
  2021-09-24 12:37     ` Wolfram Sang
  0 siblings, 1 reply; 8+ messages in thread
From: Andrew Gabbasov @ 2021-09-24 12:34 UTC (permalink / raw)
  To: 'Krzysztof Kozlowski',
	linux-renesas-soc, linux-kernel, Sergei Shtylyov,
	Geert Uytterhoeven, Wolfram Sang

Hello Krzysztof,

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
> Sent: Friday, September 24, 2021 2:13 PM
> To: Gabbasov, Andrew <Andrew_Gabbasov@mentor.com>; linux-renesas-soc@vger.kernel.org; linux-
> kernel@vger.kernel.org; Sergei Shtylyov <sergei.shtylyov@gmail.com>; Geert Uytterhoeven <geert+renesas@glider.be>
> Subject: Re: [PATCH] memory: renesas-rpc-if: Avoid unaligned bus access for HyperFlash
> 
> On 22/09/2021 20:48, Andrew Gabbasov wrote:
> > HyperFlash devices in Renesas SoCs use 2-bytes addressing, according
> > to HW manual paragraph 62.3.3 (which officially describes Serial Flash
> > access, but seems to be applicable to HyperFlash too). And 1-byte bus
> > read operations to 2-bytes unaligned addresses in external address space
> > read mode work incorrectly (returns the other byte from the same word).
> >
> > Function memcpy_fromio(), used by the driver to read data from the bus,
> > in ARM64 architecture (to which Renesas cores belong) uses 8-bytes
> > bus accesses for appropriate aligned addresses, and 1-bytes accesses
> > for other addresses. This results in incorrect data read from HyperFlash
> > in unaligned cases.
> >
> > This issue can be reproduced using something like the following commands
> > (where mtd1 is a parition on Hyperflash storage, defined properly
> > in a device tree):
> >
> > [Correct fragment, read from Hyperflash]
> >
> >     root@rcar-gen3:~# dd if=/dev/mtd1 of=/tmp/zz bs=32 count=1
> >     1+0 records in
> >     1+0 records out
> >     root@rcar-gen3:~# hexdump -C /tmp/zz
> >     00000000  f4 03 00 aa f5 03 01 aa  f6 03 02 aa f7 03 03 aa  |................|
> >     00000010  00 00 80 d2 40 20 18 d5  00 06 81 d2 a0 18 a6 f2  |....@ ..........|
> >     00000020
> >
> > [Incorrect read of the same fragment: see the difference at offsets 8-11]
> >
> >     root@rcar-gen3:~# dd if=/dev/mtd1 of=/tmp/zz bs=12 count=1
> >     1+0 records in
> >     1+0 records out
> >     root@rcar-gen3:~# hexdump -C /tmp/zz
> >     00000000  f4 03 00 aa f5 03 01 aa  03 03 aa aa              |............|
> >     0000000c
> >
> > Fix this issue by creating a local replacement of the copying function,
> > that performs only properly aligned bus accesses, and is used for reading
> > from HyperFlash.
> >
> > Fixes: ca7d8b980b67f ("memory: add Renesas RPC-IF driver")
> > Signed-off-by: Andrew Gabbasov <andrew_gabbasov@mentor.com>
> > ---
> >  drivers/memory/renesas-rpc-if.c | 47 ++++++++++++++++++++++++++++++++-
> >  1 file changed, 46 insertions(+), 1 deletion(-)
> >
> 
> Thanks for the patch.
> 
> Please rebase and test on a recent Linux kernel. This looks like work on
> something slightly older or stable kernel, since you Cc not the address
> from maintainers.

The patch is already against the recent kernel versions.
Sorry for using wrong address, I have probably taken it from some
older mailing lists.

> The patch came slightly after Wolfram's and I wonder whether you hit
> similar issue:
> https://lore.kernel.org/lkml/20210922091007.5516-1-wsa+renesas@sang-engineering.com/

If I correctly understand, the underlying issue looks similar (improperly aligned
memory/register accesses), but the affected areas are different, even non-intersecting:
Wolfram fixes register access, affecting manual mode reads/writes, having problems
with QSPI devices, while my fix is related to external address space reads (mapped
memory access) with Hyperflash devices.

Thanks.

Best regards,
Andrew


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] memory: renesas-rpc-if: Avoid unaligned bus access for HyperFlash
  2021-09-24 12:34   ` Andrew Gabbasov
@ 2021-09-24 12:37     ` Wolfram Sang
  0 siblings, 0 replies; 8+ messages in thread
From: Wolfram Sang @ 2021-09-24 12:37 UTC (permalink / raw)
  To: Andrew Gabbasov
  Cc: 'Krzysztof Kozlowski',
	linux-renesas-soc, linux-kernel, Sergei Shtylyov,
	Geert Uytterhoeven

[-- Attachment #1: Type: text/plain, Size: 394 bytes --]


> If I correctly understand, the underlying issue looks similar (improperly aligned
> memory/register accesses), but the affected areas are different, even non-intersecting:
> Wolfram fixes register access, affecting manual mode reads/writes, having problems
> with QSPI devices, while my fix is related to external address space reads (mapped
> memory access) with Hyperflash devices.

Ack.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] memory: renesas-rpc-if: Avoid unaligned bus access for HyperFlash
  2021-09-22 18:48 [PATCH] memory: renesas-rpc-if: Avoid unaligned bus access for HyperFlash Andrew Gabbasov
  2021-09-24 11:12 ` Krzysztof Kozlowski
@ 2021-09-25  9:54 ` Wolfram Sang
  2021-09-25 10:49   ` Andrew Gabbasov
  2021-09-28 10:35 ` Krzysztof Kozlowski
  2 siblings, 1 reply; 8+ messages in thread
From: Wolfram Sang @ 2021-09-25  9:54 UTC (permalink / raw)
  To: Andrew Gabbasov
  Cc: linux-renesas-soc, linux-kernel, Krzysztof Kozlowski,
	Sergei Shtylyov, Geert Uytterhoeven

[-- Attachment #1: Type: text/plain, Size: 834 bytes --]

Hi Andrew,

thanks for this patch!

> +	const int maxw = (IS_ENABLED(CONFIG_64BIT)) ? 8 : 4;
> +	u8 buf[2];

I could imagine the code becomes more readable if we make use of
something like:

	unsigned long from_ul = from;

and then use it throughout the function?

> +#ifdef CONFIG_64BIT
> +		*(u64 *)to = __raw_readq(from);
> +#else
> +		*(u32 *)to = __raw_readl(from);
> +#endif

To keep the ifdeffery minimal:

	if (maxw == 8)
		*(u64 *)to = __raw_readq(from);
	else
 		*(u32 *)to = __raw_readl(from);

and let the compiler do its job.

I wondered if this could be a helper function somewhere instead of open
coded in this driver. However, I did not find any similar code in the
kernel yet, so it might be too early to make this a helper. Have you
looked for similar code? I might have just missed it.

Happy hacking,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: [PATCH] memory: renesas-rpc-if: Avoid unaligned bus access for HyperFlash
  2021-09-25  9:54 ` Wolfram Sang
@ 2021-09-25 10:49   ` Andrew Gabbasov
  2021-09-27  9:25     ` 'Wolfram Sang'
  0 siblings, 1 reply; 8+ messages in thread
From: Andrew Gabbasov @ 2021-09-25 10:49 UTC (permalink / raw)
  To: 'Wolfram Sang'
  Cc: linux-renesas-soc, linux-kernel, Krzysztof Kozlowski,
	Sergei Shtylyov, Geert Uytterhoeven

Hello Wolfram,

Thank you for your comments!

> -----Original Message-----
> From: Wolfram Sang <wsa@kernel.org>
> Sent: Saturday, September 25, 2021 12:55 PM
> To: Gabbasov, Andrew <Andrew_Gabbasov@mentor.com>
> Cc: linux-renesas-soc@vger.kernel.org; linux-kernel@vger.kernel.org; Krzysztof Kozlowski <krzk@kernel.org>;
> Sergei Shtylyov <sergei.shtylyov@gmail.com>; Geert Uytterhoeven <geert+renesas@glider.be>
> Subject: Re: [PATCH] memory: renesas-rpc-if: Avoid unaligned bus access for HyperFlash
> 
> Hi Andrew,
> 
> thanks for this patch!
> 
> > +	const int maxw = (IS_ENABLED(CONFIG_64BIT)) ? 8 : 4;
> > +	u8 buf[2];
> 
> I could imagine the code becomes more readable if we make use of
> something like:
> 
> 	unsigned long from_ul = from;
> 
> and then use it throughout the function?

It could make sense if "from" would not change along the function.
But in case of this function "from" is changed between usages, so
it would be necessary to synchronize "from_ul" with "from" again
(make "from_ul++" together with "from++", or re-assign it)
before we could use it again correctly.

> > +#ifdef CONFIG_64BIT
> > +		*(u64 *)to = __raw_readq(from);
> > +#else
> > +		*(u32 *)to = __raw_readl(from);
> > +#endif
> 
> To keep the ifdeffery minimal:
> 
> 	if (maxw == 8)
> 		*(u64 *)to = __raw_readq(from);
> 	else
>  		*(u32 *)to = __raw_readl(from);
> 
> and let the compiler do its job.

I don't like #ifdef's inside the function body too, but the problem is that
"__raw_readq" is defined in arch/arm64/include/asm/io.h unconditionally,
but in include/asm-generic/io.h under "#ifdef CONFIG_64BIT" only.
This file drivers/memory/renesas-rpc-if.c can be compiled not only
for renesas/arm64 architecture, but for CONFIG_COMPILE_TEST case too.
It means, that the file could be compiled for some other architecture,
that does not have CONFIG_64BIT, and in this case "__raw_readq" function
will be undefined. So, we need to hide it under "#ifdef CONFIG_64BIT" here.

> I wondered if this could be a helper function somewhere instead of open
> coded in this driver. However, I did not find any similar code in the
> kernel yet, so it might be too early to make this a helper. Have you
> looked for similar code? I might have just missed it.

I looked through the whole kernel code too, and unfortunately didn't find
any similar code that could be re-used or had some parts, extractable as
a common helper. That's why I ended up with a local custom function,
at least so far, until it could be found useful by somebody else ;)

Thanks!

Best regards,
Andrew 



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] memory: renesas-rpc-if: Avoid unaligned bus access for HyperFlash
  2021-09-25 10:49   ` Andrew Gabbasov
@ 2021-09-27  9:25     ` 'Wolfram Sang'
  0 siblings, 0 replies; 8+ messages in thread
From: 'Wolfram Sang' @ 2021-09-27  9:25 UTC (permalink / raw)
  To: Andrew Gabbasov
  Cc: linux-renesas-soc, linux-kernel, Krzysztof Kozlowski,
	Sergei Shtylyov, Geert Uytterhoeven

[-- Attachment #1: Type: text/plain, Size: 1075 bytes --]

Hi Andrew,

> It could make sense if "from" would not change along the function.

Yes. And using a pointer to unsigned long would neither make the code a
lot more readable, I am afraid.

> I don't like #ifdef's inside the function body too, but the problem is that
> "__raw_readq" is defined in arch/arm64/include/asm/io.h unconditionally,
> but in include/asm-generic/io.h under "#ifdef CONFIG_64BIT" only.

I see. Bad luck then :(

> I looked through the whole kernel code too, and unfortunately didn't find
> any similar code that could be re-used or had some parts, extractable as
> a common helper. That's why I ended up with a local custom function,
> at least so far, until it could be found useful by somebody else ;)

Thanks for confirming you also had a look.

Okay, the function is not exactly pretty, but my comments have been
addressed and it fixes a serious bug, so:

Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Still, if someone has ideas how to make the function more readable, we
could incrementally improve it.

Kind regards,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] memory: renesas-rpc-if: Avoid unaligned bus access for HyperFlash
  2021-09-22 18:48 [PATCH] memory: renesas-rpc-if: Avoid unaligned bus access for HyperFlash Andrew Gabbasov
  2021-09-24 11:12 ` Krzysztof Kozlowski
  2021-09-25  9:54 ` Wolfram Sang
@ 2021-09-28 10:35 ` Krzysztof Kozlowski
  2 siblings, 0 replies; 8+ messages in thread
From: Krzysztof Kozlowski @ 2021-09-28 10:35 UTC (permalink / raw)
  To: Krzysztof Kozlowski, Andrew Gabbasov, linux-renesas-soc,
	Sergei Shtylyov, Geert Uytterhoeven, linux-kernel
  Cc: Krzysztof Kozlowski

On Wed, 22 Sep 2021 13:48:30 -0500, Andrew Gabbasov wrote:
> HyperFlash devices in Renesas SoCs use 2-bytes addressing, according
> to HW manual paragraph 62.3.3 (which officially describes Serial Flash
> access, but seems to be applicable to HyperFlash too). And 1-byte bus
> read operations to 2-bytes unaligned addresses in external address space
> read mode work incorrectly (returns the other byte from the same word).
> 
> Function memcpy_fromio(), used by the driver to read data from the bus,
> in ARM64 architecture (to which Renesas cores belong) uses 8-bytes
> bus accesses for appropriate aligned addresses, and 1-bytes accesses
> for other addresses. This results in incorrect data read from HyperFlash
> in unaligned cases.
> 
> [...]

Applied, thanks!

[1/1] memory: renesas-rpc-if: Avoid unaligned bus access for HyperFlash
      commit: 1869023e24c0de73a160a424dac4621cefd628ae

Best regards,
-- 
Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2021-09-28 10:35 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-22 18:48 [PATCH] memory: renesas-rpc-if: Avoid unaligned bus access for HyperFlash Andrew Gabbasov
2021-09-24 11:12 ` Krzysztof Kozlowski
2021-09-24 12:34   ` Andrew Gabbasov
2021-09-24 12:37     ` Wolfram Sang
2021-09-25  9:54 ` Wolfram Sang
2021-09-25 10:49   ` Andrew Gabbasov
2021-09-27  9:25     ` 'Wolfram Sang'
2021-09-28 10:35 ` Krzysztof Kozlowski

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).