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

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

[Heiko Schocher]

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.

Signed-off-by: Heiko Schocher <hs@denx.de>
---
added Richard to this EMail, as maybe this could be a problem
in linux too ... ?

 drivers/mtd/ubi/wl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
index 507b091..e823ca5 100644
--- a/drivers/mtd/ubi/wl.c
+++ b/drivers/mtd/ubi/wl.c
@@ -1528,6 +1528,7 @@ int ubi_wl_init(struct ubi_device *ubi, struct ubi_attach_info *ai)
 		INIT_LIST_HEAD(&ubi->pq[i]);
 	ubi->pq_head = 0;
 
+	ubi->free_count = 0;
 	list_for_each_entry_safe(aeb, tmp, &ai->erase, u.list) {
 		cond_resched();
 
@@ -1546,7 +1547,6 @@ int ubi_wl_init(struct ubi_device *ubi, struct ubi_attach_info *ai)
 		found_pebs++;
 	}
 
-	ubi->free_count = 0;
 	list_for_each_entry(aeb, &ai->free, u.list) {
 		cond_resched();
 
-- 
2.5.0

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

[Richard Weinberger]

Am 02.02.2016 um 11:54 schrieb Heiko Schocher:
> 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.
> 
> Signed-off-by: Heiko Schocher <hs@denx.de>
> ---
> added Richard to this EMail, as maybe this could be a problem
> in linux too ... ?

ubi_wl_init() is called by ubi_attach().
So, as soon your UBI image is attached the WL subsystem
is fully initialized and ready to use.
If you try to use it before you're in undefined state.
u-boot must not run the erase_worker() before the attach
phase is done.

Do you have more details what exactly happened?
This needs a deeper investigation.

Thanks,
//richard

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

[Heiko Schocher]

Hello Richard,

Am 02.02.2016 um 12:40 schrieb Richard Weinberger:
> Am 02.02.2016 um 11:54 schrieb Heiko Schocher:
>> 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.
>>
>> Signed-off-by: Heiko Schocher <hs@denx.de>
>> ---
>> added Richard to this EMail, as maybe this could be a problem
>> in linux too ... ?
>
> ubi_wl_init() is called by ubi_attach().

Yes, exactly, same for U-Boot.

> So, as soon your UBI image is attached the WL subsystem
> is fully initialized and ready to use.
> If you try to use it before you're in undefined state.
> u-boot must not run the erase_worker() before the attach
> phase is done.

ubi_attach() in drivers/mtd/ubi/attach.c calls
ubi_wl_init() in drivers/mtd/ubi/wl.c. There schedule_erase() is
called, which calls schedule_ubi_work(). same for linux
and U-Boot ...

Now the U-Boot special:
In U-Boot we have no threads, so in U-Boot this "work" is done
immediately...

Which means in this case, that erase_worker() is executed immediately,
so before ubi_attach() finished ... but couldn;t it be that
with schedule_work(), the erase_work() couldn;t be exeucted before
ubi_attach() is finished?

> Do you have more details what exactly happened?
> This needs a deeper investigation.

I came into this errorcase if the nand flash is empty. Hmm... why
is there erase work? I try to find out more.

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

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

[Boris Brezillon]

On Tue,  2 Feb 2016 11:54:35 +0100
Heiko Schocher <hs@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@free-electrons.com>

> 
> Signed-off-by: Heiko Schocher <hs@denx.de>
> ---
> added Richard to this EMail, as maybe this could be a problem
> in linux too ... ?
> 
>  drivers/mtd/ubi/wl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
> index 507b091..e823ca5 100644
> --- a/drivers/mtd/ubi/wl.c
> +++ b/drivers/mtd/ubi/wl.c
> @@ -1528,6 +1528,7 @@ int ubi_wl_init(struct ubi_device *ubi, struct ubi_attach_info *ai)
>  		INIT_LIST_HEAD(&ubi->pq[i]);
>  	ubi->pq_head = 0;
>  
> +	ubi->free_count = 0;
>  	list_for_each_entry_safe(aeb, tmp, &ai->erase, u.list) {
>  		cond_resched();
>  
> @@ -1546,7 +1547,6 @@ int ubi_wl_init(struct ubi_device *ubi, struct ubi_attach_info *ai)
>  		found_pebs++;
>  	}
>  
> -	ubi->free_count = 0;
>  	list_for_each_entry(aeb, &ai->free, u.list) {
>  		cond_resched();
>  



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

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

[Heiko Schocher]

Hello Boris,

Am 21.04.2016 um 10:58 schrieb Boris Brezillon:
> On Tue,  2 Feb 2016 11:54:35 +0100
> Heiko Schocher <hs@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@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 ...

Do you have some time to check such a fix as Richard suggested?

bye,
Heiko
>
>>
>> Signed-off-by: Heiko Schocher <hs@denx.de>
>> ---
>> added Richard to this EMail, as maybe this could be a problem
>> in linux too ... ?
>>
>>   drivers/mtd/ubi/wl.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
>> index 507b091..e823ca5 100644
>> --- a/drivers/mtd/ubi/wl.c
>> +++ b/drivers/mtd/ubi/wl.c
>> @@ -1528,6 +1528,7 @@ int ubi_wl_init(struct ubi_device *ubi, struct ubi_attach_info *ai)
>>   		INIT_LIST_HEAD(&ubi->pq[i]);
>>   	ubi->pq_head = 0;
>>
>> +	ubi->free_count = 0;
>>   	list_for_each_entry_safe(aeb, tmp, &ai->erase, u.list) {
>>   		cond_resched();
>>
>> @@ -1546,7 +1547,6 @@ int ubi_wl_init(struct ubi_device *ubi, struct ubi_attach_info *ai)
>>   		found_pebs++;
>>   	}
>>
>> -	ubi->free_count = 0;
>>   	list_for_each_entry(aeb, &ai->free, u.list) {
>>   		cond_resched();
>>
>
>
>

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

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

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

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

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

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

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.

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". ;-)

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.
Beside of code complexity it will also reduce u-boot's .text size.
Thomas' SPL patches are a very good start.
I'd also offer my help.

Thanks,
//richard

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

[Heiko Schocher]

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

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

[Richard Weinberger]

Heiko,

Am 22.04.2016 um 12:20 schrieb Heiko Schocher:
>> 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?

As I said, when you are in a code region where parallel
work must not happen as it will, for example, confuse your state
you block the worker.
But you are still allowed to schedule new work which will
executed after you unblock it.
For the fastmap example I gave it should be fine but I didn't check
all code paths in UBI for u-boot single thread safety. :-)

What I wanted to say is that executing work directly at schedule
time does not match 1:1 the POV of Linux UBI and is error prone.
These are issues you won't notice by compile testing.

An alternative approach would be not executing work
directly while scheduling it but in produce_free_peb().
UBI is designed to work with the worker being disabled.
All UBI work will then happen synchronous and should also work
in u-boot.

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

Depends.
IMHO a bootloader has exactly one job, loading a kernel
and booting it. And not being a poor man's general purpose operating
system where you can also do management stuff like managing UBI volumes. ;-)

Thanks,
//richard

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

[Heiko Schocher]

Hello Richard,

Am 22.04.2016 um 12:48 schrieb Richard Weinberger:
> Heiko,
>
> Am 22.04.2016 um 12:20 schrieb Heiko Schocher:
>>> 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?
>
> As I said, when you are in a code region where parallel
> work must not happen as it will, for example, confuse your state
> you block the worker.
> But you are still allowed to schedule new work which will
> executed after you unblock it.
> For the fastmap example I gave it should be fine but I didn't check
> all code paths in UBI for u-boot single thread safety. :-)
>
> What I wanted to say is that executing work directly at schedule
> time does not match 1:1 the POV of Linux UBI and is error prone.
> These are issues you won't notice by compile testing.

