From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Brezillon Date: Thu, 21 Apr 2016 14:14:52 +0200 Subject: [U-Boot] [PATCH] mtd, ubi: set free_count to zero before walking through erase list In-Reply-To: <5718B012.8030006@denx.de> References: <1454410475-29929-1-git-send-email-hs@denx.de> <20160421105846.696b0eff@bbrezillon> <5718A6DE.6040109@denx.de> <20160421122533.7367cb7c@bbrezillon> <5718B012.8030006@denx.de> Message-ID: <20160421141452.4d9257d3@bbrezillon> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On Thu, 21 Apr 2016 12:48:50 +0200 Heiko Schocher wrote: > 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? Well, it's not required, but it's making the code more future proof IMO. So again, I'll let Richard decide on this aspect. -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com