[PATCH 1/1] mtd: add | as a separator after mtd-id

[PATCH 1/1] mtd: add | as a separator after mtd-id

[ron minnich]

The MTD subsystem can support command-line defined partitions
for one or more MTD devices.

The format is:
 * mtdparts=<mtddef>[;<mtddef]
 * <mtddef>  := <mtd-id>:<partdef>[,<partdef>]

The ':' currently separates the id from the partdef.

The mtdparts can define more than one part, in which case
there will be more than one <mtd-id>:<partdef> component.

The problem comes in with newer systems which have MTDs
attached to a PCI device, which has a PCI name including
several :'s, e.g. 0000:00:1f.5 on an Intel chipset. Although
this is largely an x86 problem at the moment, PCI is coming
to newer ARM systems, and they will hit this issue in future.

There are two : in the name alone. strchr is used to find
the <mtd-id>, and in this case it will return the wrong
result. Using strrchr is not an option, as there may
be more than one mtddef in the argument.

This patch defines a new delimiter, |, to seperate
the <mtd-id> from the <partdef>. | is rarely used
in device names, so seems a reasonable choice.

The code first searches for | and, if that fails, searches
for the old :. Eventually, it ought to be possible to remove
the support for : entirely, but since mtdparts are also defined
in FLASH in the device tree on many ARM boards, wholesale removal
is not yet practical.

This code has been used on real hardware and allowed us to use a
squashfs in SPI-NOR flash as a root file system, with partitions
defined on the cmdline.

Signed-off-by: Ronald G. Minnich <rminnich@google.com>
Change-Id: Ifce3627cb03247bf9e54c8b19d24b60baeed2ec3
---
 drivers/mtd/parsers/cmdlinepart.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/parsers/cmdlinepart.c
b/drivers/mtd/parsers/cmdlinepart.c
index c86f2db8c882..eca8ec026d89 100644
--- a/drivers/mtd/parsers/cmdlinepart.c
+++ b/drivers/mtd/parsers/cmdlinepart.c
@@ -223,7 +223,14 @@ static int mtdpart_setup_real(char *s)
         mtd_id = s;

         /* fetch <mtd-id> */
