Linux-mtd Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 1/1] mtd/intel-spi: Support cmdline-based partition
@ 2020-03-23 19:58 ron minnich
  2020-03-23 23:19 ` ron minnich
  0 siblings, 1 reply; 17+ messages in thread
From: ron minnich @ 2020-03-23 19:58 UTC (permalink / raw)
  To: Miquel Raynal, Richard Weinberger, Vignesh Raghavendra, linux-mtd

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

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/1] mtd/intel-spi: Support cmdline-based partition
  2020-03-23 19:58 [PATCH 1/1] mtd/intel-spi: Support cmdline-based partition ron minnich
@ 2020-03-23 23:19 ` ron minnich
  2020-03-25 17:34   ` ron minnich
  0 siblings, 1 reply; 17+ messages in thread
From: ron minnich @ 2020-03-23 23:19 UTC (permalink / raw)
  To: Miquel Raynal, Richard Weinberger, Vignesh Raghavendra,
	linux-mtd, mika.westerberg

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

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/1] mtd/intel-spi: Support cmdline-based partition
  2020-03-23 23:19 ` ron minnich
@ 2020-03-25 17:34   ` ron minnich
  2020-03-25 17:34     ` ron minnich
  0 siblings, 1 reply; 17+ messages in thread
From: ron minnich @ 2020-03-25 17:34 UTC (permalink / raw)
  To: Miquel Raynal, Richard Weinberger, Vignesh Raghavendra,
	linux-mtd, mika.westerberg

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

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/1] mtd/intel-spi: Support cmdline-based partition
  2020-03-25 17:34   ` ron minnich
@ 2020-03-25 17:34     ` ron minnich
  2020-03-27 15:48       ` ron minnich
  0 siblings, 1 reply; 17+ messages in thread
From: ron minnich @ 2020-03-25 17:34 UTC (permalink / raw)
  To: Miquel Raynal, Richard Weinberger, Vignesh Raghavendra,
	linux-mtd, mika.westerberg

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

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/1] mtd/intel-spi: Support cmdline-based partition
  2020-03-25 17:34     ` ron minnich
@ 2020-03-27 15:48       ` ron minnich
  2020-03-27 15:56         ` Mika Westerberg
  2020-03-27 16:15         ` Miquel Raynal
  0 siblings, 2 replies; 17+ messages in thread
From: ron minnich @ 2020-03-27 15:48 UTC (permalink / raw)
  To: Miquel Raynal, Richard Weinberger, Vignesh Raghavendra,
	linux-mtd, mika.westerberg

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

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/1] mtd/intel-spi: Support cmdline-based partition
  2020-03-27 15:48       ` ron minnich
@ 2020-03-27 15:56         ` Mika Westerberg
  2020-03-27 16:19           ` Miquel Raynal
  2020-03-27 16:15         ` Miquel Raynal
  1 sibling, 1 reply; 17+ messages in thread
From: Mika Westerberg @ 2020-03-27 15:56 UTC (permalink / raw)
  To: ron minnich
  Cc: Richard Weinberger, linux-mtd, Vignesh Raghavendra, Miquel Raynal

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

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/1] mtd/intel-spi: Support cmdline-based partition
  2020-03-27 15:48       ` ron minnich
  2020-03-27 15:56         ` Mika Westerberg
@ 2020-03-27 16:15         ` Miquel Raynal
  1 sibling, 0 replies; 17+ messages in thread
From: Miquel Raynal @ 2020-03-27 16:15 UTC (permalink / raw)
  To: ron minnich
  Cc: Richard Weinberger, linux-mtd, Vignesh Raghavendra, mika.westerberg

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

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/1] mtd/intel-spi: Support cmdline-based partition
  2020-03-27 15:56         ` Mika Westerberg
@ 2020-03-27 16:19           ` Miquel Raynal
  2020-03-27 16:48             ` Mika Westerberg
  2020-03-27 16:53             ` ron minnich
  0 siblings, 2 replies; 17+ messages in thread
From: Miquel Raynal @ 2020-03-27 16:19 UTC (permalink / raw)
  To: Mika Westerberg
  Cc: ron minnich, linux-mtd, Vignesh Raghavendra, Richard Weinberger

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

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/1] mtd/intel-spi: Support cmdline-based partition
  2020-03-27 16:19           ` Miquel Raynal
@ 2020-03-27 16:48             ` Mika Westerberg
  2020-03-27 16:52               ` Miquel Raynal
  2020-03-27 16:53             ` ron minnich
  1 sibling, 1 reply; 17+ messages in thread
