* [U-Boot] [PATCH 0/2] omap_gpmc nand ecc switch bug fixes @ 2013-12-16 17:19 Nikita Kiryanov 2013-12-16 17:19 ` [U-Boot] [PATCH 1/2] mtd: nand: omap: fix sw->hw->sw ecc switch Nikita Kiryanov 2013-12-16 17:19 ` [U-Boot] [PATCH 2/2] mtd: nand: omap: fix ecc ops assignment when changing ecc Nikita Kiryanov 0 siblings, 2 replies; 8+ messages in thread From: Nikita Kiryanov @ 2013-12-16 17:19 UTC (permalink / raw) To: u-boot This patchset fixes two omap_gpmc.c bugs which are related to ecc switching. The first bug fixes a situation in which sw->hw->sw ecc switch corrupts ecc layout data, resulting in incorrect sw ecc layout. The second bug fixes an issue where switching from sw to hw ecc results in hw ecc data being read/written using sw ecc functions. Cc: Scott Wood <scottwood@freescale.com> Cc: Pekon Gupta <pekon@ti.com> Nikita Kiryanov (2): mtd: nand: omap: fix sw->hw->sw ecc switch mtd: nand: omap: fix ecc ops assignment when changing ecc drivers/mtd/nand/omap_gpmc.c | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) -- 1.8.1.2 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [U-Boot] [PATCH 1/2] mtd: nand: omap: fix sw->hw->sw ecc switch 2013-12-16 17:19 [U-Boot] [PATCH 0/2] omap_gpmc nand ecc switch bug fixes Nikita Kiryanov @ 2013-12-16 17:19 ` Nikita Kiryanov 2013-12-17 23:47 ` [U-Boot] [U-Boot, " Scott Wood 2013-12-16 17:19 ` [U-Boot] [PATCH 2/2] mtd: nand: omap: fix ecc ops assignment when changing ecc Nikita Kiryanov 1 sibling, 1 reply; 8+ messages in thread From: Nikita Kiryanov @ 2013-12-16 17:19 UTC (permalink / raw) To: u-boot When switching ecc mode, omap_select_ecc_scheme() assigns the appropriate values into the current nand chip's ecc.layout struct. This is done under the assumption that the struct exists only to store values, so it is OK to overwrite it, but there is at least one situation where this assumption is incorrect: When switching to 1 bit hamming code sw ecc, the job of assigning layout data is outsourced to nand_scan_tail(), which simply assigns into ecc.layout a pointer to an existing struct prefilled with the appropriate values. This struct doubles as both data and layout definition, and therefore shouldn't be overwritten, but on the next switch to hardware ecc, this is exactly what's going to happen. The next time the user switches to software ecc, they're going to get a messed up ecc layout. Prevent this and possible similar bugs by explicitly using the private-to-omap_gpmc.c omap_ecclayout struct when switching ecc mode. Cc: Scott Wood <scottwood@freescale.com> Cc: Pekon Gupta <pekon@ti.com> Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il> --- drivers/mtd/nand/omap_gpmc.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/drivers/mtd/nand/omap_gpmc.c b/drivers/mtd/nand/omap_gpmc.c index e61788f..fda1df2 100644 --- a/drivers/mtd/nand/omap_gpmc.c +++ b/drivers/mtd/nand/omap_gpmc.c @@ -761,7 +761,7 @@ static void __maybe_unused omap_free_bch(struct mtd_info *mtd) static int omap_select_ecc_scheme(struct nand_chip *nand, enum omap_ecc ecc_scheme, unsigned int pagesize, unsigned int oobsize) { struct nand_bch_priv *bch = nand->priv; - struct nand_ecclayout *ecclayout = nand->ecc.layout; + struct nand_ecclayout *ecclayout = &omap_ecclayout; int eccsteps = pagesize / SECTOR_BYTES; int i; @@ -895,6 +895,11 @@ static int omap_select_ecc_scheme(struct nand_chip *nand, debug("nand: error: ecc scheme not enabled or supported\n"); return -EINVAL; } + + /* nand_scan_tail() sets ham1 sw ecc; hw ecc layout is set by driver */ + if (ecc_scheme != OMAP_ECC_HAM1_CODE_SW) + nand->ecc.layout = ecclayout; + return 0; } -- 1.8.1.2 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [U-Boot] [U-Boot, 1/2] mtd: nand: omap: fix sw->hw->sw ecc switch 2013-12-16 17:19 ` [U-Boot] [PATCH 1/2] mtd: nand: omap: fix sw->hw->sw ecc switch Nikita Kiryanov @ 2013-12-17 23:47 ` Scott Wood 0 siblings, 0 replies; 8+ messages in thread From: Scott Wood @ 2013-12-17 23:47 UTC (permalink / raw) To: u-boot On Mon, Dec 16, 2013 at 07:19:01PM +0200, Nikita Kiryanov wrote: > When switching ecc mode, omap_select_ecc_scheme() assigns the appropriate values > into the current nand chip's ecc.layout struct. This is done under the > assumption that the struct exists only to store values, so it is OK to overwrite > it, but there is at least one situation where this assumption is incorrect: > > When switching to 1 bit hamming code sw ecc, the job of assigning layout data > is outsourced to nand_scan_tail(), which simply assigns into ecc.layout a > pointer to an existing struct prefilled with the appropriate values. This struct > doubles as both data and layout definition, and therefore shouldn't be > overwritten, but on the next switch to hardware ecc, this is exactly what's > going to happen. The next time the user switches to software ecc, they're > going to get a messed up ecc layout. > > Prevent this and possible similar bugs by explicitly using the > private-to-omap_gpmc.c omap_ecclayout struct when switching ecc mode. > > Cc: Scott Wood <scottwood@freescale.com> > Cc: Pekon Gupta <pekon@ti.com> > Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il> > > --- > drivers/mtd/nand/omap_gpmc.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) Applied to u-boot-nand-flash.git -Scott ^ permalink raw reply [flat|nested] 8+ messages in thread
* [U-Boot] [PATCH 2/2] mtd: nand: omap: fix ecc ops assignment when changing ecc 2013-12-16 17:19 [U-Boot] [PATCH 0/2] omap_gpmc nand ecc switch bug fixes Nikita Kiryanov 2013-12-16 17:19 ` [U-Boot] [PATCH 1/2] mtd: nand: omap: fix sw->hw->sw ecc switch Nikita Kiryanov @ 2013-12-16 17:19 ` Nikita Kiryanov 2013-12-16 23:03 ` Scott Wood 2013-12-17 13:18 ` [U-Boot] [PATCH V2 " Nikita Kiryanov 1 sibling, 2 replies; 8+ messages in thread From: Nikita Kiryanov @ 2013-12-16 17:19 UTC (permalink / raw) To: u-boot If we change to software ecc and then back to hardware ecc, the nand ecc ops pointers are populated with incorrect function pointers. This is related to the way nand_scan_tail() handles assigning functions to ecc ops: If we are switching to software ecc/no ecc, it assigns default functions to the ecc ops pointers unconditionally, but if we are switching to hardware ecc, the default hardware ecc functions are assigned to ops pointers only if these pointers are NULL (so that drivers could set their own functions). In the case of omap_gpmc.c driver, when we switch to sw ecc, sw ecc functions are assigned to ecc ops by nand_scan_tail(), and when we later switch to hw ecc, the ecc ops pointers are not NULL, so nand_scan_tail() does not overwrite them with hw ecc functions. The result: sw ecc functions used to write hw ecc data. Clear the ecc ops pointers in omap_gpmc.c when switching ecc types, so that ops which were not assigned by the driver will get the correct default values from nand_scan_tail(). Cc: Scott Wood <scottwood@freescale.com> Cc: Pekon Gupta <pekon@ti.com> Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il> --- drivers/mtd/nand/omap_gpmc.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/drivers/mtd/nand/omap_gpmc.c b/drivers/mtd/nand/omap_gpmc.c index fda1df2..19dcd45 100644 --- a/drivers/mtd/nand/omap_gpmc.c +++ b/drivers/mtd/nand/omap_gpmc.c @@ -765,6 +765,19 @@ static int omap_select_ecc_scheme(struct nand_chip *nand, int eccsteps = pagesize / SECTOR_BYTES; int i; + nand->ecc.calculate = NULL; + nand->ecc.correct = NULL; + nand->ecc.hwctl = NULL; + nand->ecc.read_oob = NULL; + nand->ecc.read_oob_raw = NULL; + nand->ecc.read_page = NULL; + nand->ecc.read_page_raw = NULL; + nand->ecc.read_subpage = NULL; + nand->ecc.write_oob = NULL; + nand->ecc.write_oob_raw = NULL; + nand->ecc.write_page = NULL; + nand->ecc.write_page_raw = NULL; + switch (ecc_scheme) { case OMAP_ECC_HAM1_CODE_SW: debug("nand: selected OMAP_ECC_HAM1_CODE_SW\n"); -- 1.8.1.2 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [U-Boot] [PATCH 2/2] mtd: nand: omap: fix ecc ops assignment when changing ecc 2013-12-16 17:19 ` [U-Boot] [PATCH 2/2] mtd: nand: omap: fix ecc ops assignment when changing ecc Nikita Kiryanov @ 2013-12-16 23:03 ` Scott Wood 2013-12-17 13:16 ` Nikita Kiryanov 2013-12-17 13:18 ` [U-Boot] [PATCH V2 " Nikita Kiryanov 1 sibling, 1 reply; 8+ messages in thread From: Scott Wood @ 2013-12-16 23:03 UTC (permalink / raw) To: u-boot On Mon, 2013-12-16 at 19:19 +0200, Nikita Kiryanov wrote: > If we change to software ecc and then back to hardware ecc, the nand ecc ops > pointers are populated with incorrect function pointers. This is related to the > way nand_scan_tail() handles assigning functions to ecc ops: > > If we are switching to software ecc/no ecc, it assigns default functions to the > ecc ops pointers unconditionally, but if we are switching to hardware ecc, > the default hardware ecc functions are assigned to ops pointers only if these > pointers are NULL (so that drivers could set their own functions). In the case > of omap_gpmc.c driver, when we switch to sw ecc, sw ecc functions are > assigned to ecc ops by nand_scan_tail(), and when we later switch to hw ecc, > the ecc ops pointers are not NULL, so nand_scan_tail() does not overwrite > them with hw ecc functions. > The result: sw ecc functions used to write hw ecc data. > > Clear the ecc ops pointers in omap_gpmc.c when switching ecc types, so that > ops which were not assigned by the driver will get the correct default values > from nand_scan_tail(). > > Cc: Scott Wood <scottwood@freescale.com> > Cc: Pekon Gupta <pekon@ti.com> > Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il> > --- > drivers/mtd/nand/omap_gpmc.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/drivers/mtd/nand/omap_gpmc.c b/drivers/mtd/nand/omap_gpmc.c > index fda1df2..19dcd45 100644 > --- a/drivers/mtd/nand/omap_gpmc.c > +++ b/drivers/mtd/nand/omap_gpmc.c > @@ -765,6 +765,19 @@ static int omap_select_ecc_scheme(struct nand_chip *nand, > int eccsteps = pagesize / SECTOR_BYTES; > int i; > > + nand->ecc.calculate = NULL; > + nand->ecc.correct = NULL; > + nand->ecc.hwctl = NULL; > + nand->ecc.read_oob = NULL; > + nand->ecc.read_oob_raw = NULL; > + nand->ecc.read_page = NULL; > + nand->ecc.read_page_raw = NULL; > + nand->ecc.read_subpage = NULL; > + nand->ecc.write_oob = NULL; > + nand->ecc.write_oob_raw = NULL; > + nand->ecc.write_page = NULL; > + nand->ecc.write_page_raw = NULL; > + > switch (ecc_scheme) { > case OMAP_ECC_HAM1_CODE_SW: > debug("nand: selected OMAP_ECC_HAM1_CODE_SW\n"); This will leave you with a broken nand->ecc if the function returns an error. Instead, each case in the switch should NULL out whichever members it is not initializing (or perhaps just memset it, once past the error checks). -Scott ^ permalink raw reply [flat|nested] 8+ messages in thread
* [U-Boot] [PATCH 2/2] mtd: nand: omap: fix ecc ops assignment when changing ecc 2013-12-16 23:03 ` Scott Wood @ 2013-12-17 13:16 ` Nikita Kiryanov 0 siblings, 0 replies; 8+ messages in thread From: Nikita Kiryanov @ 2013-12-17 13:16 UTC (permalink / raw) To: u-boot On 12/17/2013 01:03 AM, Scott Wood wrote: > On Mon, 2013-12-16 at 19:19 +0200, Nikita Kiryanov wrote: >> If we change to software ecc and then back to hardware ecc, the nand ecc ops >> pointers are populated with incorrect function pointers. This is related to the >> way nand_scan_tail() handles assigning functions to ecc ops: >> >> If we are switching to software ecc/no ecc, it assigns default functions to the >> ecc ops pointers unconditionally, but if we are switching to hardware ecc, >> the default hardware ecc functions are assigned to ops pointers only if these >> pointers are NULL (so that drivers could set their own functions). In the case >> of omap_gpmc.c driver, when we switch to sw ecc, sw ecc functions are >> assigned to ecc ops by nand_scan_tail(), and when we later switch to hw ecc, >> the ecc ops pointers are not NULL, so nand_scan_tail() does not overwrite >> them with hw ecc functions. >> The result: sw ecc functions used to write hw ecc data. >> >> Clear the ecc ops pointers in omap_gpmc.c when switching ecc types, so that >> ops which were not assigned by the driver will get the correct default values >> from nand_scan_tail(). >> >> Cc: Scott Wood <scottwood@freescale.com> >> Cc: Pekon Gupta <pekon@ti.com> >> Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il> >> --- >> drivers/mtd/nand/omap_gpmc.c | 13 +++++++++++++ >> 1 file changed, 13 insertions(+) >> >> diff --git a/drivers/mtd/nand/omap_gpmc.c b/drivers/mtd/nand/omap_gpmc.c >> index fda1df2..19dcd45 100644 >> --- a/drivers/mtd/nand/omap_gpmc.c >> +++ b/drivers/mtd/nand/omap_gpmc.c >> @@ -765,6 +765,19 @@ static int omap_select_ecc_scheme(struct nand_chip *nand, >> int eccsteps = pagesize / SECTOR_BYTES; >> int i; >> >> + nand->ecc.calculate = NULL; >> + nand->ecc.correct = NULL; >> + nand->ecc.hwctl = NULL; >> + nand->ecc.read_oob = NULL; >> + nand->ecc.read_oob_raw = NULL; >> + nand->ecc.read_page = NULL; >> + nand->ecc.read_page_raw = NULL; >> + nand->ecc.read_subpage = NULL; >> + nand->ecc.write_oob = NULL; >> + nand->ecc.write_oob_raw = NULL; >> + nand->ecc.write_page = NULL; >> + nand->ecc.write_page_raw = NULL; >> + >> switch (ecc_scheme) { >> case OMAP_ECC_HAM1_CODE_SW: >> debug("nand: selected OMAP_ECC_HAM1_CODE_SW\n"); > > This will leave you with a broken nand->ecc if the function returns an > error. Instead, each case in the switch should NULL out whichever > members it is not initializing (or perhaps just memset it, once past the > error checks). Yes you're right. Guess I should've resisted doing a last minue refactor. V2 coming up. > > -Scott > > -- Regards, Nikita. ^ permalink raw reply [flat|nested] 8+ messages in thread
* [U-Boot] [PATCH V2 2/2] mtd: nand: omap: fix ecc ops assignment when changing ecc 2013-12-16 17:19 ` [U-Boot] [PATCH 2/2] mtd: nand: omap: fix ecc ops assignment when changing ecc Nikita Kiryanov 2013-12-16 23:03 ` Scott Wood @ 2013-12-17 13:18 ` Nikita Kiryanov 2013-12-17 23:49 ` [U-Boot] [U-Boot, V2, " Scott Wood 1 sibling, 1 reply; 8+ messages in thread From: Nikita Kiryanov @ 2013-12-17 13:18 UTC (permalink / raw) To: u-boot If we change to software ecc and then back to hardware ecc, the nand ecc ops pointers are populated with incorrect function pointers. This is related to the way nand_scan_tail() handles assigning functions to ecc ops: If we are switching to software ecc/no ecc, it assigns default functions to the ecc ops pointers unconditionally, but if we are switching to hardware ecc, the default hardware ecc functions are assigned to ops pointers only if these pointers are NULL (so that drivers could set their own functions). In the case of omap_gpmc.c driver, when we switch to sw ecc, sw ecc functions are assigned to ecc ops by nand_scan_tail(), and when we later switch to hw ecc, the ecc ops pointers are not NULL, so nand_scan_tail() does not overwrite them with hw ecc functions. The result: sw ecc functions used to write hw ecc data. Clear the ecc ops pointers in omap_gpmc.c when switching ecc types, so that ops which were not assigned by the driver will get the correct default values from nand_scan_tail(). Cc: Scott Wood <scottwood@freescale.com> Cc: Pekon Gupta <pekon@ti.com> Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il> --- Changes in V2: - Clear the ops after error checks, not before. - Use memset on ecc struct to clear the ops. drivers/mtd/nand/omap_gpmc.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/mtd/nand/omap_gpmc.c b/drivers/mtd/nand/omap_gpmc.c index fda1df2..37822bc 100644 --- a/drivers/mtd/nand/omap_gpmc.c +++ b/drivers/mtd/nand/omap_gpmc.c @@ -789,6 +789,7 @@ static int omap_select_ecc_scheme(struct nand_chip *nand, bch_priv.control = NULL; bch_priv.type = 0; /* populate ecc specific fields */ + memset(&nand->ecc, 0, sizeof(struct nand_ecc_ctrl)); nand->ecc.mode = NAND_ECC_HW; nand->ecc.strength = 1; nand->ecc.size = SECTOR_BYTES; @@ -827,6 +828,7 @@ static int omap_select_ecc_scheme(struct nand_chip *nand, } bch_priv.type = ECC_BCH8; /* populate ecc specific fields */ + memset(&nand->ecc, 0, sizeof(struct nand_ecc_ctrl)); nand->ecc.mode = NAND_ECC_HW; nand->ecc.strength = 8; nand->ecc.size = SECTOR_BYTES; @@ -869,6 +871,7 @@ static int omap_select_ecc_scheme(struct nand_chip *nand, elm_init(); bch_priv.type = ECC_BCH8; /* populate ecc specific fields */ + memset(&nand->ecc, 0, sizeof(struct nand_ecc_ctrl)); nand->ecc.mode = NAND_ECC_HW; nand->ecc.strength = 8; nand->ecc.size = SECTOR_BYTES; -- 1.8.1.2 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [U-Boot] [U-Boot, V2, 2/2] mtd: nand: omap: fix ecc ops assignment when changing ecc 2013-12-17 13:18 ` [U-Boot] [PATCH V2 " Nikita Kiryanov @ 2013-12-17 23:49 ` Scott Wood 0 siblings, 0 replies; 8+ messages in thread From: Scott Wood @ 2013-12-17 23:49 UTC (permalink / raw) To: u-boot On Tue, Dec 17, 2013 at 03:18:01PM +0200, Nikita Kiryanov wrote: > If we change to software ecc and then back to hardware ecc, the nand ecc ops > pointers are populated with incorrect function pointers. This is related to the > way nand_scan_tail() handles assigning functions to ecc ops: > > If we are switching to software ecc/no ecc, it assigns default functions to the > ecc ops pointers unconditionally, but if we are switching to hardware ecc, > the default hardware ecc functions are assigned to ops pointers only if these > pointers are NULL (so that drivers could set their own functions). In the case > of omap_gpmc.c driver, when we switch to sw ecc, sw ecc functions are > assigned to ecc ops by nand_scan_tail(), and when we later switch to hw ecc, > the ecc ops pointers are not NULL, so nand_scan_tail() does not overwrite > them with hw ecc functions. > The result: sw ecc functions used to write hw ecc data. > > Clear the ecc ops pointers in omap_gpmc.c when switching ecc types, so that > ops which were not assigned by the driver will get the correct default values > from nand_scan_tail(). > > Cc: Scott Wood <scottwood@freescale.com> > Cc: Pekon Gupta <pekon@ti.com> > Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il> > > --- > Changes in V2: > - Clear the ops after error checks, not before. > - Use memset on ecc struct to clear the ops. > > drivers/mtd/nand/omap_gpmc.c | 3 +++ > 1 file changed, 3 insertions(+) Applied to u-boot-nand-flash.git -Scott ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2013-12-17 23:49 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-12-16 17:19 [U-Boot] [PATCH 0/2] omap_gpmc nand ecc switch bug fixes Nikita Kiryanov 2013-12-16 17:19 ` [U-Boot] [PATCH 1/2] mtd: nand: omap: fix sw->hw->sw ecc switch Nikita Kiryanov 2013-12-17 23:47 ` [U-Boot] [U-Boot, " Scott Wood 2013-12-16 17:19 ` [U-Boot] [PATCH 2/2] mtd: nand: omap: fix ecc ops assignment when changing ecc Nikita Kiryanov 2013-12-16 23:03 ` Scott Wood 2013-12-17 13:16 ` Nikita Kiryanov 2013-12-17 13:18 ` [U-Boot] [PATCH V2 " Nikita Kiryanov 2013-12-17 23:49 ` [U-Boot] [U-Boot, V2, " Scott Wood
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.