Message ID | CAP6exYL889zuXgDhLE3SdwzC4idZ6tbe2oqXQRpZT2M6SrRbFg@mail.gmail.com |
---|---|
State | Changes Requested |
Delegated to: | Miquel Raynal |
Headers | show |
Series | [1/1] mtd: add | as a separator after mtd-id | expand |
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
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
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
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
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;
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(-)