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

Message ID	CAP6exYJ3xk46PODgT=5WAdiLDNY=9MggxyaEwov6qyDQstLj7g@mail.gmail.com
State	Not Applicable
Headers	show Return-Path: <linux-mtd-bounces+incoming=patchwork.ozlabs.org@lists.infradead.org> MIME-Version: 1.0 From: ron minnich <rminnich@gmail.com> Date: Mon, 23 Mar 2020 12:58:22 -0700 Message-ID: <CAP6exYJ3xk46PODgT=5WAdiLDNY=9MggxyaEwov6qyDQstLj7g@mail.gmail.com> Subject: [PATCH 1/1] mtd/intel-spi: Support cmdline-based partition To: Miquel Raynal <miquel.raynal@bootlin.com>, Richard Weinberger <richard@nod.at>, Vignesh Raghavendra <vigneshr@ti.com>, linux-mtd@lists.infradead.org summary: Content analysis details: (-0.2 points) pts rule name description ---- ---------------------- -------------------------------------------------- -0.0 RCVD_IN_DNSWL_NONE RBL: Sender listed at https://www.dnswl.org/, no trust [2607:f8b0:4864:20:0:0:0:244 listed in] [list.dnswl.org] -0.0 SPF_PASS SPF: sender matches SPF record 0.0 SPF_HELO_NONE SPF: HELO does not publish an SPF Record 0.0 FREEMAIL_FROM Sender email is commonly abused enduser mail provider [rminnich[at]gmail.com] -0.1 DKIM_VALID_AU Message has a valid DKIM or DK signature from author's domain -0.1 DKIM_VALID Message has at least one valid DKIM or DK signature 0.1 DKIM_SIGNED Message has a DKIM or DK signature, not necessarily valid -0.1 DKIM_VALID_EF Message has a valid DKIM or DK signature from envelope-from domain Precedence: list Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-mtd" <linux-mtd-bounces@lists.infradead.org> Errors-To: linux-mtd-bounces+incoming=patchwork.ozlabs.org@lists.infradead.org
Series	[1/1] mtd/intel-spi: Support cmdline-based partition \| expand [1/1] mtd/intel-spi: Support cmdline-based partition

ron minnich March 23, 2020, 7:58 p.m. UTC

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.

On Intel spi devices, the name is set to the PCI slot name,
e.g. 0000:00:1f.5.  There are two : in the name alone.
Since strchr is used to find the <mtd-id>,
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 change modifies the name attached to the intel spi
device, changing all ':' to '.', e.g. 0000:00:1f.5
becomes 0000.00.1f.5. It also adds command line partition
parsing registration to the intel_spi_probe function.

This change has been tested and works on a modern Intel chipset with
a 64 MiB FLASH part.

