| Message ID | 1354564667-9549-3-git-send-email-richard@nod.at |
|---|---|
| State | Accepted |
| Commit | 894aef215775b56b725e9dde856b7a8b091ddfcc |
| Headers | show |
On Mon, 2012-12-03 at 20:57 +0100, Richard Weinberger wrote: > + err = ubi_self_check_all_ff(ubi, peb, ubi->vid_hdr_aloffset, > + ubi->peb_size - ubi->vid_hdr_aloffset); > + if (err) { > + ubi_err("new PEB %d does not contain all 0xFF bytes", peb); > + return err; > + } > + The original idea was to verify that all PEBs from the free list (tree actually) are really filled with 0xFFs. What I see is that with fastmap this is not done. I think the code should be re-thought and re-structured to make the verification happen with fastmap enabled as well.
On Mon, 2012-12-03 at 20:57 +0100, Richard Weinberger wrote: > As ubi_self_check_all_ff() might sleep we are not allowed > to call it from atomic context. > For now we call it only from ubi_wl_get_peb(). > There are some code paths where it would also make sense, > but these paths are currently atomic and only enabled > when fastmap is used. > > Signed-off-by: Richard Weinberger <richard@nod.at> Can you confirm that you enabled the 3 debugging options I sent and tested both FM and non-FM configurations with those options enabled?
On Tue, 2012-12-04 at 09:56 +0200, Artem Bityutskiy wrote: > On Mon, 2012-12-03 at 20:57 +0100, Richard Weinberger wrote: > > As ubi_self_check_all_ff() might sleep we are not allowed > > to call it from atomic context. > > For now we call it only from ubi_wl_get_peb(). > > There are some code paths where it would also make sense, > > but these paths are currently atomic and only enabled > > when fastmap is used. > > > > Signed-off-by: Richard Weinberger <richard@nod.at> > > Can you confirm that you enabled the 3 debugging options I sent and > tested both FM and non-FM configurations with those options enabled? Let me elaborate why I am asking. 1. If you did the tests and with this fix the debugging stuff works fine, then it is OK to take this patch now and let you improve the FM debugging coverage a bit later. 2. If you did not verify, then the assumption is _may_ be broken anyway, then I would prefer to not take this patch, because I consider it to be a band aid.
Am Tue, 04 Dec 2012 10:15:01 +0200 schrieb Artem Bityutskiy <dedekind1@gmail.com>: > On Tue, 2012-12-04 at 09:56 +0200, Artem Bityutskiy wrote: > > On Mon, 2012-12-03 at 20:57 +0100, Richard Weinberger wrote: > > > As ubi_self_check_all_ff() might sleep we are not allowed > > > to call it from atomic context. > > > For now we call it only from ubi_wl_get_peb(). > > > There are some code paths where it would also make sense, > > > but these paths are currently atomic and only enabled > > > when fastmap is used. > > > > > > Signed-off-by: Richard Weinberger <richard@nod.at> > > > > Can you confirm that you enabled the 3 debugging options I sent and > > tested both FM and non-FM configurations with those options enabled? > > Let me elaborate why I am asking. > > 1. If you did the tests and with this fix the debugging stuff works > fine, then it is OK to take this patch now and let you improve the FM > debugging coverage a bit later. Of course I did the tests. Namely chk_gen, chk_io, tst_emulate_bit_flips and tst_disable_bgt. (Also in various combinations) Please note, there is one issue regarding tst_disable_bgt and fastmap. Fastmap needs the UBI background thread. Therefore ubi-tests fail if fastmap is enabled and tst_disable_bgt=1. For 3.8 I can try to rework my code such that the test passes... But IMHO the tst_disable_bgt test is useless. I'll rework the fastmap code paths such that ubi_self_check_all_ff() can be used everywhere. This is planned for 3.8. As we are at 3.7-rc8 now I get very nervous if I have to touch much UBI/fastmap code. Again, with CONFIG_MTD_UBI_FASTMAP=n I'm no longer able to reproduce any UBI regression. If there is still a regression, please report it to me I'll fix it immediately. Thanks, //richard
diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c index 1f9f5f7..2144f61 100644 --- a/drivers/mtd/ubi/wl.c +++ b/drivers/mtd/ubi/wl.c @@ -498,7 +498,7 @@ out: * @ubi: UBI device description object * * This function returns a physical eraseblock in case of success and a - * negative error code in case of failure. Might sleep. + * negative error code in case of failure. */ static int __wl_get_peb(struct ubi_device *ubi) { @@ -540,13 +540,6 @@ retry: * ubi_wl_get_peb() after removing e from the pool. */ prot_queue_add(ubi, e); #endif - err = ubi_self_check_all_ff(ubi, e->pnum, ubi->vid_hdr_aloffset, - ubi->peb_size - ubi->vid_hdr_aloffset); - if (err) { - ubi_err("new PEB %d does not contain all 0xFF bytes", e->pnum); - return err; - } - return e->pnum; } @@ -690,12 +683,19 @@ static struct ubi_wl_entry *get_peb_for_wl(struct ubi_device *ubi) int ubi_wl_get_peb(struct ubi_device *ubi) { - int peb; + int peb, err; spin_lock(&ubi->wl_lock); peb = __wl_get_peb(ubi); spin_unlock(&ubi->wl_lock); + err = ubi_self_check_all_ff(ubi, peb, ubi->vid_hdr_aloffset, + ubi->peb_size - ubi->vid_hdr_aloffset); + if (err) { + ubi_err("new PEB %d does not contain all 0xFF bytes", peb); + return err; + } + return peb; } #endif
As ubi_self_check_all_ff() might sleep we are not allowed to call it from atomic context. For now we call it only from ubi_wl_get_peb(). There are some code paths where it would also make sense, but these paths are currently atomic and only enabled when fastmap is used. Signed-off-by: Richard Weinberger <richard@nod.at> --- drivers/mtd/ubi/wl.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)