From mboxrd@z Thu Jan  1 00:00:00 1970
From: Heiko Schocher <hs@denx.de>
Date: Wed, 05 Nov 2014 16:12:40 +0100
Subject: [U-Boot] [PATCH 1/2] ubi: enable error reporting in
	initialization
In-Reply-To: <20141105142702.GA12864@og3k>
References: <1415117278-1255-1-git-send-email-andrew.ruder@elecsyscorp.com>
	<5459CA57.9090009@denx.de> <20141105142702.GA12864@og3k>
Message-ID: <545A3E68.30703@denx.de>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

Hello Andrew,

Am 05.11.2014 15:27, schrieb 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;>
> Cheers,
> Andy
>

> 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.

Ah, now I got you! Good catch!

> 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!

Great.

>> 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.

No, I just not realized that you fix a bug ... sorry.

I think your change is good, hope I can test it soon. May I ask you
to add a comment in "include/ubi_uboot.h" why we want to define
CONFIG_MTD_UBI_MODULE ? That would help a lot!

Thanks!

bye,
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany