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

[Miquel Raynal]

Hi Geert,

geert at 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 at bootlin.com> wrote:
> > geert at 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 at 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 at 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