Ok, thanks for the explanation!

> An alternative approach would be not executing work
> directly while scheduling it but in produce_free_peb().
> UBI is designed to work with the worker being disabled.
> All UBI work will then happen synchronous and should also work
> in u-boot.

Sounds good!

>>> 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 ...
>
> Depends.
> IMHO a bootloader has exactly one job, loading a kernel
> and booting it. And not being a poor man's general purpose operating
> system where you can also do management stuff like managing UBI volumes. ;-)

Heh...

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

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

[Boris Brezillon]

On Fri, 22 Apr 2016 13:53:00 +0200
Heiko Schocher <hs@denx.de> wrote:

> 
> > An alternative approach would be not executing work
> > directly while scheduling it but in produce_free_peb().
> > UBI is designed to work with the worker being disabled.
> > All UBI work will then happen synchronous and should also work
> > in u-boot.
> 
> Sounds good!

Not so good actually. I tried that, and ended up with tasks stalled in
the work queue because the implementation was never "scheduling" the
do_work() loop.

Let's keep it simple, in uboot everything is synchronous, and you can't
be preempted by another task, so it's safe to assume that "scheduling a
work" == "executing it right away". IMHO, the kernel should also assume
that "scheduling a work" might involve "the work may have been done
before the ubi_schedule_work() function returns": when you schedule a
work to be done and wake up the thread responsible for dequeuing UBI
works, the scheduler can decide to schedule this thread right away,
which means this work can be done before the caller gets back to the
instruction just after ubi_schedule_work().

