[U-Boot] [PATCH] mtd, ubi: set free_count to zero before walking through erase list

[Boris Brezillon]

On Thu, 21 Apr 2016 12:48:50 +0200
Heiko Schocher <hs at denx.de> 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 <hs at denx.de> wrote:
> >
> >> Hello Boris,
> >>
> >> Am 21.04.2016 um 10:58 schrieb Boris Brezillon:
> >>> On Tue,  2 Feb 2016 11:54:35 +0100
> >>> Heiko Schocher <hs at denx.de> 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 <boris.brezillon at free-electrons.com>
> >>
> >> 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