| Message ID | 20210922184830.29147-1-andrew_gabbasov@mentor.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | memory: renesas-rpc-if: Avoid unaligned bus access for HyperFlash | expand |
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
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
> 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.
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
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
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
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,
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);
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(-)