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

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

[Andrew Ruder]

The UBI layer will disable much of its error reporting when it is
compiled into the linux kernel to avoid stopping boot.  We want this
error reporting in U-Boot since we don't initialize the UBI layer until
it is used and want the error reporting.

We force this by telling the UBI layer we are building as a module.

Signed-off-by: Andrew Ruder <andrew.ruder@elecsyscorp.com>
Cc: Wolfgang Denk <wd@denx.de>
Cc: Heiko Schocher <hs@denx.de>
Cc: Kyungmin Park <kmpark@infradead.org>
---
 include/ubi_uboot.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/ubi_uboot.h b/include/ubi_uboot.h
index 1fd15f4..6ff0e23 100644
--- a/include/ubi_uboot.h
+++ b/include/ubi_uboot.h
@@ -51,6 +51,8 @@
 
 #undef CONFIG_MTD_UBI_BLOCK
 
+#define CONFIG_MTD_UBI_MODULE
+
 #if !defined(CONFIG_MTD_UBI_BEB_LIMIT)
 #define CONFIG_MTD_UBI_BEB_LIMIT	20
 #endif
-- 
2.1.1

[U-Boot] [PATCH 2/2] mtd: nor: initialize writebufsize field

[Andrew Ruder]

UBI drivers error out if writebufsize is not filled in correctly.  Grab
this information from the CFI flash_info struct.

Signed-off-by: Andrew Ruder <andrew.ruder@elecsyscorp.com>
Cc: Wolfgang Denk <wd@denx.de>
Cc: Heiko Schocher <hs@denx.de>
Cc: Stefan Roese <sr@denx.de>
---
 drivers/mtd/cfi_mtd.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/mtd/cfi_mtd.c b/drivers/mtd/cfi_mtd.c
index ac805ff..d4c9609 100644
--- a/drivers/mtd/cfi_mtd.c
+++ b/drivers/mtd/cfi_mtd.c
@@ -226,6 +226,7 @@ int cfi_mtd_init(void)
 		mtd->flags		= MTD_CAP_NORFLASH;
 		mtd->size		= fi->size;
 		mtd->writesize		= 1;
+		mtd->writebufsize	= fi->buffer_size;
 
 		mtd->_erase		= cfi_mtd_erase;
 		mtd->_read		= cfi_mtd_read;
-- 
2.1.1

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

[Heiko Schocher]

Hello Andrew,

Am 04.11.2014 17:07, schrieb Andrew Ruder:
> The UBI layer will disable much of its error reporting when it is
> compiled into the linux kernel to avoid stopping boot.  We want this
> error reporting in U-Boot since we don't initialize the UBI layer until
> it is used and want the error reporting.
>
> We force this by telling the UBI layer we are building as a module.
>
> Signed-off-by: Andrew Ruder<andrew.ruder@elecsyscorp.com>
> Cc: Wolfgang Denk<wd@denx.de>
> Cc: Heiko Schocher<hs@denx.de>
> Cc: Kyungmin Park<kmpark@infradead.org>
> ---
>   include/ubi_uboot.h | 2 ++
>   1 file changed, 2 insertions(+)

I posted such a question [1] on the ML ...

The problem is in generally enabling this feature in the size impact ...
This should be discussed if we want this for all boards ...

> diff --git a/include/ubi_uboot.h b/include/ubi_uboot.h
> index 1fd15f4..6ff0e23 100644
> --- a/include/ubi_uboot.h
> +++ b/include/ubi_uboot.h
> @@ -51,6 +51,8 @@
>
>   #undef CONFIG_MTD_UBI_BLOCK
>
> +#define CONFIG_MTD_UBI_MODULE
> +
>   #if !defined(CONFIG_MTD_UBI_BEB_LIMIT)
>   #define CONFIG_MTD_UBI_BEB_LIMIT	20
>   #endif

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

bye,
Heiko

[1] http://patchwork.ozlabs.org/patch/405259/
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[U-Boot] [PATCH 2/2] mtd: nor: initialize writebufsize field

