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

[Heiko Schocher <hs@denx.de>]

Hello Richard,

Am 22.04.2016 um 11:34 schrieb Richard Weinberger:
> Am 21.04.2016 um 14:14 schrieb Boris Brezillon:
>>>>> 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.
>
> As discussed with Boris, I'm not a huge fan of the said patch but I
> understand the need for it.
> Please send it to linux-mtd I'll apply it.

Ok, done.

> That said, the root cause of the whole issue is that due to the
> single thread nature of u-boot UBI work is directly executed
> at schedule time. For u-boot this works more or less.
> But UBI allows work being scheduled when the background thread is
> disabled/paused or not spawned.
> The free_count patch papers exactly over one of these cases.
> Let's hope that there are not other (more nasty) cases where
> u-boot and Linux UBI behave differently.

:-(

> Think of places where work is scheduled but the caller blocked
> the worker because the work has to be done later.
> Fastmap is one of these use cases but AFAIK it won't matter
> as no CPU scheduler is involved and will interrupt Fastmap.

Can you explain this a little bit?

> Boris and I worked the last months on a bigger UBI project
> where we also had to port our UBI changes to u-boot.
> Now I'm not so sure anymore whether it is a good idea to
> copy&paste UBI from Linux to u-boot. We faced a lot of
> issues due to the single thread model. I changed the work
> model in UBI to make locking less complicated. It turned out
> that on u-boot it made things more complicated.
> At least Boris had a lot of "fun". ;-)

Heh...

> In the long run I suggest removing the whole Linux UBI implementation
> from u-boot and add a small (read only!) implementation which can
> also read UBIFS. Reading UBIFS is not a big deal. Also journal reply
> can be done in-memory.

Hmm.. I think read only is not for all boards an option, as we also
create UBI Volumes and/or write to them in U-Boot ...

> Beside of code complexity it will also reduce u-boot's .text size.
> Thomas' SPL patches are a very good start.

Yes, I hope to get soon an Ack/Response from Ladislav and/or Enric,
so we can integrate them into mainline.

> I'd also offer my help.

Thanks!

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany