[PATCH] mtd: spi-nor: controllers: intel-spi: Add support for command line partitions

[PATCH] mtd: spi-nor: controllers: intel-spi: Add support for command line partitions

[Ronald G. Minnich]

On Intel platforms, the usable SPI area is located several
MiB in from the start, to leave room for descriptors and
the Management Engine binary. Further, not all the remaining
space can be used, as the last 16 MiB contains firmware.

To make the SPI usable for mtdblock and other devices,
it is necessary to enable command line partitions so the
middle usable region can be specified.

Add a part_probes array which includes only "cmdelineparts",
and change to mtd_device_parse_register to use this part_probes.

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

diff --git a/drivers/mtd/spi-nor/controllers/intel-spi.c b/drivers/mtd/spi-nor/controllers/intel-spi.c
index 61d2a0ad2131..132129e89d07 100644
--- a/drivers/mtd/spi-nor/controllers/intel-spi.c
+++ b/drivers/mtd/spi-nor/controllers/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.26.1.301.g55bc3eb7cb9-goog


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

Re: [PATCH] mtd: spi-nor: controllers: intel-spi: Add support for command line partitions

[Mika Westerberg]

On Fri, Apr 17, 2020 at 08:26:11AM -0700, Ronald G. Minnich wrote:
> On Intel platforms, the usable SPI area is located several
> MiB in from the start, to leave room for descriptors and
> the Management Engine binary. Further, not all the remaining
> space can be used, as the last 16 MiB contains firmware.
> 
> To make the SPI usable for mtdblock and other devices,
> it is necessary to enable command line partitions so the
> middle usable region can be specified.
> 
> Add a part_probes array which includes only "cmdelineparts",
> and change to mtd_device_parse_register to use this part_probes.
> 
> Signed-off-by: Ronald G. Minnich <rminnich@google.com>

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

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

Re: [PATCH] mtd: spi-nor: controllers: intel-spi: Add support for command line partitions

[Vignesh Raghavendra]

On 4/17/20 9:37 PM, Mika Westerberg wrote:
> On Fri, Apr 17, 2020 at 08:26:11AM -0700, Ronald G. Minnich wrote:
>> On Intel platforms, the usable SPI area is located several
>> MiB in from the start, to leave room for descriptors and
>> the Management Engine binary. Further, not all the remaining
>> space can be used, as the last 16 MiB contains firmware.
>>
>> To make the SPI usable for mtdblock and other devices,
>> it is necessary to enable command line partitions so the
>> middle usable region can be specified.
>>
>> Add a part_probes array which includes only "cmdelineparts",
>> and change to mtd_device_parse_register to use this part_probes.
>>
>> Signed-off-by: Ronald G. Minnich <rminnich@google.com>
> 
> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> 

scripts/checkpatch.pl --strict complains:

CHECK: Alignment should match open parenthesis
#46: FILE: drivers/mtd/spi-nor/controllers/intel-spi.c:956:
+	ret = mtd_device_parse_register(&ispi->nor.mtd, part_probes,
+				       NULL, &part, 1);

WARNING: Missing Signed-off-by: line by nominal patch author '"Ronald G. Minnich" <rminnich@gmail.com>'

Regards
Vignesh

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

[PATCH] mtd: parser: cmdline: Support MTD names containing one or more colons

[Ian Goegebuer]

On Intel platforms, the usable SPI area is located several
MiB in from the start, to leave room for descriptors and
the Management Engine binary. Further, not all the remaining
space can be used, as the last 16 MiB contains firmware.

To make the SPI usable for mtdblock and other devices,
it is necessary to enable command line partitions so the
middle usable region can be specified.

Add a part_probes array which includes only "cmdelineparts",
and change to mtd_device_parse_register to use this part_probes.

Signed-off-by: "Ronald G. Minnich" <rminnich@google.com>
Signed-off-by: Ian Goegebuer <goegebuer@google.com>
---
 drivers/mtd/spi-nor/controllers/intel-spi.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/spi-nor/controllers/intel-spi.c b/drivers/mtd/spi-nor/controllers/intel-spi.c