[Heiko Schocher]

Hello Andrew,

Am 04.11.2014 17:07, schrieb Andrew Ruder:
> UBI drivers error out if writebufsize is not filled in correctly.  Grab
> this information from the CFI flash_info struct.
>
> Signed-off-by: Andrew Ruder<andrew.ruder@elecsyscorp.com>
> Cc: Wolfgang Denk<wd@denx.de>
> Cc: Heiko Schocher<hs@denx.de>
> Cc: Stefan Roese<sr@denx.de>
> ---
>   drivers/mtd/cfi_mtd.c | 1 +
>   1 file changed, 1 insertion(+)

Patch already posted on the ML, see [1]

If this Patch works for you, could you send an "Acked-by" for it?

>
> diff --git a/drivers/mtd/cfi_mtd.c b/drivers/mtd/cfi_mtd.c
> index ac805ff..d4c9609 100644
> --- a/drivers/mtd/cfi_mtd.c
> +++ b/drivers/mtd/cfi_mtd.c
> @@ -226,6 +226,7 @@ int cfi_mtd_init(void)
>   		mtd->flags		= MTD_CAP_NORFLASH;
>   		mtd->size		= fi->size;
>   		mtd->writesize		= 1;
> +		mtd->writebufsize	= fi->buffer_size;
                                           ^ should be cfi

This is a typo ... your patch would not compile ... please just
ack the patch [1], thanks!

bye,
Heiko

[1] Patchwork [U-Boot,v2] mtd, cfi, ubi: add missing writebufsize initialization
http://patchwork.ozlabs.org/patch/405260/
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[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

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

[Heiko Schocher]

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

[U-Boot] [PATCH 2/2] mtd: nor: initialize writebufsize field

[Andrew Ruder]

On Wed, Nov 05, 2014 at 08:00:21AM +0100, Heiko Schocher wrote:
> >diff --git a/drivers/mtd/cfi_mtd.c b/drivers/mtd/cfi_mtd.c
> >index ac805ff..d4c9609 100644
> >--- a/drivers/mtd/cfi_mtd.c
> >+++ b/drivers/mtd/cfi_mtd.c
> >@@ -226,6 +226,7 @@ int cfi_mtd_init(void)
> >  		mtd->flags		= MTD_CAP_NORFLASH;
> >  		mtd->size		= fi->size;
> >  		mtd->writesize		= 1;
> >+		mtd->writebufsize	= fi->buffer_size;
>                                           ^ should be cfi
> 
> This is a typo ... your patch would not compile ... please just
> ack the patch [1], thanks!

I responded on the other thread with your other patch.  I apologize for
not seeing it originally.  This patch, imo, is correct though and
definitely compiles - not a typo!

- Andy

[U-Boot] [PATCH 2/2] mtd: nor: initialize writebufsize field

[Heiko Schocher]

Hello Andrew,

Am 05.11.2014 20:23, schrieb Andrew Ruder:
> On Wed, Nov 05, 2014 at 08:00:21AM +0100, Heiko Schocher wrote:
>>> diff --git a/drivers/mtd/cfi_mtd.c b/drivers/mtd/cfi_mtd.c
>>> index ac805ff..d4c9609 100644
>>> --- a/drivers/mtd/cfi_mtd.c
>>> +++ b/drivers/mtd/cfi_mtd.c
>>> @@ -226,6 +226,7 @@ int cfi_mtd_init(void)
>>>   		mtd->flags		= MTD_CAP_NORFLASH;
>>>   		mtd->size		= fi->size;
>>>   		mtd->writesize		= 1;
>>> +		mtd->writebufsize	= fi->buffer_size;
>>                                            ^ should be cfi
>>
>> This is a typo ... your patch would not compile ... please just
>> ack the patch [1], thanks!
>
> I responded on the other thread with your other patch.  I apologize for
> not seeing it originally.  This patch, imo, is correct though and

No problem!

> definitely compiles - not a typo!

Yes, sorry for that.

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