From mboxrd@z Thu Jan 1 00:00:00 1970 From: Heiko Schocher Date: Thu, 21 Apr 2016 12:48:50 +0200 Subject: [U-Boot] [PATCH] mtd, ubi: set free_count to zero before walking through erase list In-Reply-To: <20160421122533.7367cb7c@bbrezillon> References: <1454410475-29929-1-git-send-email-hs@denx.de> <20160421105846.696b0eff@bbrezillon> <5718A6DE.6040109@denx.de> <20160421122533.7367cb7c@bbrezillon> Message-ID: <5718B012.8030006@denx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hello Boris, Am 21.04.2016 um 12:25 schrieb Boris Brezillon: > Hi Heiko, > > On Thu, 21 Apr 2016 12:09:34 +0200 > Heiko Schocher wrote: > >> Hello Boris, >> >> Am 21.04.2016 um 10:58 schrieb Boris Brezillon: >>> On Tue, 2 Feb 2016 11:54:35 +0100 >>> Heiko Schocher wrote: >>> >>>> Set free_count to zero before walking through ai->erase list >>>> in wl_init(). >>>> >>>> As U-Boot has no workqueue/threads, it immediately calls >>>> erase_worker(), which increase for each erased block >>>> free_count. Without this patch, free_count gets after >>>> this initialized to zero in wl_init(), so the free_count >>>> variable always has the maybe wrong value 0. >>>> >>>> Detected this behaviour on the dxr2 board, where the >>>> UBI fastmap gets not written when attaching/dettaching >>>> on an empty NAND. It drops instead the error message: >>>> >>>> could not find any anchor PEB >>>> >>>> With this patch, fastmap gets written on dettach. >>> >>> I ran into the same problem, and produced the exact same patch to >>> fix it, so >>> >>> Reviewed-by: Boris Brezillon >> >> Thanks! >> >> I did not yet found time, to investigate this problem deeper, >> sorry. >> >> The real reason to me seems, on an empty nand flash, we call >> scan_all() which calls scan_peb() which calls ubi_io_read_ec_hdr() >> which returns UBI_IO_FF as the nand is empty. >> >> This adds the PEB to the erase list, and here comes the difference >> between U-Boot and linux, we have no threads in U-Boot, so we call >> the erase_worker function immediately ... which increments the >> "ubi->free_count" variable ... after that it get set to >> "ubi->free_count = 0" ... which leads into the error we see ... >> >> No idea, if the correct fix not would be to move this erase_worker >> call after the attach phase ended, as Richard suggested, or if this >> fix is also valid ... > > I discussed that with Richard, and I think moving the ->free_count > assignment before iterating over the ->erase list is a good solution. Ah! Ok, than its fine for me too. > I know the Linux code is assuming that the UBI thread is not launched > yet when we call ubi_wl_init(), but to me it seems a bit risky to rely > on this assumption (what if we do the UBI thread creation a bit > earlier for some reason?). And, of course, as you explained, uboot does > not know anything about threads, so all UBI works are executed > synchronously, which makes this implementation buggy in uboot. Hmm... is it also a valid fix for linux then? >> Do you have some time to check such a fix as Richard suggested? > > Hm, IMO it complicates the whole implementation for no real benefit, > but I'll let Richard answer that one. Ok, thanks for your efforts. bye, Heiko -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany