Re: [PATCH 2/2] mtd: Check add_mtd_device() ret code

[Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>]

Hi Boris,

On Wed, Jan 30, 2019 at 10:10 AM Boris Brezillon <bbrezillon@xxxxxxxxxx> wrote:
> On Wed, 30 Jan 2019 10:05:29 +0100
> Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
> > On Wed, Jan 30, 2019 at 9:52 AM Boris Brezillon <bbrezillon@xxxxxxxxxx> wrote:
> > > On Tue, 29 Jan 2019 16:29:55 +0100
> > > Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
> > > > On Tue, Jan 29, 2019 at 12:03 PM Boris Brezillon <bbrezillon@xxxxxxxxxx> wrote:
> > > > > On Tue, 22 Jan 2019 13:31:59 +0100
> > > > > Boris Brezillon <bbrezillon@xxxxxxxxxx> wrote:
> > > > > > On Tue, 22 Jan 2019 12:21:11 +0100
> > > > > > Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
> > > > > > > On Wed, Jan 2, 2019 at 3:37 PM Boris Brezillon <bbrezillon@xxxxxxxxxx> 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@xxxxxxxxxx>
> > > > > > > > ---
> > > > > > > > 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@xxxxxxxxx/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@xxxxxxxxx>
> > > >
> > > > 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@xxxxxxxxxxxxxx

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/