Re: [PATCH 3/3] mtd: mxc-nand: Only automatically create BBT if NAND seems to be pristine
Hallo Sascha,
On Fri, May 03, 2024 at 09:44:24AM +0200, Sascha Hauer wrote:
> On Tue, Apr 30, 2024 at 11:44:54AM +0200, Uwe Kleine-König wrote:
> > Automatically creating a BBT is the right thing to do if the NAND is
> > factory new. However when migrating from a barebox older than commit
> > v2020.03.0~28^2~1 ("mtd: nand-imx: Create BBT automatically when
> > necessary") on a used machine, this automatism is really bad because it
> > most likely marks the blocks containing the barebox image (and possibly
> > more) as bad. On such a system the vendor BBMs are gone, but it was
> > operated without that information before, so continuing to do so is a
> > sane option.
> >
> > Add a light check for the NAND to be really pristine: If the first block
> > looks like containing a barebox image or a UBI refuse to create a BBT.
> >
> > Signed-off-by: Uwe Kleine-König
> > ---
> > drivers/mtd/nand/raw/mxc_nand.c | 58 ++---
> > 1 file changed, 31 insertions(+), 27 deletions(-)
> >
> > diff --git a/drivers/mtd/nand/raw/mxc_nand.c
> > b/drivers/mtd/nand/raw/mxc_nand.c
> > index a72275480144..fd5ae447a198 100644
> > --- a/drivers/mtd/nand/raw/mxc_nand.c
> > +++ b/drivers/mtd/nand/raw/mxc_nand.c
> > @@ -1555,30 +1555,6 @@ static const struct nand_controller_ops
> > mxcnd_controller_ops = {
> > * From this point on we can forget about the BBMs and rely completely
> > * on the flash BBT.
> > */
> > -static int checkbad(struct nand_chip *chip, loff_t ofs)
> > -{
> > - struct mtd_info *mtd = nand_to_mtd(chip);
> > - int ret;
> > - uint8_t buf[mtd->writesize + mtd->oobsize];
> > - struct mtd_oob_ops ops;
> > -
> > - ops.mode = MTD_OPS_RAW;
> > - ops.ooboffs = 0;
> > - ops.datbuf = buf;
> > - ops.len = mtd->writesize;
> > - ops.oobbuf = buf + mtd->writesize;
> > - ops.ooblen = mtd->oobsize;
> > -
> > - ret = mtd_read_oob(mtd, ofs, );
> > - if (ret < 0)
> > - return ret;
> > -
> > - if (buf[2000] != 0xff)
> > - return 1;
> > -
> > - return 0;
> > -}
> > -
> > static int imxnd_create_bbt(struct nand_chip *chip)
> > {
> > struct mtd_info *mtd = nand_to_mtd(chip);
> > @@ -1598,12 +1574,40 @@ static int imxnd_create_bbt(struct nand_chip *chip)
> >
> > for (i = 0; i < numblocks; ++i) {
> > loff_t ofs = i << chip->bbt_erase_shift;
> > + uint8_t buf[mtd->writesize + mtd->oobsize];
> > + struct mtd_oob_ops ops = {
> > + .mode = MTD_OPS_RAW,
> > + .ooboffs = 0,
> > + .datbuf = buf,
> > + .len = mtd->writesize,
> > + .oobbuf = buf + mtd->writesize,
> > + .ooblen = mtd->oobsize,
> > + };
> >
> > - ret = checkbad(chip, ofs);
> > - if (ret < 0)
> > + ret = mtd_read_oob(mtd, ofs, );
> > + if (ret < 0) {
> > + dev_err(mtd->dev.parent, "Failed to read page at
> > 0x%08x\n", (unsigned int)ofs);
> > goto out;
> > + }
> >
> > - if (ret) {
> > + /*
> > +* Automatically adding a BBT based on factory BBTs is only
> > +* sensible if the NAND is pristine. Abort if the first page
> > +* looks like a bootloader or UBI block.
> > +*/
> > + if (ofs == 0 && is_barebox_arm_head(buf)) {
> > + dev_err(mtd->dev.parent, "Flash seems to contain a
> > barebox image, refusing\n");
> > + ret = -EINVAL;
> > + goto out;
> > + }
> > +
> > + if (ofs == 0 && !memcmp(buf, "UBI#", 4)) {
> > + dev_err(mtd->dev.parent, "Flash seems to contain a UBI,
> > refusing\n");
> > + ret = -EINVAL;
> > + goto out;
> > + }
> > +
> > + if (buf[2000] != 0xff) {
> > bbt[i >> 2] |= 0x03 << (2 * (i & 0x3));
> > dev_info(mtd->dev.parent, "Bad eraseblock %d at
> > 0x%08x\n",
> > i, (unsigned int)ofs);
>
> Could you add the new code to checkbad() instead of inlining it? That
> way it seems easier to adjust the code in case we have to change the way
> how we detect useful data on a page. Rename checkbad() in case the name
> doesn't feel appropriate anymore.
I hesitated to add the check for the chip being pristine to checkbad().
Then maybe read page 0 and check its contents before the check_bad()
loop? Then page 0 will be read twice but checkbad() can stay around and
only do what its name promises.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |
signature.asc
Description: PGP signature
Re: [PATCH 3/3] mtd: mxc-nand: Only automatically create BBT if NAND seems to be pristine
On Tue, Apr 30, 2024 at 11:44:54AM +0200, Uwe Kleine-König wrote:
> Automatically creating a BBT is the right thing to do if the NAND is
> factory new. However when migrating from a barebox older than commit
> v2020.03.0~28^2~1 ("mtd: nand-imx: Create BBT automatically when
> necessary") on a used machine, this automatism is really bad because it
> most likely marks the blocks containing the barebox image (and possibly
> more) as bad. On such a system the vendor BBMs are gone, but it was
> operated without that information before, so continuing to do so is a
> sane option.
>
> Add a light check for the NAND to be really pristine: If the first block
> looks like containing a barebox image or a UBI refuse to create a BBT.
>
> Signed-off-by: Uwe Kleine-König
> ---
> drivers/mtd/nand/raw/mxc_nand.c | 58 ++---
> 1 file changed, 31 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/mtd/nand/raw/mxc_nand.c b/drivers/mtd/nand/raw/mxc_nand.c
> index a72275480144..fd5ae447a198 100644
> --- a/drivers/mtd/nand/raw/mxc_nand.c
> +++ b/drivers/mtd/nand/raw/mxc_nand.c
> @@ -1555,30 +1555,6 @@ static const struct nand_controller_ops
> mxcnd_controller_ops = {
> * From this point on we can forget about the BBMs and rely completely
> * on the flash BBT.
> */
> -static int checkbad(struct nand_chip *chip, loff_t ofs)
> -{
> - struct mtd_info *mtd = nand_to_mtd(chip);
> - int ret;
> - uint8_t buf[mtd->writesize + mtd->oobsize];
> - struct mtd_oob_ops ops;
> -
> - ops.mode = MTD_OPS_RAW;
> - ops.ooboffs = 0;
> - ops.datbuf = buf;
> - ops.len = mtd->writesize;
> - ops.oobbuf = buf + mtd->writesize;
> - ops.ooblen = mtd->oobsize;
> -
> - ret = mtd_read_oob(mtd, ofs, );
> - if (ret < 0)
> - return ret;
> -
> - if (buf[2000] != 0xff)
> - return 1;
> -
> - return 0;
> -}
> -
> static int imxnd_create_bbt(struct nand_chip *chip)
> {
> struct mtd_info *mtd = nand_to_mtd(chip);
> @@ -1598,12 +1574,40 @@ static int imxnd_create_bbt(struct nand_chip *chip)
>
> for (i = 0; i < numblocks; ++i) {
> loff_t ofs = i << chip->bbt_erase_shift;
> + uint8_t buf[mtd->writesize + mtd->oobsize];
> + struct mtd_oob_ops ops = {
> + .mode = MTD_OPS_RAW,
> + .ooboffs = 0,
> + .datbuf = buf,
> + .len = mtd->writesize,
> + .oobbuf = buf + mtd->writesize,
> + .ooblen = mtd->oobsize,
> + };
>
> - ret = checkbad(chip, ofs);
> - if (ret < 0)
> + ret = mtd_read_oob(mtd, ofs, );
> + if (ret < 0) {
> + dev_err(mtd->dev.parent, "Failed to read page at
> 0x%08x\n", (unsigned int)ofs);
> goto out;
> + }
>
> - if (ret) {
> + /*
> + * Automatically adding a BBT based on factory BBTs is only
> + * sensible if the NAND is pristine. Abort if the first page
> + * looks like a bootloader or UBI block.
> + */
> + if (ofs == 0 && is_barebox_arm_head(buf)) {
> + dev_err(mtd->dev.parent, "Flash seems to contain a
> barebox image, refusing\n");
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + if (ofs == 0 && !memcmp(buf, "UBI#", 4)) {
> + dev_err(mtd->dev.parent, "Flash seems to contain a UBI,
> refusing\n");
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + if (buf[2000] != 0xff) {
> bbt[i >> 2] |= 0x03 << (2 * (i & 0x3));
> dev_info(mtd->dev.parent, "Bad eraseblock %d at
> 0x%08x\n",
>i, (unsigned int)ofs);
Could you add the new code to checkbad() instead of inlining it? That
way it seems easier to adjust the code in case we have to change the way
how we detect useful data on a page. Rename checkbad() in case the name
doesn't feel appropriate anymore.
Sascha
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
[PATCH 3/3] mtd: mxc-nand: Only automatically create BBT if NAND seems to be pristine
Automatically creating a BBT is the right thing to do if the NAND is
factory new. However when migrating from a barebox older than commit
v2020.03.0~28^2~1 ("mtd: nand-imx: Create BBT automatically when
necessary") on a used machine, this automatism is really bad because it
most likely marks the blocks containing the barebox image (and possibly
more) as bad. On such a system the vendor BBMs are gone, but it was
operated without that information before, so continuing to do so is a
sane option.
Add a light check for the NAND to be really pristine: If the first block
looks like containing a barebox image or a UBI refuse to create a BBT.
Signed-off-by: Uwe Kleine-König
---
drivers/mtd/nand/raw/mxc_nand.c | 58 ++---
1 file changed, 31 insertions(+), 27 deletions(-)
diff --git a/drivers/mtd/nand/raw/mxc_nand.c b/drivers/mtd/nand/raw/mxc_nand.c
index a72275480144..fd5ae447a198 100644
--- a/drivers/mtd/nand/raw/mxc_nand.c
+++ b/drivers/mtd/nand/raw/mxc_nand.c
@@ -1555,30 +1555,6 @@ static const struct nand_controller_ops
mxcnd_controller_ops = {
* From this point on we can forget about the BBMs and rely completely
* on the flash BBT.
*/
-static int checkbad(struct nand_chip *chip, loff_t ofs)
-{
- struct mtd_info *mtd = nand_to_mtd(chip);
- int ret;
- uint8_t buf[mtd->writesize + mtd->oobsize];
- struct mtd_oob_ops ops;
-
- ops.mode = MTD_OPS_RAW;
- ops.ooboffs = 0;
- ops.datbuf = buf;
- ops.len = mtd->writesize;
- ops.oobbuf = buf + mtd->writesize;
- ops.ooblen = mtd->oobsize;
-
- ret = mtd_read_oob(mtd, ofs, );
- if (ret < 0)
- return ret;
-
- if (buf[2000] != 0xff)
- return 1;
-
- return 0;
-}
-
static int imxnd_create_bbt(struct nand_chip *chip)
{
struct mtd_info *mtd = nand_to_mtd(chip);
@@ -1598,12 +1574,40 @@ static int imxnd_create_bbt(struct nand_chip *chip)
for (i = 0; i < numblocks; ++i) {
loff_t ofs = i << chip->bbt_erase_shift;
+ uint8_t buf[mtd->writesize + mtd->oobsize];
+ struct mtd_oob_ops ops = {
+ .mode = MTD_OPS_RAW,
+ .ooboffs = 0,
+ .datbuf = buf,
+ .len = mtd->writesize,
+ .oobbuf = buf + mtd->writesize,
+ .ooblen = mtd->oobsize,
+ };
- ret = checkbad(chip, ofs);
- if (ret < 0)
+ ret = mtd_read_oob(mtd, ofs, );
+ if (ret < 0) {
+ dev_err(mtd->dev.parent, "Failed to read page at
0x%08x\n", (unsigned int)ofs);
goto out;
+ }
- if (ret) {
+ /*
+* Automatically adding a BBT based on factory BBTs is only
+* sensible if the NAND is pristine. Abort if the first page
+* looks like a bootloader or UBI block.
+*/
+ if (ofs == 0 && is_barebox_arm_head(buf)) {
+ dev_err(mtd->dev.parent, "Flash seems to contain a
barebox image, refusing\n");
+ ret = -EINVAL;
+ goto out;
+ }
+
+ if (ofs == 0 && !memcmp(buf, "UBI#", 4)) {
+ dev_err(mtd->dev.parent, "Flash seems to contain a UBI,
refusing\n");
+ ret = -EINVAL;
+ goto out;
+ }
+
+ if (buf[2000] != 0xff) {
bbt[i >> 2] |= 0x03 << (2 * (i & 0x3));
dev_info(mtd->dev.parent, "Bad eraseblock %d at
0x%08x\n",
i, (unsigned int)ofs);
--
2.43.0
