Re: [PATCH 1/1] mtd/intel-spi: Support cmdline-based partition

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi ron,

ron minnich <rminnich@gmail.com> wrote on Mon, 27 Apr 2020 13:31:35
-0700:

> On Mon, Apr 27, 2020 at 2:50 AM Boris Brezillon
> <boris.brezillon@collabora.com> wrote:
> >
> > On Mon, 27 Apr 2020 11:16:23 +0200
> > Boris Brezillon <boris.brezillon@collabora.com> wrote:
> >  
> > > Hi all,
> > >
> > > On Wed, 1 Apr 2020 09:41:48 +0200
> > > Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > >  
> > > > Hi ron,
> > > >
> > > > ron minnich <rminnich@gmail.com> wrote on Mon, 30 Mar 2020 08:53:22
> > > > -0700:
> > > >  
> > > > > On Mon, Mar 30, 2020 at 12:27 AM Miquel Raynal
> > > > > <miquel.raynal@bootlin.com> wrote:
> > > > >  
> > > > > > Would it be hard to support an extra ':' after the MTD device name?
> > > > > > This way would would allow anything inside the optional '(' ')' but
> > > > > > would keep the trailing ':'.
> > > > > >
> > > > > > toTay:
> > > > > >         mtdparts=name:part1,part2
> > > > > >
> > > > > > So:
> > > > > >         mtdparts=(0000:00:1f.5):25165824(BIOS),-(squashfs)  
> > > > >
> > > > >
> > > > > I thought about that ':' too. It does add a bit more to the code, and
> > > > > a bit more in the way of error cases. I always worry, when code is
> > > > > going into flash,
> > > > > about errors where something looks close to right but is wrong. (says
> > > > > the person who just typed it instead of is a few times :-)
> > > > >
> > > > > What if we did this:
> > > > > mtdparts=[0000:00:1f.5]25165824(BIOS),-(squashfs)
> > > > >
> > > > > Is the "]" 'enough different' that we do not need the ':'?
> > > > >
> > > > > I kind of like the [] better anyway as it makes the mtdid stand out a
> > > > > bit more from the part names? But is it enough that we don't need the
> > > > > ':'? Would you still prefer the () as opposed to the []?  
> > > >
> > > > I like the [] as well, maybe more than () because at least it does not
> > > > conflict with the partition names. But I really prefer keeping the : if
> > > > the code is still readable.
> > > >
> > > > It is much easier to explain to people : "if you have a : in the name,
> > > > enclose it with []".  
> > >
> > > Sorry to chime in so late in the discussion, but I wonder if any of
> > > that is necessary. Can't we just split the string per device (split
> > > strings every time we see a ';'), and then find the last ':' in each of
> > > those strings and consider everything before that last ':' to be the MTD
> > > name. That should work even if the MTD name contains one or more ':'.
> > >
> > > Don't get me wrong, I'm perfectly fine with intel enclosing the PCI
> > > address in [] to make it clearer, but I see that other drivers use ':'
> > > in their MTD device names (the atmel raw NAND controller driver to name
> > > one), so I think it'd be good to make the mtd part parsing robust to
> > > this use case.  
> >
> > I just gave it a try and the following patch should solve the problem
> > (only compile-tested). As I said previously, it doesn't prevent you from
> > enclosing the PCI address in [] if you think it's clearer, but I think
> > the problem should be addressed in the cmdline parser anyway.
> >  
> > --->8---  
> > From 08b30597dd73efd9c4c8d1906ab02a9540875419 Mon Sep 17 00:00:00 2001
> > From: Boris Brezillon <boris.brezillon@collabora.com>
> > Date: Mon, 27 Apr 2020 11:44:50 +0200
> > Subject: [PATCH] mtd: parser: cmdline: Support MTD names containing one or
> >  more colons
> >
> > Looks like some drivers define MTD names with a colon in it, thus
> > making mtdpart= parsing impossible. Let's fix the parser to gracefully
> > handle that case: the last ':' in a partition definition sequence is
> > considered instead of the first one.
> >
> > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> > ---
> >  drivers/mtd/parsers/cmdlinepart.c | 23 ++++++++++++++++++++---
> >  1 file changed, 20 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/mtd/parsers/cmdlinepart.c b/drivers/mtd/parsers/cmdlinepart.c
> > index c86f2db8c882..0625b25620ca 100644
> > --- a/drivers/mtd/parsers/cmdlinepart.c
> > +++ b/drivers/mtd/parsers/cmdlinepart.c
> > @@ -218,12 +218,29 @@ static int mtdpart_setup_real(char *s)
> >                 struct cmdline_mtd_partition *this_mtd;
> >                 struct mtd_partition *parts;
> >                 int mtd_id_len, num_parts;
> > -               char *p, *mtd_id;
> > +               char *p, *mtd_id, *semicol;
> > +
> > +               /*
> > +                * Replace the first ';' by a NULL char so strrchr can work
> > +                * properly.
> > +                */
> > +               semicol = strchr(s, ';');
> > +               if (semicol)
> > +                       *semicol = '\0';
> >
> >                 mtd_id = s;
> >
> > -               /* fetch <mtd-id> */
> > -               p = strchr(s, ':');
> > +               /*
> > +                * fetch <mtd-id>. We use strrchr to ignore all ':' that could
> > +                * be present in the MTD name, only the last one is interpreted
> > +                * as an <mtd-id>/<part-definition> separator.
> > +                */
> > +               p = strrchr(s, ':');
> > +
> > +               /* Restore the ';' now. */
> > +               if (semicol)
> > +                       *semicol = ';';
> > +
> >                 if (!p) {
> >                         pr_err("no mtd-id\n");
> >                         return -EINVAL;  
> 
> 
> Tested-by: rminnich@google.com

Actually as we use tools to collect patches we need them to be sent
properly. It means: can you please copy this patch in a .txt file, then
apply it with git am, then amend it with your signed-off-by and
eventually also add your Tested-by in this case before sending it to
the ML, as usual.

Boris SoB should appear first in the list as he is the author, then
yours as you are the sender. Then your Tested-by.


Thanks,
Miquèl

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