From mboxrd@z Thu Jan 1 00:00:00 1970 From: Heiko Schocher Date: Fri, 22 Apr 2016 12:20:22 +0200 Subject: [U-Boot] [PATCH] mtd, ubi: set free_count to zero before walking through erase list In-Reply-To: <5719F01A.7090400@nod.at> References: <1454410475-29929-1-git-send-email-hs@denx.de> <20160421105846.696b0eff@bbrezillon> <5718A6DE.6040109@denx.de> <20160421122533.7367cb7c@bbrezillon> <5718B012.8030006@denx.de> <20160421141452.4d9257d3@bbrezillon> <5719F01A.7090400@nod.at> Message-ID: <5719FAE6.2010603@denx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.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