OMAP NAND driver issue with subpage reads

OMAP NAND driver issue with subpage reads

[Juha Kuikka]

Hi,

There seems to be an issue in the omap nand driver
(drivers/mtd/nand/omap2.c) concerning sub-page reads (visible when
using 16bit LP NAND,  SOFT_ECC and UBI).

Problem is caused by the omap_read_buf_pref() function using 32bit
reads from the pre-fetch engine. It takes care of the mis-aligned
length but not a mis-aligned buffer pointer. Combined with how the
nand_read_subpage() function aligns the pointer and length to NAND
width (8 or 16 bits) this can lead to situation where the handling of
the mis-aligned length in omap_read_buf_pref() causes the pointer to
not be aligned to 32bits and the data gets corrupted in the read. This
of course leds to ECC issues.

The way I see is there are several ways to fix this:

1. Change nand_read_subpage() to be more strict about alignment
2. Change omap_read_buf_pref() to handle any reads (len % 4) || (buf %
4) with omap_read_bufX() completely
3. Change omap_read_buf_pref() to use ioread16_rep() since buffer and
length are already aligned for us.

I'm leaning towards #2 because, assuming that the nand driver cannot
make assumptions of alignment, we need to be able to handle them all
anyway. The common case of reading big chunks of page data would still
be fast (since reads are always sub-page aligned) but the special case
of reading small OOB chunks would be handled gracefully.

Something like this:

diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
index 5c8978e..8a929cf 100644
--- a/drivers/mtd/nand/omap2.c
+++ b/drivers/mtd/nand/omap2.c
@@ -334,13 +334,12 @@ static void omap_read_buf_pref(struct mtd_info
*mtd, u_char *buf, int len)
        u32 *p = (u32 *)buf;

        /* take care of subpage reads */
-       if (len % 4) {
+       if (len % 4 || (unsigned long) buf % 4) {
                if (info->nand.options & NAND_BUSWIDTH_16)
                        omap_read_buf16(mtd, buf, len % 4);
                else
                        omap_read_buf8(mtd, buf, len % 4);
-               p = (u32 *) (buf + len % 4);
-               len -= len % 4;
+               return;
        }

        /* configure and start prefetch transfer */

Comments?

 - Juha

Re: OMAP NAND driver issue with subpage reads

[Tony Lindgren]

* Juha Kuikka <juha.kuikka@gmail.com> [121023 13:40]:
> Hi,
> 
> There seems to be an issue in the omap nand driver
> (drivers/mtd/nand/omap2.c) concerning sub-page reads (visible when
> using 16bit LP NAND,  SOFT_ECC and UBI).
> 
> Problem is caused by the omap_read_buf_pref() function using 32bit
> reads from the pre-fetch engine. It takes care of the mis-aligned
> length but not a mis-aligned buffer pointer. Combined with how the
> nand_read_subpage() function aligns the pointer and length to NAND
> width (8 or 16 bits) this can lead to situation where the handling of
> the mis-aligned length in omap_read_buf_pref() causes the pointer to
> not be aligned to 32bits and the data gets corrupted in the read. This
> of course leds to ECC issues.
> 
> The way I see is there are several ways to fix this:
> 
> 1. Change nand_read_subpage() to be more strict about alignment
> 2. Change omap_read_buf_pref() to handle any reads (len % 4) || (buf %
> 4) with omap_read_bufX() completely
> 3. Change omap_read_buf_pref() to use ioread16_rep() since buffer and
> length are already aligned for us.
> 
> I'm leaning towards #2 because, assuming that the nand driver cannot
> make assumptions of alignment, we need to be able to handle them all
> anyway. The common case of reading big chunks of page data would still
> be fast (since reads are always sub-page aligned) but the special case
> of reading small OOB chunks would be handled gracefully.
> 
> Something like this:
> 
> diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
> index 5c8978e..8a929cf 100644
> --- a/drivers/mtd/nand/omap2.c
> +++ b/drivers/mtd/nand/omap2.c
> @@ -334,13 +334,12 @@ static void omap_read_buf_pref(struct mtd_info
> *mtd, u_char *buf, int len)
>         u32 *p = (u32 *)buf;
> 
>         /* take care of subpage reads */
> -       if (len % 4) {
> +       if (len % 4 || (unsigned long) buf % 4) {
>                 if (info->nand.options & NAND_BUSWIDTH_16)
>                         omap_read_buf16(mtd, buf, len % 4);
>                 else
>                         omap_read_buf8(mtd, buf, len % 4);
> -               p = (u32 *) (buf + len % 4);
> -               len -= len % 4;
> +               return;
>         }
> 
>         /* configure and start prefetch transfer */
> 
> Comments?

Seems OK to me, but please cc the MTD list as this is more in the
MTD driver area.

Regards,

Tony