[PATCH 0/2] Fix index regression in nand_read_subpage

Ron

author

2014-03-15 17:39 UTC #0 lore raw

cover Fix index regression in nand_read_subpage

Hi,

I stumbled across this while reviewing changes to the code by eye, and I'm a bit surprised that it seems to have gone unnoticed so far but unless I'm missing something obvious, the "clean up" changes in 7351d3a5dbf4 really did alter the previous behaviour, which I think was correct (or if it wasn't then I'm surprised _it_ went unnoticed for as long as it did!).

I haven't actually been bitten by it myself either way (I'm currently bashing on issues in u-boot, and spotted this while comparing it with the latest kernel code), but I figured I'd pass it along for review.

Patch is against Linus' master a4ecdf82f8ea49f7d3a07.

Cheers,
Ron

Ron

author

2014-03-15 17:39 UTC #1 lore raw

PATCH 1/2 Fix index regression in nand_read_subpage

Commit 7351d3a5dbf42ba3299af71db3296be447bc1516 added an index variable as part of fixing checkpatch warnings, presumably as a tool to make some long lines shorter, however it only set that index in the case of there being no gaps in eccpos for the fragment being read. Which means the later step of filling ecccode from oob_poi will use the wrong indexing into eccpos in that case.

This patch restores the behaviour that existed prior to that change.

Signed-off-by Ron <[email protected]>

---
 drivers/mtd/nand/nand_base.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

drivers/mtd/nand/nand_base.c +2 -3

