| Message ID | 20190102143654.24362-2-bbrezillon@kernel.org |
|---|---|
| State | Accepted |
| Delegated to: | Boris Brezillon |
| Headers | show |
| Series | [1/2] mtd: Fix the check on nvmem_register() ret code | expand |
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
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
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
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
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
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.
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?
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
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?
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
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
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;
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/
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
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
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?
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);
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(-)