* [PATCH] mtd: mtk: avoid warning in mtk_ecc_encode
@ 2016-09-30 16:33 Arnd Bergmann
2016-09-30 16:51 ` Boris Brezillon
0 siblings, 1 reply; 4+ messages in thread
From: Arnd Bergmann @ 2016-09-30 16:33 UTC (permalink / raw)
To: linux-arm-kernel
When building with -Wmaybe-uninitialized, gcc produces a silly false positive
warning for the mtk_ecc_encode function:
drivers/mtd/nand/mtk_ecc.c: In function 'mtk_ecc_encode':
drivers/mtd/nand/mtk_ecc.c:402:15: error: 'val' may be used uninitialized in this function [-Werror=maybe-uninitialized]
The function for some reason contains a double byte swap on big-endian
builds to get the OOB data into the correct order again, and is written
in a slightly confusing way.
Using a simple memcpy32_fromio() to read the data simplifies it a lot
so it becomes more readable and produces no warning. However, the
output might not have 32-bit alignment, so we have to use another
memcpy to avoid taking alignment faults or writing beyond the end
of the array.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
drivers/mtd/nand/mtk_ecc.c | 18 ++++++++----------
1 file changed, 8 insertions(+), 10 deletions(-)
diff --git a/drivers/mtd/nand/mtk_ecc.c b/drivers/mtd/nand/mtk_ecc.c
index d54f666417e1..237c83124a7d 100644
--- a/drivers/mtd/nand/mtk_ecc.c
+++ b/drivers/mtd/nand/mtk_ecc.c
@@ -366,9 +366,9 @@ int mtk_ecc_encode(struct mtk_ecc *ecc, struct mtk_ecc_config *config,
u8 *data, u32 bytes)
{
dma_addr_t addr;
- u8 *p;
- u32 len, i, val;
- int ret = 0;
+ u32 len;
+ u8 eccdata[112];
+ int ret;
addr = dma_map_single(ecc->dev, data, bytes, DMA_TO_DEVICE);
ret = dma_mapping_error(ecc->dev, addr);
@@ -393,14 +393,12 @@ int mtk_ecc_encode(struct mtk_ecc *ecc, struct mtk_ecc_config *config,
/* Program ECC bytes to OOB: per sector oob = FDM + ECC + SPARE */
len = (config->strength * ECC_PARITY_BITS + 7) >> 3;
- p = data + bytes;
- /* write the parity bytes generated by the ECC back to the OOB region */
- for (i = 0; i < len; i++) {
- if ((i % 4) == 0)
- val = readl(ecc->regs + ECC_ENCPAR(i / 4));
- p[i] = (val >> ((i % 4) * 8)) & 0xff;
- }
+ /* write the parity bytes generated by the ECC back to temp buffer */
+ __ioread32_copy(eccdata, ecc->regs + ECC_ENCPAR(0), round_up(len, 4));
+
+ /* copy into possibly unaligned OOB region with actual length */
+ memcpy(data + bytes, eccdata, len);
timeout:
dma_unmap_single(ecc->dev, addr, bytes, DMA_TO_DEVICE);
--
2.9.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH] mtd: mtk: avoid warning in mtk_ecc_encode
2016-09-30 16:33 [PATCH] mtd: mtk: avoid warning in mtk_ecc_encode Arnd Bergmann
@ 2016-09-30 16:51 ` Boris Brezillon
2016-09-30 17:25 ` Arnd Bergmann
0 siblings, 1 reply; 4+ messages in thread
From: Boris Brezillon @ 2016-09-30 16:51 UTC (permalink / raw)
To: linux-arm-kernel
Hi Arnd,
On Fri, 30 Sep 2016 18:33:02 +0200
Arnd Bergmann <arnd@arndb.de> wrote:
> When building with -Wmaybe-uninitialized, gcc produces a silly false positive
> warning for the mtk_ecc_encode function:
>
> drivers/mtd/nand/mtk_ecc.c: In function 'mtk_ecc_encode':
> drivers/mtd/nand/mtk_ecc.c:402:15: error: 'val' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>
> The function for some reason contains a double byte swap on big-endian
> builds to get the OOB data into the correct order again, and is written
> in a slightly confusing way.
>
> Using a simple memcpy32_fromio() to read the data simplifies it a lot
> so it becomes more readable and produces no warning. However, the
> output might not have 32-bit alignment, so we have to use another
> memcpy to avoid taking alignment faults or writing beyond the end
> of the array.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> drivers/mtd/nand/mtk_ecc.c | 18 ++++++++----------
> 1 file changed, 8 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/mtd/nand/mtk_ecc.c b/drivers/mtd/nand/mtk_ecc.c
> index d54f666417e1..237c83124a7d 100644
> --- a/drivers/mtd/nand/mtk_ecc.c
> +++ b/drivers/mtd/nand/mtk_ecc.c
> @@ -366,9 +366,9 @@ int mtk_ecc_encode(struct mtk_ecc *ecc, struct mtk_ecc_config *config,
> u8 *data, u32 bytes)
> {
> dma_addr_t addr;
> - u8 *p;
> - u32 len, i, val;
> - int ret = 0;
> + u32 len;
> + u8 eccdata[112];
> + int ret;
>
> addr = dma_map_single(ecc->dev, data, bytes, DMA_TO_DEVICE);
> ret = dma_mapping_error(ecc->dev, addr);
> @@ -393,14 +393,12 @@ int mtk_ecc_encode(struct mtk_ecc *ecc, struct mtk_ecc_config *config,
>
> /* Program ECC bytes to OOB: per sector oob = FDM + ECC + SPARE */
> len = (config->strength * ECC_PARITY_BITS + 7) >> 3;
> - p = data + bytes;
>
> - /* write the parity bytes generated by the ECC back to the OOB region */
> - for (i = 0; i < len; i++) {
> - if ((i % 4) == 0)
> - val = readl(ecc->regs + ECC_ENCPAR(i / 4));
> - p[i] = (val >> ((i % 4) * 8)) & 0xff;
> - }
> + /* write the parity bytes generated by the ECC back to temp buffer */
> + __ioread32_copy(eccdata, ecc->regs + ECC_ENCPAR(0), round_up(len, 4));
> +
> + /* copy into possibly unaligned OOB region with actual length */
> + memcpy(data + bytes, eccdata, len);
Is it better than
for (i = 0; i < len; i += 4) {
u32 val = __raw_readl(ecc->regs + ECC_ENCPAR(i / 4));
memcpy(data + bytes + i, &val, min(len, 4));
}
I'm probably missing something, but what's the point of creating a
temporary buffer of 112 bytes on the stack since you'll have to copy
this data to the oob buffer at some point?
> timeout:
>
> dma_unmap_single(ecc->dev, addr, bytes, DMA_TO_DEVICE);
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH] mtd: mtk: avoid warning in mtk_ecc_encode
2016-09-30 16:51 ` Boris Brezillon
@ 2016-09-30 17:25 ` Arnd Bergmann
2016-10-01 9:25 ` Boris Brezillon
0 siblings, 1 reply; 4+ messages in thread
From: Arnd Bergmann @ 2016-09-30 17:25 UTC (permalink / raw)
To: linux-arm-kernel
On Friday 30 September 2016, Boris Brezillon wrote:
> > + /* copy into possibly unaligned OOB region with actual length */
> > + memcpy(data + bytes, eccdata, len);
>
> Is it better than
>
> for (i = 0; i < len; i += 4) {
> u32 val = __raw_readl(ecc->regs + ECC_ENCPAR(i / 4));
>
> memcpy(data + bytes + i, &val, min(len, 4));
> }
>
> I'm probably missing something, but what's the point of creating a
> temporary buffer of 112 bytes on the stack since you'll have to copy
> this data to the oob buffer at some point?
I tried something like that first, but wasn't too happy with it for
a number of small reasons:
- __raw_readl in a driver is not usually the right API, __memcpy32_from_io
uses it internally, but it's better for a driver not to rely on that,
in case we need some barriers (which we may in factt need for other drivers).
- the min(len,4) expression is incorrect, fixing that makes it more complicated
again
- I didn't like to call memcpy() multiple times, as that might get turned
into an external function call (the compiler is free to optimize small
memcpy calls or not).
I agree that he 112 byte buffer isn't ideal either, it just seemed to
be the lesser annoyance.
Arnd
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH] mtd: mtk: avoid warning in mtk_ecc_encode
2016-09-30 17:25 ` Arnd Bergmann
@ 2016-10-01 9:25 ` Boris Brezillon
0 siblings, 0 replies; 4+ messages in thread
From: Boris Brezillon @ 2016-10-01 9:25 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, 30 Sep 2016 19:25:17 +0200
Arnd Bergmann <arnd@arndb.de> wrote:
> On Friday 30 September 2016, Boris Brezillon wrote:
> > > + /* copy into possibly unaligned OOB region with actual length */
> > > + memcpy(data + bytes, eccdata, len);
> >
> > Is it better than
> >
> > for (i = 0; i < len; i += 4) {
> > u32 val = __raw_readl(ecc->regs + ECC_ENCPAR(i / 4));
> >
> > memcpy(data + bytes + i, &val, min(len, 4));
> > }
> >
> > I'm probably missing something, but what's the point of creating a
> > temporary buffer of 112 bytes on the stack since you'll have to copy
> > this data to the oob buffer at some point?
>
>
> I tried something like that first, but wasn't too happy with it for
> a number of small reasons:
>
> - __raw_readl in a driver is not usually the right API, __memcpy32_from_io
> uses it internally, but it's better for a driver not to rely on that,
> in case we need some barriers (which we may in factt need for other drivers).
I agree, even though calling something prefixed with __ (in this case,
__ioread32_copy()) sounds like a bad thing too :).
>
> - the min(len,4) expression is incorrect, fixing that makes it more complicated
> again
Sorry, it's min(len - i, 4), which is not that complicated :P.
>
> - I didn't like to call memcpy() multiple times, as that might get turned
> into an external function call (the compiler is free to optimize small
> memcpy calls or not).
Okay.
>
> I agree that he 112 byte buffer isn't ideal either, it just seemed to
> be the lesser annoyance.
How about we keep your approach, but put the buffer in the mtk_ecc
struct?
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-10-01 9:25 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-09-30 16:33 [PATCH] mtd: mtk: avoid warning in mtk_ecc_encode Arnd Bergmann
2016-09-30 16:51 ` Boris Brezillon
2016-09-30 17:25 ` Arnd Bergmann
2016-10-01 9:25 ` Boris Brezillon
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).