[PATCH 0/2] Fix index regression in nand_read_subpage

[PATCH 0/2] Fix index regression in nand_read_subpage

[Ron]

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

[PATCH 1/2] Fix index regression in nand_read_subpage

[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 <ron@debian.org>
---
 drivers/mtd/nand/nand_base.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 9715a7b..be23dff 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -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

[PATCH 2/2] Use the nand_read_subpage index everywhere

[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 <ron@debian.org>
---
 drivers/mtd/nand/nand_base.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index be23dff..fca43b5 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -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;
 		}
-- 
1.7.2.5

Re: [PATCH 1/2] Fix index regression in nand_read_subpage

[Brian Norris]

+ 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!)

On Sun, Mar 16, 2014 at 04:01:07AM +1030, Ron wrote:
> 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 <ron@debian.org>
> ---
>  drivers/mtd/nand/nand_base.c |    5 ++---
>  1 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index 9715a7b..be23dff 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -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
> 

Re: [PATCH 1/2] Fix index regression in nand_read_subpage

[Florian Fainelli]

Hi,

2014-03-31 9:56 GMT-07:00 Brian Norris <computersforpeace@gmail.com>:
> + 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!

>
> On Sun, Mar 16, 2014 at 04:01:07AM +1030, Ron wrote:
>> 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 <ron@debian.org>
>> ---
>>  drivers/mtd/nand/nand_base.c |    5 ++---
>>  1 files changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
>> index 9715a7b..be23dff 100644
>> --- a/drivers/mtd/nand/nand_base.c
>> +++ b/drivers/mtd/nand/nand_base.c
>> @@ -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
>>



-- 
Florian

Re: [PATCH 1/2] Fix index regression in nand_read_subpage

[Ron]

On Tue, Apr 01, 2014 at 03:14:24PM -0700, Florian Fainelli wrote:
> Hi,
> 
> 2014-03-31 9:56 GMT-07:00 Brian Norris <computersforpeace@gmail.com>:
> > + 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 <ron@debian.org>


  Cheers,
  Ron


> > On Sun, Mar 16, 2014 at 04:01:07AM +1030, Ron wrote:
> >> 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 <ron@debian.org>
> >> ---
> >>  drivers/mtd/nand/nand_base.c |    5 ++---
> >>  1 files changed, 2 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> >> index 9715a7b..be23dff 100644
> >> --- a/drivers/mtd/nand/nand_base.c
> >> +++ b/drivers/mtd/nand/nand_base.c
> >> @@ -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
> >>
> 
> 
> 
> -- 
> Florian

Re: [PATCH 1/2] Fix index regression in nand_read_subpage

[Brian Norris]

On Wed, Apr 02, 2014 at 04:28:07PM +1030, Ron wrote:
>  Signed-off-by: Ron Lee <ron@debian.org>

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

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

[Brian Norris]

On Sun, Mar 16, 2014 at 04:01:08AM +1030, Ron wrote:
> 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 <ron@debian.org>

Pushed to l2-mtd.git. Thanks!

Brian