index b54a56a68100..9de38851c411 100644
--- a/drivers/mtd/spi-nor/controllers/intel-spi.c
+++ b/drivers/mtd/spi-nor/controllers/intel-spi.c
@@ -903,6 +903,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)
 {
@@ -950,7 +952,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.29.2.729.g45daf8777d-goog


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

Re: [PATCH] mtd: parser: cmdline: Support MTD names containing one or more colons

[Miquel Raynal]

Hello Ian, 

Ian Goegebuer <goegebuer@google.com> wrote on Wed, 23 Dec 2020 13:56:30
-0800:

> On Intel platforms, the usable SPI area is located several
> MiB in from the start, to leave room for descriptors and
> the Management Engine binary. Further, not all the remaining
> space can be used, as the last 16 MiB contains firmware.
> 
> To make the SPI usable for mtdblock and other devices,
> it is necessary to enable command line partitions so the
> middle usable region can be specified.
> 
> Add a part_probes array which includes only "cmdelineparts",
> and change to mtd_device_parse_register to use this part_probes.

The commit title seem to be taken from another patch and does not
match the below change. Or am I missing something?

> Signed-off-by: "Ronald G. Minnich" <rminnich@google.com>
> Signed-off-by: Ian Goegebuer <goegebuer@google.com>
> ---
>  drivers/mtd/spi-nor/controllers/intel-spi.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/spi-nor/controllers/intel-spi.c b/drivers/mtd/spi-nor/controllers/intel-spi.c
> index b54a56a68100..9de38851c411 100644
> --- a/drivers/mtd/spi-nor/controllers/intel-spi.c
> +++ b/drivers/mtd/spi-nor/controllers/intel-spi.c
> @@ -903,6 +903,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)
>  {
> @@ -950,7 +952,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);
>  

Thanks,
Miquèl

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

Re: [PATCH] mtd: parser: cmdline: Support MTD names containing one or more colons

[Ron Minnich]

It is likely we're missing something -- I am not fully checked out on
the Linux patch process and it's Ian's first patch.

Guidance appreciated.

On Mon, Jan 4, 2021 at 1:08 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> Hello Ian,
>
> Ian Goegebuer <goegebuer@google.com> wrote on Wed, 23 Dec 2020 13:56:30
> -0800:
>
> > On Intel platforms, the usable SPI area is located several
> > MiB in from the start, to leave room for descriptors and
> > the Management Engine binary. Further, not all the remaining
> > space can be used, as the last 16 MiB contains firmware.
> >
> > To make the SPI usable for mtdblock and other devices,
> > it is necessary to enable command line partitions so the
> > middle usable region can be specified.
> >
> > Add a part_probes array which includes only "cmdelineparts",
> > and change to mtd_device_parse_register to use this part_probes.
>
> The commit title seem to be taken from another patch and does not
> match the below change. Or am I missing something?
>
> > Signed-off-by: "Ronald G. Minnich" <rminnich@google.com>
> > Signed-off-by: Ian Goegebuer <goegebuer@google.com>
> > ---
> >  drivers/mtd/spi-nor/controllers/intel-spi.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/mtd/spi-nor/controllers/intel-spi.c b/drivers/mtd/spi-nor/controllers/intel-spi.c
> > index b54a56a68100..9de38851c411 100644
> > --- a/drivers/mtd/spi-nor/controllers/intel-spi.c
> > +++ b/drivers/mtd/spi-nor/controllers/intel-spi.c
> > @@ -903,6 +903,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)
> >  {
> > @@ -950,7 +952,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);
> >
>
> Thanks,
> Miquèl

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

[PATCH] mtd: parser: cmdline: Support MTD names containing one or more colons

[Ronald G. Minnich]

From: Boris Brezillon <boris.brezillon@collabora.com>

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>
Signed-off-by: Ron Minnich <rminnich@google.com>
Tested-by: Ron Minnich <rminnich@google.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;

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

Re: [PATCH] mtd: parser: cmdline: Support MTD names containing one or more colons

[Sven Eckelmann]