-        p = strchr(s, ':');
+        p = strchr(s, '|');
+        if (!p) {
+            /*
+             * ':' is the older separator, which conflicts
+             * with PCI IDs T:B:D.F; too many  :'s!
+             */
+            p = strchr(s, ':');
+        }
         if (!p) {
             pr_err("no mtd-id\n");
             return -EINVAL;
-- 
2.25.1.696.g5e7596f4ac-goog

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

Re: [PATCH 1/1] mtd: add | as a separator after mtd-id

[ron minnich]

Anyone? This will be going into use at Google internally and I'd like
to get it upstream.

The only other option that would work is to take the pci-format names
created by intel-spi-pci that have : in them and change the : to '.'.
Is that more acceptable?

On Fri, Mar 20, 2020 at 1:21 PM ron minnich <rminnich@gmail.com> wrote:
>
> The MTD subsystem can support command-line defined partitions
> for one or more MTD devices.
>
> The format is:
>  * mtdparts=<mtddef>[;<mtddef]
>  * <mtddef>  := <mtd-id>:<partdef>[,<partdef>]
>
> The ':' currently separates the id from the partdef.
>
> The mtdparts can define more than one part, in which case
> there will be more than one <mtd-id>:<partdef> component.
>
> The problem comes in with newer systems which have MTDs
> attached to a PCI device, which has a PCI name including
> several :'s, e.g. 0000:00:1f.5 on an Intel chipset. Although
> this is largely an x86 problem at the moment, PCI is coming
> to newer ARM systems, and they will hit this issue in future.
>
> There are two : in the name alone. strchr is used to find
> the <mtd-id>, and in this case it will return the wrong
> result. Using strrchr is not an option, as there may
> be more than one mtddef in the argument.
>
> This patch defines a new delimiter, |, to seperate
> the <mtd-id> from the <partdef>. | is rarely used
> in device names, so seems a reasonable choice.
>
> The code first searches for | and, if that fails, searches
> for the old :. Eventually, it ought to be possible to remove
> the support for : entirely, but since mtdparts are also defined
> in FLASH in the device tree on many ARM boards, wholesale removal
> is not yet practical.
>
> This code has been used on real hardware and allowed us to use a
> squashfs in SPI-NOR flash as a root file system, with partitions
> defined on the cmdline.
>
> Signed-off-by: Ronald G. Minnich <rminnich@google.com>
> Change-Id: Ifce3627cb03247bf9e54c8b19d24b60baeed2ec3
> ---
>  drivers/mtd/parsers/cmdlinepart.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mtd/parsers/cmdlinepart.c
> b/drivers/mtd/parsers/cmdlinepart.c
> index c86f2db8c882..eca8ec026d89 100644
> --- a/drivers/mtd/parsers/cmdlinepart.c
> +++ b/drivers/mtd/parsers/cmdlinepart.c
> @@ -223,7 +223,14 @@ static int mtdpart_setup_real(char *s)
>          mtd_id = s;
>
>          /* fetch <mtd-id> */
> -        p = strchr(s, ':');
> +        p = strchr(s, '|');
> +        if (!p) {
> +            /*
> +             * ':' is the older separator, which conflicts
> +             * with PCI IDs T:B:D.F; too many  :'s!
> +             */
> +            p = strchr(s, ':');
> +        }
>          if (!p) {
>              pr_err("no mtd-id\n");
>              return -EINVAL;
> --
> 2.25.1.696.g5e7596f4ac-goog

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

Re: [PATCH 1/1] mtd: add | as a separator after mtd-id

[Miquel Raynal]

Hi Ronald,

ron minnich <rminnich@gmail.com> wrote on Sat, 21 Mar 2020 08:44:07
-0700:

> Anyone? This will be going into use at Google internally and I'd like
> to get it upstream.
> 
> The only other option that would work is to take the pci-format names
> created by intel-spi-pci that have : in them and change the : to '.'.
> Is that more acceptable?

One important thing to note: Bootloaders share the same mtdparts
definition and should be updated if we decide to support a new
separator. U-boot and Barebox at least.

I think changing just Intel's PCI driver name would be much more
practical for us because I don't find the '|' separator being
descriptive at all.

However, I don't have a strong position and I would welcome
Richard, Vignesh, Tudor and Boris' point of view.

Thanks,
Miquèl

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

Re: [PATCH 1/1] mtd: add | as a separator after mtd-id

[ron minnich]

I agree with you on changing the PCI driver name, as opposed to this
change. I don't like '|' very much either.

I am thinking just to change ':' to '.', e.g.
0000:00:1f.3 -> 0000.00.1f.3

It is an extremely simple change --  add one for loop in the pci map
code -- and nothing else need change.

If this sounds good to you, I'll send a new 2-patch series with that
change and with the intel-spi driver changed to show how one can use
command line partitioning?

Also, as I am coming back to this after a very long time, how do you
like your patch series to look? It seems the git command to generate
these creates 3 files, the first numbered 0 with no code in it. My
reading of the docs implies sending this no-code email is not a good
idea? Any recommendations here?

thanks very much for your comment!

ron

On Sun, Mar 22, 2020 at 4:09 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> Hi Ronald,
>
> ron minnich <rminnich@gmail.com> wrote on Sat, 21 Mar 2020 08:44:07
> -0700:
>
> > Anyone? This will be going into use at Google internally and I'd like
> > to get it upstream.
> >
> > The only other option that would work is to take the pci-format names
> > created by intel-spi-pci that have : in them and change the : to '.'.
> > Is that more acceptable?
>
> One important thing to note: Bootloaders share the same mtdparts
> definition and should be updated if we decide to support a new
> separator. U-boot and Barebox at least.
>
> I think changing just Intel's PCI driver name would be much more
> practical for us because I don't find the '|' separator being
> descriptive at all.
>
> However, I don't have a strong position and I would welcome
> Richard, Vignesh, Tudor and Boris' point of view.
>
> Thanks,
> Miquèl

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

Re: [PATCH 1/1] mtd: add | as a separator after mtd-id

[ron minnich]

ah nvm looked a bit more at the list and got my answer on patch series.


On Sun, Mar 22, 2020 at 8:42 AM ron minnich <rminnich@gmail.com> wrote:
>
> I agree with you on changing the PCI driver name, as opposed to this
> change. I don't like '|' very much either.
>
> I am thinking just to change ':' to '.', e.g.
> 0000:00:1f.3 -> 0000.00.1f.3
>
> It is an extremely simple change --  add one for loop in the pci map
> code -- and nothing else need change.
>
> If this sounds good to you, I'll send a new 2-patch series with that
> change and with the intel-spi driver changed to show how one can use
> command line partitioning?
>
> Also, as I am coming back to this after a very long time, how do you
> like your patch series to look? It seems the git command to generate
> these creates 3 files, the first numbered 0 with no code in it. My
> reading of the docs implies sending this no-code email is not a good
> idea? Any recommendations here?
>
> thanks very much for your comment!
>
> ron
>
> On Sun, Mar 22, 2020 at 4:09 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >
> > Hi Ronald,
> >
> > ron minnich <rminnich@gmail.com> wrote on Sat, 21 Mar 2020 08:44:07
> > -0700:
> >
> > > Anyone? This will be going into use at Google internally and I'd like
> > > to get it upstream.
> > >
> > > The only other option that would work is to take the pci-format names
> > > created by intel-spi-pci that have : in them and change the : to '.'.
> > > Is that more acceptable?
> >
> > One important thing to note: Bootloaders share the same mtdparts
> > definition and should be updated if we decide to support a new
> > separator. U-boot and Barebox at least.
> >
> > I think changing just Intel's PCI driver name would be much more
> > practical for us because I don't find the '|' separator being
> > descriptive at all.
> >
> > However, I don't have a strong position and I would welcome
> > Richard, Vignesh, Tudor and Boris' point of view.
> >
> > Thanks,
> > Miquèl

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