Message-ID: <20170825234816.2d6d31b4@bbrezillon>
Date:   Fri, 25 Aug 2017 23:48:16 +0200
From:   Boris Brezillon <boris.brezillon@...e-electrons.com>
To:     Andrea Adami <andrea.adami@...il.com>
Cc:     Brian Norris <computersforpeace@...il.com>,
        linux-mtd@...ts.infradead.org,
        David Woodhouse <dwmw2@...radead.org>,
        Marek Vasut <marek.vasut@...il.com>,
        Richard Weinberger <richard@....at>,
        Cyrille Pitchen <cyrille.pitchen@...ev4u.fr>,
        Rafał Miłecki <rafal@...ecki.pl>,
        Haojian Zhuang <haojian.zhuang@...il.com>,
        Dmitry Eremin-Solenikov <dbaryshkov@...il.com>,
        Robert Jarzmik <robert.jarzmik@...e.fr>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v6] mtd: sharpslpart: Add sharpslpart partition parser

On Fri, 25 Aug 2017 19:50:25 +0200
Andrea Adami <andrea.adami@...il.com> wrote:

> On Fri, Aug 25, 2017 at 6:53 AM, Brian Norris
> <computersforpeace@...il.com> wrote:
> > 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@...il.com> wrote:
> >>  
> >> > On Thu, Aug 24, 2017 at 12:04 PM, Boris Brezillon
> >> > <boris.brezillon@...e-electrons.com> wrote:  
> >> > > On Thu, 24 Aug 2017 11:19:56 +0200
> >> > > Andrea Adami <andrea.adami@...il.com> 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.  
> 
> Ok then, I have misunderstood you. The only time-saving would be to
> skip the creation of the table.
> In the specific cases, the older devices with erasesize 16KiB have
> just 448 blocks and the routine doesn't really slow down.
> I can imagine it would be a breeze for modern devices.
> 
> I will just return -EINVAl and this error, as well as the eventual
> parity-check error on a specific block, will be cut-off by the checks
> and the cycle will move to next block.
> 
> So I understand the sharpsl_nand_check_ooblayout()  could be also avoided.
> Please confirm this, thanks.

No, that check is still needed to bail out if someone tries to use
this parser with an incompatible NAND controller/ECC engine. Though it
should only be called once from your sharpsl_parse_mtd_partitions()
function (one of the first thing you should do actually).

> 
> >  
> >> > 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.  
> 
> Brian, the first problem in this approach is the size: there are only
> 1264KiB available for the zImage.
> We accepted the challenge and wrote linux-kexecboot bootloader, a
> kernel + couple of klibc-static binaries in the initramfs and we
> special-cased the Zaurus adding the partition parser in userspace.
> 
> Now, as widely discussed before, there are limits to this solution:
> - first, we cannot oblige to flash this kernel+initramfs.

Just like you cannot oblige people to update to a recent/mainline
kernel.

> - second, we need to build one kernel+initramfs for each model

Why is that? If you have a userspace implementation of your part-parser
only one initramfs is needed, or am I missing something?

> - third, the machines are really the same from kernel pov, just mtdparts differ

I don't get that one, sorry. If you have a userspace tool in your
initramfs that creates the partitions from the partinfo definitions
using BLKPG ioctls you'll still be able to keep one kernel and various
partitioning.

> 
> Soon we'll test a single kernel for the pxa25x and for the pxa27x models.

Still don't see the link with the suggestion made by Brian.

> 
> Honestly I did just glimpse at BLKPG ioctl because resizing for mtd
> was added recently (iirc).
> As maintainer for the cross-toolchain and of kexecboot I am of the
> opinion that the cleanest solution is the in-kernel partition parser.

We already had this discussion. As a maintainer of the MTD subsystem
I'm concerned about adding support for an old FTL and part parser that
has never been mainlined before and more importantly which you don't
seem to understand.
The question is, who is responsible for the code if something does not
work. As I said many times, I'm not against adding new FTLs or
part-parsers in the kernel, just find it worrisome that you failed to
answer to some question about how it's supposed to work.

> 
> Thanks for your time reviewing this...is just a partition parser for
> some of the first commercial linux handhelds.

Come on, not this argument again.

On a final note, I'd like to say I spend time reviewing the code, and
it looks good overall, but my concern about the FTL design still stands
and I keep thinking that it's not such a good idea to support it in
mainline just because some old devices used to use this FTL in their
proprietary firmware. If people want to update their kernels why not
forcing them to use an initramfs that hides all the ugly things in some
specific userspace tools?

Powered by blists - more mailing lists