Of course, this has to be nuanced for the "attach procedure", because at
this time the UBI thread is not launched yet. But even in this
specific case, I think it's safer to assume that, maybe one day, the UBI
thread might be running when ubi_wl_init() is called, which is why I
suggested to also apply this patch to Linux.

> 
> >>> 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 ...
> >
> > Depends.
> > IMHO a bootloader has exactly one job, loading a kernel
> > and booting it. And not being a poor man's general purpose operating
> > system where you can also do management stuff like managing UBI volumes. ;-)
> 
> Heh...

That's another topic ;).

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

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

[Heiko Schocher]

Hello Boris,

Am 22.04.2016 um 14:21 schrieb Boris Brezillon:
> On Fri, 22 Apr 2016 13:53:00 +0200
> Heiko Schocher <hs@denx.de> wrote:
>
>>
>>> An alternative approach would be not executing work
>>> directly while scheduling it but in produce_free_peb().
>>> UBI is designed to work with the worker being disabled.
>>> All UBI work will then happen synchronous and should also work
>>> in u-boot.
>>
>> Sounds good!
>
> Not so good actually. I tried that, and ended up with tasks stalled in
> the work queue because the implementation was never "scheduling" the
> do_work() loop.
>
> Let's keep it simple, in uboot everything is synchronous, and you can't
> be preempted by another task, so it's safe to assume that "scheduling a
> work" == "executing it right away". IMHO, the kernel should also assume
> that "scheduling a work" might involve "the work may have been done
> before the ubi_schedule_work() function returns": when you schedule a
> work to be done and wake up the thread responsible for dequeuing UBI
> works, the scheduler can decide to schedule this thread right away,
> which means this work can be done before the caller gets back to the
> instruction just after ubi_schedule_work().
>
> Of course, this has to be nuanced for the "attach procedure", because at
> this time the UBI thread is not launched yet. But even in this
> specific case, I think it's safer to assume that, maybe one day, the UBI
> thread might be running when ubi_wl_init() is called, which is why I
> suggested to also apply this patch to Linux.

Ok, thanks for this explanation! I posted this patch also on linux-mtd, see:

https://patchwork.ozlabs.org/patch/613492/

>>>>> 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 ...
>>>
>>> Depends.
>>> IMHO a bootloader has exactly one job, loading a kernel
>>> and booting it. And not being a poor man's general purpose operating
>>> system where you can also do management stuff like managing UBI volumes. ;-)
>>
>> Heh...
>
> That's another topic ;).

Oh, yes...

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

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

[Richard Weinberger]

Am 25.04.2016 um 07:46 schrieb Heiko Schocher:
> Hello Boris,
> 
> Am 22.04.2016 um 14:21 schrieb Boris Brezillon:
>> On Fri, 22 Apr 2016 13:53:00 +0200
>> Heiko Schocher <hs@denx.de> wrote:
>>
>>>
>>>> An alternative approach would be not executing work
>>>> directly while scheduling it but in produce_free_peb().
>>>> UBI is designed to work with the worker being disabled.
>>>> All UBI work will then happen synchronous and should also work
>>>> in u-boot.
>>>
>>> Sounds good!
>>
>> Not so good actually. I tried that, and ended up with tasks stalled in
>> the work queue because the implementation was never "scheduling" the
>> do_work() loop.
>>
>> Let's keep it simple, in uboot everything is synchronous, and you can't
>> be preempted by another task, so it's safe to assume that "scheduling a
>> work" == "executing it right away". IMHO, the kernel should also assume
>> that "scheduling a work" might involve "the work may have been done
>> before the ubi_schedule_work() function returns": when you schedule a
>> work to be done and wake up the thread responsible for dequeuing UBI
>> works, the scheduler can decide to schedule this thread right away,
>> which means this work can be done before the caller gets back to the
>> instruction just after ubi_schedule_work().
>>
>> Of course, this has to be nuanced for the "attach procedure", because at
>> this time the UBI thread is not launched yet. But even in this
>> specific case, I think it's safer to assume that, maybe one day, the UBI
>> thread might be running when ubi_wl_init() is called, which is why I
>> suggested to also apply this patch to Linux.
> 
> Ok, thanks for this explanation! I posted this patch also on linux-mtd, see:
> 
> https://patchwork.ozlabs.org/patch/613492/

Let's hope that attaching is the only place. :-)
Heiko, it would be cool if you could double check.

Thanks,
//richard