Message ID | 20211118114659.1282855-1-miquel.raynal@bootlin.com |
---|---|
State | Accepted |
Headers | show |
Series | mtd: Introduce an expert mode for forensics and debugging purposes | expand |
On Thu, 2021-11-18 at 11:46:59 UTC, Miquel Raynal wrote: > When developping NAND controller drivers or when debugging filesystem > corruptions, it is quite common to need hacking locally into the > MTD/NAND core in order to get access to the content of the bad > blocks. Instead of having multiple implementations out there let's > provide a simple yet effective specific MTD-wide debugfs entry to fully > disable these checks on purpose. > > A warning is added to inform the user when this mode gets enabled. > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/next. Miquel
Hi Miquel, On Thu, Nov 18, 2021 at 12:47 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > When developping NAND controller drivers or when debugging filesystem > corruptions, it is quite common to need hacking locally into the > MTD/NAND core in order to get access to the content of the bad > blocks. Instead of having multiple implementations out there let's > provide a simple yet effective specific MTD-wide debugfs entry to fully > disable these checks on purpose. > > A warning is added to inform the user when this mode gets enabled. > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> Thanks for your patch, which is now commit 67b967ddd93d0ed5 ("mtd: Introduce an expert mode for forensics and debugging purposes") in mtd/next. > --- a/drivers/mtd/mtdcore.c > +++ b/drivers/mtd/mtdcore.c > @@ -2365,6 +2365,14 @@ static struct backing_dev_info * __init mtd_bdi_init(const char *name) > return ret ? ERR_PTR(ret) : bdi; > } > > +char *mtd_expert_analysis_warning = const > + "Bad block checks have been entirely disabled.\n" > + "This is only reserved for post-mortem forensics and debug purposes.\n" > + "Never enable this mode if you do not know what you are doing!\n"; > +EXPORT_SYMBOL_GPL(mtd_expert_analysis_warning); Shouldn't this depend on CONFIG_DEBUG_FS? > +bool mtd_expert_analysis_mode; > +EXPORT_SYMBOL_GPL(mtd_expert_analysis_mode); Do you really need to export these two symbols? > + > static struct proc_dir_entry *proc_mtd; > > static int __init init_mtd(void) = > --- a/drivers/mtd/nand/core.c > +++ b/drivers/mtd/nand/core.c > @@ -21,6 +21,9 @@ > */ > bool nanddev_isbad(struct nand_device *nand, const struct nand_pos *pos) > { > + if (WARN_ONCE(mtd_expert_analysis_mode, mtd_expert_analysis_warning)) > + return 0; > + > if (nanddev_bbt_is_initialized(nand)) { > unsigned int entry; > int status; > diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c > index 3d6c6e880520..b3a9bc08b4bb 100644 > --- a/drivers/mtd/nand/raw/nand_base.c > +++ b/drivers/mtd/nand/raw/nand_base.c > @@ -321,6 +321,9 @@ static int nand_isbad_bbm(struct nand_chip *chip, loff_t ofs) > if (nand_region_is_secured(chip, ofs, mtd->erasesize)) > return -EIO; > > + if (WARN_ONCE(mtd_expert_analysis_mode, mtd_expert_analysis_warning)) > + return 0; > + > if (chip->legacy.block_bad) > return chip->legacy.block_bad(chip, ofs); > > diff --git a/drivers/mtd/nand/raw/nand_bbt.c b/drivers/mtd/nand/raw/nand_bbt.c > index b7ad030225f8..ab630af3a309 100644 > --- a/drivers/mtd/nand/raw/nand_bbt.c > +++ b/drivers/mtd/nand/raw/nand_bbt.c > @@ -1455,6 +1455,9 @@ int nand_isbad_bbt(struct nand_chip *this, loff_t offs, int allowbbt) > pr_debug("nand_isbad_bbt(): bbt info for offs 0x%08x: (block %d) 0x%02x\n", > (unsigned int)offs, block, res); > > + if (WARN_ONCE(mtd_expert_analysis_mode, mtd_expert_analysis_warning)) > + return 0; > + These are all the same. What about letting drivers/mtd/mtdcore.c export a simple function mtd_check_expert_analysis_mode() that calls the WARN_ONCE(...) if CONFIG_DEBUG_FS=y, else providing a dummy? The backtrace will identify the caller anyway. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi Geert, geert@linux-m68k.org wrote on Mon, 10 Jan 2022 14:15:27 +0100: > Hi Miquel, > > On Thu, Nov 18, 2021 at 12:47 PM Miquel Raynal > <miquel.raynal@bootlin.com> wrote: > > When developping NAND controller drivers or when debugging filesystem > > corruptions, it is quite common to need hacking locally into the > > MTD/NAND core in order to get access to the content of the bad > > blocks. Instead of having multiple implementations out there let's > > provide a simple yet effective specific MTD-wide debugfs entry to fully > > disable these checks on purpose. > > > > A warning is added to inform the user when this mode gets enabled. > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > Thanks for your patch, which is now commit 67b967ddd93d0ed5 ("mtd: > Introduce an expert mode for forensics and debugging purposes") > in mtd/next. Thanks for reviewing! Unfortunately I've sent the MTD pull-request to Linus this morning so I'll have to address this in subsequent commits. > > --- a/drivers/mtd/mtdcore.c > > +++ b/drivers/mtd/mtdcore.c > > @@ -2365,6 +2365,14 @@ static struct backing_dev_info * __init mtd_bdi_init(const char *name) > > return ret ? ERR_PTR(ret) : bdi; > > } > > > > +char *mtd_expert_analysis_warning = > > const With the function you propose, I'll even have to turn it static. > > + "Bad block checks have been entirely disabled.\n" > > + "This is only reserved for post-mortem forensics and debug purposes.\n" > > + "Never enable this mode if you do not know what you are doing!\n"; > > +EXPORT_SYMBOL_GPL(mtd_expert_analysis_warning); > > Shouldn't this depend on CONFIG_DEBUG_FS? I haven't received any robot warnings about this (generally speaking random configs are quite efficient to trigger those errors) so I believe it is safe? But I'll double check. > > +bool mtd_expert_analysis_mode; > > +EXPORT_SYMBOL_GPL(mtd_expert_analysis_mode); > > Do you really need to export these two symbols? > > > + > > static struct proc_dir_entry *proc_mtd; > > > > static int __init init_mtd(void) > = > > --- a/drivers/mtd/nand/core.c > > +++ b/drivers/mtd/nand/core.c > > @@ -21,6 +21,9 @@ > > */ > > bool nanddev_isbad(struct nand_device *nand, const struct nand_pos *pos) > > { > > + if (WARN_ONCE(mtd_expert_analysis_mode, mtd_expert_analysis_warning)) > > + return 0; > > + > > if (nanddev_bbt_is_initialized(nand)) { > > unsigned int entry; > > int status; > > diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c > > index 3d6c6e880520..b3a9bc08b4bb 100644 > > --- a/drivers/mtd/nand/raw/nand_base.c > > +++ b/drivers/mtd/nand/raw/nand_base.c > > @@ -321,6 +321,9 @@ static int nand_isbad_bbm(struct nand_chip *chip, loff_t ofs) > > if (nand_region_is_secured(chip, ofs, mtd->erasesize)) > > return -EIO; > > > > + if (WARN_ONCE(mtd_expert_analysis_mode, mtd_expert_analysis_warning)) > > + return 0; > > + > > if (chip->legacy.block_bad) > > return chip->legacy.block_bad(chip, ofs); > > > > diff --git a/drivers/mtd/nand/raw/nand_bbt.c b/drivers/mtd/nand/raw/nand_bbt.c > > index b7ad030225f8..ab630af3a309 100644 > > --- a/drivers/mtd/nand/raw/nand_bbt.c > > +++ b/drivers/mtd/nand/raw/nand_bbt.c > > @@ -1455,6 +1455,9 @@ int nand_isbad_bbt(struct nand_chip *this, loff_t offs, int allowbbt) > > pr_debug("nand_isbad_bbt(): bbt info for offs 0x%08x: (block %d) 0x%02x\n", > > (unsigned int)offs, block, res); > > > > + if (WARN_ONCE(mtd_expert_analysis_mode, mtd_expert_analysis_warning)) > > + return 0; > > + > > These are all the same. > > What about letting drivers/mtd/mtdcore.c export a simple function > mtd_check_expert_analysis_mode() that calls the WARN_ONCE(...) if > CONFIG_DEBUG_FS=y, else providing a dummy? > The backtrace will identify the caller anyway. Yep that's a good idea. Thanks, Miquèl
Hi Miquèl, On Mon, Jan 10, 2022 at 3:41 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > geert@linux-m68k.org wrote on Mon, 10 Jan 2022 14:15:27 +0100: > > On Thu, Nov 18, 2021 at 12:47 PM Miquel Raynal > > <miquel.raynal@bootlin.com> wrote: > > > When developping NAND controller drivers or when debugging filesystem > > > corruptions, it is quite common to need hacking locally into the > > > MTD/NAND core in order to get access to the content of the bad > > > blocks. Instead of having multiple implementations out there let's > > > provide a simple yet effective specific MTD-wide debugfs entry to fully > > > disable these checks on purpose. > > > > > > A warning is added to inform the user when this mode gets enabled. > > > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > > > Thanks for your patch, which is now commit 67b967ddd93d0ed5 ("mtd: > > Introduce an expert mode for forensics and debugging purposes") > > in mtd/next. > > Thanks for reviewing! Unfortunately I've sent the MTD pull-request to > Linus this morning so I'll have to address this in subsequent > commits. Np, as long as no out-of-tree modules start using these symbols ;-) This is just one of the things I noticed right before the Xmas and NY holidays, but didn't get to reporting or fixing before... Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi Geert, geert@linux-m68k.org wrote on Mon, 10 Jan 2022 15:57:49 +0100: > Hi Miquèl, > > On Mon, Jan 10, 2022 at 3:41 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > geert@linux-m68k.org wrote on Mon, 10 Jan 2022 14:15:27 +0100: > > > On Thu, Nov 18, 2021 at 12:47 PM Miquel Raynal > > > <miquel.raynal@bootlin.com> wrote: > > > > When developping NAND controller drivers or when debugging filesystem > > > > corruptions, it is quite common to need hacking locally into the > > > > MTD/NAND core in order to get access to the content of the bad > > > > blocks. Instead of having multiple implementations out there let's > > > > provide a simple yet effective specific MTD-wide debugfs entry to fully > > > > disable these checks on purpose. > > > > > > > > A warning is added to inform the user when this mode gets enabled. > > > > > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > > > > > Thanks for your patch, which is now commit 67b967ddd93d0ed5 ("mtd: > > > Introduce an expert mode for forensics and debugging purposes") > > > in mtd/next. > > > > Thanks for reviewing! Unfortunately I've sent the MTD pull-request to > > Linus this morning so I'll have to address this in subsequent > > commits. > > Np, as long as no out-of-tree modules start using these symbols ;-) > > This is just one of the things I noticed right before the Xmas and > NY holidays, but didn't get to reporting or fixing before... BTW if you want to contribute this change you're welcome, otherwise I'll do it when I have a bit of time. Thanks, Miquèl
Hi Geert, geert@linux-m68k.org wrote on Mon, 10 Jan 2022 14:15:27 +0100: > Hi Miquel, > > On Thu, Nov 18, 2021 at 12:47 PM Miquel Raynal > <miquel.raynal@bootlin.com> wrote: > > When developping NAND controller drivers or when debugging filesystem > > corruptions, it is quite common to need hacking locally into the > > MTD/NAND core in order to get access to the content of the bad > > blocks. Instead of having multiple implementations out there let's > > provide a simple yet effective specific MTD-wide debugfs entry to fully > > disable these checks on purpose. > > > > A warning is added to inform the user when this mode gets enabled. > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > Thanks for your patch, which is now commit 67b967ddd93d0ed5 ("mtd: > Introduce an expert mode for forensics and debugging purposes") > in mtd/next. > > > --- a/drivers/mtd/mtdcore.c > > +++ b/drivers/mtd/mtdcore.c > > @@ -2365,6 +2365,14 @@ static struct backing_dev_info * __init mtd_bdi_init(const char *name) > > return ret ? ERR_PTR(ret) : bdi; > > } > > > > +char *mtd_expert_analysis_warning = > > const > > > + "Bad block checks have been entirely disabled.\n" > > + "This is only reserved for post-mortem forensics and debug purposes.\n" > > + "Never enable this mode if you do not know what you are doing!\n"; > > +EXPORT_SYMBOL_GPL(mtd_expert_analysis_warning); > > Shouldn't this depend on CONFIG_DEBUG_FS? > > > +bool mtd_expert_analysis_mode; > > +EXPORT_SYMBOL_GPL(mtd_expert_analysis_mode); > > Do you really need to export these two symbols? > > > + > > static struct proc_dir_entry *proc_mtd; > > > > static int __init init_mtd(void) > = > > --- a/drivers/mtd/nand/core.c > > +++ b/drivers/mtd/nand/core.c > > @@ -21,6 +21,9 @@ > > */ > > bool nanddev_isbad(struct nand_device *nand, const struct nand_pos *pos) > > { > > + if (WARN_ONCE(mtd_expert_analysis_mode, mtd_expert_analysis_warning)) > > + return 0; > > + > > if (nanddev_bbt_is_initialized(nand)) { > > unsigned int entry; > > int status; > > diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c > > index 3d6c6e880520..b3a9bc08b4bb 100644 > > --- a/drivers/mtd/nand/raw/nand_base.c > > +++ b/drivers/mtd/nand/raw/nand_base.c > > @@ -321,6 +321,9 @@ static int nand_isbad_bbm(struct nand_chip *chip, loff_t ofs) > > if (nand_region_is_secured(chip, ofs, mtd->erasesize)) > > return -EIO; > > > > + if (WARN_ONCE(mtd_expert_analysis_mode, mtd_expert_analysis_warning)) > > + return 0; > > + > > if (chip->legacy.block_bad) > > return chip->legacy.block_bad(chip, ofs); > > > > diff --git a/drivers/mtd/nand/raw/nand_bbt.c b/drivers/mtd/nand/raw/nand_bbt.c > > index b7ad030225f8..ab630af3a309 100644 > > --- a/drivers/mtd/nand/raw/nand_bbt.c > > +++ b/drivers/mtd/nand/raw/nand_bbt.c > > @@ -1455,6 +1455,9 @@ int nand_isbad_bbt(struct nand_chip *this, loff_t offs, int allowbbt) > > pr_debug("nand_isbad_bbt(): bbt info for offs 0x%08x: (block %d) 0x%02x\n", > > (unsigned int)offs, block, res); > > > > + if (WARN_ONCE(mtd_expert_analysis_mode, mtd_expert_analysis_warning)) > > + return 0; > > + > > These are all the same. > > What about letting drivers/mtd/mtdcore.c export a simple function > mtd_check_expert_analysis_mode() that calls the WARN_ONCE(...) if > CONFIG_DEBUG_FS=y, else providing a dummy? > The backtrace will identify the caller anyway. I took the time to address your comments. You're right a single exported function is better. However I don't see the need for a CONFIG_DEBUG_FS check here, if unset the boolean will stay false forever, I believe we don't need to bother with it. Thanks, Miquèl
Hi Miquel, On Thu, Jan 27, 2022 at 11:46 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > geert@linux-m68k.org wrote on Mon, 10 Jan 2022 14:15:27 +0100: > > On Thu, Nov 18, 2021 at 12:47 PM Miquel Raynal > > <miquel.raynal@bootlin.com> wrote: > > > When developping NAND controller drivers or when debugging filesystem > > > corruptions, it is quite common to need hacking locally into the > > > MTD/NAND core in order to get access to the content of the bad > > > blocks. Instead of having multiple implementations out there let's > > > provide a simple yet effective specific MTD-wide debugfs entry to fully > > > disable these checks on purpose. > > > > > > A warning is added to inform the user when this mode gets enabled. > > > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > > > Thanks for your patch, which is now commit 67b967ddd93d0ed5 ("mtd: > > Introduce an expert mode for forensics and debugging purposes") > > in mtd/next. > > > > > --- a/drivers/mtd/mtdcore.c > > > +++ b/drivers/mtd/mtdcore.c > > > @@ -2365,6 +2365,14 @@ static struct backing_dev_info * __init mtd_bdi_init(const char *name) > > > return ret ? ERR_PTR(ret) : bdi; > > > } > > > > > > +char *mtd_expert_analysis_warning = > > > > const > > > > > + "Bad block checks have been entirely disabled.\n" > > > + "This is only reserved for post-mortem forensics and debug purposes.\n" > > > + "Never enable this mode if you do not know what you are doing!\n"; > > > +EXPORT_SYMBOL_GPL(mtd_expert_analysis_warning); > > > > Shouldn't this depend on CONFIG_DEBUG_FS? > > > > > +bool mtd_expert_analysis_mode; > > > +EXPORT_SYMBOL_GPL(mtd_expert_analysis_mode); > > > > Do you really need to export these two symbols? > > > > > + > > > static struct proc_dir_entry *proc_mtd; > > > > > > static int __init init_mtd(void) > > = > > > --- a/drivers/mtd/nand/core.c > > > +++ b/drivers/mtd/nand/core.c > > > @@ -21,6 +21,9 @@ > > > */ > > > bool nanddev_isbad(struct nand_device *nand, const struct nand_pos *pos) > > > { > > > + if (WARN_ONCE(mtd_expert_analysis_mode, mtd_expert_analysis_warning)) > > > + return 0; > > > + > > > if (nanddev_bbt_is_initialized(nand)) { > > > unsigned int entry; > > > int status; > > > diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c > > > index 3d6c6e880520..b3a9bc08b4bb 100644 > > > --- a/drivers/mtd/nand/raw/nand_base.c > > > +++ b/drivers/mtd/nand/raw/nand_base.c > > > @@ -321,6 +321,9 @@ static int nand_isbad_bbm(struct nand_chip *chip, loff_t ofs) > > > if (nand_region_is_secured(chip, ofs, mtd->erasesize)) > > > return -EIO; > > > > > > + if (WARN_ONCE(mtd_expert_analysis_mode, mtd_expert_analysis_warning)) > > > + return 0; > > > + > > > if (chip->legacy.block_bad) > > > return chip->legacy.block_bad(chip, ofs); > > > > > > diff --git a/drivers/mtd/nand/raw/nand_bbt.c b/drivers/mtd/nand/raw/nand_bbt.c > > > index b7ad030225f8..ab630af3a309 100644 > > > --- a/drivers/mtd/nand/raw/nand_bbt.c > > > +++ b/drivers/mtd/nand/raw/nand_bbt.c > > > @@ -1455,6 +1455,9 @@ int nand_isbad_bbt(struct nand_chip *this, loff_t offs, int allowbbt) > > > pr_debug("nand_isbad_bbt(): bbt info for offs 0x%08x: (block %d) 0x%02x\n", > > > (unsigned int)offs, block, res); > > > > > > + if (WARN_ONCE(mtd_expert_analysis_mode, mtd_expert_analysis_warning)) > > > + return 0; > > > + > > > > These are all the same. > > > > What about letting drivers/mtd/mtdcore.c export a simple function > > mtd_check_expert_analysis_mode() that calls the WARN_ONCE(...) if > > CONFIG_DEBUG_FS=y, else providing a dummy? > > The backtrace will identify the caller anyway. > > I took the time to address your comments. You're right a single exported > function is better. > > However I don't see the need for a CONFIG_DEBUG_FS check here, if unset > the boolean will stay false forever, I believe we don't need to bother > with it. If CONFIG_DEBUG_FS=n, there is no need for the code or the export, so the check can become a dummy. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi Geert, geert@linux-m68k.org wrote on Thu, 27 Jan 2022 12:07:31 +0100: > Hi Miquel, > > On Thu, Jan 27, 2022 at 11:46 AM Miquel Raynal > <miquel.raynal@bootlin.com> wrote: > > geert@linux-m68k.org wrote on Mon, 10 Jan 2022 14:15:27 +0100: > > > On Thu, Nov 18, 2021 at 12:47 PM Miquel Raynal > > > <miquel.raynal@bootlin.com> wrote: > > > > When developping NAND controller drivers or when debugging filesystem > > > > corruptions, it is quite common to need hacking locally into the > > > > MTD/NAND core in order to get access to the content of the bad > > > > blocks. Instead of having multiple implementations out there let's > > > > provide a simple yet effective specific MTD-wide debugfs entry to fully > > > > disable these checks on purpose. > > > > > > > > A warning is added to inform the user when this mode gets enabled. > > > > > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > > > > > Thanks for your patch, which is now commit 67b967ddd93d0ed5 ("mtd: > > > Introduce an expert mode for forensics and debugging purposes") > > > in mtd/next. > > > > > > > --- a/drivers/mtd/mtdcore.c > > > > +++ b/drivers/mtd/mtdcore.c > > > > @@ -2365,6 +2365,14 @@ static struct backing_dev_info * __init mtd_bdi_init(const char *name) > > > > return ret ? ERR_PTR(ret) : bdi; > > > > } > > > > > > > > +char *mtd_expert_analysis_warning = > > > > > > const > > > > > > > + "Bad block checks have been entirely disabled.\n" > > > > + "This is only reserved for post-mortem forensics and debug purposes.\n" > > > > + "Never enable this mode if you do not know what you are doing!\n"; > > > > +EXPORT_SYMBOL_GPL(mtd_expert_analysis_warning); > > > > > > Shouldn't this depend on CONFIG_DEBUG_FS? > > > > > > > +bool mtd_expert_analysis_mode; > > > > +EXPORT_SYMBOL_GPL(mtd_expert_analysis_mode); > > > > > > Do you really need to export these two symbols? > > > > > > > + > > > > static struct proc_dir_entry *proc_mtd; > > > > > > > > static int __init init_mtd(void) > > > = > > > > --- a/drivers/mtd/nand/core.c > > > > +++ b/drivers/mtd/nand/core.c > > > > @@ -21,6 +21,9 @@ > > > > */ > > > > bool nanddev_isbad(struct nand_device *nand, const struct nand_pos *pos) > > > > { > > > > + if (WARN_ONCE(mtd_expert_analysis_mode, mtd_expert_analysis_warning)) > > > > + return 0; > > > > + > > > > if (nanddev_bbt_is_initialized(nand)) { > > > > unsigned int entry; > > > > int status; > > > > diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c > > > > index 3d6c6e880520..b3a9bc08b4bb 100644 > > > > --- a/drivers/mtd/nand/raw/nand_base.c > > > > +++ b/drivers/mtd/nand/raw/nand_base.c > > > > @@ -321,6 +321,9 @@ static int nand_isbad_bbm(struct nand_chip *chip, loff_t ofs) > > > > if (nand_region_is_secured(chip, ofs, mtd->erasesize)) > > > > return -EIO; > > > > > > > > + if (WARN_ONCE(mtd_expert_analysis_mode, mtd_expert_analysis_warning)) > > > > + return 0; > > > > + > > > > if (chip->legacy.block_bad) > > > > return chip->legacy.block_bad(chip, ofs); > > > > > > > > diff --git a/drivers/mtd/nand/raw/nand_bbt.c b/drivers/mtd/nand/raw/nand_bbt.c > > > > index b7ad030225f8..ab630af3a309 100644 > > > > --- a/drivers/mtd/nand/raw/nand_bbt.c > > > > +++ b/drivers/mtd/nand/raw/nand_bbt.c > > > > @@ -1455,6 +1455,9 @@ int nand_isbad_bbt(struct nand_chip *this, loff_t offs, int allowbbt) > > > > pr_debug("nand_isbad_bbt(): bbt info for offs 0x%08x: (block %d) 0x%02x\n", > > > > (unsigned int)offs, block, res); > > > > > > > > + if (WARN_ONCE(mtd_expert_analysis_mode, mtd_expert_analysis_warning)) > > > > + return 0; > > > > + > > > > > > These are all the same. > > > > > > What about letting drivers/mtd/mtdcore.c export a simple function > > > mtd_check_expert_analysis_mode() that calls the WARN_ONCE(...) if > > > CONFIG_DEBUG_FS=y, else providing a dummy? > > > The backtrace will identify the caller anyway. > > > > I took the time to address your comments. You're right a single exported > > function is better. > > > > However I don't see the need for a CONFIG_DEBUG_FS check here, if unset > > the boolean will stay false forever, I believe we don't need to bother > > with it. > > If CONFIG_DEBUG_FS=n, there is no need for the code or the export, > so the check can become a dummy. Agreed, but I truly don't like using #ifdefs when I can skip these, I think they darken the code and prevent good build coverage. Using if (IS_ENABLED()) is an option but would not bring the memory savings that we could expect with an #ifdef, so I don't see the point here. Should we use unlikely() to give branch predictors a clue about what is going to happen? Thanks, Miquèl
Hi Miquel, On Thu, Jan 27, 2022 at 12:18 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > geert@linux-m68k.org wrote on Thu, 27 Jan 2022 12:07:31 +0100: > > On Thu, Jan 27, 2022 at 11:46 AM Miquel Raynal > > <miquel.raynal@bootlin.com> wrote: > > > > What about letting drivers/mtd/mtdcore.c export a simple function > > > > mtd_check_expert_analysis_mode() that calls the WARN_ONCE(...) if > > > > CONFIG_DEBUG_FS=y, else providing a dummy? > > > > The backtrace will identify the caller anyway. > > > > > > I took the time to address your comments. You're right a single exported > > > function is better. > > > > > > However I don't see the need for a CONFIG_DEBUG_FS check here, if unset > > > the boolean will stay false forever, I believe we don't need to bother > > > with it. > > > > If CONFIG_DEBUG_FS=n, there is no need for the code or the export, > > so the check can become a dummy. > > Agreed, but I truly don't like using #ifdefs when I can skip these, I > think they darken the code and prevent good build coverage. > > Using if (IS_ENABLED()) is an option but would not bring the memory > savings that we could expect with an #ifdef, so I don't see the point > here. > > Should we use unlikely() to give branch predictors a clue about what is > going to happen? That will be thwarted by the out-of-line call to the exported symbol. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c index 9186268d361b..4d367d5c4021 100644 --- a/drivers/mtd/mtdcore.c +++ b/drivers/mtd/mtdcore.c @@ -2365,6 +2365,14 @@ static struct backing_dev_info * __init mtd_bdi_init(const char *name) return ret ? ERR_PTR(ret) : bdi; } +char *mtd_expert_analysis_warning = + "Bad block checks have been entirely disabled.\n" + "This is only reserved for post-mortem forensics and debug purposes.\n" + "Never enable this mode if you do not know what you are doing!\n"; +EXPORT_SYMBOL_GPL(mtd_expert_analysis_warning); +bool mtd_expert_analysis_mode; +EXPORT_SYMBOL_GPL(mtd_expert_analysis_mode); + static struct proc_dir_entry *proc_mtd; static int __init init_mtd(void) @@ -2388,6 +2396,8 @@ static int __init init_mtd(void) goto out_procfs; dfs_dir_mtd = debugfs_create_dir("mtd", NULL); + debugfs_create_bool("expert_analysis_mode", 0600, dfs_dir_mtd, + &mtd_expert_analysis_mode); return 0; diff --git a/drivers/mtd/nand/core.c b/drivers/mtd/nand/core.c index 5e13a03d2b32..33c7788d76c2 100644 --- a/drivers/mtd/nand/core.c +++ b/drivers/mtd/nand/core.c @@ -21,6 +21,9 @@ */ bool nanddev_isbad(struct nand_device *nand, const struct nand_pos *pos) { + if (WARN_ONCE(mtd_expert_analysis_mode, mtd_expert_analysis_warning)) + return 0; + if (nanddev_bbt_is_initialized(nand)) { unsigned int entry; int status; diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c index 3d6c6e880520..b3a9bc08b4bb 100644 --- a/drivers/mtd/nand/raw/nand_base.c +++ b/drivers/mtd/nand/raw/nand_base.c @@ -321,6 +321,9 @@ static int nand_isbad_bbm(struct nand_chip *chip, loff_t ofs) if (nand_region_is_secured(chip, ofs, mtd->erasesize)) return -EIO; + if (WARN_ONCE(mtd_expert_analysis_mode, mtd_expert_analysis_warning)) + return 0; + if (chip->legacy.block_bad) return chip->legacy.block_bad(chip, ofs); diff --git a/drivers/mtd/nand/raw/nand_bbt.c b/drivers/mtd/nand/raw/nand_bbt.c index b7ad030225f8..ab630af3a309 100644 --- a/drivers/mtd/nand/raw/nand_bbt.c +++ b/drivers/mtd/nand/raw/nand_bbt.c @@ -1455,6 +1455,9 @@ int nand_isbad_bbt(struct nand_chip *this, loff_t offs, int allowbbt) pr_debug("nand_isbad_bbt(): bbt info for offs 0x%08x: (block %d) 0x%02x\n", (unsigned int)offs, block, res); + if (WARN_ONCE(mtd_expert_analysis_mode, mtd_expert_analysis_warning)) + return 0; + switch (res) { case BBT_BLOCK_GOOD: return 0; diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h index f5e7dfc2e4e9..1ffa933121f6 100644 --- a/include/linux/mtd/mtd.h +++ b/include/linux/mtd/mtd.h @@ -711,4 +711,7 @@ static inline int mtd_is_bitflip_or_eccerr(int err) { unsigned mtd_mmap_capabilities(struct mtd_info *mtd); +extern char *mtd_expert_analysis_warning; +extern bool mtd_expert_analysis_mode; + #endif /* __MTD_MTD_H__ */
When developping NAND controller drivers or when debugging filesystem corruptions, it is quite common to need hacking locally into the MTD/NAND core in order to get access to the content of the bad blocks. Instead of having multiple implementations out there let's provide a simple yet effective specific MTD-wide debugfs entry to fully disable these checks on purpose. A warning is added to inform the user when this mode gets enabled. Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> --- drivers/mtd/mtdcore.c | 10 ++++++++++ drivers/mtd/nand/core.c | 3 +++ drivers/mtd/nand/raw/nand_base.c | 3 +++ drivers/mtd/nand/raw/nand_bbt.c | 3 +++ include/linux/mtd/mtd.h | 3 +++ 5 files changed, 22 insertions(+)