[U-Boot] [PATCH 1/2] ubi: enable error reporting in initialization

[Andrew Ruder]

On Wed, Nov 05, 2014 at 07:57:27AM +0100, Heiko Schocher wrote:
> The problem is in generally enabling this feature in the size impact ...
> This should be discussed if we want this for all boards ...

This change actually only triggers a few changes that affect whether or
not ubi_init() returns error codes or not.  For the purpose of
discussion, I've added the only places where the CONFIG_MTD_UBI_MODULE
is checked (ubi_is_module()).

drivers/mtd/ubi/build.c:

1:       /* Attach MTD devices */
2:       for (i = 0; i < mtd_devs; i++) {
3:           [...]
4:           mtd = open_mtd_device(p->name);
5:           if (IS_ERR(mtd)) {
6:               err = PTR_ERR(mtd);
7:               ubi_err("cannot open mtd %s, error %d", p->name, err);
8:               /* See comment below re-ubi_is_module(). */
9:               if (ubi_is_module())
10:                  goto out_detach;
11:              continue;
12:          }
13:
14:          mutex_lock(&ubi_devices_mutex);
15:          err = ubi_attach_mtd_dev(mtd, p->ubi_num,
16:                       p->vid_hdr_offs, p->max_beb_per1024);
17:          mutex_unlock(&ubi_devices_mutex);
18:          if (err < 0) {
19:              ubi_err("cannot attach mtd%d", mtd->index);
20:              put_mtd_device(mtd);
21:
22:              /*
23:               * Originally UBI stopped initializing on any error.
24:               * However, later on it was found out that this
25:               * behavior is not very good when UBI is compiled into
26:               * the kernel and the MTD devices to attach are passed
27:               * through the command line. Indeed, UBI failure
28:               * stopped whole boot sequence.
29:               *
30:               * To fix this, we changed the behavior for the
31:               * non-module case, but preserved the old behavior for
32:               * the module case, just for compatibility. This is a
33:               * little inconsistent, though.
34:               */
35:              if (ubi_is_module())
36:                  goto out_detach;
37:          }
38:      }
39:
40:      err = ubiblock_init();
41:      if (err) {
42:          ubi_err("block: cannot initialize, error %d", err);
43:
44:          /* See comment above re-ubi_is_module(). */
45:          if (ubi_is_module())
46:              goto out_detach;
47:      }
48:
49:      return 0;
50:
51:  out_detach:
52:      [...]
53:      return err;


Note that errors at lines 5, 18, and 40 are ignored (and the function
returns 0) when not building UBI as a module.  This means that when an
error occurs in this function, U-Boot believes it has succeeded which
leads to the corruption and lock up issues.  No new error messages are
enabled via this change, simply making ubi_init return a proper return
code.

With and without this change (size check) on a ARM target:

% make -j5 > /dev/null 2>&1 && wc -c u-boot.bin && git revert --no-edit ca28e16192 && make -j5 > /dev/null 2>&1 && wc -c u-boot.bin
383436 u-boot.bin
[elecsys_falcon/next c9a88c1] Revert "ubi: enable error reporting in initialization"
 1 file changed, 2 deletions(-)
383356 u-boot.bin
%

Note that it actually makes the size go down by 80 bytes on my target!

> Ok, didn;t tried this with enabling "CONFIG_MTD_UBI_MODULE" ...
> But the name CONFIG_MTD_UBI_MODULE is not perfect, if we want
> just to enable error messages ...

Agree, didn't know what the general feelings were towards making changes
to the UBI layer specifically.  It would be nice if the function was
called ubi_allow_init_errors() instead of ubi_is_module() and checked
for either __UBOOT__ or the CONFIG_MTD_UBI_MODULE being defined.

Cheers,
Andy