[PATCH] mtd: Introduce an expert mode for forensics and debugging purposes

[PATCH] mtd: Introduce an expert mode for forensics and debugging purposes

[Miquel Raynal]

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

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__ */
-- 
2.27.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] mtd: Introduce an expert mode for forensics and debugging purposes

[Miquel Raynal]

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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] mtd: Introduce an expert mode for forensics and debugging purposes

[Geert Uytterhoeven]

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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] mtd: Introduce an expert mode for forensics and debugging purposes

[Miquel Raynal]

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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] mtd: Introduce an expert mode for forensics and debugging purposes

[Geert Uytterhoeven]

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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] mtd: Introduce an expert mode for forensics and debugging purposes

[Miquel Raynal]

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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] mtd: Introduce an expert mode for forensics and debugging purposes

[Miquel Raynal]

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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] mtd: Introduce an expert mode for forensics and debugging purposes

[Geert Uytterhoeven]

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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] mtd: Introduce an expert mode for forensics and debugging purposes

[Miquel Raynal]

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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] mtd: Introduce an expert mode for forensics and debugging purposes

[Geert Uytterhoeven]

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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/