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

[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.

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.

> 
> 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.

Best Regards,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com