From: Boris Brezillon <[email protected]>
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 <[email protected]>
Signed-off-by: Ron Minnich <[email protected]>
Tested-by: Ron Minnich <[email protected]>
---
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;
On Wednesday, 29 April 2020 18:53:47 CET Ronald G. Minnich wrote:
> From: Boris Brezillon <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Ron Minnich <[email protected]>
> Tested-by: Ron Minnich <[email protected]>
> ---
> 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
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 <[email protected]> wrote:
>
> On Wednesday, 29 April 2020 18:53:47 CET Ronald G. Minnich wrote:
> > From: Boris Brezillon <[email protected]>
> >
> > 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 <[email protected]>
> > Signed-off-by: Ron Minnich <[email protected]>
> > Tested-by: Ron Minnich <[email protected]>
> > ---
> > 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
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
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 <[email protected]> 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
>
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/[email protected]/
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 <[email protected]> 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/[email protected]/
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
I pinged the person again. Hope to hear today. Sorry for delay.
On Sun, Dec 6, 2020 at 11:52 PM Sven Eckelmann <[email protected]> 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
Tested-by: Ian Goegebuer <[email protected]>
On Mon, Dec 7, 2020 at 7:24 AM ron minnich <[email protected]> wrote:
>
> I pinged the person again. Hope to hear today. Sorry for delay.
>
> On Sun, Dec 6, 2020 at 11:52 PM Sven Eckelmann <[email protected]> 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