[-- Attachment #1.1.1: Type: text/plain, Size: 2146 bytes --]

On Wednesday, 29 April 2020 18:53:47 CET Ronald G. Minnich wrote:
> From: Boris Brezillon <boris.brezillon@collabora.com>
> 
> 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>
> Signed-off-by: Ron Minnich <rminnich@google.com>
> Tested-by: Ron Minnich <rminnich@google.com>
> ---
>  drivers/mtd/parsers/cmdlinepart.c | 23 ++++++++++++++++++++---
>  1 file changed, 20 insertions(+), 3 deletions(-)

This change broke OpenWrt booting on some IPQ40xx devices. Here for example 
the parts of the cmdline which the u-boot on an OpenMesh A62 sets 
automatically:

    root=31:11 mtdparts=spi0.0:256k(0:SBL1),128k(0:MIBIB),384k(0:QSEE),64k(0:CDT),64k(0:DDRPARAMS),64k(0:APPSBLENV),512k(0:APPSBL),64k(0:ART),64k(custom),64k(0:KEYS),0x002b0000(kernel),0x00c80000(rootfs),15552k(inactive) rootfsname=rootfs rootwait

This is then parsed by newpart as: KEYS),0x002b0000(kernel),0x00c80000(rootfs),15552k(inactive)

And of course, this results in:

    mtd: partition has size 0
    [...]
    /dev/root: Can't open blockdev
    VFS: Cannot open root device "31:11" or unknown-block(31,11): error -6
    Please append a correct "root=" boot option; here are the available partitions:
    1f00           32768 mtdblock0
    (driver?)
    Kernel panic - not syncing: VFS: Unable to mount root fs on unknown-block(31,11)
    CPU1: stopping
    CPU: 1 PID: 0 Comm: 

