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

[Heiko Schocher]

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?

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