Linux-mtd Archive on lore.kernel.org
 help / color / Atom feed
From: Miquel Raynal <miquel.raynal@bootlin.com>
To: ron minnich <rminnich@gmail.com>
Cc: linux-mtd@lists.infradead.org,
	Boris Brezillon <boris.brezillon@collabora.com>,
	Mika Westerberg <mika.westerberg@linux.intel.com>,
	Vignesh Raghavendra <vigneshr@ti.com>,
	Richard Weinberger <richard@nod.at>
Subject: Re: [PATCH 1/1] mtd/intel-spi: Support cmdline-based partition
Date: Mon, 27 Apr 2020 23:20:30 +0200
Message-ID: <20200427232030.478f0048@xps13> (raw)
In-Reply-To: <CAP6exYJnKjzvC1g1qa8Odxg=HydD-CVbcz9yPi93Sbdg2K+bfw@mail.gmail.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/

  reply index

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-23 19:58 ron minnich
2020-03-23 23:19 ` ron minnich
2020-03-25 17:34   ` ron minnich
2020-03-25 17:34     ` ron minnich
2020-03-27 15:48       ` ron minnich
2020-03-27 15:56         ` Mika Westerberg
2020-03-27 16:19           ` Miquel Raynal
2020-03-27 16:48             ` Mika Westerberg
2020-03-27 16:52               ` Miquel Raynal
2020-03-27 17:05                 ` ron minnich
2020-03-27 17:16                   ` Mika Westerberg
2020-03-27 17:39                     ` ron minnich
2020-03-27 23:53                       ` ron minnich
2020-03-30  6:08                         ` Mika Westerberg
2020-03-30  7:27                           ` Miquel Raynal
2020-03-30 15:53                             ` ron minnich
2020-04-01  7:41                               ` Miquel Raynal
2020-04-01 15:42                                 ` ron minnich
2020-04-01 19:49                                   ` ron minnich
2020-04-27  9:16                                 ` Boris Brezillon
2020-04-27  9:49                                   ` Boris Brezillon
2020-04-27 14:41                                     ` Miquel Raynal
2020-04-27 18:48                                       ` ron minnich
2020-04-27 18:50                                         ` ron minnich
2020-04-27 18:56                                           ` Miquel Raynal
2020-04-27 18:59                                             ` ron minnich
2020-04-27 19:21                                             ` Boris Brezillon
2020-04-27 19:31                                               ` ron minnich
2020-04-27 20:31                                     ` ron minnich
2020-04-27 21:20                                       ` Miquel Raynal [this message]
2020-03-27 16:53             ` ron minnich
2020-03-27 16:15         ` Miquel Raynal

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200427232030.478f0048@xps13 \
    --to=miquel.raynal@bootlin.com \
    --cc=boris.brezillon@collabora.com \
    --cc=linux-mtd@lists.infradead.org \
    --cc=mika.westerberg@linux.intel.com \
    --cc=richard@nod.at \
    --cc=rminnich@gmail.com \
    --cc=vigneshr@ti.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-mtd Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-mtd/0 linux-mtd/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-mtd linux-mtd/ https://lore.kernel.org/linux-mtd \
		linux-mtd@lists.infradead.org
	public-inbox-index linux-mtd

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-mtd


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git