This affects OpenWrt since the commit d6a9a92e3217 ("kernel: bump 5.4 to 
5.4.69") because this change was backported to Linux v5.4.69. Reverting 
this change fixes the problem for me.

And if I see it correctly, this is also affecting the kernel (4.14) for
the OpenWrt 18.06.x + 19.07.x branch because it can also be found in
there as part of the v4.14.200 release.

Another workaround is to replace the first "(" with a NULL too. See the
attached patch for the one which I used to fix the OpenWrt bootup.

Kind regards,
	Sven

[-- Attachment #1.1.2: 499-mtd-parser-cmdline-Fix-parsing-of-part-names-with-co.patch --]
[-- Type: text/x-patch, Size: 2308 bytes --]

From: Sven Eckelmann <sven@narfation.org>
Date: Sun, 22 Nov 2020 00:48:33 +0100
Subject: [PATCH RFC] mtd: parser: cmdline: Fix parsing of part-names with colons

Some devices (especially QCA ones) are already using hardcoded partition
names with colons in it. The OpenMesh A62 for example provides following
mtd relevant information via cmdline:

  root=31:11 mtdparts=spi0.0:256k(0:SBL1),128k(0:MIBIB),384k(0:QSEE),64k(0:CDT),64k(0:DDRPARAMS),64k(0:APPSBLENV),512k(0:APPSBL),64k(0:ART),64k(custom),64k(0:KEYS),0x002b0000(kernel),0x00c80000(rootfs),15552k(inactive) rootfsname=rootfs rootwait

The change to split only on the last colon between mtd-id and partitions
will cause newpart to see following string for the first partition:

  KEYS),0x002b0000(kernel),0x00c80000(rootfs),15552k(inactive)

Such a partition list cannot be parsed and thus the device fails to boot.

Avoid this behavior by making sure that the start of the first part-name
("(") will also be the last byte the colon search algorithm is using for
its mtd-id search.

Fixes: eb13fa022741 ("mtd: parser: cmdline: Support MTD names containing one or more colons")
Signed-off-by: Sven Eckelmann <sven@narfation.org>

diff --git a/drivers/mtd/parsers/cmdlinepart.c b/drivers/mtd/parsers/cmdlinepart.c
index 0625b25620ca766318ea4646a6e3761ff4d3a4cc..22881ea4c132ea5a5ba7aebd025d91bf1cd023af 100644
--- a/drivers/mtd/parsers/cmdlinepart.c
+++ b/drivers/mtd/parsers/cmdlinepart.c
@@ -218,7 +218,7 @@ 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, *semicol;
+		char *p, *mtd_id, *semicol, *open_parenth;
 
 		/*
 		 * Replace the first ';' by a NULL char so strrchr can work
@@ -228,6 +228,13 @@ static int mtdpart_setup_real(char *s)
 		if (semicol)
 			*semicol = '\0';
 
+		/* make sure that part-names with ":" will not be handled as
+		 * part of the mtd-id with an ":"
+		 */
+		open_parenth = strchr(s, '(');
+		if (open_parenth)
+			*open_parenth = '\0';
+
 		mtd_id = s;
 
 		/*
@@ -237,6 +244,10 @@ static int mtdpart_setup_real(char *s)
 		 */
 		p = strrchr(s, ':');
 
+		/* Restore the '(' now. */
+		if (open_parenth)
+			*open_parenth = '(';
+
 		/* Restore the ';' now. */
 		if (semicol)
 			*semicol = ';';

[-- Attachment #1.2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 144 bytes --]

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

Re: [PATCH] mtd: parser: cmdline: Support MTD names containing one or more colons

[ron minnich]

I'm a bit worried about how tricky this starts to get. I'm inclined to
go back to an earlier implementation which used a character that had
not yet been used (iirc I used [] around the PCI ID in a very early
version). What if we used, e.g, a single ! and searched for that? It
need not be !; pick a character. Just something not already in use, as
the ambiguity around which ':' delimits the device has become an
issue, as you show.

Almost nothing in the original patch would change, save the character
being searched for. By using a character we'd never used, we'd avoid
breaking existing usage.

Comments?

On Sat, Nov 21, 2020 at 4:14 PM Sven Eckelmann <sven@narfation.org> wrote:
>
> On Wednesday, 29 April 2020 18:53:47 CET Ronald G. Minnich wrote:
> > From: Boris Brezillon <boris.brezillon@collabora.com>
> >
> > 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>
> > Signed-off-by: Ron Minnich <rminnich@google.com>
> > Tested-by: Ron Minnich <rminnich@google.com>
> > ---
> >  drivers/mtd/parsers/cmdlinepart.c | 23 ++++++++++++++++++++---
> >  1 file changed, 20 insertions(+), 3 deletions(-)
>
> This change broke OpenWrt booting on some IPQ40xx devices. Here for example
> the parts of the cmdline which the u-boot on an OpenMesh A62 sets
> automatically:
>
>     root=31:11 mtdparts=spi0.0:256k(0:SBL1),128k(0:MIBIB),384k(0:QSEE),64k(0:CDT),64k(0:DDRPARAMS),64k(0:APPSBLENV),512k(0:APPSBL),64k(0:ART),64k(custom),64k(0:KEYS),0x002b0000(kernel),0x00c80000(rootfs),15552k(inactive) rootfsname=rootfs rootwait
>
> This is then parsed by newpart as: KEYS),0x002b0000(kernel),0x00c80000(rootfs),15552k(inactive)
>
> And of course, this results in:
>
>     mtd: partition has size 0
>     [...]
>     /dev/root: Can't open blockdev
>     VFS: Cannot open root device "31:11" or unknown-block(31,11): error -6
>     Please append a correct "root=" boot option; here are the available partitions:
>     1f00           32768 mtdblock0
>     (driver?)
>     Kernel panic - not syncing: VFS: Unable to mount root fs on unknown-block(31,11)
>     CPU1: stopping
>     CPU: 1 PID: 0 Comm:
>
> This affects OpenWrt since the commit d6a9a92e3217 ("kernel: bump 5.4 to
> 5.4.69") because this change was backported to Linux v5.4.69. Reverting
> this change fixes the problem for me.
>
> And if I see it correctly, this is also affecting the kernel (4.14) for
> the OpenWrt 18.06.x + 19.07.x branch because it can also be found in
> there as part of the v4.14.200 release.
>
> Another workaround is to replace the first "(" with a NULL too. See the
> attached patch for the one which I used to fix the OpenWrt bootup.
>
> Kind regards,
>         Sven

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

Re: [PATCH] mtd: parser: cmdline: Support MTD names containing one or more colons

[Sven Eckelmann]

[-- Attachment #1.1: Type: text/plain, Size: 983 bytes --]

On Friday, 27 November 2020 17:32:02 CET ron minnich wrote:
> I'm a bit worried about how tricky this starts to get. I'm inclined to
> go back to an earlier implementation which used a character that had
> not yet been used (iirc I used [] around the PCI ID in a very early
> version). What if we used, e.g, a single ! and searched for that? It
> need not be !; pick a character. Just something not already in use, as
> the ambiguity around which ':' delimits the device has become an
> issue, as you show.
> 
> Almost nothing in the original patch would change, save the character
> being searched for. By using a character we'd never used, we'd avoid
> breaking existing usage.

What? Doesn't make any sense to me. The mtdparts shown in the the commit 
message is as it is. I cannot simply change it because it is in the control of 
the bootloader - not the linux kernel or me. So I can also not introduce a 
different character like ! for separating things.

KInd regards,
	Sven


[-- Attachment #1.2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 144 bytes --]

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

Re: [PATCH] mtd: parser: cmdline: Support MTD names containing one or more colons

[ron minnich]

Ah ha, I see how different my world is from yours :-)

My world is linuxboot, which means the command line is always under
control of linux, as linux is my bootloader. So this kind of thing is
very easy for me to change.

Let me go back to the initial problem and hope I can make better sense.

The original problem was that ':' was used to separate the device from
the partitions. A problem came along in that PCI device specifiers
have ':', and the original parsing code broke. The patch that caused
you trouble fixed that by using the right-most ':' as the separator
for device and partitions.

What none of the people involved in the original patch knew was that
there would be other ':' in use. Sorry!

But you are right, my idea is a complete non-starter, don't know what
I was thinking.

So it seems your patch, if it works, is the way to go? I can't think
of anything better that lets us preserve current behavior and support
PCI device specifiers?

Ron


On Fri, Nov 27, 2020 at 9:06 AM Sven Eckelmann <sven@narfation.org> wrote:
>
> On Friday, 27 November 2020 17:32:02 CET ron minnich wrote:
> > I'm a bit worried about how tricky this starts to get. I'm inclined to
> > go back to an earlier implementation which used a character that had
> > not yet been used (iirc I used [] around the PCI ID in a very early
> > version). What if we used, e.g, a single ! and searched for that? It
> > need not be !; pick a character. Just something not already in use, as
> > the ambiguity around which ':' delimits the device has become an
> > issue, as you show.
> >
> > Almost nothing in the original patch would change, save the character
> > being searched for. By using a character we'd never used, we'd avoid
> > breaking existing usage.
>
> What? Doesn't make any sense to me. The mtdparts shown in the the commit
> message is as it is. I cannot simply change it because it is in the control of
> the bootloader - not the linux kernel or me. So I can also not introduce a
> different character like ! for separating things.
>
> KInd regards,
>         Sven
>

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

Re: [PATCH] mtd: parser: cmdline: Support MTD names containing one or more colons

[Sven Eckelmann]

[-- Attachment #1.1: Type: text/plain, Size: 1804 bytes --]

On Friday, 27 November 2020 18:16:54 CET ron minnich wrote:
> What none of the people involved in the original patch knew was that
> there would be other ':' in use. Sorry!
> 
> But you are right, my idea is a complete non-starter, don't know what
> I was thinking.

I am still not sure because I still didn't get what you actually wanted to 
change. I first thought that you wanted to change

    mtdparts=spi0.0:256k(0:SBL1)

to

    mtdparts=spi0.0!256k(0:SBL1)

which wouldn't work for me when ":" is not supported anymore. And it would 
break a lot of already working installations.

But maybe I completely misread it. Maybe you wanted to introduce an 
optional(!!!) stop marker like !

    mtdparts=spi0.0!:256k(0:SBL1)

to inform the parser that it doesn't have to search for : before the !. While 
this could work for me, I am not qualified enough to say which character is 
not yet used and can be utilized.

But the note about [ and ] at least makes sense to me (if it is optional):

    mtdparts=[spi0.0]:256k(0:SBL1)

But I am not sure if this will be a problem for people which already adopted 
PCI IDs inside the mtdparts without [ and ].
 
> So it seems your patch, if it works, is the way to go?

At least this is a workaround [1] which can be pushed to all the stable 
kernels which broke with the "Support MTD names containing one or more colons" 
patch. And the one which OpenWrt adopted now to get the devices booting again. 
It is only waiting for a Tested-by from you.

> I can't think
> of anything better that lets us preserve current behavior and support
> PCI device specifiers?

I am not that deep in this topic. So I am not sure what else could be done.

Kind regards,
	Sven

[1] https://patchwork.ozlabs.org/project/linux-mtd/patch/20201124062506.185392-1-sven@narfation.org/

[-- Attachment #1.2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 144 bytes --]

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

Re: [PATCH] mtd: parser: cmdline: Support MTD names containing one or more colons

[ron minnich]

Thanks, Sven, for your patience, I will indeed try to test this next week.

On Fri, Nov 27, 2020 at 9:35 AM Sven Eckelmann <sven@narfation.org> wrote:
>
> On Friday, 27 November 2020 18:16:54 CET ron minnich wrote:
> > What none of the people involved in the original patch knew was that
> > there would be other ':' in use. Sorry!
> >
> > But you are right, my idea is a complete non-starter, don't know what
> > I was thinking.
>
> I am still not sure because I still didn't get what you actually wanted to
> change. I first thought that you wanted to change
>
>     mtdparts=spi0.0:256k(0:SBL1)
>
> to
>
>     mtdparts=spi0.0!256k(0:SBL1)
>
> which wouldn't work for me when ":" is not supported anymore. And it would
> break a lot of already working installations.
>
> But maybe I completely misread it. Maybe you wanted to introduce an
> optional(!!!) stop marker like !
>
>     mtdparts=spi0.0!:256k(0:SBL1)
>
> to inform the parser that it doesn't have to search for : before the !. While
> this could work for me, I am not qualified enough to say which character is
> not yet used and can be utilized.
>
> But the note about [ and ] at least makes sense to me (if it is optional):
>
>     mtdparts=[spi0.0]:256k(0:SBL1)
>
> But I am not sure if this will be a problem for people which already adopted
> PCI IDs inside the mtdparts without [ and ].
>
> > So it seems your patch, if it works, is the way to go?
>
> At least this is a workaround [1] which can be pushed to all the stable
> kernels which broke with the "Support MTD names containing one or more colons"
> patch. And the one which OpenWrt adopted now to get the devices booting again.
> It is only waiting for a Tested-by from you.
>
> > I can't think
> > of anything better that lets us preserve current behavior and support
> > PCI device specifiers?
>
> I am not that deep in this topic. So I am not sure what else could be done.
>
> Kind regards,
>         Sven
>
> [1] https://patchwork.ozlabs.org/project/linux-mtd/patch/20201124062506.185392-1-sven@narfation.org/

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

Re: [PATCH] mtd: parser: cmdline: Support MTD names containing one or more colons

[Sven Eckelmann]

[-- Attachment #1.1: Type: text/plain, Size: 176 bytes --]

On Friday, 27 November 2020 19:54:30 CET ron minnich wrote:
> Thanks, Sven, for your patience, I will indeed try to test this next week.

Any test results?

Kind regards,
	Sven

[-- Attachment #1.2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 144 bytes --]

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

Re: [PATCH] mtd: parser: cmdline: Support MTD names containing one or more colons

[ron minnich]

I pinged the person again. Hope to hear today. Sorry for delay.

On Sun, Dec 6, 2020 at 11:52 PM Sven Eckelmann <sven@narfation.org> wrote:
>
> On Friday, 27 November 2020 19:54:30 CET ron minnich wrote:
> > Thanks, Sven, for your patience, I will indeed try to test this next week.
>
> Any test results?
>
> Kind regards,
>         Sven

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

Re: [PATCH] mtd: parser: cmdline: Support MTD names containing one or more colons

[Ron Minnich]

Tested-by: Ian Goegebuer <goegebuer@google.com>

On Mon, Dec 7, 2020 at 7:24 AM ron minnich <rminnich@gmail.com> wrote:
>
> I pinged the person again. Hope to hear today. Sorry for delay.
>
> On Sun, Dec 6, 2020 at 11:52 PM Sven Eckelmann <sven@narfation.org> wrote:
> >
> > On Friday, 27 November 2020 19:54:30 CET ron minnich wrote:
> > > Thanks, Sven, for your patience, I will indeed try to test this next week.
> >
> > Any test results?
> >
> > Kind regards,
> >         Sven

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