index 9715a7b..be23dff 100644
@@ -1170,13 +1170,14 @@ static int nand_read_subpage(struct mtd_info *mtd, struct nand_chip *chip,
 	int data_col_addr, i, gaps = 0;
 	int datafrag_len, eccfrag_len, aligned_len, aligned_pos;
 	int busw = (chip->options & NAND_BUSWIDTH_16) ? 2 : 1;
-	int index = 0;
+	int index;
 	unsigned int max_bitflips = 0;
 
 	/* Column address within the page aligned to ECC size (256bytes) */
 	start_step = data_offs / chip->ecc.size;
 	end_step = (data_offs + readlen - 1) / chip->ecc.size;
 	num_steps = end_step - start_step + 1;
+	index = start_step * chip->ecc.bytes;
 
 	/* Data size aligned to ECC ecc.size */
 	datafrag_len = num_steps * chip->ecc.size;

@@ -1213,8 +1214,6 @@ static int nand_read_subpage(struct mtd_info *mtd, struct nand_chip *chip,
 		 * Send the command to read the particular ECC bytes take care
 		 * about buswidth alignment in read_buf.
 		 */
-		index = start_step * chip->ecc.bytes;
-
 		aligned_pos = eccpos[index] & ~(busw - 1);
 		aligned_len = eccfrag_len;
 		if (eccpos[index] & (busw - 1))

Signature · 1 line

1.7.2.5

Brian Norris

2014-03-31 16:57 UTC #2 lore raw

+ Florian

(FYI Ron: 3 of your emails were filtered as spam in my mailbox, for some reason. I only pass over my spam mailbox every once in a few weeks; I only noticed this because of your IRC ping!)

Quoted 46 lines · Ron

Commit 7351d3a5dbf42ba3299af71db3296be447bc1516 added an index variable as part of fixing checkpatch warnings, presumably as a tool to make some long lines shorter, however it only set that index in the case of there being no gaps in eccpos for the fragment being read. Which means the later step of filling ecccode from oob_poi will use the wrong indexing into eccpos in that case.

This patch restores the behaviour that existed prior to that change.

Signed-off-by Ron <[email protected]>

---
 drivers/mtd/nand/nand_base.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

drivers/mtd/nand/nand_base.c +2 -3

index 9715a7b..be23dff 100644
@@ -1170,13 +1170,14 @@ static int nand_read_subpage(struct mtd_info *mtd, struct nand_chip *chip,
 	int data_col_addr, i, gaps = 0;
 	int datafrag_len, eccfrag_len, aligned_len, aligned_pos;
 	int busw = (chip->options & NAND_BUSWIDTH_16) ? 2 : 1;
-	int index = 0;
+	int index;
 	unsigned int max_bitflips = 0;
 
 	/* Column address within the page aligned to ECC size (256bytes) */
 	start_step = data_offs / chip->ecc.size;
 	end_step = (data_offs + readlen - 1) / chip->ecc.size;
 	num_steps = end_step - start_step + 1;
+	index = start_step * chip->ecc.bytes;
 
 	/* Data size aligned to ECC ecc.size */
 	datafrag_len = num_steps * chip->ecc.size;

@@ -1213,8 +1214,6 @@ static int nand_read_subpage(struct mtd_info *mtd, struct nand_chip *chip,
 		 * Send the command to read the particular ECC bytes take care
 		 * about buswidth alignment in read_buf.
 		 */
-		index = start_step * chip->ecc.bytes;
-
 		aligned_pos = eccpos[index] & ~(busw - 1);
 		aligned_len = eccfrag_len;
 		if (eccpos[index] & (busw - 1))

Signature · 1 line

1.7.2.5

Florian Fainelli

2014-04-01 22:15 UTC #3 lore raw

Hi,

2014-03-31 9:56 GMT-07:00 Brian Norris <[email protected]>:

Quoted 5 lines

+ Florian
(FYI Ron: 3 of your emails were filtered as spam in my mailbox, for some reason. I only pass over my spam mailbox every once in a few weeks; I only noticed this because of your IRC ping!)

BTW, you will need to sign-off on these patches using your full name. Your patch does look good to me. Thanks!

Quoted 48 lines · Ron

Quoted 46 lines

Commit 7351d3a5dbf42ba3299af71db3296be447bc1516 added an index variable as part of fixing checkpatch warnings, presumably as a tool to make some long lines shorter, however it only set that index in the case of there being no gaps in eccpos for the fragment being read. Which means the later step of filling ecccode from oob_poi will use the wrong indexing into eccpos in that case.

This patch restores the behaviour that existed prior to that change.

Signed-off-by Ron <[email protected]>

---
 drivers/mtd/nand/nand_base.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

drivers/mtd/nand/nand_base.c +2 -4

index 9715a7b..be23dff 100644
@@ -1170,13 +1170,14 @@ static int nand_read_subpage(struct mtd_info *mtd, struct nand_chip *chip,
      int data_col_addr, i, gaps = 0;
      int datafrag_len, eccfrag_len, aligned_len, aligned_pos;
      int busw = (chip->options & NAND_BUSWIDTH_16) ? 2 : 1;
-     int index = 0;
+     int index;
      unsigned int max_bitflips = 0;
 
      /* Column address within the page aligned to ECC size (256bytes) */
      start_step = data_offs / chip->ecc.size;
      end_step = (data_offs + readlen - 1) / chip->ecc.size;
      num_steps = end_step - start_step + 1;
+     index = start_step * chip->ecc.bytes;
 
      /* Data size aligned to ECC ecc.size */
      datafrag_len = num_steps * chip->ecc.size;

@@ -1213,8 +1214,6 @@ static int nand_read_subpage(struct mtd_info *mtd, struct nand_chip *chip,
               * Send the command to read the particular ECC bytes take care
               * about buswidth alignment in read_buf.
               */
-             index = start_step * chip->ecc.bytes;
-
              aligned_pos = eccpos[index] & ~(busw - 1);
              aligned_len = eccfrag_len;
              if (eccpos[index] & (busw - 1))
--

1.7.2.5

Signature · 1 line

Florian

Ron

author

2014-04-02 06:09 UTC #4 lore raw

Quoted 11 lines · Florian Fainelli

Hi,
2014-03-31 9:56 GMT-07:00 Brian Norris <[email protected]>:
Quoted 5 lines
+ Florian
(FYI Ron: 3 of your emails were filtered as spam in my mailbox, for some reason. I only pass over my spam mailbox every once in a few weeks; I only noticed this because of your IRC ping!)
BTW, you will need to sign-off on these patches using your full name. Your patch does look good to me. Thanks!

Which would be:

Signed-off-by Ron Lee <[email protected]>

Cheers,
Ron

Quoted 52 lines

Quoted 47 lines · Ron

Quoted 46 lines

Commit 7351d3a5dbf42ba3299af71db3296be447bc1516 added an index variable as part of fixing checkpatch warnings, presumably as a tool to make some long lines shorter, however it only set that index in the case of there being no gaps in eccpos for the fragment being read. Which means the later step of filling ecccode from oob_poi will use the wrong indexing into eccpos in that case.

This patch restores the behaviour that existed prior to that change.

Signed-off-by Ron <[email protected]>

---
 drivers/mtd/nand/nand_base.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

drivers/mtd/nand/nand_base.c +2 -4

index 9715a7b..be23dff 100644
@@ -1170,13 +1170,14 @@ static int nand_read_subpage(struct mtd_info *mtd, struct nand_chip *chip,
      int data_col_addr, i, gaps = 0;
      int datafrag_len, eccfrag_len, aligned_len, aligned_pos;
      int busw = (chip->options & NAND_BUSWIDTH_16) ? 2 : 1;
-     int index = 0;
+     int index;
      unsigned int max_bitflips = 0;
 
      /* Column address within the page aligned to ECC size (256bytes) */
      start_step = data_offs / chip->ecc.size;
      end_step = (data_offs + readlen - 1) / chip->ecc.size;
      num_steps = end_step - start_step + 1;
+     index = start_step * chip->ecc.bytes;
 
      /* Data size aligned to ECC ecc.size */
      datafrag_len = num_steps * chip->ecc.size;

@@ -1213,8 +1214,6 @@ static int nand_read_subpage(struct mtd_info *mtd, struct nand_chip *chip,
               * Send the command to read the particular ECC bytes take care
               * about buswidth alignment in read_buf.
               */
-             index = start_step * chip->ecc.bytes;
-
              aligned_pos = eccpos[index] & ~(busw - 1);
              aligned_len = eccfrag_len;
              if (eccpos[index] & (busw - 1))
--

1.7.2.5

Signature · 1 line

Florian

Brian Norris

2014-04-05 06:17 UTC #5 lore raw

Quoted 1 line · Ron

Signed-off-by Ron Lee <[email protected]>

Thanks. Pushed the first one to l2-mtd.git (should go out for 3.15, I think), but I'll leave the other trivial one for later.

Thanks,
Brian

Ron

author

2014-03-15 17:39 UTC #6 lore raw

PATCH 2/2 Use the nand_read_subpage index everywhere

In reply to Ron

Now that the index variable is correctly set earlier in this function we can use it in other places that compute the same thing too.

Signed-off-by Ron <[email protected]>

---
 drivers/mtd/nand/nand_base.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

drivers/mtd/nand/nand_base.c +1 -2

index be23dff..fca43b5 100644
@@ -1200,8 +1200,7 @@ static int nand_read_subpage(struct mtd_info *mtd, struct nand_chip *chip,
 	 * ecc.pos. Let's make sure that there are no gaps in ECC positions.
 	 */
 	for (i = 0; i < eccfrag_len - 1; i++) {
-		if (eccpos[i + start_step * chip->ecc.bytes] + 1 !=
-			eccpos[i + start_step * chip->ecc.bytes + 1]) {
+		if (eccpos[i + index] + 1 != eccpos[i + index + 1]) {
 			gaps = 1;
 			break;
 		}

Signature · 1 line

1.7.2.5

Brian Norris

Applied

2014-04-16 06:26 UTC #7 lore raw

Re: [PATCH 2/2] Use the nand_read_subpage index everywhere

Quoted 4 lines · Ron

Now that the index variable is correctly set earlier in this function we can use it in other places that compute the same thing too.
Signed-off-by Ron <[email protected]>

Pushed to l2-mtd.git. Thanks!

Brian