Signed-off-by: Ronald G. Minnich <rminnich@google.com>
---
 drivers/mtd/spi-nor/intel-spi-pci.c | 22 ++++++++++++++++++++++
 drivers/mtd/spi-nor/intel-spi.c     |  4 +++-
 2 files changed, 25 insertions(+), 1 deletion(-)

 {
@@ -941,7 +943,7 @@ struct intel_spi *intel_spi_probe(struct device *dev,
        if (!ispi->writeable || !writeable)
                ispi->nor.mtd.flags &= ~MTD_WRITEABLE;

-       ret = mtd_device_register(&ispi->nor.mtd, &part, 1);
+       ret = mtd_device_parse_register(&ispi->nor.mtd, part_probes,
NULL, &part, 1);
        if (ret)
                return ERR_PTR(ret);

ron minnich March 23, 2020, 11:19 p.m. UTC | #1

On Mon, Mar 23, 2020 at 12:58 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.
>
> On Intel spi devices, the name is set to the PCI slot name,
> e.g. 0000:00:1f.5.  There are two : in the name alone.
> Since strchr is used to find the <mtd-id>,
> 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 change modifies the name attached to the intel spi
> device, changing all ':' to '.', e.g. 0000:00:1f.5
> becomes 0000.00.1f.5. It also adds command line partition
> parsing registration to the intel_spi_probe function.
>
> This change has been tested and works on a modern Intel chipset with
> a 64 MiB FLASH part.
>
> Signed-off-by: Ronald G. Minnich <rminnich@google.com>
> ---
>  drivers/mtd/spi-nor/intel-spi-pci.c | 22 ++++++++++++++++++++++
>  drivers/mtd/spi-nor/intel-spi.c     |  4 +++-
>  2 files changed, 25 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mtd/spi-nor/intel-spi-pci.c
> b/drivers/mtd/spi-nor/intel-spi-pci.c
> index 81329f680bec..57716e53c219 100644
> --- a/drivers/mtd/spi-nor/intel-spi-pci.c
> +++ b/drivers/mtd/spi-nor/intel-spi-pci.c
> @@ -24,6 +24,23 @@ static const struct intel_spi_boardinfo cnl_info = {
>         .type = INTEL_SPI_CNL,
>  };
>
> +/*
> + * PCI names use a ':' as a separator, which conflicts
> + * with the mtdparts cmdline parameter. Dup the name and
> + * replace : with .
> + */
> +static int fixname(struct pci_dev *pdev) {
> +       char *name;
> +        name = kstrdup(pci_name(pdev), GFP_KERNEL);
> +       if (! name) {
> +               return -ENOMEM;
> +       }
> +       strreplace(name, ':', '.');
> +       dev_set_name(&pdev->dev, name);
> +       kfree(name);
> +       return 0;
> +}
> +
>  static int intel_spi_pci_probe(struct pci_dev *pdev,
>                                const struct pci_device_id *id)
>  {
> @@ -41,6 +58,11 @@ static int intel_spi_pci_probe(struct pci_dev *pdev,
>         if (!info)
>                 return -ENOMEM;
>
> +       if (fixname(pdev)) {
> +               kfree(info);
> +               return -ENOMEM;
> +       }
> +
>         /* Try to make the chip read/write */
>         pci_read_config_dword(pdev, BCR, &bcr);
>         if (!(bcr & BCR_WPD)) {
> diff --git a/drivers/mtd/spi-nor/intel-spi.c b/drivers/mtd/spi-nor/intel-spi.c
> index 61d2a0ad2131..261b10cf5076 100644
> --- a/drivers/mtd/spi-nor/intel-spi.c
> +++ b/drivers/mtd/spi-nor/intel-spi.c
> @@ -894,6 +894,8 @@ static const struct spi_nor_controller_ops
> intel_spi_controller_ops = {
>         .erase = intel_spi_erase,
>  };
>
> +static const char * const part_probes[] = { "cmdlinepart", NULL };
> +
>  struct intel_spi *intel_spi_probe(struct device *dev,
>         struct resource *mem, const struct intel_spi_boardinfo *info)
>  {
> @@ -941,7 +943,7 @@ struct intel_spi *intel_spi_probe(struct device *dev,
>         if (!ispi->writeable || !writeable)
>                 ispi->nor.mtd.flags &= ~MTD_WRITEABLE;
>
> -       ret = mtd_device_register(&ispi->nor.mtd, &part, 1);
> +       ret = mtd_device_parse_register(&ispi->nor.mtd, part_probes,
> NULL, &part, 1);
>         if (ret)
>                 return ERR_PTR(ret);
>
> --
> 2.25.1.696.g5e7596f4ac-goog

ron minnich March 25, 2020, 5:34 p.m. UTC | #2

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.

On Intel spi devices, the name is set to the PCI slot name,
e.g. 0000:00:1f.5.  There are two : in the name alone.
Since strchr is used to find the <mtd-id>,
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 change modifies the name attached to the intel spi
device, changing all ':' to '.', e.g. 0000:00:1f.5
becomes 0000.00.1f.5. It also adds command line partition
parsing registration to the intel_spi_probe function.

Signed-off-by: Ronald G. Minnich <rminnich@google.com>
Change-Id: Ibfa5caba51b8cdd3c41c60b15f8f8c583ded82ff
---
 drivers/mtd/spi-nor/intel-spi-pci.c | 23 +++++++++++++++++++++++
 drivers/mtd/spi-nor/intel-spi.c     |  5 ++++-
 2 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/spi-nor/intel-spi-pci.c
b/drivers/mtd/spi-nor/intel-spi-pci.c
index 81329f680bec..dc608d74e824 100644
--- a/drivers/mtd/spi-nor/intel-spi-pci.c
+++ b/drivers/mtd/spi-nor/intel-spi-pci.c
@@ -24,6 +24,24 @@ static const struct intel_spi_boardinfo cnl_info = {
     .type = INTEL_SPI_CNL,
 };

+/*
+ * PCI names use a ':' as a separator, which conflicts
+ * with the mtdparts cmdline parameter. Dup the name and
+ * replace : with .
+ */
+static int fixname(struct pci_dev *pdev)
+{
+    char *name;
+
+    name = kstrdup(pci_name(pdev), GFP_KERNEL);
+    if (!name)
+        return -ENOMEM;
+    strreplace(name, ':', '.');
+    dev_set_name(&pdev->dev, name);
+    kfree(name);
+    return 0;
+}
+
 static int intel_spi_pci_probe(struct pci_dev *pdev,
                    const struct pci_device_id *id)
 {
@@ -41,6 +59,11 @@ static int intel_spi_pci_probe(struct pci_dev *pdev,
     if (!info)
         return -ENOMEM;

+    if (fixname(pdev)) {
+        kfree(info);
+        return -ENOMEM;
+    }
+
     /* Try to make the chip read/write */
     pci_read_config_dword(pdev, BCR, &bcr);
     if (!(bcr & BCR_WPD)) {
diff --git a/drivers/mtd/spi-nor/intel-spi.c b/drivers/mtd/spi-nor/intel-spi.c
index 61d2a0ad2131..cb08ee4d2029 100644
--- a/drivers/mtd/spi-nor/intel-spi.c
+++ b/drivers/mtd/spi-nor/intel-spi.c
@@ -894,6 +894,8 @@ static const struct spi_nor_controller_ops
intel_spi_controller_ops = {
     .erase = intel_spi_erase,
 };

+static const char * const part_probes[] = { "cmdlinepart", NULL };
+
 struct intel_spi *intel_spi_probe(struct device *dev,
     struct resource *mem, const struct intel_spi_boardinfo *info)
 {
@@ -941,7 +943,8 @@ struct intel_spi *intel_spi_probe(struct device *dev,
     if (!ispi->writeable || !writeable)
         ispi->nor.mtd.flags &= ~MTD_WRITEABLE;

-    ret = mtd_device_register(&ispi->nor.mtd, &part, 1);
+    ret = mtd_device_parse_register(&ispi->nor.mtd, part_probes,
+                    NULL, &part, 1);

     if (ret)
         return ERR_PTR(ret);

--
2.25.1.696.g5e7596f4ac-goog

ron minnich March 25, 2020, 5:34 p.m. UTC | #3

Sorry, on that first one, I forgot the checkpatch. This one is clean.

thanks

On Wed, Mar 25, 2020 at 10:34 AM 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.
>
> On Intel spi devices, the name is set to the PCI slot name,
> e.g. 0000:00:1f.5.  There are two : in the name alone.
> Since strchr is used to find the <mtd-id>,
> 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 change modifies the name attached to the intel spi
> device, changing all ':' to '.', e.g. 0000:00:1f.5
> becomes 0000.00.1f.5. It also adds command line partition
> parsing registration to the intel_spi_probe function.
>
> Signed-off-by: Ronald G. Minnich <rminnich@google.com>
> Change-Id: Ibfa5caba51b8cdd3c41c60b15f8f8c583ded82ff
> ---
>  drivers/mtd/spi-nor/intel-spi-pci.c | 23 +++++++++++++++++++++++
>  drivers/mtd/spi-nor/intel-spi.c     |  5 ++++-
>  2 files changed, 27 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mtd/spi-nor/intel-spi-pci.c
> b/drivers/mtd/spi-nor/intel-spi-pci.c
> index 81329f680bec..dc608d74e824 100644
> --- a/drivers/mtd/spi-nor/intel-spi-pci.c
> +++ b/drivers/mtd/spi-nor/intel-spi-pci.c
> @@ -24,6 +24,24 @@ static const struct intel_spi_boardinfo cnl_info = {
>      .type = INTEL_SPI_CNL,
>  };
>
> +/*
> + * PCI names use a ':' as a separator, which conflicts
> + * with the mtdparts cmdline parameter. Dup the name and
> + * replace : with .
> + */
> +static int fixname(struct pci_dev *pdev)
> +{
> +    char *name;
> +
> +    name = kstrdup(pci_name(pdev), GFP_KERNEL);
> +    if (!name)
> +        return -ENOMEM;
> +    strreplace(name, ':', '.');
> +    dev_set_name(&pdev->dev, name);
> +    kfree(name);
> +    return 0;
> +}
> +
>  static int intel_spi_pci_probe(struct pci_dev *pdev,
>                     const struct pci_device_id *id)
>  {
> @@ -41,6 +59,11 @@ static int intel_spi_pci_probe(struct pci_dev *pdev,
>      if (!info)
>          return -ENOMEM;
>
> +    if (fixname(pdev)) {
> +        kfree(info);
> +        return -ENOMEM;
> +    }
> +
>      /* Try to make the chip read/write */
>      pci_read_config_dword(pdev, BCR, &bcr);
>      if (!(bcr & BCR_WPD)) {
> diff --git a/drivers/mtd/spi-nor/intel-spi.c b/drivers/mtd/spi-nor/intel-spi.c
> index 61d2a0ad2131..cb08ee4d2029 100644
> --- a/drivers/mtd/spi-nor/intel-spi.c
> +++ b/drivers/mtd/spi-nor/intel-spi.c
> @@ -894,6 +894,8 @@ static const struct spi_nor_controller_ops
> intel_spi_controller_ops = {
>      .erase = intel_spi_erase,
>  };
>
> +static const char * const part_probes[] = { "cmdlinepart", NULL };
> +
>  struct intel_spi *intel_spi_probe(struct device *dev,
>      struct resource *mem, const struct intel_spi_boardinfo *info)
>  {
> @@ -941,7 +943,8 @@ struct intel_spi *intel_spi_probe(struct device *dev,
>      if (!ispi->writeable || !writeable)
>          ispi->nor.mtd.flags &= ~MTD_WRITEABLE;
>
> -    ret = mtd_device_register(&ispi->nor.mtd, &part, 1);
> +    ret = mtd_device_parse_register(&ispi->nor.mtd, part_probes,
> +                    NULL, &part, 1);
>
>      if (ret)
>          return ERR_PTR(ret);
>
> --
> 2.25.1.696.g5e7596f4ac-goog

ron minnich March 27, 2020, 3:48 p.m. UTC | #4

anyone? This is in our internal tree but I'd like to get it upstreamed
if possible.

On Wed, Mar 25, 2020 at 10:34 AM ron minnich <rminnich@gmail.com> wrote:
>
> Sorry, on that first one, I forgot the checkpatch. This one is clean.
>
> thanks
>
> On Wed, Mar 25, 2020 at 10:34 AM 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.
> >
> > On Intel spi devices, the name is set to the PCI slot name,
> > e.g. 0000:00:1f.5.  There are two : in the name alone.
> > Since strchr is used to find the <mtd-id>,
> > 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 change modifies the name attached to the intel spi
> > device, changing all ':' to '.', e.g. 0000:00:1f.5
> > becomes 0000.00.1f.5. It also adds command line partition
> > parsing registration to the intel_spi_probe function.
> >
> > Signed-off-by: Ronald G. Minnich <rminnich@google.com>
> > Change-Id: Ibfa5caba51b8cdd3c41c60b15f8f8c583ded82ff
> > ---
> >  drivers/mtd/spi-nor/intel-spi-pci.c | 23 +++++++++++++++++++++++
> >  drivers/mtd/spi-nor/intel-spi.c     |  5 ++++-
> >  2 files changed, 27 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/mtd/spi-nor/intel-spi-pci.c
> > b/drivers/mtd/spi-nor/intel-spi-pci.c
> > index 81329f680bec..dc608d74e824 100644
> > --- a/drivers/mtd/spi-nor/intel-spi-pci.c
> > +++ b/drivers/mtd/spi-nor/intel-spi-pci.c
> > @@ -24,6 +24,24 @@ static const struct intel_spi_boardinfo cnl_info = {
> >      .type = INTEL_SPI_CNL,
> >  };
> >
> > +/*
> > + * PCI names use a ':' as a separator, which conflicts
> > + * with the mtdparts cmdline parameter. Dup the name and
> > + * replace : with .
> > + */
> > +static int fixname(struct pci_dev *pdev)
> > +{
> > +    char *name;
> > +
> > +    name = kstrdup(pci_name(pdev), GFP_KERNEL);
> > +    if (!name)
> > +        return -ENOMEM;
> > +    strreplace(name, ':', '.');
> > +    dev_set_name(&pdev->dev, name);
> > +    kfree(name);
> > +    return 0;
> > +}
> > +
> >  static int intel_spi_pci_probe(struct pci_dev *pdev,
> >                     const struct pci_device_id *id)
> >  {
> > @@ -41,6 +59,11 @@ static int intel_spi_pci_probe(struct pci_dev *pdev,
> >      if (!info)
> >          return -ENOMEM;
> >
> > +    if (fixname(pdev)) {
> > +        kfree(info);
> > +        return -ENOMEM;
> > +    }
> > +
> >      /* Try to make the chip read/write */
> >      pci_read_config_dword(pdev, BCR, &bcr);
> >      if (!(bcr & BCR_WPD)) {
> > diff --git a/drivers/mtd/spi-nor/intel-spi.c b/drivers/mtd/spi-nor/intel-spi.c
> > index 61d2a0ad2131..cb08ee4d2029 100644
> > --- a/drivers/mtd/spi-nor/intel-spi.c
> > +++ b/drivers/mtd/spi-nor/intel-spi.c
> > @@ -894,6 +894,8 @@ static const struct spi_nor_controller_ops
> > intel_spi_controller_ops = {
> >      .erase = intel_spi_erase,
> >  };
> >
> > +static const char * const part_probes[] = { "cmdlinepart", NULL };
> > +
> >  struct intel_spi *intel_spi_probe(struct device *dev,
> >      struct resource *mem, const struct intel_spi_boardinfo *info)
> >  {
> > @@ -941,7 +943,8 @@ struct intel_spi *intel_spi_probe(struct device *dev,
> >      if (!ispi->writeable || !writeable)
> >          ispi->nor.mtd.flags &= ~MTD_WRITEABLE;
> >
> > -    ret = mtd_device_register(&ispi->nor.mtd, &part, 1);
> > +    ret = mtd_device_parse_register(&ispi->nor.mtd, part_probes,
> > +                    NULL, &part, 1);
> >
> >      if (ret)
> >          return ERR_PTR(ret);
> >
> > --
> > 2.25.1.696.g5e7596f4ac-goog

Mika Westerberg March 27, 2020, 3:56 p.m. UTC | #5

Hi,

I don't think it is good idea to change PCI device name like that.

Instead the MTD cmdline parser should be teach to handle names like this
properly.

On Fri, Mar 27, 2020 at 08:48:39AM -0700, ron minnich wrote:
> anyone? This is in our internal tree but I'd like to get it upstreamed
> if possible.
> 
> On Wed, Mar 25, 2020 at 10:34 AM ron minnich <rminnich@gmail.com> wrote:
> >
> > Sorry, on that first one, I forgot the checkpatch. This one is clean.
> >
> > thanks
> >
> > On Wed, Mar 25, 2020 at 10:34 AM 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.
> > >
> > > On Intel spi devices, the name is set to the PCI slot name,
> > > e.g. 0000:00:1f.5.  There are two : in the name alone.
> > > Since strchr is used to find the <mtd-id>,
> > > 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 change modifies the name attached to the intel spi
> > > device, changing all ':' to '.', e.g. 0000:00:1f.5
> > > becomes 0000.00.1f.5. It also adds command line partition
> > > parsing registration to the intel_spi_probe function.
> > >
> > > Signed-off-by: Ronald G. Minnich <rminnich@google.com>
> > > Change-Id: Ibfa5caba51b8cdd3c41c60b15f8f8c583ded82ff
> > > ---
> > >  drivers/mtd/spi-nor/intel-spi-pci.c | 23 +++++++++++++++++++++++
> > >  drivers/mtd/spi-nor/intel-spi.c     |  5 ++++-
> > >  2 files changed, 27 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/mtd/spi-nor/intel-spi-pci.c
> > > b/drivers/mtd/spi-nor/intel-spi-pci.c
> > > index 81329f680bec..dc608d74e824 100644
> > > --- a/drivers/mtd/spi-nor/intel-spi-pci.c
> > > +++ b/drivers/mtd/spi-nor/intel-spi-pci.c
> > > @@ -24,6 +24,24 @@ static const struct intel_spi_boardinfo cnl_info = {
> > >      .type = INTEL_SPI_CNL,
> > >  };
> > >
> > > +/*
> > > + * PCI names use a ':' as a separator, which conflicts
> > > + * with the mtdparts cmdline parameter. Dup the name and
> > > + * replace : with .
> > > + */
> > > +static int fixname(struct pci_dev *pdev)
> > > +{
> > > +    char *name;
> > > +
> > > +    name = kstrdup(pci_name(pdev), GFP_KERNEL);
> > > +    if (!name)
> > > +        return -ENOMEM;
> > > +    strreplace(name, ':', '.');
> > > +    dev_set_name(&pdev->dev, name);
> > > +    kfree(name);
> > > +    return 0;
> > > +}
> > > +
> > >  static int intel_spi_pci_probe(struct pci_dev *pdev,
> > >                     const struct pci_device_id *id)
> > >  {
> > > @@ -41,6 +59,11 @@ static int intel_spi_pci_probe(struct pci_dev *pdev,
> > >      if (!info)
> > >          return -ENOMEM;
> > >
> > > +    if (fixname(pdev)) {
> > > +        kfree(info);
> > > +        return -ENOMEM;
> > > +    }
> > > +
> > >      /* Try to make the chip read/write */
> > >      pci_read_config_dword(pdev, BCR, &bcr);
> > >      if (!(bcr & BCR_WPD)) {
> > > diff --git a/drivers/mtd/spi-nor/intel-spi.c b/drivers/mtd/spi-nor/intel-spi.c
> > > index 61d2a0ad2131..cb08ee4d2029 100644
> > > --- a/drivers/mtd/spi-nor/intel-spi.c
> > > +++ b/drivers/mtd/spi-nor/intel-spi.c
> > > @@ -894,6 +894,8 @@ static const struct spi_nor_controller_ops
> > > intel_spi_controller_ops = {
> > >      .erase = intel_spi_erase,
> > >  };
> > >
> > > +static const char * const part_probes[] = { "cmdlinepart", NULL };
> > > +
> > >  struct intel_spi *intel_spi_probe(struct device *dev,
> > >      struct resource *mem, const struct intel_spi_boardinfo *info)
> > >  {
> > > @@ -941,7 +943,8 @@ struct intel_spi *intel_spi_probe(struct device *dev,
> > >      if (!ispi->writeable || !writeable)
> > >          ispi->nor.mtd.flags &= ~MTD_WRITEABLE;
> > >
> > > -    ret = mtd_device_register(&ispi->nor.mtd, &part, 1);
> > > +    ret = mtd_device_parse_register(&ispi->nor.mtd, part_probes,
> > > +                    NULL, &part, 1);
> > >
> > >      if (ret)
> > >          return ERR_PTR(ret);
> > >
> > > --
> > > 2.25.1.696.g5e7596f4ac-goog

Miquel Raynal March 27, 2020, 4:15 p.m. UTC | #6

Hi Ronald,

ron minnich <rminnich@gmail.com> wrote on Fri, 27 Mar 2020 08:48:39
-0700:

> anyone? This is in our internal tree but I'd like to get it upstreamed
> if possible.

Maybe it is one of your fist contributions and I thank you very much
for trying to keep Google's tree aligned with mainline by upstreaming
this kind of change with enthusiasm, this is appreciated.

However, please note that a 2 days ping (and this is the second time you
do it) is a bit of an impatient move. Also, please have in mind the
maintainer's schedule: we freeze our branches about a week before
sending them to Linus. The SPI-NOR pull-request has been sent earlier
this week already so this patch will not reach mainline in v5.7 anyway.

Last generic comment, the subject prefix is wrong, so if you want the
SPI-NOR maintainer to care, better use the right one to catch his eye
;) "mtd: spi-nor: intel-spi:" seems more appropriate.

More comments below :)

> 
> On Wed, Mar 25, 2020 at 10:34 AM ron minnich <rminnich@gmail.com> wrote:
> >
> > Sorry, on that first one, I forgot the checkpatch. This one is clean.

When resending, please:
- increment the version with git-format-patch -v<x>
- write a changelog explaining what are the changes below the three
  dashes '---'.
- do not answer your previous version

> >
> > thanks
> >
> > On Wed, Mar 25, 2020 at 10:34 AM 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.

s/id/Linux MTD device ID/             ^^

s/partdef/partition definition/                   ^^^^^^^

> > >
> > > The mtdparts can define more than one part, in which case

What about:

"The mtdparts cmdline parameter can define the partitioning of several
MTD devices, in which case"...
                                            
> > > there will be more than one <mtd-id>:<partdef> component.
> > >
> > > On Intel spi devices, the name is set to the PCI slot name,

               SPI

> > > e.g. 0000:00:1f.5.  There are two : in the name alone.

                                        ':'

> > > Since strchr is used to find the <mtd-id>,
> > > in this case it will return the wrong
> > > result. Using strrchr is not an option, as there may

What about:
      result as ':' is part of the parameter syntax.
 
> > > be more than one mtddef in the argument.

Not sure the strrchr mentiont is relevant here. I'd drop it.

> > >
> > > This change modifies the name attached to the intel spi

      Change the name attached to the Intel SPI device, ...

> > > device, changing all ':' to '.', e.g. 0000:00:1f.5
> > > becomes 0000.00.1f.5. It also adds command line partition
> > > parsing registration to the intel_spi_probe function.
> > >
> > > Signed-off-by: Ronald G. Minnich <rminnich@google.com>
> > > Change-Id: Ibfa5caba51b8cdd3c41c60b15f8f8c583ded82ff

Change-ID can be dropped too.

> > > ---
> > >  drivers/mtd/spi-nor/intel-spi-pci.c | 23 +++++++++++++++++++++++
> > >  drivers/mtd/spi-nor/intel-spi.c     |  5 ++++-
> > >  2 files changed, 27 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/mtd/spi-nor/intel-spi-pci.c
> > > b/drivers/mtd/spi-nor/intel-spi-pci.c
> > > index 81329f680bec..dc608d74e824 100644
> > > --- a/drivers/mtd/spi-nor/intel-spi-pci.c
> > > +++ b/drivers/mtd/spi-nor/intel-spi-pci.c
> > > @@ -24,6 +24,24 @@ static const struct intel_spi_boardinfo cnl_info = {
> > >      .type = INTEL_SPI_CNL,
> > >  };
> > >
> > > +/*
> > > + * PCI names use a ':' as a separator, which conflicts
> > > + * with the mtdparts cmdline parameter. Dup the name and

s/dup/duplicate/

> > > + * replace : with .
> > > + */
> > > +static int fixname(struct pci_dev *pdev)
> > > +{
> > > +    char *name;
> > > +
> > > +    name = kstrdup(pci_name(pdev), GFP_KERNEL);
> > > +    if (!name)
> > > +        return -ENOMEM;

new line

> > > +    strreplace(name, ':', '.');
> > > +    dev_set_name(&pdev->dev, name);
> > > +    kfree(name);

new line

> > > +    return 0;
> > > +}
> > > +
> > >  static int intel_spi_pci_probe(struct pci_dev *pdev,
> > >                     const struct pci_device_id *id)
> > >  {
> > > @@ -41,6 +59,11 @@ static int intel_spi_pci_probe(struct pci_dev *pdev,
> > >      if (!info)
> > >          return -ENOMEM;
> > >
> > > +    if (fixname(pdev)) {

	ret = fixname(pdev);
	if (ret) {
		kfree(info);
		return ret;
	}

> > > +        kfree(info);
> > > +        return -ENOMEM;
> > > +    }
> > > +
> > >      /* Try to make the chip read/write */
> > >      pci_read_config_dword(pdev, BCR, &bcr);
> > >      if (!(bcr & BCR_WPD)) {
> > > diff --git a/drivers/mtd/spi-nor/intel-spi.c b/drivers/mtd/spi-nor/intel-spi.c
> > > index 61d2a0ad2131..cb08ee4d2029 100644
> > > --- a/drivers/mtd/spi-nor/intel-spi.c
> > > +++ b/drivers/mtd/spi-nor/intel-spi.c
> > > @@ -894,6 +894,8 @@ static const struct spi_nor_controller_ops
> > > intel_spi_controller_ops = {
> > >      .erase = intel_spi_erase,
> > >  };
> > >
> > > +static const char * const part_probes[] = { "cmdlinepart", NULL };
> > > +
> > >  struct intel_spi *intel_spi_probe(struct device *dev,
> > >      struct resource *mem, const struct intel_spi_boardinfo *info)
> > >  {
> > > @@ -941,7 +943,8 @@ struct intel_spi *intel_spi_probe(struct device *dev,
> > >      if (!ispi->writeable || !writeable)
> > >          ispi->nor.mtd.flags &= ~MTD_WRITEABLE;
> > >
> > > -    ret = mtd_device_register(&ispi->nor.mtd, &part, 1);
> > > +    ret = mtd_device_parse_register(&ispi->nor.mtd, part_probes,
> > > +                    NULL, &part, 1);

Wrong alignment here I guess


Should these two changes be separated (changing the name, registering
the device with the cmdline parser)?

> > >
> > >      if (ret)
> > >          return ERR_PTR(ret);
> > >
> > > --
> > > 2.25.1.696.g5e7596f4ac-goog  

Thanks,
Miquèl

Miquel Raynal March 27, 2020, 4:19 p.m. UTC | #7

Hi Mika,

Mika Westerberg <mika.westerberg@linux.intel.com> wrote on Fri, 27 Mar
2020 17:56:08 +0200:

> Hi,
> 
> I don't think it is good idea to change PCI device name like that.
> 
> Instead the MTD cmdline parser should be teach to handle names like this
> properly.

It is a bit more complicated than that since parsers have been using
this syntax for a long time and, more importantly, it means
potentially updating all bootloaders.

I am not against updating the parser, but the s/:/|/ solution proposed
before is rather undescriptive and it is hard to find an alternative
character that would have a meaning here.

Thanks,
Miquèl

Mika Westerberg March 27, 2020, 4:48 p.m. UTC | #8

On Fri, Mar 27, 2020 at 05:19:07PM +0100, Miquel Raynal wrote:
> Hi Mika,
> 
> Mika Westerberg <mika.westerberg@linux.intel.com> wrote on Fri, 27 Mar
> 2020 17:56:08 +0200:
> 
> > Hi,
> > 
> > I don't think it is good idea to change PCI device name like that.
> > 
> > Instead the MTD cmdline parser should be teach to handle names like this
> > properly.
> 
> It is a bit more complicated than that since parsers have been using
> this syntax for a long time and, more importantly, it means
> potentially updating all bootloaders.
> 
> I am not against updating the parser, but the s/:/|/ solution proposed
> before is rather undescriptive and it is hard to find an alternative
> character that would have a meaning here.

I'm completely unfamiliar with these but would escape char work here? I
mean if you want ":" to be parsed literally then you pass "\:" in the
command line. That should work with the existing and also allow
supporting SPI NOR controllers on PCI bus.

Miquel Raynal March 27, 2020, 4:52 p.m. UTC | #9

Hi Mika,

Mika Westerberg <mika.westerberg@linux.intel.com> wrote on Fri, 27 Mar
2020 18:48:02 +0200:

> On Fri, Mar 27, 2020 at 05:19:07PM +0100, Miquel Raynal wrote:
> > Hi Mika,
> > 
> > Mika Westerberg <mika.westerberg@linux.intel.com> wrote on Fri, 27 Mar
> > 2020 17:56:08 +0200:
> >   
> > > Hi,
> > > 
> > > I don't think it is good idea to change PCI device name like that.
> > > 
> > > Instead the MTD cmdline parser should be teach to handle names like this
> > > properly.  
> > 
> > It is a bit more complicated than that since parsers have been using
> > this syntax for a long time and, more importantly, it means
> > potentially updating all bootloaders.
> > 
> > I am not against updating the parser, but the s/:/|/ solution proposed
> > before is rather undescriptive and it is hard to find an alternative
> > character that would have a meaning here.  
> 
> I'm completely unfamiliar with these but would escape char work here? I
> mean if you want ":" to be parsed literally then you pass "\:" in the
> command line. That should work with the existing and also allow
> supporting SPI NOR controllers on PCI bus.

We would still have to update bootloaders code but that would be easy
to handle. The logic being "search for the next ':', when you have one
check if there is a '\' in front of it. If yes, search again". Why
not... This also means reconstructing the name by dropping manually the
additional '\' in Linux.

Thanks,
Miquèl

ron minnich March 27, 2020, 4:53 p.m. UTC | #10

Mika, I have to agree with your comment on parsing the name correctly,
but the pci syntax is sufficiently flexible that I expect it would
create an explosion in code size and complexity and that is a concern.

OTOH, I can think of a few options: we could require, for example,
that PCI names ALWAYS be in the form AAAA:BB:CC.D, and then the test
is a bit simpler: it's a PCI name if id[4] == ':' && id [7] == ':' &&
id[10] == '.', and we can break the id from the parts at id[12]; it it
is not a PCI id, we do a strchr as is done now.

That would mean I could just dump this admittedly ugly change. I admit
the test above is a bit nasty, but it's pretty reliable. WDYT?

Miquel, your patience with this patch as I learn how to contribute is
greatly appreciated. Thanks for the tips. My last real contribution to
Linux was over 15 years ago, with 9p, and the real upstreaming work on
that was done by others; the kernel community is all new to me.

I'm going to drop this patch and start over and try not to make such a
mess of it.

thanks again everyone

ron

ron minnich March 27, 2020, 5:05 p.m. UTC | #11

I did try the \ thing but found it a bit tricky to work with, with
lots of potential for simple errors.

It would require cmdlines like this
mtdparts=0000\:00\:0.1f:etcetc

A lot of these mtdparts definitions are produced by scripts and
Makefiles and there are many, many places where \\ have a way of
vanishing.

We talked here about an option where the name would be in parens or brackets:
mtdparts=(00:00:0.0):etcetc

which I think looks a lot nicer but and leaves room for growth, where
in a few years some other strange name comes along that might break
our rules. () anyone?

Another problem is, the name matching is a strcmp. There's no
semantics in the names. So where, technically, these  PCI addresses
are the same:
1f.3 and 0:1f.3 and  0:0:1f.3 and  0000:00:1f.3
the strcmp would fail between 1f.3 and 0000:00:1f.3 -- but they're the same.

This means that the PCI names must be strongly structured: they must
follow the rules such that the : and . occur in a fixed place in the
string, meaning determining that a string is a PCI TBDF is relatively
simple.

So I keep coming back to the simple "name matches pattern ****:**:*.*
(where * means any char) -- it's a PCI address" as maybe the easiest
thing to do.
But possibly the () are the best future-proofing. I really am not a fan of \.

ron

On Fri, Mar 27, 2020 at 9:52 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> Hi Mika,
>
> Mika Westerberg <mika.westerberg@linux.intel.com> wrote on Fri, 27 Mar
> 2020 18:48:02 +0200:
>
> > On Fri, Mar 27, 2020 at 05:19:07PM +0100, Miquel Raynal wrote:
> > > Hi Mika,
> > >
> > > Mika Westerberg <mika.westerberg@linux.intel.com> wrote on Fri, 27 Mar
> > > 2020 17:56:08 +0200:
> > >
> > > > Hi,
> > > >
> > > > I don't think it is good idea to change PCI device name like that.
> > > >
> > > > Instead the MTD cmdline parser should be teach to handle names like this
> > > > properly.
> > >
> > > It is a bit more complicated than that since parsers have been using
> > > this syntax for a long time and, more importantly, it means
> > > potentially updating all bootloaders.
> > >
> > > I am not against updating the parser, but the s/:/|/ solution proposed
> > > before is rather undescriptive and it is hard to find an alternative
> > > character that would have a meaning here.
> >
> > I'm completely unfamiliar with these but would escape char work here? I
> > mean if you want ":" to be parsed literally then you pass "\:" in the
> > command line. That should work with the existing and also allow
> > supporting SPI NOR controllers on PCI bus.
>
> We would still have to update bootloaders code but that would be easy
> to handle. The logic being "search for the next ':', when you have one
> check if there is a '\' in front of it. If yes, search again". Why
> not... This also means reconstructing the name by dropping manually the
> additional '\' in Linux.
>
> Thanks,
> Miquèl

Mika Westerberg March 27, 2020, 5:16 p.m. UTC | #12

On Fri, Mar 27, 2020 at 10:05:52AM -0700, ron minnich wrote:
> I did try the \ thing but found it a bit tricky to work with, with
> lots of potential for simple errors.
> 
> It would require cmdlines like this
> mtdparts=0000\:00\:0.1f:etcetc
> 
> A lot of these mtdparts definitions are produced by scripts and
> Makefiles and there are many, many places where \\ have a way of
> vanishing.

Right. One option would be to use the printf() style escaping and make
:: to be literal : in the same way %% is literal %.

ron minnich March 27, 2020, 5:39 p.m. UTC | #13

yeah, :: is not so bad, but you've got a lot of corner cases as you check for ::
mtdparts=0:
for one example.

Covering all the ways things can go wrong gets messy. You can pretty
much guarantee all those corner cases will get exercised ...

And people are going to mess this up and end up with hard to debug errors:
mtdparts=0000::0:1f.3:parts

That could be a hard error to spot.

I still wonder if we should not just define some character as
available in addition to :. I realize | was pretty awful, but ... is
there some other character we might use? I kind of like the simplicity
of the current scheme; there really would be no issue had it been
almost anything but a : :-)

But if the sense is that :: is the way to go, I can give it a shot.

ron

On Fri, Mar 27, 2020 at 10:16 AM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>
> On Fri, Mar 27, 2020 at 10:05:52AM -0700, ron minnich wrote:
> > I did try the \ thing but found it a bit tricky to work with, with
> > lots of potential for simple errors.
> >
> > It would require cmdlines like this
> > mtdparts=0000\:00\:0.1f:etcetc
> >
> > A lot of these mtdparts definitions are produced by scripts and
> > Makefiles and there are many, many places where \\ have a way of
> > vanishing.
>
> Right. One option would be to use the printf() style escaping and make
> :: to be literal : in the same way %% is literal %.

ron minnich March 27, 2020, 11:53 p.m. UTC | #14

OK, I've done a quick prototype of using () as one way to specify the
ID. The mtparts can look like this (tested)

mtdparts=(0000:00:1f.5)25165824(BIOS),-(squashfs)

The text in () can be pretty arbitrary; only ) is disallowed.
It's about 10 more lines of code in cmdlinepart.c and that's it.
Further, the existing syntax is still supported:
mtdparts=id:parts

what do you think?

thanks

ron

On Fri, Mar 27, 2020 at 10:39 AM ron minnich <rminnich@gmail.com> wrote:
>
> yeah, :: is not so bad, but you've got a lot of corner cases as you check for ::
> mtdparts=0:
> for one example.
>
> Covering all the ways things can go wrong gets messy. You can pretty
> much guarantee all those corner cases will get exercised ...
>
> And people are going to mess this up and end up with hard to debug errors:
> mtdparts=0000::0:1f.3:parts
>
> That could be a hard error to spot.
>
> I still wonder if we should not just define some character as
> available in addition to :. I realize | was pretty awful, but ... is
> there some other character we might use? I kind of like the simplicity
> of the current scheme; there really would be no issue had it been
> almost anything but a : :-)
>
> But if the sense is that :: is the way to go, I can give it a shot.
>
> ron
>
> On Fri, Mar 27, 2020 at 10:16 AM Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> >
> > On Fri, Mar 27, 2020 at 10:05:52AM -0700, ron minnich wrote:
> > > I did try the \ thing but found it a bit tricky to work with, with
> > > lots of potential for simple errors.
> > >
> > > It would require cmdlines like this
> > > mtdparts=0000\:00\:0.1f:etcetc
> > >
> > > A lot of these mtdparts definitions are produced by scripts and
> > > Makefiles and there are many, many places where \\ have a way of
> > > vanishing.
> >
> > Right. One option would be to use the printf() style escaping and make
> > :: to be literal : in the same way %% is literal %.

Mika Westerberg March 30, 2020, 6:08 a.m. UTC | #15

No objections from my side :)

On Fri, Mar 27, 2020 at 04:53:28PM -0700, ron minnich wrote:
> OK, I've done a quick prototype of using () as one way to specify the
> ID. The mtparts can look like this (tested)
> 
> mtdparts=(0000:00:1f.5)25165824(BIOS),-(squashfs)
> 
> The text in () can be pretty arbitrary; only ) is disallowed.
> It's about 10 more lines of code in cmdlinepart.c and that's it.
> Further, the existing syntax is still supported:
> mtdparts=id:parts
> 
> what do you think?
> 
> thanks
> 
> ron
> 
> On Fri, Mar 27, 2020 at 10:39 AM ron minnich <rminnich@gmail.com> wrote:
> >
> > yeah, :: is not so bad, but you've got a lot of corner cases as you check for ::
> > mtdparts=0:
> > for one example.
> >
> > Covering all the ways things can go wrong gets messy. You can pretty
> > much guarantee all those corner cases will get exercised ...
> >
> > And people are going to mess this up and end up with hard to debug errors:
> > mtdparts=0000::0:1f.3:parts
> >
> > That could be a hard error to spot.
> >
> > I still wonder if we should not just define some character as
> > available in addition to :. I realize | was pretty awful, but ... is
> > there some other character we might use? I kind of like the simplicity
> > of the current scheme; there really would be no issue had it been
> > almost anything but a : :-)
> >
> > But if the sense is that :: is the way to go, I can give it a shot.
> >
> > ron
> >
> > On Fri, Mar 27, 2020 at 10:16 AM Mika Westerberg
> > <mika.westerberg@linux.intel.com> wrote:
> > >
> > > On Fri, Mar 27, 2020 at 10:05:52AM -0700, ron minnich wrote:
> > > > I did try the \ thing but found it a bit tricky to work with, with
> > > > lots of potential for simple errors.
> > > >
> > > > It would require cmdlines like this
> > > > mtdparts=0000\:00\:0.1f:etcetc
> > > >
> > > > A lot of these mtdparts definitions are produced by scripts and
> > > > Makefiles and there are many, many places where \\ have a way of
> > > > vanishing.
> > >
> > > Right. One option would be to use the printf() style escaping and make
> > > :: to be literal : in the same way %% is literal %.

Miquel Raynal March 30, 2020, 7:27 a.m. UTC | #16

Hi Mika,

Mika Westerberg <mika.westerberg@linux.intel.com> wrote on Mon, 30 Mar
2020 09:08:59 +0300:

> No objections from my side :)
> 
> On Fri, Mar 27, 2020 at 04:53:28PM -0700, ron minnich wrote:
> > OK, I've done a quick prototype of using () as one way to specify the
> > ID. The mtparts can look like this (tested)
> > 
> > mtdparts=(0000:00:1f.5)25165824(BIOS),-(squashfs)

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)

?

> > 
> > The text in () can be pretty arbitrary; only ) is disallowed.
> > It's about 10 more lines of code in cmdlinepart.c and that's it.
> > Further, the existing syntax is still supported:
> > mtdparts=id:parts
> > 
> > what do you think?
> > 
> > thanks
> > 
> > ron
> > 
> > On Fri, Mar 27, 2020 at 10:39 AM ron minnich <rminnich@gmail.com> wrote:  
> > >
> > > yeah, :: is not so bad, but you've got a lot of corner cases as you check for ::
> > > mtdparts=0:
> > > for one example.
> > >
> > > Covering all the ways things can go wrong gets messy. You can pretty
> > > much guarantee all those corner cases will get exercised ...
> > >
> > > And people are going to mess this up and end up with hard to debug errors:
> > > mtdparts=0000::0:1f.3:parts
> > >
> > > That could be a hard error to spot.
> > >
> > > I still wonder if we should not just define some character as
> > > available in addition to :. I realize | was pretty awful, but ... is
> > > there some other character we might use? I kind of like the simplicity
> > > of the current scheme; there really would be no issue had it been
> > > almost anything but a : :-)
> > >
> > > But if the sense is that :: is the way to go, I can give it a shot.
> > >
> > > ron
> > >
> > > On Fri, Mar 27, 2020 at 10:16 AM Mika Westerberg
> > > <mika.westerberg@linux.intel.com> wrote:  
> > > >
> > > > On Fri, Mar 27, 2020 at 10:05:52AM -0700, ron minnich wrote:  
> > > > > I did try the \ thing but found it a bit tricky to work with, with
> > > > > lots of potential for simple errors.
> > > > >
> > > > > It would require cmdlines like this
> > > > > mtdparts=0000\:00\:0.1f:etcetc
> > > > >
> > > > > A lot of these mtdparts definitions are produced by scripts and
> > > > > Makefiles and there are many, many places where \\ have a way of
> > > > > vanishing.  
> > > >
> > > > Right. One option would be to use the printf() style escaping and make
> > > > :: to be literal : in the same way %% is literal %.  

Thanks,
Miquèl

ron minnich March 30, 2020, 3:53 p.m. UTC | #17

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'll do what you feel is best, however, I'm still getting back into this area.

Thanks again!

ron

Miquel Raynal April 1, 2020, 7:41 a.m. UTC | #18

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 []".

Thanks,
Miquèl

ron minnich April 1, 2020, 3:42 p.m. UTC | #19

yeah, I agree with you, and am wrapping up the patch to support the :

thanks for your comments!

ron

On Wed, Apr 1, 2020 at 12:41 AM 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 []".
>
> Thanks,
> Miquèl

ron minnich April 1, 2020, 7:49 p.m. UTC | #20

just fyi this works fine and it's easy on the eyes:
mtdparts=[0000:00:1f.5]:25165824(BIOS),-(squashfs)

so I'm preparing the patch.

On Wed, Apr 1, 2020 at 8:42 AM ron minnich <rminnich@gmail.com> wrote:
>
> yeah, I agree with you, and am wrapping up the patch to support the :
>
> thanks for your comments!
>
> ron
>
> On Wed, Apr 1, 2020 at 12:41 AM 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 []".
> >
> > Thanks,
> > Miquèl

Boris Brezillon April 27, 2020, 9:16 a.m. UTC | #21

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.

Boris Brezillon April 27, 2020, 9:49 a.m. UTC | #22

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;

Miquel Raynal April 27, 2020, 2:41 p.m. UTC | #23

Hi Ron,

Boris Brezillon <boris.brezillon@collabora.com> wrote on Mon, 27 Apr
2020 11:49:54 +0200:

> 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;

This is also the approach I like the most. It avoids modifying the
syntax on the cmdline (no change in Bootloaders needed) and we don't
have to change the parser of a driver every time a colon is in the
name.

I would like to drop "mtd: parsers: support '[]' fir ud ub mtdparts" as
welle as "mtd: spi-nor: controllers: intel-spi: Add support for command
line partitions", what do you think?


Thanks,
Miquèl

ron minnich April 27, 2020, 6:48 p.m. UTC | #24

I'm fine with that. I have not had a chance to test it but it should be fine.

On Mon, Apr 27, 2020 at 7:41 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> Hi Ron,
>
> Boris Brezillon <boris.brezillon@collabora.com> wrote on Mon, 27 Apr
> 2020 11:49:54 +0200:
>
> > 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;
>
> This is also the approach I like the most. It avoids modifying the
> syntax on the cmdline (no change in Bootloaders needed) and we don't
> have to change the parser of a driver every time a colon is in the
> name.
>
> I would like to drop "mtd: parsers: support '[]' fir ud ub mtdparts" as
> welle as "mtd: spi-nor: controllers: intel-spi: Add support for command
> line partitions", what do you think?
>
>
> Thanks,
> Miquèl

ron minnich April 27, 2020, 6:50 p.m. UTC | #25

I will add that the syntax looks less nice to me than the [] notation
but still ... it should work.

On Mon, Apr 27, 2020 at 11:48 AM ron minnich <rminnich@gmail.com> wrote:
>
> I'm fine with that. I have not had a chance to test it but it should be fine.
>
> On Mon, Apr 27, 2020 at 7:41 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >
> > Hi Ron,
> >
> > Boris Brezillon <boris.brezillon@collabora.com> wrote on Mon, 27 Apr
> > 2020 11:49:54 +0200:
> >
> > > 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;
> >
> > This is also the approach I like the most. It avoids modifying the
> > syntax on the cmdline (no change in Bootloaders needed) and we don't
> > have to change the parser of a driver every time a colon is in the
> > name.
> >
> > I would like to drop "mtd: parsers: support '[]' fir ud ub mtdparts" as
> > welle as "mtd: spi-nor: controllers: intel-spi: Add support for command
> > line partitions", what do you think?
> >
> >
> > Thanks,
> > Miquèl

Miquel Raynal April 27, 2020, 6:56 p.m. UTC | #26

Hi ron,

ron minnich <rminnich@gmail.com> wrote on Mon, 27 Apr 2020 11:50:46
-0700:

> I will add that the syntax looks less nice to me than the [] notation
> but still ... it should work.

I agree, but we need to keep cmdline parsing in line with bootloaders
and this is more problematic to do than to say.

Would you mind testing if it works for you, then post Boris' diff with
your Tested-by?


Thanks,
Miquèl

ron minnich April 27, 2020, 6:59 p.m. UTC | #27

yep, on it.

On Mon, Apr 27, 2020 at 11:56 AM Miquel Raynal
<miquel.raynal@bootlin.com> wrote:
>
> Hi ron,
>
> ron minnich <rminnich@gmail.com> wrote on Mon, 27 Apr 2020 11:50:46
> -0700:
>
> > I will add that the syntax looks less nice to me than the [] notation
> > but still ... it should work.
>
> I agree, but we need to keep cmdline parsing in line with bootloaders
> and this is more problematic to do than to say.
>
> Would you mind testing if it works for you, then post Boris' diff with
> your Tested-by?
>
>
> Thanks,
> Miquèl

Boris Brezillon April 27, 2020, 7:21 p.m. UTC | #28

On Mon, 27 Apr 2020 20:56:36 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Hi ron,
> 
> ron minnich <rminnich@gmail.com> wrote on Mon, 27 Apr 2020 11:50:46
> -0700:
> 
> > I will add that the syntax looks less nice to me than the [] notation
> > but still ... it should work.  
> 
> I agree, but we need to keep cmdline parsing in line with bootloaders
> and this is more problematic to do than to say.

Note that this patch might have to be ported to bootloaders anyway (I
don't know how the parsing is done there).

Hm, if you do:

[<PCI-dev-id>]:<part-defs>

you can keep the PCI device id nicely enclosed in the [] brackets. So
that's up to you. The main advantage of this simple patch is that it
nicely supports existing device names containing one or more colons.

> 
> Would you mind testing if it works for you, then post Boris' diff with
> your Tested-by?
> 
> 
> Thanks,
> Miquèl

ron minnich April 27, 2020, 7:31 p.m. UTC | #29

On Mon, Apr 27, 2020 at 12:21 PM Boris Brezillon
<boris.brezillon@collabora.com> wrote:

> you can keep the PCI device id nicely enclosed in the [] brackets. So
> that's up to you. The main advantage of this simple patch is that it
> nicely supports existing device names containing one or more colons.

totally agree.

So, sadly, today I am using gmail. Do you know if a reply from me to
Boris' email will break everything as gmail messes up email so badly
(tab -> space, etc.)

advice on that?

ron minnich April 27, 2020, 8:31 p.m. UTC | #30

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

Miquel Raynal April 27, 2020, 9:20 p.m. UTC | #31

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

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

Commit Message

Comments

Patch