* [PATCH 0/2] Fix index regression in nand_read_subpage @ 2014-03-15 17:31 Ron 2014-03-15 17:31 ` [PATCH 1/2] " Ron 2014-03-15 17:31 ` [PATCH 2/2] Use the nand_read_subpage index everywhere Ron 0 siblings, 2 replies; 8+ messages in thread From: Ron @ 2014-03-15 17:31 UTC (permalink / raw) To: David Woodhouse, Brian Norris, linux-mtd 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/2] Fix index regression in nand_read_subpage 2014-03-15 17:31 [PATCH 0/2] Fix index regression in nand_read_subpage Ron @ 2014-03-15 17:31 ` Ron 2014-03-31 16:56 ` Brian Norris 2014-03-15 17:31 ` [PATCH 2/2] Use the nand_read_subpage index everywhere Ron 1 sibling, 1 reply; 8+ messages in thread From: Ron @ 2014-03-15 17:31 UTC (permalink / raw) To: David Woodhouse, Brian Norris, linux-mtd 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] Fix index regression in nand_read_subpage 2014-03-15 17:31 ` [PATCH 1/2] " Ron @ 2014-03-31 16:56 ` Brian Norris 2014-04-01 22:14 ` Florian Fainelli 0 siblings, 1 reply; 8+ messages in thread From: Brian Norris @ 2014-03-31 16:56 UTC (permalink / raw) To: Ron; +Cc: linux-mtd, Florian Fainelli, David Woodhouse + 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 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] Fix index regression in nand_read_subpage 2014-03-31 16:56 ` Brian Norris @ 2014-04-01 22:14 ` Florian Fainelli 2014-04-02 5:58 ` Ron 0 siblings, 1 reply; 8+ messages in thread From: Florian Fainelli @ 2014-04-01 22:14 UTC (permalink / raw) To: Brian Norris; +Cc: linux-mtd, David Woodhouse, Ron 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] Fix index regression in nand_read_subpage 2014-04-01 22:14 ` Florian Fainelli @ 2014-04-02 5:58 ` Ron 2014-04-05 6:16 ` Brian Norris 0 siblings, 1 reply; 8+ messages in thread From: Ron @ 2014-04-02 5:58 UTC (permalink / raw) To: Florian Fainelli; +Cc: linux-mtd, Brian Norris, David Woodhouse 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] Fix index regression in nand_read_subpage 2014-04-02 5:58 ` Ron @ 2014-04-05 6:16 ` Brian Norris 0 siblings, 0 replies; 8+ messages in thread From: Brian Norris @ 2014-04-05 6:16 UTC (permalink / raw) To: Ron; +Cc: linux-mtd, Florian Fainelli, David Woodhouse 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 2/2] Use the nand_read_subpage index everywhere 2014-03-15 17:31 [PATCH 0/2] Fix index regression in nand_read_subpage Ron 2014-03-15 17:31 ` [PATCH 1/2] " Ron @ 2014-03-15 17:31 ` Ron 2014-04-16 6:26 ` Brian Norris 1 sibling, 1 reply; 8+ messages in thread From: Ron @ 2014-03-15 17:31 UTC (permalink / raw) To: David Woodhouse, Brian Norris, linux-mtd 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] Use the nand_read_subpage index everywhere 2014-03-15 17:31 ` [PATCH 2/2] Use the nand_read_subpage index everywhere Ron @ 2014-04-16 6:26 ` Brian Norris 0 siblings, 0 replies; 8+ messages in thread From: Brian Norris @ 2014-04-16 6:26 UTC (permalink / raw) To: Ron; +Cc: linux-mtd, David Woodhouse 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2014-04-16 6:26 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-03-15 17:31 [PATCH 0/2] Fix index regression in nand_read_subpage Ron 2014-03-15 17:31 ` [PATCH 1/2] " Ron 2014-03-31 16:56 ` Brian Norris 2014-04-01 22:14 ` Florian Fainelli 2014-04-02 5:58 ` Ron 2014-04-05 6:16 ` Brian Norris 2014-03-15 17:31 ` [PATCH 2/2] Use the nand_read_subpage index everywhere Ron 2014-04-16 6:26 ` Brian Norris
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.