| Message ID | 20161116031535.20403-1-joel@jms.id.au |
|---|---|
| State | Accepted |
| Headers | show |
Patrick Sent from my iPhone > On Nov 15, 2016, at 9:15 PM, Joel Stanley <joel@jms.id.au> wrote: > > pflash relies on arch_flash_arm parsing /proc/mtd to discover the pnor > partition. It helpfully uses strcasestr so it can handle the string > changing, which is what has happened as we moved to upstream compliant > mtd device tree bindings. > > We currently have a string like this: > > dev: size erasesize name > mtd0: 00060000 00001000 "u-boot" > mtd1: 00020000 00001000 "u-boot-env" > mtd2: 00280000 00001000 "kernel" > mtd3: 001c0000 00001000 "initramfs" > mtd4: 01740000 00001000 "rofs" > mtd5: 00400000 00001000 "rwfs" > mtd6: 02000000 00001000 "1e620000.flash-controller:flash@1" > mtd7: 08000000 00001000 "1e630000.flash-controller:pnor@0" > > Unfortunately arch_flash_arm assumes the string will be at most 50 > characters. That's right before the label we're looking for starts so > we ignore that line and keep searching. > > Fix it by allowing for a 255 character line. > > Fixes: 48ab7ce09504 (external/pflash: Add --mtd) > Signed-off-by: Joel Stanley <joel@jms.id.au> > --- > external/common/arch_flash_arm.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/external/common/arch_flash_arm.c b/external/common/arch_flash_arm.c > index bb8800ff556d..3cdd41ded0a9 100644 > --- a/external/common/arch_flash_arm.c > +++ b/external/common/arch_flash_arm.c > @@ -241,7 +241,7 @@ static char *get_dev_mtd(enum flash_access access) > { > FILE *f; > char *ret = NULL, *pos = NULL; > - char line[50]; > + char line[255]; Isn't there a max path length define in a system header we could use instead? > > if (access != BMC_MTD && access != PNOR_MTD) > return NULL; > -- > 2.10.2 > > _______________________________________________ > Skiboot mailing list > Skiboot@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/skiboot
On Thu, Nov 17, 2016 at 11:19 AM, Patrick Williams <patrick@stwcx.xyz> wrote: >> diff --git a/external/common/arch_flash_arm.c b/external/common/arch_flash_arm.c >> index bb8800ff556d..3cdd41ded0a9 100644 >> --- a/external/common/arch_flash_arm.c >> +++ b/external/common/arch_flash_arm.c >> @@ -241,7 +241,7 @@ static char *get_dev_mtd(enum flash_access access) >> { >> FILE *f; >> char *ret = NULL, *pos = NULL; >> - char line[50]; >> + char line[255]; > > Isn't there a max path length define in a system header we could use instead? This isn't a path. I just chose 255 as a nice round number :) There's no good indication of what the length will be. However, I don't expect it to change in the future, so 255 should be plenty. Cheers, Joel
On Wed, 2016-11-16 at 13:45 +1030, Joel Stanley wrote: > pflash relies on arch_flash_arm parsing /proc/mtd to discover the > pnor > partition. It helpfully uses strcasestr so it can handle the string > changing, which is what has happened as we moved to upstream > compliant > mtd device tree bindings. > > We currently have a string like this: > > dev: size erasesize name > mtd0: 00060000 00001000 "u-boot" > mtd1: 00020000 00001000 "u-boot-env" > mtd2: 00280000 00001000 "kernel" > mtd3: 001c0000 00001000 "initramfs" > mtd4: 01740000 00001000 "rofs" > mtd5: 00400000 00001000 "rwfs" > mtd6: 02000000 00001000 "1e620000.flash-controller:flash@1" > mtd7: 08000000 00001000 "1e630000.flash-controller:pnor@0" > > Unfortunately arch_flash_arm assumes the string will be at most 50 > characters. That's right before the label we're looking for starts so > we ignore that line and keep searching. > > Fix it by allowing for a 255 character line. > > Fixes: 48ab7ce09504 (external/pflash: Add --mtd) > Signed-off-by: Joel Stanley <joel@jms.id.au> Reviewed-by: Cyril Bur <cyril.bur@au1.ibm.com> > --- > external/common/arch_flash_arm.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/external/common/arch_flash_arm.c > b/external/common/arch_flash_arm.c > index bb8800ff556d..3cdd41ded0a9 100644 > --- a/external/common/arch_flash_arm.c > +++ b/external/common/arch_flash_arm.c > @@ -241,7 +241,7 @@ static char *get_dev_mtd(enum flash_access > access) > { > FILE *f; > char *ret = NULL, *pos = NULL; > - char line[50]; > + char line[255]; > > if (access != BMC_MTD && access != PNOR_MTD) > return NULL;
Joel Stanley <joel@jms.id.au> writes: > pflash relies on arch_flash_arm parsing /proc/mtd to discover the pnor > partition. It helpfully uses strcasestr so it can handle the string > changing, which is what has happened as we moved to upstream compliant > mtd device tree bindings. > > We currently have a string like this: > > dev: size erasesize name > mtd0: 00060000 00001000 "u-boot" > mtd1: 00020000 00001000 "u-boot-env" > mtd2: 00280000 00001000 "kernel" > mtd3: 001c0000 00001000 "initramfs" > mtd4: 01740000 00001000 "rofs" > mtd5: 00400000 00001000 "rwfs" > mtd6: 02000000 00001000 "1e620000.flash-controller:flash@1" > mtd7: 08000000 00001000 "1e630000.flash-controller:pnor@0" > > Unfortunately arch_flash_arm assumes the string will be at most 50 > characters. That's right before the label we're looking for starts so > we ignore that line and keep searching. > > Fix it by allowing for a 255 character line. > > Fixes: 48ab7ce09504 (external/pflash: Add --mtd) > Signed-off-by: Joel Stanley <joel@jms.id.au> > --- > external/common/arch_flash_arm.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Cheers, merged to master as of 7d6e73810dec029678a0d14a3f47485d4025520e and skiboot-5.4.x (due to Fixes: being applicable here, and just in case openbmc folk don't want to jump on the 'build random master commits' bandwagon of fun) as of bb3ec49
diff --git a/external/common/arch_flash_arm.c b/external/common/arch_flash_arm.c index bb8800ff556d..3cdd41ded0a9 100644 --- a/external/common/arch_flash_arm.c +++ b/external/common/arch_flash_arm.c @@ -241,7 +241,7 @@ static char *get_dev_mtd(enum flash_access access) { FILE *f; char *ret = NULL, *pos = NULL; - char line[50]; + char line[255]; if (access != BMC_MTD && access != PNOR_MTD) return NULL;
pflash relies on arch_flash_arm parsing /proc/mtd to discover the pnor partition. It helpfully uses strcasestr so it can handle the string changing, which is what has happened as we moved to upstream compliant mtd device tree bindings. We currently have a string like this: dev: size erasesize name mtd0: 00060000 00001000 "u-boot" mtd1: 00020000 00001000 "u-boot-env" mtd2: 00280000 00001000 "kernel" mtd3: 001c0000 00001000 "initramfs" mtd4: 01740000 00001000 "rofs" mtd5: 00400000 00001000 "rwfs" mtd6: 02000000 00001000 "1e620000.flash-controller:flash@1" mtd7: 08000000 00001000 "1e630000.flash-controller:pnor@0" Unfortunately arch_flash_arm assumes the string will be at most 50 characters. That's right before the label we're looking for starts so we ignore that line and keep searching. Fix it by allowing for a 255 character line. Fixes: 48ab7ce09504 (external/pflash: Add --mtd) Signed-off-by: Joel Stanley <joel@jms.id.au> --- external/common/arch_flash_arm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)