Message ID | CAP6exYJ3xk46PODgT=5WAdiLDNY=9MggxyaEwov6qyDQstLj7g@mail.gmail.com |
---|---|
State | Not Applicable |
Headers | show |
Series | [1/1] mtd/intel-spi: Support cmdline-based partition | expand |
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
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
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
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
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
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
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
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.
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, 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
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
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 %.
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 %.
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 %.
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 %.
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
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
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
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
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
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.
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;
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
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
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
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
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
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
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?
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
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
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)
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);