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

On Sat, 26 Aug 2017 00:09:49 +0200
Andrea Adami <andrea.adami@...il.com> wrote:

> On Fri, Aug 25, 2017 at 11:48 PM, Boris Brezillon
> <boris.brezillon@...e-electrons.com> wrote:
> > 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?
> >  
> 
> Boris,
> 
> I am really sorry but I won't answer again in detail about the issues
> with repartitioning and the fact that I have no control over other
> distributions, etc. etc.

And do you have control on what those distributions enable in there
kernel? Will they really enable this SHARPSL part parser?

> 
> I don't understand where is your problem now, after 6 reviews.

My concerns didn't vanish, and I still don't buy your different
arguments which are either non-technical or are not related to the
suggestions we do. In this regards, this thread is a good example, when
Brian suggests to use a userspace program to parse partinfo and create
partitions with the BLKPG ioctl, you answer that you'll require one
initramfs per board, which AFAICT is not true.

> I'll be happy to send v7 with the added oob check and I'll have to add
> you to the authors now :)

Hell no! Send a v7 if you want but don't flag me as one of the author.

> 
> My point of view is:
> 1- Does Zaurus devices exist under arm/mach-pxa  YES
> 2- Are the devices maintained                                   YES
> 3- Is a new module/functionality provided                  YES
> 
> What is wrong in this?

What is wrong?! Maybe the fact that you're trying to add support for
something that sharp didn't even dare discussing with the community
when they designed it. Also, no one seems to know enough about this FTL
to explain some of the choices they made. And last, it seems this FTL
was designed with old NANDs in mind (those 16K eraseblock NANDs) and
then hacked to support bigger chips, which is not a good sign.

Now, I'd like to stop this discussion here because we clearly disagree
and I don't think one can convince the other.
As I said from the beginning, if other MTD maintainers are fine with
this parser I won't block its inclusion. I've actually done more than I
initially planned to do: I reviewed the code, but don't expect me to
agree with what you're doing, because I won't.

Regards,

Boris

Powered by blists - more mailing lists