Re: [PATCH v6] mtd: sharpslpart: Add sharpslpart partition parser

[Brian Norris]

On Thu, Aug 24, 2017 at 01:27:10PM +0200, Boris Brezillon wrote:
> On Thu, 24 Aug 2017 12:30:02 +0200
> Andrea Adami <andrea.adami@xxxxxxxxx> wrote:
> 
> > On Thu, Aug 24, 2017 at 12:04 PM, Boris Brezillon
> > <boris.brezillon@xxxxxxxxxxxxxxxxxx> wrote:
> > > On Thu, 24 Aug 2017 11:19:56 +0200
> > > Andrea Adami <andrea.adami@xxxxxxxxx> wrote:

...

> > >> >> +     /* create physical-logical table */
> > >> >> +     for (block_num = 0; block_num < phymax; block_num++) {
> > >> >> +             block_adr = block_num * mtd->erasesize;
> > >> >> +
> > >> >> +             if (mtd_block_isbad(mtd, block_adr))
> > >> >> +                     continue;
> > >> >> +
> > >> >> +             if (sharpsl_nand_read_oob(mtd, block_adr, mtd->oobsize, oob))
> > >> >> +                     continue;
> > >> >> +
> > >> >> +             /* get logical block */
> > >> >> +             log_num = sharpsl_nand_get_logical_num(oob);
> > >> >> +
> > >> >> +             /* FTL is not used? Exit here if the oob fingerprint is wrong */
> > >> >> +             if (log_num == UINT_MAX) {
> > >> >> +                     pr_info("sharpslpart: Sharp SL FTL not found.\n");
> > >> >> +                     ret = -EINVAL;
> > >> >> +                     goto exit;
> > >> >> +             }  
> > >
> > > Okay, I overlooked that part. Why do you exit if there's a fingerprint
> > > mismatch? Can't you just ignore this physical block and continue
> > > scanning the remaining ones?  
> > 
> > Norris asked to quit immediately in this case.
> > https://patchwork.kernel.org/patch/9758361/

I didn't specifically ask for you to quit in *that* case. Quoting myself
here, as you did:

> > "I wouldn't expect people to want to use this parser, but if we have a
> > quick way to say "this doesn't match, skip me", then that would be
> > helpful."
> > "We don't want to waste too much time scanning for this partition
> > table if possible."

That means, is there something (not necessarily writting in the
"original code" that you're massaging) that could be used to reliably
detect that this is/isn't a valid "Sharp FTL"? I don't think the check
you wrote is a good one. Particularly, you *don't* want to just abort
completely because there's one corrupt block. This check is a
reliability check (so you can possibly ignore old/bad copies and skip
onto better blocks), not a validity check. It is counter-productive to
abort here, IIUC.

> Actually, you don't save much by exiting on "bad OOB fingerprint". If
> you look at the code you'll see that the only thing you check is
> whether some oob portions are equal or not, and most of the time the
> OOB area is left untouched by the upper layer, which means all free
> bytes will be left to 0xff, which in turn means the "is fingerprint
> good?" test will pass.

Agreed.

I'd drop this "abort early" check and just admit that it's not possible
to do what I asked.

> > Now we are quitting ever before checking for parity erors ...
> 
> Honestly, I'd recommend not using this parser for anything but SHARPSL
> platforms, so I don't think we care much about the "scan all blocks"
> overhead.

Sounds about right.

> Moreover, the sharpsl parser is the last one in the
> part_parsers list, so it should be quite fast if the user specifies a
> valid mtdparts= on the cmdline or valid partitions in the DT.

Brian

P.S. I alluded to it earlier, but I figured I should write it down
properly here sometime, as food for thought; you don't actually need any
of this parser at all if you're willing to contruct an initramfs that
will do the parsing in user space (e.g., some scripting and 'nanddump';
or link to libmtd) and then add partitions yourself (e.g., with
'mtdpart add ...', or calling the BLKPG ioctls directly). This would
just require you have a kernel with CONFIG_MTD_PARTITIONED_MASTER=y.