From: Mika Westerberg @ 2020-03-27 16:48 UTC (permalink / raw)
  To: Miquel Raynal
  Cc: ron minnich, linux-mtd, Vignesh Raghavendra, Richard Weinberger

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.

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/1] mtd/intel-spi: Support cmdline-based partition
  2020-03-27 16:48             ` Mika Westerberg
@ 2020-03-27 16:52               ` Miquel Raynal
  2020-03-27 17:05                 ` ron minnich
  0 siblings, 1 reply; 17+ messages in thread
From: Miquel Raynal @ 2020-03-27 16:52 UTC (permalink / raw)
  To: Mika Westerberg
  Cc: ron minnich, linux-mtd, Vignesh Raghavendra, Richard Weinberger

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

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/1] mtd/intel-spi: Support cmdline-based partition
  2020-03-27 16:19           ` Miquel Raynal
  2020-03-27 16:48             ` Mika Westerberg
@ 2020-03-27 16:53             ` ron minnich
  1 sibling, 0 replies; 17+ messages in thread
From: ron minnich @ 2020-03-27 16:53 UTC (permalink / raw)
  To: Miquel Raynal
  Cc: Richard Weinberger, Mika Westerberg, Vignesh Raghavendra, linux-mtd

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

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/1] mtd/intel-spi: Support cmdline-based partition
  2020-03-27 16:52               ` Miquel Raynal
@ 2020-03-27 17:05                 ` ron minnich
  2020-03-27 17:16                   ` Mika Westerberg
  0 siblings, 1 reply; 17+ messages in thread
From: ron minnich @ 2020-03-27 17:05 UTC (permalink / raw)
  To: Miquel Raynal
  Cc: Richard Weinberger, Mika Westerberg, Vignesh Raghavendra, linux-mtd

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

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/1] mtd/intel-spi: Support cmdline-based partition
  2020-03-27 17:05                 ` ron minnich
@ 2020-03-27 17:16                   ` Mika Westerberg
  2020-03-27 17:39                     ` ron minnich
  0 siblings, 1 reply; 17+ messages in thread
From: Mika Westerberg @ 2020-03-27 17:16 UTC (permalink / raw)
  To: ron minnich
  Cc: Richard Weinberger, linux-mtd, Vignesh Raghavendra, Miquel Raynal

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 %.

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/1] mtd/intel-spi: Support cmdline-based partition
  2020-03-27 17:16                   ` Mika Westerberg
@ 2020-03-27 17:39                     ` ron minnich
  2020-03-27 23:53                       ` ron minnich
  0 siblings, 1 reply; 17+ messages in thread
From: ron minnich @ 2020-03-27 17:39 UTC (permalink / raw)
  To: Mika Westerberg
  Cc: Richard Weinberger, linux-mtd, Vignesh Raghavendra, Miquel Raynal

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 %.

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/1] mtd/intel-spi: Support cmdline-based partition
  2020-03-27 17:39                     ` ron minnich
@ 2020-03-27 23:53                       ` ron minnich
  2020-03-30  6:08                         ` Mika Westerberg
  0 siblings, 1 reply; 17+ messages in thread
From: ron minnich @ 2020-03-27 23:53 UTC (permalink / raw)
  To: Mika Westerberg
  Cc: Richard Weinberger, linux-mtd, Vignesh Raghavendra, Miquel Raynal

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 %.

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/1] mtd/intel-spi: Support cmdline-based partition
  2020-03-27 23:53                       ` ron minnich
@ 2020-03-30  6:08                         ` Mika Westerberg
  2020-03-30  7:27                           ` Miquel Raynal
  0 siblings, 1 reply; 17+ messages in thread
From: Mika Westerberg @ 2020-03-30  6:08 UTC (permalink / raw)
  To: ron minnich
  Cc: Richard Weinberger, linux-mtd, Vignesh Raghavendra, Miquel Raynal

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 %.

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/1] mtd/intel-spi: Support cmdline-based partition
  2020-03-30  6:08                         ` Mika Westerberg
@ 2020-03-30  7:27                           ` Miquel Raynal
  0 siblings, 0 replies; 17+ messages in thread
From: Miquel Raynal @ 2020-03-30  7:27 UTC (permalink / raw)
  To: Mika Westerberg
  Cc: ron minnich, linux-mtd, Vignesh Raghavendra, Richard Weinberger

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

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, back to index

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-23 19:58 [PATCH 1/1] mtd/intel-spi: Support cmdline-based partition ron minnich
2020-03-23 23:19 ` ron minnich
2020-03-25 17:34   ` ron minnich
2020-03-25 17:34     ` ron minnich
2020-03-27 15:48       ` ron minnich
2020-03-27 15:56         ` Mika Westerberg
2020-03-27 16:19           ` Miquel Raynal
2020-03-27 16:48             ` Mika Westerberg
2020-03-27 16:52               ` Miquel Raynal
2020-03-27 17:05                 ` ron minnich
2020-03-27 17:16                   ` Mika Westerberg
2020-03-27 17:39                     ` ron minnich
2020-03-27 23:53                       ` ron minnich
2020-03-30  6:08                         ` Mika Westerberg
2020-03-30  7:27                           ` Miquel Raynal
2020-03-27 16:53             ` ron minnich
2020-03-27 16:15         ` Miquel Raynal

Linux-mtd Archive on lore.kernel.org

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

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

Example config snippet for mirrors

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


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