| Message ID | 1415117278-1255-1-git-send-email-andrew.ruder@elecsyscorp.com |
|---|---|
| State | Superseded |
| Delegated to: | Heiko Schocher |
| Headers | show |
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/
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
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
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
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(+)