* [PATCH 1/2] mtd: Fix the check on nvmem_register() ret code
@ 2019-01-02 14:36 Boris Brezillon
2019-01-02 14:36 ` [PATCH 2/2] mtd: Check add_mtd_device() " Boris Brezillon
` (2 more replies)
0 siblings, 3 replies; 20+ messages in thread
From: Boris Brezillon @ 2019-01-02 14:36 UTC (permalink / raw)
To: David Woodhouse, Brian Norris, Boris Brezillon, Marek Vasut,
Richard Weinberger, linux-mtd
Cc: Boris Brezillon, Bartosz Golaszewski, Alban Bedel
Commit 20167b70c894 ("nvmem: use EOPNOTSUPP instead of ENOSYS") changed
the nvmem_register() ret code from ENOSYS to EOPNOTSUPP when
CONFIG_NVMEM is not enabled, but the check in mtd_nvmem_add() was not
adjusted accordingly.
Cc: Bartosz Golaszewski <brgl@bgdev.pl>
Cc: Alban Bedel <albeu@free.fr>
Fixes: c4dfa25ab307 ("mtd: add support for reading MTD devices via the nvmem API")
Reported-by: kernel test robot <rong.a.chen@intel.com>
Signed-off-by: Boris Brezillon <bbrezillon@kernel.org>
---
drivers/mtd/mtdcore.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index 21e3cdc04036..999b705769a8 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -522,7 +522,7 @@ static int mtd_nvmem_add(struct mtd_info *mtd)
mtd->nvmem = nvmem_register(&config);
if (IS_ERR(mtd->nvmem)) {
/* Just ignore if there is no NVMEM support in the kernel */
- if (PTR_ERR(mtd->nvmem) == -ENOSYS) {
+ if (PTR_ERR(mtd->nvmem) == -EOPNOTSUPP) {
mtd->nvmem = NULL;
} else {
dev_err(&mtd->dev, "Failed to register NVMEM device\n");
--
2.17.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH 2/2] mtd: Check add_mtd_device() ret code 2019-01-02 14:36 [PATCH 1/2] mtd: Fix the check on nvmem_register() ret code Boris Brezillon @ 2019-01-02 14:36 ` Boris Brezillon 2019-01-08 8:29 ` [2/2] " Boris Brezillon 2019-01-22 11:21 ` [PATCH 2/2] " Geert Uytterhoeven 2019-01-02 15:13 ` [PATCH 1/2] mtd: Fix the check on nvmem_register() " Bartosz Golaszewski 2019-01-08 8:30 ` [1/2] " Boris Brezillon 2 siblings, 2 replies; 20+ messages in thread From: Boris Brezillon @ 2019-01-02 14:36 UTC (permalink / raw) To: David Woodhouse, Brian Norris, Boris Brezillon, Marek Vasut, Richard Weinberger, linux-mtd Cc: Boris Brezillon add_mtd_device() can fail. We should always check its return value and gracefully handle the failure case. Fix the call sites where this not done (in mtdpart.c) and add a __must_check attribute to the prototype to avoid this kind of mistakes. Signed-off-by: Boris Brezillon <bbrezillon@kernel.org> --- No Fixes or Cc-stable tag here, as this seems to have worked just fine without checking add_mtd_device() ret code until we started to expose MTD devices as NVMEM providers (queued for 4.21). --- drivers/mtd/mtdcore.h | 2 +- drivers/mtd/mtdpart.c | 36 +++++++++++++++++++++++++++++++----- 2 files changed, 32 insertions(+), 6 deletions(-) diff --git a/drivers/mtd/mtdcore.h b/drivers/mtd/mtdcore.h index 9887bda317cd..b31c868019ad 100644 --- a/drivers/mtd/mtdcore.h +++ b/drivers/mtd/mtdcore.h @@ -7,7 +7,7 @@ extern struct mutex mtd_table_mutex; struct mtd_info *__mtd_next_device(int i); -int add_mtd_device(struct mtd_info *mtd); +int __must_check add_mtd_device(struct mtd_info *mtd); int del_mtd_device(struct mtd_info *mtd); int add_mtd_partitions(struct mtd_info *, const struct mtd_partition *, int); int del_mtd_partitions(struct mtd_info *); diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c index b6af41b04622..60104e1079c5 100644 --- a/drivers/mtd/mtdpart.c +++ b/drivers/mtd/mtdpart.c @@ -618,10 +618,22 @@ int mtd_add_partition(struct mtd_info *parent, const char *name, list_add(&new->list, &mtd_partitions); mutex_unlock(&mtd_partitions_mutex); - add_mtd_device(&new->mtd); + ret = add_mtd_device(&new->mtd); + if (ret) + goto err_remove_part; mtd_add_partition_attrs(new); + return 0; + +err_remove_part: + mutex_lock(&mtd_partitions_mutex); + list_del(&new->list); + mutex_unlock(&mtd_partitions_mutex); + + free_partition(new); + pr_info("%s:%i\n", __func__, __LINE__); + return ret; } EXPORT_SYMBOL_GPL(mtd_add_partition); @@ -712,22 +724,31 @@ int add_mtd_partitions(struct mtd_info *master, { struct mtd_part *slave; uint64_t cur_offset = 0; - int i; + int i, ret; printk(KERN_NOTICE "Creating %d MTD partitions on \"%s\":\n", nbparts, master->name); for (i = 0; i < nbparts; i++) { slave = allocate_partition(master, parts + i, i, cur_offset); if (IS_ERR(slave)) { - del_mtd_partitions(master); - return PTR_ERR(slave); + ret = PTR_ERR(slave); + goto err_del_partitions; } mutex_lock(&mtd_partitions_mutex); list_add(&slave->list, &mtd_partitions); mutex_unlock(&mtd_partitions_mutex); - add_mtd_device(&slave->mtd); + ret = add_mtd_device(&slave->mtd); + if (ret) { + mutex_lock(&mtd_partitions_mutex); + list_del(&slave->list); + mutex_unlock(&mtd_partitions_mutex); + + free_partition(slave); + goto err_del_partitions; + } + mtd_add_partition_attrs(slave); /* Look for subpartitions */ parse_mtd_partitions(&slave->mtd, parts[i].types, NULL); @@ -736,6 +757,11 @@ int add_mtd_partitions(struct mtd_info *master, } return 0; + +err_del_partitions: + del_mtd_partitions(master); + + return ret; } static DEFINE_SPINLOCK(part_parser_lock); -- 2.17.1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [2/2] mtd: Check add_mtd_device() ret code 2019-01-02 14:36 ` [PATCH 2/2] mtd: Check add_mtd_device() " Boris Brezillon @ 2019-01-08 8:29 ` Boris Brezillon 2019-01-22 11:21 ` [PATCH 2/2] " Geert Uytterhoeven 1 sibling, 0 replies; 20+ messages in thread From: Boris Brezillon @ 2019-01-08 8:29 UTC (permalink / raw) To: Boris Brezillon, David Woodhouse, Brian Norris, Boris Brezillon, Marek Vasut, Richard Weinberger, linux-mtd From: Boris Brezillon <boris.brezillon@bootlin.com> On Wed, 2019-01-02 at 14:36:54 UTC, Boris Brezillon wrote: > add_mtd_device() can fail. We should always check its return value > and gracefully handle the failure case. Fix the call sites where this > not done (in mtdpart.c) and add a __must_check attribute to the > prototype to avoid this kind of mistakes. > > Signed-off-by: Boris Brezillon <bbrezillon@kernel.org> Applied to http://git.infradead.org/linux-mtd.git master, thanks. Boris ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] mtd: Check add_mtd_device() ret code 2019-01-02 14:36 ` [PATCH 2/2] mtd: Check add_mtd_device() " Boris Brezillon 2019-01-08 8:29 ` [2/2] " Boris Brezillon @ 2019-01-22 11:21 ` Geert Uytterhoeven 2019-01-22 12:31 ` Boris Brezillon 1 sibling, 1 reply; 20+ messages in thread From: Geert Uytterhoeven @ 2019-01-22 11:21 UTC (permalink / raw) To: Boris Brezillon Cc: Marek Vasut, Richard Weinberger, Boris Brezillon, MTD Maling List, Brian Norris, David Woodhouse Hi Boris, On Wed, Jan 2, 2019 at 3:37 PM Boris Brezillon <bbrezillon@kernel.org> wrote: > add_mtd_device() can fail. We should always check its return value > and gracefully handle the failure case. Fix the call sites where this > not done (in mtdpart.c) and add a __must_check attribute to the > prototype to avoid this kind of mistakes. > > Signed-off-by: Boris Brezillon <bbrezillon@kernel.org> > --- > No Fixes or Cc-stable tag here, as this seems to have worked just fine > without checking add_mtd_device() ret code until we started to expose > MTD devices as NVMEM providers (queued for 4.21). Oh yes ;-) https://lore.kernel.org/lkml/87feac02-e955-1897-d4a4-d6d6d1082e45@gmail.com/t/ Your patch is very similar to mine, so the crash is gone. However, the warning is still there: m25p80 spi0.0: s25sl032p (4096 Kbytes) 3 fixed-partitions partitions found on MTD device spi0.0 Creating 3 MTD partitions on "spi0.0": 0x000000000000-0x000000080000 : "loader" 0x000000080000-0x000000600000 : "user" mtd: partition "user" extends beyond the end of device "spi0.0" -- size truncated to 0x380000 0x000000600000-0x000004000000 : "flash" mtd: partition "flash" is out of reach -- disabled ------------[ cut here ]------------ WARNING: CPU: 0 PID: 1 at drivers/mtd/mtdcore.c:571 add_mtd_device+0x90/0x3b0 Interestingly, only one partition is created, covering the full size of the device: # cat /proc/partitions major minor #blocks name 31 0 4096 mtdblock0 While I would expect two partitions, "loader" and truncated "user": 31 0 512 mtdblock0 31 1 3584 mtdblock1 Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] mtd: Check add_mtd_device() ret code 2019-01-22 11:21 ` [PATCH 2/2] " Geert Uytterhoeven @ 2019-01-22 12:31 ` Boris Brezillon 2019-01-29 11:03 ` Boris Brezillon 0 siblings, 1 reply; 20+ messages in thread From: Boris Brezillon @ 2019-01-22 12:31 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Marek Vasut, Richard Weinberger, Boris Brezillon, MTD Maling List, Brian Norris, David Woodhouse On Tue, 22 Jan 2019 12:21:11 +0100 Geert Uytterhoeven <geert@linux-m68k.org> wrote: > Hi Boris, > > On Wed, Jan 2, 2019 at 3:37 PM Boris Brezillon <bbrezillon@kernel.org> wrote: > > add_mtd_device() can fail. We should always check its return value > > and gracefully handle the failure case. Fix the call sites where this > > not done (in mtdpart.c) and add a __must_check attribute to the > > prototype to avoid this kind of mistakes. > > > > Signed-off-by: Boris Brezillon <bbrezillon@kernel.org> > > --- > > No Fixes or Cc-stable tag here, as this seems to have worked just fine > > without checking add_mtd_device() ret code until we started to expose > > MTD devices as NVMEM providers (queued for 4.21). > > Oh yes ;-) > > https://lore.kernel.org/lkml/87feac02-e955-1897-d4a4-d6d6d1082e45@gmail.com/t/ > > Your patch is very similar to mine, so the crash is gone. Oops, sorry about that. I completely forgot about this patch. It seems the discussion led to a different conclusion though (patch allocate_partitions() to reject wrong parts early) and the v2 was never sent (or I missed it). Anyway, I guess we should have done both (check add_mtd_device() ret code everywhere and patch allocate_partitions() to reject bad parts early). > However, the warning is still there: > > m25p80 spi0.0: s25sl032p (4096 Kbytes) > 3 fixed-partitions partitions found on MTD device spi0.0 > Creating 3 MTD partitions on "spi0.0": > 0x000000000000-0x000000080000 : "loader" > 0x000000080000-0x000000600000 : "user" > mtd: partition "user" extends beyond the end of device "spi0.0" -- > size truncated to 0x380000 > 0x000000600000-0x000004000000 : "flash" > mtd: partition "flash" is out of reach -- disabled > ------------[ cut here ]------------ > WARNING: CPU: 0 PID: 1 at drivers/mtd/mtdcore.c:571 > add_mtd_device+0x90/0x3b0 > > Interestingly, only one partition is created, covering the full size of the > device: > > # cat /proc/partitions > major minor #blocks name > > 31 0 4096 mtdblock0 > > While I would expect two partitions, "loader" and truncated "user": > > 31 0 512 mtdblock0 > 31 1 3584 mtdblock1 Yes, makes sense, I guess your patch was better than mine :-/. Can you try with the following diff applied and let me know if it solves the problem? --->8--- diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c index 60104e1079c5..aefd3344991f 100644 --- a/drivers/mtd/mtdpart.c +++ b/drivers/mtd/mtdpart.c @@ -724,16 +724,14 @@ int add_mtd_partitions(struct mtd_info *master, { struct mtd_part *slave; uint64_t cur_offset = 0; - int i, ret; + int i, ret, actual_nbparts = 0; printk(KERN_NOTICE "Creating %d MTD partitions on \"%s\":\n", nbparts, master->name); for (i = 0; i < nbparts; i++) { slave = allocate_partition(master, parts + i, i, cur_offset); - if (IS_ERR(slave)) { - ret = PTR_ERR(slave); - goto err_del_partitions; - } + if (IS_ERR(slave)) + continue; mutex_lock(&mtd_partitions_mutex); list_add(&slave->list, &mtd_partitions); @@ -746,7 +744,7 @@ int add_mtd_partitions(struct mtd_info *master, mutex_unlock(&mtd_partitions_mutex); free_partition(slave); - goto err_del_partitions; + continue; } mtd_add_partition_attrs(slave); @@ -754,14 +752,10 @@ int add_mtd_partitions(struct mtd_info *master, parse_mtd_partitions(&slave->mtd, parts[i].types, NULL); cur_offset = slave->offset + slave->mtd.size; + actual_nbparts++; } - return 0; - -err_del_partitions: - del_mtd_partitions(master); - - return ret; + return actual_nbparts; } static DEFINE_SPINLOCK(part_parser_lock); @@ -1003,10 +997,10 @@ int parse_mtd_partitions(struct mtd_info *master, const char *const *types, } /* Found partitions! */ if (ret > 0) { - err = add_mtd_partitions(master, pparts.parts, + ret = add_mtd_partitions(master, pparts.parts, pparts.nr_parts); mtd_part_parser_cleanup(&pparts); - return err ? err : pparts.nr_parts; + return ret; } /* * Stash the first error we see; only report it if no parser ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] mtd: Check add_mtd_device() ret code 2019-01-22 12:31 ` Boris Brezillon @ 2019-01-29 11:03 ` Boris Brezillon 2019-01-29 15:29 ` Geert Uytterhoeven 0 siblings, 1 reply; 20+ messages in thread From: Boris Brezillon @ 2019-01-29 11:03 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Marek Vasut, Richard Weinberger, Boris Brezillon, MTD Maling List, Brian Norris, David Woodhouse Hi Geert, On Tue, 22 Jan 2019 13:31:59 +0100 Boris Brezillon <bbrezillon@kernel.org> wrote: > On Tue, 22 Jan 2019 12:21:11 +0100 > Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > Hi Boris, > > > > On Wed, Jan 2, 2019 at 3:37 PM Boris Brezillon <bbrezillon@kernel.org> wrote: > > > add_mtd_device() can fail. We should always check its return value > > > and gracefully handle the failure case. Fix the call sites where this > > > not done (in mtdpart.c) and add a __must_check attribute to the > > > prototype to avoid this kind of mistakes. > > > > > > Signed-off-by: Boris Brezillon <bbrezillon@kernel.org> > > > --- > > > No Fixes or Cc-stable tag here, as this seems to have worked just fine > > > without checking add_mtd_device() ret code until we started to expose > > > MTD devices as NVMEM providers (queued for 4.21). > > > > Oh yes ;-) > > > > https://lore.kernel.org/lkml/87feac02-e955-1897-d4a4-d6d6d1082e45@gmail.com/t/ > > > > Your patch is very similar to mine, so the crash is gone. > > Oops, sorry about that. I completely forgot about this patch. It seems > the discussion led to a different conclusion though (patch > allocate_partitions() to reject wrong parts early) and the v2 was never > sent (or I missed it). Anyway, I guess we should have done both (check > add_mtd_device() ret code everywhere and patch allocate_partitions() to > reject bad parts early). > > > However, the warning is still there: > > > > m25p80 spi0.0: s25sl032p (4096 Kbytes) > > 3 fixed-partitions partitions found on MTD device spi0.0 > > Creating 3 MTD partitions on "spi0.0": > > 0x000000000000-0x000000080000 : "loader" > > 0x000000080000-0x000000600000 : "user" > > mtd: partition "user" extends beyond the end of device "spi0.0" -- > > size truncated to 0x380000 > > 0x000000600000-0x000004000000 : "flash" > > mtd: partition "flash" is out of reach -- disabled > > ------------[ cut here ]------------ > > WARNING: CPU: 0 PID: 1 at drivers/mtd/mtdcore.c:571 > > add_mtd_device+0x90/0x3b0 > > > > Interestingly, only one partition is created, covering the full size of the > > device: > > > > # cat /proc/partitions > > major minor #blocks name > > > > 31 0 4096 mtdblock0 > > > > While I would expect two partitions, "loader" and truncated "user": > > > > 31 0 512 mtdblock0 > > 31 1 3584 mtdblock1 > > Yes, makes sense, I guess your patch was better than mine :-/. Can you > try with the following diff applied and let me know if it solves the > problem? Gentle ping: is this diff fixing your problem, and do you want me to send a proper patch for it or should I let you send one? > > --->8--- > diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c > index 60104e1079c5..aefd3344991f 100644 > --- a/drivers/mtd/mtdpart.c > +++ b/drivers/mtd/mtdpart.c > @@ -724,16 +724,14 @@ int add_mtd_partitions(struct mtd_info *master, > { > struct mtd_part *slave; > uint64_t cur_offset = 0; > - int i, ret; > + int i, ret, actual_nbparts = 0; > > printk(KERN_NOTICE "Creating %d MTD partitions on \"%s\":\n", nbparts, master->name); > > for (i = 0; i < nbparts; i++) { > slave = allocate_partition(master, parts + i, i, cur_offset); > - if (IS_ERR(slave)) { > - ret = PTR_ERR(slave); > - goto err_del_partitions; > - } > + if (IS_ERR(slave)) > + continue; > > mutex_lock(&mtd_partitions_mutex); > list_add(&slave->list, &mtd_partitions); > @@ -746,7 +744,7 @@ int add_mtd_partitions(struct mtd_info *master, > mutex_unlock(&mtd_partitions_mutex); > > free_partition(slave); > - goto err_del_partitions; > + continue; > } > > mtd_add_partition_attrs(slave); > @@ -754,14 +752,10 @@ int add_mtd_partitions(struct mtd_info *master, > parse_mtd_partitions(&slave->mtd, parts[i].types, NULL); > > cur_offset = slave->offset + slave->mtd.size; > + actual_nbparts++; > } > > - return 0; > - > -err_del_partitions: > - del_mtd_partitions(master); > - > - return ret; > + return actual_nbparts; > } > > static DEFINE_SPINLOCK(part_parser_lock); > @@ -1003,10 +997,10 @@ int parse_mtd_partitions(struct mtd_info *master, const char *const *types, > } > /* Found partitions! */ > if (ret > 0) { > - err = add_mtd_partitions(master, pparts.parts, > + ret = add_mtd_partitions(master, pparts.parts, > pparts.nr_parts); > mtd_part_parser_cleanup(&pparts); > - return err ? err : pparts.nr_parts; > + return ret; > } > /* > * Stash the first error we see; only report it if no parser ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] mtd: Check add_mtd_device() ret code 2019-01-29 11:03 ` Boris Brezillon @ 2019-01-29 15:29 ` Geert Uytterhoeven 2019-01-30 8:52 ` Boris Brezillon ` (2 more replies) 0 siblings, 3 replies; 20+ messages in thread From: Geert Uytterhoeven @ 2019-01-29 15:29 UTC (permalink / raw) To: Boris Brezillon Cc: Marek Vasut, Richard Weinberger, Boris Brezillon, MTD Maling List, Brian Norris, David Woodhouse Hi Boris, On Tue, Jan 29, 2019 at 12:03 PM Boris Brezillon <bbrezillon@kernel.org> wrote: > On Tue, 22 Jan 2019 13:31:59 +0100 > Boris Brezillon <bbrezillon@kernel.org> wrote: > > On Tue, 22 Jan 2019 12:21:11 +0100 > > Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > On Wed, Jan 2, 2019 at 3:37 PM Boris Brezillon <bbrezillon@kernel.org> wrote: > > > > add_mtd_device() can fail. We should always check its return value > > > > and gracefully handle the failure case. Fix the call sites where this > > > > not done (in mtdpart.c) and add a __must_check attribute to the > > > > prototype to avoid this kind of mistakes. > > > > > > > > Signed-off-by: Boris Brezillon <bbrezillon@kernel.org> > > > > --- > > > > No Fixes or Cc-stable tag here, as this seems to have worked just fine > > > > without checking add_mtd_device() ret code until we started to expose > > > > MTD devices as NVMEM providers (queued for 4.21). > > > > > > Oh yes ;-) > > > > > > https://lore.kernel.org/lkml/87feac02-e955-1897-d4a4-d6d6d1082e45@gmail.com/t/ > > > > > > Your patch is very similar to mine, so the crash is gone. > > > > Oops, sorry about that. I completely forgot about this patch. It seems > > the discussion led to a different conclusion though (patch > > allocate_partitions() to reject wrong parts early) and the v2 was never > > sent (or I missed it). Anyway, I guess we should have done both (check > > add_mtd_device() ret code everywhere and patch allocate_partitions() to > > reject bad parts early). > > > > > However, the warning is still there: > > > > > > m25p80 spi0.0: s25sl032p (4096 Kbytes) > > > 3 fixed-partitions partitions found on MTD device spi0.0 > > > Creating 3 MTD partitions on "spi0.0": > > > 0x000000000000-0x000000080000 : "loader" > > > 0x000000080000-0x000000600000 : "user" > > > mtd: partition "user" extends beyond the end of device "spi0.0" -- > > > size truncated to 0x380000 > > > 0x000000600000-0x000004000000 : "flash" > > > mtd: partition "flash" is out of reach -- disabled > > > ------------[ cut here ]------------ > > > WARNING: CPU: 0 PID: 1 at drivers/mtd/mtdcore.c:571 > > > add_mtd_device+0x90/0x3b0 > > > > > > Interestingly, only one partition is created, covering the full size of the > > > device: > > > > > > # cat /proc/partitions > > > major minor #blocks name > > > > > > 31 0 4096 mtdblock0 > > > > > > While I would expect two partitions, "loader" and truncated "user": > > > > > > 31 0 512 mtdblock0 > > > 31 1 3584 mtdblock1 > > > > Yes, makes sense, I guess your patch was better than mine :-/. Can you > > try with the following diff applied and let me know if it solves the > > problem? > > Gentle ping: is this diff fixing your problem, and do you want me to > send a proper patch for it or should I let you send one? Yes, the diff below fixes the partitions for me, so Tested-by: Geert Uytterhoeven <geert+renesas@glider.be> Note that the warning is still there, but that's probably OK. > > --- a/drivers/mtd/mtdpart.c > > +++ b/drivers/mtd/mtdpart.c > > @@ -724,16 +724,14 @@ int add_mtd_partitions(struct mtd_info *master, > > { > > struct mtd_part *slave; > > uint64_t cur_offset = 0; > > - int i, ret; > > + int i, ret, actual_nbparts = 0; > > > > printk(KERN_NOTICE "Creating %d MTD partitions on \"%s\":\n", nbparts, master->name); > > > > for (i = 0; i < nbparts; i++) { > > slave = allocate_partition(master, parts + i, i, cur_offset); > > - if (IS_ERR(slave)) { > > - ret = PTR_ERR(slave); > > - goto err_del_partitions; > > - } > > + if (IS_ERR(slave)) > > + continue; > > > > mutex_lock(&mtd_partitions_mutex); > > list_add(&slave->list, &mtd_partitions); > > @@ -746,7 +744,7 @@ int add_mtd_partitions(struct mtd_info *master, > > mutex_unlock(&mtd_partitions_mutex); > > > > free_partition(slave); > > - goto err_del_partitions; > > + continue; > > } > > > > mtd_add_partition_attrs(slave); > > @@ -754,14 +752,10 @@ int add_mtd_partitions(struct mtd_info *master, > > parse_mtd_partitions(&slave->mtd, parts[i].types, NULL); > > > > cur_offset = slave->offset + slave->mtd.size; > > + actual_nbparts++; > > } > > > > - return 0; > > - > > -err_del_partitions: > > - del_mtd_partitions(master); > > - > > - return ret; > > + return actual_nbparts; > > } > > > > static DEFINE_SPINLOCK(part_parser_lock); > > @@ -1003,10 +997,10 @@ int parse_mtd_partitions(struct mtd_info *master, const char *const *types, > > } > > /* Found partitions! */ > > if (ret > 0) { > > - err = add_mtd_partitions(master, pparts.parts, > > + ret = add_mtd_partitions(master, pparts.parts, > > pparts.nr_parts); > > mtd_part_parser_cleanup(&pparts); > > - return err ? err : pparts.nr_parts; > > + return ret; > > } > > /* > > * Stash the first error we see; only report it if no parser Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] mtd: Check add_mtd_device() ret code 2019-01-29 15:29 ` Geert Uytterhoeven @ 2019-01-30 8:52 ` Boris Brezillon 2019-01-30 9:05 ` Geert Uytterhoeven 2019-01-30 8:55 ` Boris Brezillon 2019-04-01 9:50 ` Geert Uytterhoeven 2 siblings, 1 reply; 20+ messages in thread From: Boris Brezillon @ 2019-01-30 8:52 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Marek Vasut, Richard Weinberger, Boris Brezillon, MTD Maling List, Brian Norris, David Woodhouse On Tue, 29 Jan 2019 16:29:55 +0100 Geert Uytterhoeven <geert@linux-m68k.org> wrote: > Hi Boris, > > On Tue, Jan 29, 2019 at 12:03 PM Boris Brezillon <bbrezillon@kernel.org> wrote: > > On Tue, 22 Jan 2019 13:31:59 +0100 > > Boris Brezillon <bbrezillon@kernel.org> wrote: > > > On Tue, 22 Jan 2019 12:21:11 +0100 > > > Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > > On Wed, Jan 2, 2019 at 3:37 PM Boris Brezillon <bbrezillon@kernel.org> wrote: > > > > > add_mtd_device() can fail. We should always check its return value > > > > > and gracefully handle the failure case. Fix the call sites where this > > > > > not done (in mtdpart.c) and add a __must_check attribute to the > > > > > prototype to avoid this kind of mistakes. > > > > > > > > > > Signed-off-by: Boris Brezillon <bbrezillon@kernel.org> > > > > > --- > > > > > No Fixes or Cc-stable tag here, as this seems to have worked just fine > > > > > without checking add_mtd_device() ret code until we started to expose > > > > > MTD devices as NVMEM providers (queued for 4.21). > > > > > > > > Oh yes ;-) > > > > > > > > https://lore.kernel.org/lkml/87feac02-e955-1897-d4a4-d6d6d1082e45@gmail.com/t/ > > > > > > > > Your patch is very similar to mine, so the crash is gone. > > > > > > Oops, sorry about that. I completely forgot about this patch. It seems > > > the discussion led to a different conclusion though (patch > > > allocate_partitions() to reject wrong parts early) and the v2 was never > > > sent (or I missed it). Anyway, I guess we should have done both (check > > > add_mtd_device() ret code everywhere and patch allocate_partitions() to > > > reject bad parts early). > > > > > > > However, the warning is still there: > > > > > > > > m25p80 spi0.0: s25sl032p (4096 Kbytes) > > > > 3 fixed-partitions partitions found on MTD device spi0.0 > > > > Creating 3 MTD partitions on "spi0.0": > > > > 0x000000000000-0x000000080000 : "loader" > > > > 0x000000080000-0x000000600000 : "user" > > > > mtd: partition "user" extends beyond the end of device "spi0.0" -- > > > > size truncated to 0x380000 > > > > 0x000000600000-0x000004000000 : "flash" > > > > mtd: partition "flash" is out of reach -- disabled > > > > ------------[ cut here ]------------ > > > > WARNING: CPU: 0 PID: 1 at drivers/mtd/mtdcore.c:571 > > > > add_mtd_device+0x90/0x3b0 > > > > > > > > Interestingly, only one partition is created, covering the full size of the > > > > device: > > > > > > > > # cat /proc/partitions > > > > major minor #blocks name > > > > > > > > 31 0 4096 mtdblock0 > > > > > > > > While I would expect two partitions, "loader" and truncated "user": > > > > > > > > 31 0 512 mtdblock0 > > > > 31 1 3584 mtdblock1 > > > > > > Yes, makes sense, I guess your patch was better than mine :-/. Can you > > > try with the following diff applied and let me know if it solves the > > > problem? > > > > Gentle ping: is this diff fixing your problem, and do you want me to > > send a proper patch for it or should I let you send one? > > Yes, the diff below fixes the partitions for me, so > Tested-by: Geert Uytterhoeven <geert+renesas@glider.be> > > Note that the warning is still there, but that's probably OK. You mean the pr_warn() or the WARN_ON() backtrace? The former is expected not the latter. ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] mtd: Check add_mtd_device() ret code 2019-01-30 8:52 ` Boris Brezillon @ 2019-01-30 9:05 ` Geert Uytterhoeven 2019-01-30 9:10 ` Boris Brezillon 2019-01-30 9:17 ` Boris Brezillon 0 siblings, 2 replies; 20+ messages in thread From: Geert Uytterhoeven @ 2019-01-30 9:05 UTC (permalink / raw) To: Boris Brezillon Cc: Marek Vasut, Richard Weinberger, Boris Brezillon, MTD Maling List, Brian Norris, David Woodhouse Hi Boris, On Wed, Jan 30, 2019 at 9:52 AM Boris Brezillon <bbrezillon@kernel.org> wrote: > On Tue, 29 Jan 2019 16:29:55 +0100 > Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > On Tue, Jan 29, 2019 at 12:03 PM Boris Brezillon <bbrezillon@kernel.org> wrote: > > > On Tue, 22 Jan 2019 13:31:59 +0100 > > > Boris Brezillon <bbrezillon@kernel.org> wrote: > > > > On Tue, 22 Jan 2019 12:21:11 +0100 > > > > Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > > > On Wed, Jan 2, 2019 at 3:37 PM Boris Brezillon <bbrezillon@kernel.org> wrote: > > > > > > add_mtd_device() can fail. We should always check its return value > > > > > > and gracefully handle the failure case. Fix the call sites where this > > > > > > not done (in mtdpart.c) and add a __must_check attribute to the > > > > > > prototype to avoid this kind of mistakes. > > > > > > > > > > > > Signed-off-by: Boris Brezillon <bbrezillon@kernel.org> > > > > > > --- > > > > > > No Fixes or Cc-stable tag here, as this seems to have worked just fine > > > > > > without checking add_mtd_device() ret code until we started to expose > > > > > > MTD devices as NVMEM providers (queued for 4.21). > > > > > > > > > > Oh yes ;-) > > > > > > > > > > https://lore.kernel.org/lkml/87feac02-e955-1897-d4a4-d6d6d1082e45@gmail.com/t/ > > > > > > > > > > Your patch is very similar to mine, so the crash is gone. > > > > > > > > Oops, sorry about that. I completely forgot about this patch. It seems > > > > the discussion led to a different conclusion though (patch > > > > allocate_partitions() to reject wrong parts early) and the v2 was never > > > > sent (or I missed it). Anyway, I guess we should have done both (check > > > > add_mtd_device() ret code everywhere and patch allocate_partitions() to > > > > reject bad parts early). > > > > > > > > > However, the warning is still there: > > > > > > > > > > m25p80 spi0.0: s25sl032p (4096 Kbytes) > > > > > 3 fixed-partitions partitions found on MTD device spi0.0 > > > > > Creating 3 MTD partitions on "spi0.0": > > > > > 0x000000000000-0x000000080000 : "loader" > > > > > 0x000000080000-0x000000600000 : "user" > > > > > mtd: partition "user" extends beyond the end of device "spi0.0" -- > > > > > size truncated to 0x380000 > > > > > 0x000000600000-0x000004000000 : "flash" > > > > > mtd: partition "flash" is out of reach -- disabled > > > > > ------------[ cut here ]------------ > > > > > WARNING: CPU: 0 PID: 1 at drivers/mtd/mtdcore.c:571 > > > > > add_mtd_device+0x90/0x3b0 > > > > > > > > > > Interestingly, only one partition is created, covering the full size of the > > > > > device: > > > > > > > > > > # cat /proc/partitions > > > > > major minor #blocks name > > > > > > > > > > 31 0 4096 mtdblock0 > > > > > > > > > > While I would expect two partitions, "loader" and truncated "user": > > > > > > > > > > 31 0 512 mtdblock0 > > > > > 31 1 3584 mtdblock1 > > > > > > > > Yes, makes sense, I guess your patch was better than mine :-/. Can you > > > > try with the following diff applied and let me know if it solves the > > > > problem? > > > > > > Gentle ping: is this diff fixing your problem, and do you want me to > > > send a proper patch for it or should I let you send one? > > > > Yes, the diff below fixes the partitions for me, so > > Tested-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > > Note that the warning is still there, but that's probably OK. > > You mean the pr_warn() or the WARN_ON() backtrace? The former is > expected not the latter. The WARN_ON() backtrace is still there. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] mtd: Check add_mtd_device() ret code 2019-01-30 9:05 ` Geert Uytterhoeven @ 2019-01-30 9:10 ` Boris Brezillon 2019-01-30 9:16 ` Geert Uytterhoeven 2019-01-30 9:17 ` Boris Brezillon 1 sibling, 1 reply; 20+ messages in thread From: Boris Brezillon @ 2019-01-30 9:10 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Marek Vasut, Richard Weinberger, Boris Brezillon, MTD Maling List, Brian Norris, David Woodhouse On Wed, 30 Jan 2019 10:05:29 +0100 Geert Uytterhoeven <geert@linux-m68k.org> wrote: > Hi Boris, > > On Wed, Jan 30, 2019 at 9:52 AM Boris Brezillon <bbrezillon@kernel.org> wrote: > > On Tue, 29 Jan 2019 16:29:55 +0100 > > Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > On Tue, Jan 29, 2019 at 12:03 PM Boris Brezillon <bbrezillon@kernel.org> wrote: > > > > On Tue, 22 Jan 2019 13:31:59 +0100 > > > > Boris Brezillon <bbrezillon@kernel.org> wrote: > > > > > On Tue, 22 Jan 2019 12:21:11 +0100 > > > > > Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > > > > On Wed, Jan 2, 2019 at 3:37 PM Boris Brezillon <bbrezillon@kernel.org> wrote: > > > > > > > add_mtd_device() can fail. We should always check its return value > > > > > > > and gracefully handle the failure case. Fix the call sites where this > > > > > > > not done (in mtdpart.c) and add a __must_check attribute to the > > > > > > > prototype to avoid this kind of mistakes. > > > > > > > > > > > > > > Signed-off-by: Boris Brezillon <bbrezillon@kernel.org> > > > > > > > --- > > > > > > > No Fixes or Cc-stable tag here, as this seems to have worked just fine > > > > > > > without checking add_mtd_device() ret code until we started to expose > > > > > > > MTD devices as NVMEM providers (queued for 4.21). > > > > > > > > > > > > Oh yes ;-) > > > > > > > > > > > > https://lore.kernel.org/lkml/87feac02-e955-1897-d4a4-d6d6d1082e45@gmail.com/t/ > > > > > > > > > > > > Your patch is very similar to mine, so the crash is gone. > > > > > > > > > > Oops, sorry about that. I completely forgot about this patch. It seems > > > > > the discussion led to a different conclusion though (patch > > > > > allocate_partitions() to reject wrong parts early) and the v2 was never > > > > > sent (or I missed it). Anyway, I guess we should have done both (check > > > > > add_mtd_device() ret code everywhere and patch allocate_partitions() to > > > > > reject bad parts early). > > > > > > > > > > > However, the warning is still there: > > > > > > > > > > > > m25p80 spi0.0: s25sl032p (4096 Kbytes) > > > > > > 3 fixed-partitions partitions found on MTD device spi0.0 > > > > > > Creating 3 MTD partitions on "spi0.0": > > > > > > 0x000000000000-0x000000080000 : "loader" > > > > > > 0x000000080000-0x000000600000 : "user" > > > > > > mtd: partition "user" extends beyond the end of device "spi0.0" -- > > > > > > size truncated to 0x380000 > > > > > > 0x000000600000-0x000004000000 : "flash" > > > > > > mtd: partition "flash" is out of reach -- disabled > > > > > > ------------[ cut here ]------------ > > > > > > WARNING: CPU: 0 PID: 1 at drivers/mtd/mtdcore.c:571 > > > > > > add_mtd_device+0x90/0x3b0 > > > > > > > > > > > > Interestingly, only one partition is created, covering the full size of the > > > > > > device: > > > > > > > > > > > > # cat /proc/partitions > > > > > > major minor #blocks name > > > > > > > > > > > > 31 0 4096 mtdblock0 > > > > > > > > > > > > While I would expect two partitions, "loader" and truncated "user": > > > > > > > > > > > > 31 0 512 mtdblock0 > > > > > > 31 1 3584 mtdblock1 > > > > > > > > > > Yes, makes sense, I guess your patch was better than mine :-/. Can you > > > > > try with the following diff applied and let me know if it solves the > > > > > problem? > > > > > > > > Gentle ping: is this diff fixing your problem, and do you want me to > > > > send a proper patch for it or should I let you send one? > > > > > > Yes, the diff below fixes the partitions for me, so > > > Tested-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > > > > Note that the warning is still there, but that's probably OK. > > > > You mean the pr_warn() or the WARN_ON() backtrace? The former is > > expected not the latter. > > The WARN_ON() backtrace is still there. I don't see a WARN_ON() at drivers/mtd/mtdcore.c:571. Which branch are you using for your tests? ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] mtd: Check add_mtd_device() ret code 2019-01-30 9:10 ` Boris Brezillon @ 2019-01-30 9:16 ` Geert Uytterhoeven 2019-02-01 8:50 ` Boris Brezillon 0 siblings, 1 reply; 20+ messages in thread From: Geert Uytterhoeven @ 2019-01-30 9:16 UTC (permalink / raw) To: Boris Brezillon Cc: Marek Vasut, Richard Weinberger, Boris Brezillon, MTD Maling List, Brian Norris, David Woodhouse Hi Boris, On Wed, Jan 30, 2019 at 10:10 AM Boris Brezillon <bbrezillon@kernel.org> wrote: > On Wed, 30 Jan 2019 10:05:29 +0100 > Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > On Wed, Jan 30, 2019 at 9:52 AM Boris Brezillon <bbrezillon@kernel.org> wrote: > > > On Tue, 29 Jan 2019 16:29:55 +0100 > > > Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > > On Tue, Jan 29, 2019 at 12:03 PM Boris Brezillon <bbrezillon@kernel.org> wrote: > > > > > On Tue, 22 Jan 2019 13:31:59 +0100 > > > > > Boris Brezillon <bbrezillon@kernel.org> wrote: > > > > > > On Tue, 22 Jan 2019 12:21:11 +0100 > > > > > > Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > > > > > On Wed, Jan 2, 2019 at 3:37 PM Boris Brezillon <bbrezillon@kernel.org> wrote: > > > > > > > > add_mtd_device() can fail. We should always check its return value > > > > > > > > and gracefully handle the failure case. Fix the call sites where this > > > > > > > > not done (in mtdpart.c) and add a __must_check attribute to the > > > > > > > > prototype to avoid this kind of mistakes. > > > > > > > > > > > > > > > > Signed-off-by: Boris Brezillon <bbrezillon@kernel.org> > > > > > > > > --- > > > > > > > > No Fixes or Cc-stable tag here, as this seems to have worked just fine > > > > > > > > without checking add_mtd_device() ret code until we started to expose > > > > > > > > MTD devices as NVMEM providers (queued for 4.21). > > > > > > > > > > > > > > Oh yes ;-) > > > > > > > > > > > > > > https://lore.kernel.org/lkml/87feac02-e955-1897-d4a4-d6d6d1082e45@gmail.com/t/ > > > > > > > > > > > > > > Your patch is very similar to mine, so the crash is gone. > > > > > > > > > > > > Oops, sorry about that. I completely forgot about this patch. It seems > > > > > > the discussion led to a different conclusion though (patch > > > > > > allocate_partitions() to reject wrong parts early) and the v2 was never > > > > > > sent (or I missed it). Anyway, I guess we should have done both (check > > > > > > add_mtd_device() ret code everywhere and patch allocate_partitions() to > > > > > > reject bad parts early). > > > > > > > > > > > > > However, the warning is still there: > > > > > > > > > > > > > > m25p80 spi0.0: s25sl032p (4096 Kbytes) > > > > > > > 3 fixed-partitions partitions found on MTD device spi0.0 > > > > > > > Creating 3 MTD partitions on "spi0.0": > > > > > > > 0x000000000000-0x000000080000 : "loader" > > > > > > > 0x000000080000-0x000000600000 : "user" > > > > > > > mtd: partition "user" extends beyond the end of device "spi0.0" -- > > > > > > > size truncated to 0x380000 > > > > > > > 0x000000600000-0x000004000000 : "flash" > > > > > > > mtd: partition "flash" is out of reach -- disabled > > > > > > > ------------[ cut here ]------------ > > > > > > > WARNING: CPU: 0 PID: 1 at drivers/mtd/mtdcore.c:571 > > > > > > > add_mtd_device+0x90/0x3b0 > > > > > > > > > > > > > > Interestingly, only one partition is created, covering the full size of the > > > > > > > device: > > > > > > > > > > > > > > # cat /proc/partitions > > > > > > > major minor #blocks name > > > > > > > > > > > > > > 31 0 4096 mtdblock0 > > > > > > > > > > > > > > While I would expect two partitions, "loader" and truncated "user": > > > > > > > > > > > > > > 31 0 512 mtdblock0 > > > > > > > 31 1 3584 mtdblock1 > > > > > > > > > > > > Yes, makes sense, I guess your patch was better than mine :-/. Can you > > > > > > try with the following diff applied and let me know if it solves the > > > > > > problem? > > > > > > > > > > Gentle ping: is this diff fixing your problem, and do you want me to > > > > > send a proper patch for it or should I let you send one? > > > > > > > > Yes, the diff below fixes the partitions for me, so > > > > Tested-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > > > > > > Note that the warning is still there, but that's probably OK. > > > > > > You mean the pr_warn() or the WARN_ON() backtrace? The former is > > > expected not the latter. > > > > The WARN_ON() backtrace is still there. > > I don't see a WARN_ON() at drivers/mtd/mtdcore.c:571. Which branch are > you using for your tests? A tree based on last week's l2-mtd/master, i.e. lacking commit f7fd818cca0cea3d ("mtd: Remove empty lines at end of sysfs related functions"). This is the one triggering: if (WARN_ON((!mtd->erasesize || !mtd->_erase) && !(mtd->flags & MTD_NO_ERASE))) return -EINVAL; Thanks! Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] mtd: Check add_mtd_device() ret code 2019-01-30 9:16 ` Geert Uytterhoeven @ 2019-02-01 8:50 ` Boris Brezillon 2019-02-01 9:02 ` Geert Uytterhoeven 0 siblings, 1 reply; 20+ messages in thread From: Boris Brezillon @ 2019-02-01 8:50 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Marek Vasut, Richard Weinberger, Boris Brezillon, MTD Maling List, Brian Norris, David Woodhouse Hi Geert, On Wed, 30 Jan 2019 10:16:33 +0100 Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > A tree based on last week's l2-mtd/master, i.e. lacking commit f7fd818cca0cea3d > ("mtd: Remove empty lines at end of sysfs related functions"). > > This is the one triggering: > > if (WARN_ON((!mtd->erasesize || !mtd->_erase) && > !(mtd->flags & MTD_NO_ERASE))) > return -EINVAL; Do you think you'll have time to test [1]? I'd like to make that patch part of my fixes PR if possible. If you can't, that's fine, just let me know. Thanks, Boris [1]http://patchwork.ozlabs.org/patch/1033458/ ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] mtd: Check add_mtd_device() ret code 2019-02-01 8:50 ` Boris Brezillon @ 2019-02-01 9:02 ` Geert Uytterhoeven 0 siblings, 0 replies; 20+ messages in thread From: Geert Uytterhoeven @ 2019-02-01 9:02 UTC (permalink / raw) To: Boris Brezillon Cc: Marek Vasut, Richard Weinberger, Boris Brezillon, MTD Maling List, Brian Norris, David Woodhouse Hi Boris, On Fri, Feb 1, 2019 at 9:50 AM Boris Brezillon <bbrezillon@kernel.org> wrote: > On Wed, 30 Jan 2019 10:16:33 +0100 > Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > A tree based on last week's l2-mtd/master, i.e. lacking commit f7fd818cca0cea3d > > ("mtd: Remove empty lines at end of sysfs related functions"). > > > > This is the one triggering: > > > > if (WARN_ON((!mtd->erasesize || !mtd->_erase) && > > !(mtd->flags & MTD_NO_ERASE))) > > return -EINVAL; > > Do you think you'll have time to test [1]? I'd like to make that patch > part of my fixes PR if possible. If you can't, that's fine, just let me > know. Sure, I will, after FOSDEM ;-) > [1]http://patchwork.ozlabs.org/patch/1033458/ Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] mtd: Check add_mtd_device() ret code 2019-01-30 9:05 ` Geert Uytterhoeven 2019-01-30 9:10 ` Boris Brezillon @ 2019-01-30 9:17 ` Boris Brezillon 1 sibling, 0 replies; 20+ messages in thread From: Boris Brezillon @ 2019-01-30 9:17 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Marek Vasut, Richard Weinberger, Boris Brezillon, MTD Maling List, Brian Norris, David Woodhouse On Wed, 30 Jan 2019 10:05:29 +0100 Geert Uytterhoeven <geert@linux-m68k.org> wrote: > Hi Boris, > > On Wed, Jan 30, 2019 at 9:52 AM Boris Brezillon <bbrezillon@kernel.org> wrote: > > On Tue, 29 Jan 2019 16:29:55 +0100 > > Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > On Tue, Jan 29, 2019 at 12:03 PM Boris Brezillon <bbrezillon@kernel.org> wrote: > > > > On Tue, 22 Jan 2019 13:31:59 +0100 > > > > Boris Brezillon <bbrezillon@kernel.org> wrote: > > > > > On Tue, 22 Jan 2019 12:21:11 +0100 > > > > > Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > > > > On Wed, Jan 2, 2019 at 3:37 PM Boris Brezillon <bbrezillon@kernel.org> wrote: > > > > > > > add_mtd_device() can fail. We should always check its return value > > > > > > > and gracefully handle the failure case. Fix the call sites where this > > > > > > > not done (in mtdpart.c) and add a __must_check attribute to the > > > > > > > prototype to avoid this kind of mistakes. > > > > > > > > > > > > > > Signed-off-by: Boris Brezillon <bbrezillon@kernel.org> > > > > > > > --- > > > > > > > No Fixes or Cc-stable tag here, as this seems to have worked just fine > > > > > > > without checking add_mtd_device() ret code until we started to expose > > > > > > > MTD devices as NVMEM providers (queued for 4.21). > > > > > > > > > > > > Oh yes ;-) > > > > > > > > > > > > https://lore.kernel.org/lkml/87feac02-e955-1897-d4a4-d6d6d1082e45@gmail.com/t/ > > > > > > > > > > > > Your patch is very similar to mine, so the crash is gone. > > > > > > > > > > Oops, sorry about that. I completely forgot about this patch. It seems > > > > > the discussion led to a different conclusion though (patch > > > > > allocate_partitions() to reject wrong parts early) and the v2 was never > > > > > sent (or I missed it). Anyway, I guess we should have done both (check > > > > > add_mtd_device() ret code everywhere and patch allocate_partitions() to > > > > > reject bad parts early). > > > > > > > > > > > However, the warning is still there: > > > > > > > > > > > > m25p80 spi0.0: s25sl032p (4096 Kbytes) > > > > > > 3 fixed-partitions partitions found on MTD device spi0.0 > > > > > > Creating 3 MTD partitions on "spi0.0": > > > > > > 0x000000000000-0x000000080000 : "loader" > > > > > > 0x000000080000-0x000000600000 : "user" > > > > > > mtd: partition "user" extends beyond the end of device "spi0.0" -- > > > > > > size truncated to 0x380000 > > > > > > 0x000000600000-0x000004000000 : "flash" > > > > > > mtd: partition "flash" is out of reach -- disabled > > > > > > ------------[ cut here ]------------ > > > > > > WARNING: CPU: 0 PID: 1 at drivers/mtd/mtdcore.c:571 > > > > > > add_mtd_device+0x90/0x3b0 > > > > > > > > > > > > Interestingly, only one partition is created, covering the full size of the > > > > > > device: > > > > > > > > > > > > # cat /proc/partitions > > > > > > major minor #blocks name > > > > > > > > > > > > 31 0 4096 mtdblock0 > > > > > > > > > > > > While I would expect two partitions, "loader" and truncated "user": > > > > > > > > > > > > 31 0 512 mtdblock0 > > > > > > 31 1 3584 mtdblock1 > > > > > > > > > > Yes, makes sense, I guess your patch was better than mine :-/. Can you > > > > > try with the following diff applied and let me know if it solves the > > > > > problem? > > > > > > > > Gentle ping: is this diff fixing your problem, and do you want me to > > > > send a proper patch for it or should I let you send one? > > > > > > Yes, the diff below fixes the partitions for me, so > > > Tested-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > > > > Note that the warning is still there, but that's probably OK. > > > > You mean the pr_warn() or the WARN_ON() backtrace? The former is > > expected not the latter. > > The WARN_ON() backtrace is still there. Can you try with this patch instead of the previous one? --->8--- diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c index e6d9467f6be0..37f174ccbcec 100644 --- a/drivers/mtd/mtdpart.c +++ b/drivers/mtd/mtdpart.c @@ -480,6 +480,10 @@ static struct mtd_part *allocate_partition(struct mtd_info *parent, /* let's register it anyway to preserve ordering */ slave->offset = 0; slave->mtd.size = 0; + + /* Initialize ->erasesize to make add_mtd_device() happy. */ + slave->mtd.erasesize = parent->erasesize; + printk(KERN_ERR"mtd: partition \"%s\" is out of reach -- disabled\n", part->name); goto out_register; ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] mtd: Check add_mtd_device() ret code 2019-01-29 15:29 ` Geert Uytterhoeven 2019-01-30 8:52 ` Boris Brezillon @ 2019-01-30 8:55 ` Boris Brezillon 2019-01-30 9:12 ` Geert Uytterhoeven 2019-04-01 9:50 ` Geert Uytterhoeven 2 siblings, 1 reply; 20+ messages in thread From: Boris Brezillon @ 2019-01-30 8:55 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Marek Vasut, Richard Weinberger, Boris Brezillon, MTD Maling List, Brian Norris, David Woodhouse On Tue, 29 Jan 2019 16:29:55 +0100 Geert Uytterhoeven <geert@linux-m68k.org> wrote: > Hi Boris, > > On Tue, Jan 29, 2019 at 12:03 PM Boris Brezillon <bbrezillon@kernel.org> wrote: > > On Tue, 22 Jan 2019 13:31:59 +0100 > > Boris Brezillon <bbrezillon@kernel.org> wrote: > > > On Tue, 22 Jan 2019 12:21:11 +0100 > > > Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > > On Wed, Jan 2, 2019 at 3:37 PM Boris Brezillon <bbrezillon@kernel.org> wrote: > > > > > add_mtd_device() can fail. We should always check its return value > > > > > and gracefully handle the failure case. Fix the call sites where this > > > > > not done (in mtdpart.c) and add a __must_check attribute to the > > > > > prototype to avoid this kind of mistakes. > > > > > > > > > > Signed-off-by: Boris Brezillon <bbrezillon@kernel.org> > > > > > --- > > > > > No Fixes or Cc-stable tag here, as this seems to have worked just fine > > > > > without checking add_mtd_device() ret code until we started to expose > > > > > MTD devices as NVMEM providers (queued for 4.21). > > > > > > > > Oh yes ;-) > > > > > > > > https://lore.kernel.org/lkml/87feac02-e955-1897-d4a4-d6d6d1082e45@gmail.com/t/ > > > > > > > > Your patch is very similar to mine, so the crash is gone. > > > > > > Oops, sorry about that. I completely forgot about this patch. It seems > > > the discussion led to a different conclusion though (patch > > > allocate_partitions() to reject wrong parts early) and the v2 was never > > > sent (or I missed it). Anyway, I guess we should have done both (check > > > add_mtd_device() ret code everywhere and patch allocate_partitions() to > > > reject bad parts early). > > > > > > > However, the warning is still there: > > > > > > > > m25p80 spi0.0: s25sl032p (4096 Kbytes) > > > > 3 fixed-partitions partitions found on MTD device spi0.0 > > > > Creating 3 MTD partitions on "spi0.0": > > > > 0x000000000000-0x000000080000 : "loader" > > > > 0x000000080000-0x000000600000 : "user" > > > > mtd: partition "user" extends beyond the end of device "spi0.0" -- > > > > size truncated to 0x380000 > > > > 0x000000600000-0x000004000000 : "flash" > > > > mtd: partition "flash" is out of reach -- disabled > > > > ------------[ cut here ]------------ > > > > WARNING: CPU: 0 PID: 1 at drivers/mtd/mtdcore.c:571 > > > > add_mtd_device+0x90/0x3b0 > > > > > > > > Interestingly, only one partition is created, covering the full size of the > > > > device: > > > > > > > > # cat /proc/partitions > > > > major minor #blocks name > > > > > > > > 31 0 4096 mtdblock0 > > > > > > > > While I would expect two partitions, "loader" and truncated "user": > > > > > > > > 31 0 512 mtdblock0 > > > > 31 1 3584 mtdblock1 > > > > > > Yes, makes sense, I guess your patch was better than mine :-/. Can you > > > try with the following diff applied and let me know if it solves the > > > problem? > > > > Gentle ping: is this diff fixing your problem, and do you want me to > > send a proper patch for it or should I let you send one? > > Yes, the diff below fixes the partitions for me, so > Tested-by: Geert Uytterhoeven <geert+renesas@glider.be> > > Note that the warning is still there, but that's probably OK. Just out of curiosity, why do you need to define parts that do not fit in the flash? ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] mtd: Check add_mtd_device() ret code 2019-01-30 8:55 ` Boris Brezillon @ 2019-01-30 9:12 ` Geert Uytterhoeven 0 siblings, 0 replies; 20+ messages in thread From: Geert Uytterhoeven @ 2019-01-30 9:12 UTC (permalink / raw) To: Boris Brezillon Cc: Marek Vasut, Richard Weinberger, Boris Brezillon, MTD Maling List, Brian Norris, David Woodhouse Hi Boris, On Wed, Jan 30, 2019 at 9:55 AM Boris Brezillon <bbrezillon@kernel.org> wrote: > On Tue, 29 Jan 2019 16:29:55 +0100 > Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > On Tue, Jan 29, 2019 at 12:03 PM Boris Brezillon <bbrezillon@kernel.org> wrote: > > > On Tue, 22 Jan 2019 13:31:59 +0100 > > > Boris Brezillon <bbrezillon@kernel.org> wrote: > > > > On Tue, 22 Jan 2019 12:21:11 +0100 > > > > Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > > > On Wed, Jan 2, 2019 at 3:37 PM Boris Brezillon <bbrezillon@kernel.org> wrote: > > > > > > add_mtd_device() can fail. We should always check its return value > > > > > > and gracefully handle the failure case. Fix the call sites where this > > > > > > not done (in mtdpart.c) and add a __must_check attribute to the > > > > > > prototype to avoid this kind of mistakes. > > > > > > > > > > > > Signed-off-by: Boris Brezillon <bbrezillon@kernel.org> > > > > > > --- > > > > > > No Fixes or Cc-stable tag here, as this seems to have worked just fine > > > > > > without checking add_mtd_device() ret code until we started to expose > > > > > > MTD devices as NVMEM providers (queued for 4.21). > > > > > > > > > > Oh yes ;-) > > > > > > > > > > https://lore.kernel.org/lkml/87feac02-e955-1897-d4a4-d6d6d1082e45@gmail.com/t/ > > > > > > > > > > Your patch is very similar to mine, so the crash is gone. > > > > > > > > Oops, sorry about that. I completely forgot about this patch. It seems > > > > the discussion led to a different conclusion though (patch > > > > allocate_partitions() to reject wrong parts early) and the v2 was never > > > > sent (or I missed it). Anyway, I guess we should have done both (check > > > > add_mtd_device() ret code everywhere and patch allocate_partitions() to > > > > reject bad parts early). > > > > > > > > > However, the warning is still there: > > > > > > > > > > m25p80 spi0.0: s25sl032p (4096 Kbytes) > > > > > 3 fixed-partitions partitions found on MTD device spi0.0 > > > > > Creating 3 MTD partitions on "spi0.0": > > > > > 0x000000000000-0x000000080000 : "loader" > > > > > 0x000000080000-0x000000600000 : "user" > > > > > mtd: partition "user" extends beyond the end of device "spi0.0" -- > > > > > size truncated to 0x380000 > > > > > 0x000000600000-0x000004000000 : "flash" > > > > > mtd: partition "flash" is out of reach -- disabled > > > > > ------------[ cut here ]------------ > > > > > WARNING: CPU: 0 PID: 1 at drivers/mtd/mtdcore.c:571 > > > > > add_mtd_device+0x90/0x3b0 > > > > > > > > > > Interestingly, only one partition is created, covering the full size of the > > > > > device: > > > > > > > > > > # cat /proc/partitions > > > > > major minor #blocks name > > > > > > > > > > 31 0 4096 mtdblock0 > > > > > > > > > > While I would expect two partitions, "loader" and truncated "user": > > > > > > > > > > 31 0 512 mtdblock0 > > > > > 31 1 3584 mtdblock1 > > > > > > > > Yes, makes sense, I guess your patch was better than mine :-/. Can you > > > > try with the following diff applied and let me know if it solves the > > > > problem? > > > > > > Gentle ping: is this diff fixing your problem, and do you want me to > > > send a proper patch for it or should I let you send one? > > > > Yes, the diff below fixes the partitions for me, so > > Tested-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > > Note that the warning is still there, but that's probably OK. > > Just out of curiosity, why do you need to define parts that do not fit > in the flash? All of this started as an accident on my side ;-) The Koelsch board has 2 QSPI FLASHes: the first is 64 MiB large, the second is 4 MiB large. Which FLASH is used is selected by a switch, but the partitioning is specified in DT. I wanted to use the smaller FLASH for some test, but forgot to update the partitioning in DT, leading to: mtd: partition "user" extends beyond the end of device "spi0.0" -- size truncated to 0x380000 That kernel message indicated MTD tried to handle this gracefully, but still, the kernel crashed later. Anyway, it may be considered good practice to handle bad partition tables, like is done for disk partitioning, so I think it's worthwhile to have this fixed, which is what your patches achieve. There's still a WARN_ON() drawing the user's attention, though. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] mtd: Check add_mtd_device() ret code 2019-01-29 15:29 ` Geert Uytterhoeven 2019-01-30 8:52 ` Boris Brezillon 2019-01-30 8:55 ` Boris Brezillon @ 2019-04-01 9:50 ` Geert Uytterhoeven 2019-04-01 13:27 ` Boris Brezillon 2 siblings, 1 reply; 20+ messages in thread From: Geert Uytterhoeven @ 2019-04-01 9:50 UTC (permalink / raw) To: Boris Brezillon Cc: Marek Vasut, Richard Weinberger, Boris Brezillon, MTD Maling List, Brian Norris, David Woodhouse Hi Boris, On Tue, Jan 29, 2019 at 4:29 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Tue, Jan 29, 2019 at 12:03 PM Boris Brezillon <bbrezillon@kernel.org> wrote: > > On Tue, 22 Jan 2019 13:31:59 +0100 > > Boris Brezillon <bbrezillon@kernel.org> wrote: > > > On Tue, 22 Jan 2019 12:21:11 +0100 > > > Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > > On Wed, Jan 2, 2019 at 3:37 PM Boris Brezillon <bbrezillon@kernel.org> wrote: > > > > > add_mtd_device() can fail. We should always check its return value > > > > > and gracefully handle the failure case. Fix the call sites where this > > > > > not done (in mtdpart.c) and add a __must_check attribute to the > > > > > prototype to avoid this kind of mistakes. > > > > > > > > > > Signed-off-by: Boris Brezillon <bbrezillon@kernel.org> > > > > > --- > > > > > No Fixes or Cc-stable tag here, as this seems to have worked just fine > > > > > without checking add_mtd_device() ret code until we started to expose > > > > > MTD devices as NVMEM providers (queued for 4.21). > > > > > > > > Oh yes ;-) > > > > > > > > https://lore.kernel.org/lkml/87feac02-e955-1897-d4a4-d6d6d1082e45@gmail.com/t/ > > > > > > > > Your patch is very similar to mine, so the crash is gone. > > > > > > Oops, sorry about that. I completely forgot about this patch. It seems > > > the discussion led to a different conclusion though (patch > > > allocate_partitions() to reject wrong parts early) and the v2 was never > > > sent (or I missed it). Anyway, I guess we should have done both (check > > > add_mtd_device() ret code everywhere and patch allocate_partitions() to > > > reject bad parts early). > > > > > > > However, the warning is still there: > > > > > > > > m25p80 spi0.0: s25sl032p (4096 Kbytes) > > > > 3 fixed-partitions partitions found on MTD device spi0.0 > > > > Creating 3 MTD partitions on "spi0.0": > > > > 0x000000000000-0x000000080000 : "loader" > > > > 0x000000080000-0x000000600000 : "user" > > > > mtd: partition "user" extends beyond the end of device "spi0.0" -- > > > > size truncated to 0x380000 > > > > 0x000000600000-0x000004000000 : "flash" > > > > mtd: partition "flash" is out of reach -- disabled > > > > ------------[ cut here ]------------ > > > > WARNING: CPU: 0 PID: 1 at drivers/mtd/mtdcore.c:571 > > > > add_mtd_device+0x90/0x3b0 > > > > > > > > Interestingly, only one partition is created, covering the full size of the > > > > device: > > > > > > > > # cat /proc/partitions > > > > major minor #blocks name > > > > > > > > 31 0 4096 mtdblock0 > > > > > > > > While I would expect two partitions, "loader" and truncated "user": > > > > > > > > 31 0 512 mtdblock0 > > > > 31 1 3584 mtdblock1 > > > > > > Yes, makes sense, I guess your patch was better than mine :-/. Can you > > > try with the following diff applied and let me know if it solves the > > > problem? > > > > Gentle ping: is this diff fixing your problem, and do you want me to > > send a proper patch for it or should I let you send one? > > Yes, the diff below fixes the partitions for me, so > Tested-by: Geert Uytterhoeven <geert+renesas@glider.be> > > Note that the warning is still there, but that's probably OK. Given the warning was fixed by commit ad4635153034c20c ("mtd: Make sure mtd->erasesize is valid even if the partition is of size 0"), do you have any plans to apply the below? Thanks! > > > --- a/drivers/mtd/mtdpart.c > > > +++ b/drivers/mtd/mtdpart.c > > > @@ -724,16 +724,14 @@ int add_mtd_partitions(struct mtd_info *master, > > > { > > > struct mtd_part *slave; > > > uint64_t cur_offset = 0; > > > - int i, ret; > > > + int i, ret, actual_nbparts = 0; > > > > > > printk(KERN_NOTICE "Creating %d MTD partitions on \"%s\":\n", nbparts, master->name); > > > > > > for (i = 0; i < nbparts; i++) { > > > slave = allocate_partition(master, parts + i, i, cur_offset); > > > - if (IS_ERR(slave)) { > > > - ret = PTR_ERR(slave); > > > - goto err_del_partitions; > > > - } > > > + if (IS_ERR(slave)) > > > + continue; > > > > > > mutex_lock(&mtd_partitions_mutex); > > > list_add(&slave->list, &mtd_partitions); > > > @@ -746,7 +744,7 @@ int add_mtd_partitions(struct mtd_info *master, > > > mutex_unlock(&mtd_partitions_mutex); > > > > > > free_partition(slave); > > > - goto err_del_partitions; > > > + continue; > > > } > > > > > > mtd_add_partition_attrs(slave); > > > @@ -754,14 +752,10 @@ int add_mtd_partitions(struct mtd_info *master, > > > parse_mtd_partitions(&slave->mtd, parts[i].types, NULL); > > > > > > cur_offset = slave->offset + slave->mtd.size; > > > + actual_nbparts++; > > > } > > > > > > - return 0; > > > - > > > -err_del_partitions: > > > - del_mtd_partitions(master); > > > - > > > - return ret; > > > + return actual_nbparts; > > > } > > > > > > static DEFINE_SPINLOCK(part_parser_lock); > > > @@ -1003,10 +997,10 @@ int parse_mtd_partitions(struct mtd_info *master, const char *const *types, > > > } > > > /* Found partitions! */ > > > if (ret > 0) { > > > - err = add_mtd_partitions(master, pparts.parts, > > > + ret = add_mtd_partitions(master, pparts.parts, > > > pparts.nr_parts); > > > mtd_part_parser_cleanup(&pparts); > > > - return err ? err : pparts.nr_parts; > > > + return ret; > > > } > > > /* > > > * Stash the first error we see; only report it if no parser Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] mtd: Check add_mtd_device() ret code 2019-04-01 9:50 ` Geert Uytterhoeven @ 2019-04-01 13:27 ` Boris Brezillon 0 siblings, 0 replies; 20+ messages in thread From: Boris Brezillon @ 2019-04-01 13:27 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Marek Vasut, Richard Weinberger, Boris Brezillon, Boris Brezillon, MTD Maling List, Brian Norris, David Woodhouse On Mon, 1 Apr 2019 11:50:01 +0200 Geert Uytterhoeven <geert@linux-m68k.org> wrote: > Hi Boris, > > On Tue, Jan 29, 2019 at 4:29 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > On Tue, Jan 29, 2019 at 12:03 PM Boris Brezillon <bbrezillon@kernel.org> wrote: > > > On Tue, 22 Jan 2019 13:31:59 +0100 > > > Boris Brezillon <bbrezillon@kernel.org> wrote: > > > > On Tue, 22 Jan 2019 12:21:11 +0100 > > > > Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > > > On Wed, Jan 2, 2019 at 3:37 PM Boris Brezillon <bbrezillon@kernel.org> wrote: > > > > > > add_mtd_device() can fail. We should always check its return value > > > > > > and gracefully handle the failure case. Fix the call sites where this > > > > > > not done (in mtdpart.c) and add a __must_check attribute to the > > > > > > prototype to avoid this kind of mistakes. > > > > > > > > > > > > Signed-off-by: Boris Brezillon <bbrezillon@kernel.org> > > > > > > --- > > > > > > No Fixes or Cc-stable tag here, as this seems to have worked just fine > > > > > > without checking add_mtd_device() ret code until we started to expose > > > > > > MTD devices as NVMEM providers (queued for 4.21). > > > > > > > > > > Oh yes ;-) > > > > > > > > > > https://lore.kernel.org/lkml/87feac02-e955-1897-d4a4-d6d6d1082e45@gmail.com/t/ > > > > > > > > > > Your patch is very similar to mine, so the crash is gone. > > > > > > > > Oops, sorry about that. I completely forgot about this patch. It seems > > > > the discussion led to a different conclusion though (patch > > > > allocate_partitions() to reject wrong parts early) and the v2 was never > > > > sent (or I missed it). Anyway, I guess we should have done both (check > > > > add_mtd_device() ret code everywhere and patch allocate_partitions() to > > > > reject bad parts early). > > > > > > > > > However, the warning is still there: > > > > > > > > > > m25p80 spi0.0: s25sl032p (4096 Kbytes) > > > > > 3 fixed-partitions partitions found on MTD device spi0.0 > > > > > Creating 3 MTD partitions on "spi0.0": > > > > > 0x000000000000-0x000000080000 : "loader" > > > > > 0x000000080000-0x000000600000 : "user" > > > > > mtd: partition "user" extends beyond the end of device "spi0.0" -- > > > > > size truncated to 0x380000 > > > > > 0x000000600000-0x000004000000 : "flash" > > > > > mtd: partition "flash" is out of reach -- disabled > > > > > ------------[ cut here ]------------ > > > > > WARNING: CPU: 0 PID: 1 at drivers/mtd/mtdcore.c:571 > > > > > add_mtd_device+0x90/0x3b0 > > > > > > > > > > Interestingly, only one partition is created, covering the full size of the > > > > > device: > > > > > > > > > > # cat /proc/partitions > > > > > major minor #blocks name > > > > > > > > > > 31 0 4096 mtdblock0 > > > > > > > > > > While I would expect two partitions, "loader" and truncated "user": > > > > > > > > > > 31 0 512 mtdblock0 > > > > > 31 1 3584 mtdblock1 > > > > > > > > Yes, makes sense, I guess your patch was better than mine :-/. Can you > > > > try with the following diff applied and let me know if it solves the > > > > problem? > > > > > > Gentle ping: is this diff fixing your problem, and do you want me to > > > send a proper patch for it or should I let you send one? > > > > Yes, the diff below fixes the partitions for me, so > > Tested-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > > Note that the warning is still there, but that's probably OK. > > Given the warning was fixed by commit ad4635153034c20c ("mtd: Make sure > mtd->erasesize is valid even if the partition is of size 0"), do you have any > plans to apply the below? Is it still needed now that the only case we care about has been fixed? ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] mtd: Fix the check on nvmem_register() ret code 2019-01-02 14:36 [PATCH 1/2] mtd: Fix the check on nvmem_register() ret code Boris Brezillon 2019-01-02 14:36 ` [PATCH 2/2] mtd: Check add_mtd_device() " Boris Brezillon @ 2019-01-02 15:13 ` Bartosz Golaszewski 2019-01-08 8:30 ` [1/2] " Boris Brezillon 2 siblings, 0 replies; 20+ messages in thread From: Bartosz Golaszewski @ 2019-01-02 15:13 UTC (permalink / raw) To: Boris Brezillon Cc: David Woodhouse, Brian Norris, Boris Brezillon, Marek Vasut, Richard Weinberger, open list:MEMORY TECHNOLOGY..., Alban Bedel śr., 2 sty 2019 o 15:37 Boris Brezillon <bbrezillon@kernel.org> napisał(a): > > Commit 20167b70c894 ("nvmem: use EOPNOTSUPP instead of ENOSYS") changed > the nvmem_register() ret code from ENOSYS to EOPNOTSUPP when > CONFIG_NVMEM is not enabled, but the check in mtd_nvmem_add() was not > adjusted accordingly. > > Cc: Bartosz Golaszewski <brgl@bgdev.pl> > Cc: Alban Bedel <albeu@free.fr> > Fixes: c4dfa25ab307 ("mtd: add support for reading MTD devices via the nvmem API") > Reported-by: kernel test robot <rong.a.chen@intel.com> > Signed-off-by: Boris Brezillon <bbrezillon@kernel.org> > --- > drivers/mtd/mtdcore.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c > index 21e3cdc04036..999b705769a8 100644 > --- a/drivers/mtd/mtdcore.c > +++ b/drivers/mtd/mtdcore.c > @@ -522,7 +522,7 @@ static int mtd_nvmem_add(struct mtd_info *mtd) > mtd->nvmem = nvmem_register(&config); > if (IS_ERR(mtd->nvmem)) { > /* Just ignore if there is no NVMEM support in the kernel */ > - if (PTR_ERR(mtd->nvmem) == -ENOSYS) { > + if (PTR_ERR(mtd->nvmem) == -EOPNOTSUPP) { > mtd->nvmem = NULL; > } else { > dev_err(&mtd->dev, "Failed to register NVMEM device\n"); > -- > 2.17.1 > Reviewed-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [1/2] mtd: Fix the check on nvmem_register() ret code 2019-01-02 14:36 [PATCH 1/2] mtd: Fix the check on nvmem_register() ret code Boris Brezillon 2019-01-02 14:36 ` [PATCH 2/2] mtd: Check add_mtd_device() " Boris Brezillon 2019-01-02 15:13 ` [PATCH 1/2] mtd: Fix the check on nvmem_register() " Bartosz Golaszewski @ 2019-01-08 8:30 ` Boris Brezillon 2 siblings, 0 replies; 20+ messages in thread From: Boris Brezillon @ 2019-01-08 8:30 UTC (permalink / raw) To: Boris Brezillon, David Woodhouse, Brian Norris, Boris Brezillon, Marek Vasut, Richard Weinberger, linux-mtd Cc: Bartosz Golaszewski, Alban Bedel From: Boris Brezillon <boris.brezillon@bootlin.com> On Wed, 2019-01-02 at 14:36:53 UTC, Boris Brezillon wrote: > Commit 20167b70c894 ("nvmem: use EOPNOTSUPP instead of ENOSYS") changed > the nvmem_register() ret code from ENOSYS to EOPNOTSUPP when > CONFIG_NVMEM is not enabled, but the check in mtd_nvmem_add() was not > adjusted accordingly. > > Cc: Bartosz Golaszewski <brgl@bgdev.pl> > Cc: Alban Bedel <albeu@free.fr> > Fixes: c4dfa25ab307 ("mtd: add support for reading MTD devices via the nvmem API") > Reported-by: kernel test robot <rong.a.chen@intel.com> > Signed-off-by: Boris Brezillon <bbrezillon@kernel.org> > Reviewed-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> Applied to http://git.infradead.org/linux-mtd.git master, thanks. Boris ^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2019-04-01 13:28 UTC | newest] Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-01-02 14:36 [PATCH 1/2] mtd: Fix the check on nvmem_register() ret code Boris Brezillon 2019-01-02 14:36 ` [PATCH 2/2] mtd: Check add_mtd_device() " Boris Brezillon 2019-01-08 8:29 ` [2/2] " Boris Brezillon 2019-01-22 11:21 ` [PATCH 2/2] " Geert Uytterhoeven 2019-01-22 12:31 ` Boris Brezillon 2019-01-29 11:03 ` Boris Brezillon 2019-01-29 15:29 ` Geert Uytterhoeven 2019-01-30 8:52 ` Boris Brezillon 2019-01-30 9:05 ` Geert Uytterhoeven 2019-01-30 9:10 ` Boris Brezillon 2019-01-30 9:16 ` Geert Uytterhoeven 2019-02-01 8:50 ` Boris Brezillon 2019-02-01 9:02 ` Geert Uytterhoeven 2019-01-30 9:17 ` Boris Brezillon 2019-01-30 8:55 ` Boris Brezillon 2019-01-30 9:12 ` Geert Uytterhoeven 2019-04-01 9:50 ` Geert Uytterhoeven 2019-04-01 13:27 ` Boris Brezillon 2019-01-02 15:13 ` [PATCH 1/2] mtd: Fix the check on nvmem_register() " Bartosz Golaszewski 2019-01-08 8:30 ` [1/2] " Boris Brezillon
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).