[U-Boot] UBI: regression since "mtd: ubi: Fix worker handling"

[U-Boot] UBI: regression since "mtd: ubi: Fix worker handling"

[Patrice CHOTARD]

Hi Richard, Heiko

Since f82290afc847 ("mtd: ubi: Fix worker handling"),
when booting from NAND, on a fresh NAND just after being flashed (and 
only in this case), we got the following log:

ubi0: default fastmap pool size: 200
ubi0: default fastmap WL pool size: 100
ubi0: attaching mtd2
ubi0: scanning is finished
ubi0 error: ubi_update_fastmap: could not find any anchor PEB
ubi0 error: ubi_update_fastmap: could not find any anchor PEB
ubi0 error: ubi_wl_get_peb: Unable to get a free PEB from user WL pool
ubi0 error: autoresize: cannot auto-resize volume 1
UBI error: cannot attach mtd2UBI error: cannot initialize UBI, error 
-28UBI init error 28


After analysis, in ubi_wl_init(), when performing schedule_erase(),
thread_enabled flag is not yet set to 1, which forbids ubi_do_worker()
to execute pending works.

This has to effect to not populate ubi->free with free physical
eraseblocks.

As temporary workaround i did the following update on my side which fix 
this issue, but i don't consider it neither as a clean patch nor certain 
that it is enough :


@@ -1560,15 +1560,17 @@ int ubi_wl_init(struct ubi_device *ubi, struct 
ubi_attach_info *ai)
  			goto out_free;

  		e->pnum = aeb->pnum;
  		e->ec = aeb->ec;
  		ubi->lookuptbl[e->pnum] = e;
+		ubi->thread_enabled = 1;
  		if (schedule_erase(ubi, e, aeb->vol_id, aeb->lnum, 0)) {
+			ubi->thread_enabled = 0;
  			wl_entry_destroy(ubi, e);
  			goto out_free;
  		}
+		ubi->thread_enabled = 0;
  		found_pebs++;
  	}

  	list_for_each_entry(aeb, &ai->free, u.list) {


Regards

Patrice

[U-Boot] UBI: regression since "mtd: ubi: Fix worker handling"

[Richard Weinberger]

Patrice,

Am Montag, 21. Mai 2018, 15:38:57 CEST schrieb Patrice CHOTARD:
> Hi Richard, Heiko
> 
> Since f82290afc847 ("mtd: ubi: Fix worker handling"),
> when booting from NAND, on a fresh NAND just after being flashed (and 
> only in this case), we got the following log:
> 
> ubi0: default fastmap pool size: 200
> ubi0: default fastmap WL pool size: 100
> ubi0: attaching mtd2
> ubi0: scanning is finished
> ubi0 error: ubi_update_fastmap: could not find any anchor PEB
> ubi0 error: ubi_update_fastmap: could not find any anchor PEB
> ubi0 error: ubi_wl_get_peb: Unable to get a free PEB from user WL pool
> ubi0 error: autoresize: cannot auto-resize volume 1
> UBI error: cannot attach mtd2UBI error: cannot initialize UBI, error 
> -28UBI init error 28
> 
> 
> After analysis, in ubi_wl_init(), when performing schedule_erase(),
> thread_enabled flag is not yet set to 1, which forbids ubi_do_worker()
> to execute pending works.
> 
> This has to effect to not populate ubi->free with free physical
> eraseblocks.
> 
> As temporary workaround i did the following update on my side which fix 
> this issue, but i don't consider it neither as a clean patch nor certain 
> that it is enough :
> 
> 
> @@ -1560,15 +1560,17 @@ int ubi_wl_init(struct ubi_device *ubi, struct 
> ubi_attach_info *ai)
>   			goto out_free;
> 
>   		e->pnum = aeb->pnum;
>   		e->ec = aeb->ec;
>   		ubi->lookuptbl[e->pnum] = e;
> +		ubi->thread_enabled = 1;

This is not correct. At this point the UBI thread is not ready.
I know, I know, U-Boot has no threads but some data structures might
not be ready.

Let me think how to work around this properly.

Thanks,
//richard

[U-Boot] UBI: regression since "mtd: ubi: Fix worker handling"

[Richard Weinberger]

Patrice,

Am Montag, 21. Mai 2018, 16:07:41 CEST schrieb Richard Weinberger:
> >   		e->pnum = aeb->pnum;
> >   		e->ec = aeb->ec;
> >   		ubi->lookuptbl[e->pnum] = e;
> > +		ubi->thread_enabled = 1;
> 
> This is not correct. At this point the UBI thread is not ready.
> I know, I know, U-Boot has no threads but some data structures might
> not be ready.
> 
> Let me think how to work around this properly.

The root cause seems to be that U-Boot misses this fix from Linux:

commit 1cb8f9776c7dcadc57885c6653943511d282633b
Author: Richard Weinberger <richard@nod.at>
Date:   Tue Aug 11 23:27:44 2015 +0200

    ubi: fastmap: Implement produce_free_peb()
    
    If fastmap requests a free PEB for a pool and UBI is busy
    with erasing PEBs we need to offer a function to wait for one.
    We can reuse produce_free_peb() from the non-fastmap WL code
    but with different locking semantics.
    
    Cc: stable at vger.kernel.org # 4.1.x-
    Reported-and-tested-by: Jörg Krause <joerg.krause@embedded.rocks>
    Signed-off-by: Richard Weinberger <richard@nod.at

Heiko, I'm currently working on Fastmap in Linux[0].
I suggest to re-syncing with Linux if my changes go upstream, I can ping you them. :-)
Fastmap gained many improvements since the last sync.

Thanks,
//richard

[0] http://lists.infradead.org/pipermail/linux-mtd/2018-May/080965.html

[U-Boot] UBI: regression since "mtd: ubi: Fix worker handling"

[Heiko Schocher]

Hello Richard,

Am 21.05.2018 um 21:31 schrieb Richard Weinberger:
> Patrice,
> 
> Am Montag, 21. Mai 2018, 16:07:41 CEST schrieb Richard Weinberger:
>>>    		e->pnum = aeb->pnum;
>>>    		e->ec = aeb->ec;
>>>    		ubi->lookuptbl[e->pnum] = e;
>>> +		ubi->thread_enabled = 1;
>>
>> This is not correct. At this point the UBI thread is not ready.
>> I know, I know, U-Boot has no threads but some data structures might
>> not be ready.
>>
>> Let me think how to work around this properly.
> 
> The root cause seems to be that U-Boot misses this fix from Linux:
> 
> commit 1cb8f9776c7dcadc57885c6653943511d282633b
> Author: Richard Weinberger <richard@nod.at>
> Date:   Tue Aug 11 23:27:44 2015 +0200
> 
>      ubi: fastmap: Implement produce_free_peb()
>      
>      If fastmap requests a free PEB for a pool and UBI is busy
>      with erasing PEBs we need to offer a function to wait for one.
>      We can reuse produce_free_peb() from the non-fastmap WL code
>      but with different locking semantics.
>      
>      Cc: stable at vger.kernel.org # 4.1.x-
>      Reported-and-tested-by: Jörg Krause <joerg.krause@embedded.rocks>
>      Signed-off-by: Richard Weinberger <richard@nod.at
> 
> Heiko, I'm currently working on Fastmap in Linux[0].
> I suggest to re-syncing with Linux if my changes go upstream, I can ping you them. :-)
> Fastmap gained many improvements since the last sync.

Yes, but I am currently under load, and doing a rebase is a bigger
task I fear (I speculate currently to automate this task with tbot,
let me think a little bit about this, if this would work, U-Boot UBI
would autaomtically follow linux tree, and I immediately would see
merge errors ...)

Also before a rebase can go mainline, we need deep testing. So no
fast chance here for me ... sorry. If we can find commits, which
fix this regression, it may makes sense to apply them before such a
rebase ... ?

bye,
Heiko
> 
> Thanks,
> //richard
> 
> [0] http://lists.infradead.org/pipermail/linux-mtd/2018-May/080965.html
> 

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de

[U-Boot] UBI: regression since "mtd: ubi: Fix worker handling"

[Richard Weinberger]

Am Dienstag, 22. Mai 2018, 08:30:45 CEST schrieb Heiko Schocher:
> Hello Richard,
> 
> Am 21.05.2018 um 21:31 schrieb Richard Weinberger:
> > Patrice,
> > 
> > Am Montag, 21. Mai 2018, 16:07:41 CEST schrieb Richard Weinberger:
> >>>    		e->pnum = aeb->pnum;
> >>>    		e->ec = aeb->ec;
> >>>    		ubi->lookuptbl[e->pnum] = e;
> >>> +		ubi->thread_enabled = 1;
> >>
> >> This is not correct. At this point the UBI thread is not ready.
> >> I know, I know, U-Boot has no threads but some data structures might
> >> not be ready.
> >>
> >> Let me think how to work around this properly.
> > 
> > The root cause seems to be that U-Boot misses this fix from Linux:
> > 
> > commit 1cb8f9776c7dcadc57885c6653943511d282633b
> > Author: Richard Weinberger <richard@nod.at>
> > Date:   Tue Aug 11 23:27:44 2015 +0200
> > 
> >      ubi: fastmap: Implement produce_free_peb()
> >      
> >      If fastmap requests a free PEB for a pool and UBI is busy
> >      with erasing PEBs we need to offer a function to wait for one.
> >      We can reuse produce_free_peb() from the non-fastmap WL code
> >      but with different locking semantics.
> >      
> >      Cc: stable at vger.kernel.org # 4.1.x-
> >      Reported-and-tested-by: Jörg Krause <joerg.krause@embedded.rocks>
> >      Signed-off-by: Richard Weinberger <richard@nod.at
> > 
> > Heiko, I'm currently working on Fastmap in Linux[0].
> > I suggest to re-syncing with Linux if my changes go upstream, I can ping you them. :-)
> > Fastmap gained many improvements since the last sync.
> 
> Yes, but I am currently under load, and doing a rebase is a bigger
> task I fear (I speculate currently to automate this task with tbot,
> let me think a little bit about this, if this would work, U-Boot UBI
> would autaomtically follow linux tree, and I immediately would see
> merge errors ...)

Ok. :-)

> Also before a rebase can go mainline, we need deep testing. So no
> fast chance here for me ... sorry. If we can find commits, which
> fix this regression, it may makes sense to apply them before such a
> rebase ... ?

As I said, commit 1cb8f9776c7dcadc57885c6653943511d282633b should do the trick.
Patrice, can you please give it a try?

Thanks,
//richard

[U-Boot] UBI: regression since "mtd: ubi: Fix worker handling"

[Patrice CHOTARD]

Hi

On 05/22/2018 08:37 AM, Richard Weinberger wrote:
> Am Dienstag, 22. Mai 2018, 08:30:45 CEST schrieb Heiko Schocher:
>> Hello Richard,
>>
>> Am 21.05.2018 um 21:31 schrieb Richard Weinberger:
>>> Patrice,
>>>
>>> Am Montag, 21. Mai 2018, 16:07:41 CEST schrieb Richard Weinberger:
>>>>>     		e->pnum = aeb->pnum;
>>>>>     		e->ec = aeb->ec;
>>>>>     		ubi->lookuptbl[e->pnum] = e;
>>>>> +		ubi->thread_enabled = 1;
>>>>
>>>> This is not correct. At this point the UBI thread is not ready.
>>>> I know, I know, U-Boot has no threads but some data structures might
>>>> not be ready.
>>>>
>>>> Let me think how to work around this properly.
>>>
>>> The root cause seems to be that U-Boot misses this fix from Linux:
>>>
>>> commit 1cb8f9776c7dcadc57885c6653943511d282633b
>>> Author: Richard Weinberger <richard@nod.at>
>>> Date:   Tue Aug 11 23:27:44 2015 +0200
>>>
>>>       ubi: fastmap: Implement produce_free_peb()
>>>       
>>>       If fastmap requests a free PEB for a pool and UBI is busy
>>>       with erasing PEBs we need to offer a function to wait for one.
>>>       We can reuse produce_free_peb() from the non-fastmap WL code
>>>       but with different locking semantics.
>>>       
>>>       Cc: stable at vger.kernel.org # 4.1.x-
>>>       Reported-and-tested-by: Jörg Krause <joerg.krause@embedded.rocks>
>>>       Signed-off-by: Richard Weinberger <richard@nod.at
>>>
>>> Heiko, I'm currently working on Fastmap in Linux[0].
>>> I suggest to re-syncing with Linux if my changes go upstream, I can ping you them. :-)
>>> Fastmap gained many improvements since the last sync.
>>
>> Yes, but I am currently under load, and doing a rebase is a bigger
>> task I fear (I speculate currently to automate this task with tbot,
>> let me think a little bit about this, if this would work, U-Boot UBI
>> would autaomtically follow linux tree, and I immediately would see
>> merge errors ...)
> 
> Ok. :-)
> 
>> Also before a rebase can go mainline, we need deep testing. So no
>> fast chance here for me ... sorry. If we can find commits, which
>> fix this regression, it may makes sense to apply them before such a
>> rebase ... ?
> 
> As I said, commit 1cb8f9776c7dcadc57885c6653943511d282633b should do the trick.
> Patrice, can you please give it a try?

Yes sure,

Thanks

Patrice

> 
> Thanks,
> //richard
> 

[U-Boot] UBI: regression since "mtd: ubi: Fix worker handling"

[Patrice CHOTARD]

Hi Richard, Heiko

On 05/22/2018 08:51 AM, Patrice CHOTARD wrote:
> Hi
> 
> On 05/22/2018 08:37 AM, Richard Weinberger wrote:
>> Am Dienstag, 22. Mai 2018, 08:30:45 CEST schrieb Heiko Schocher:
>>> Hello Richard,
>>>
>>> Am 21.05.2018 um 21:31 schrieb Richard Weinberger:
>>>> Patrice,
>>>>
>>>> Am Montag, 21. Mai 2018, 16:07:41 CEST schrieb Richard Weinberger:
>>>>>>             e->pnum = aeb->pnum;
>>>>>>             e->ec = aeb->ec;
>>>>>>             ubi->lookuptbl[e->pnum] = e;
>>>>>> +        ubi->thread_enabled = 1;
>>>>>
>>>>> This is not correct. At this point the UBI thread is not ready.
>>>>> I know, I know, U-Boot has no threads but some data structures might
>>>>> not be ready.
>>>>>
>>>>> Let me think how to work around this properly.
>>>>
>>>> The root cause seems to be that U-Boot misses this fix from Linux:
>>>>
>>>> commit 1cb8f9776c7dcadc57885c6653943511d282633b
>>>> Author: Richard Weinberger <richard@nod.at>
>>>> Date:   Tue Aug 11 23:27:44 2015 +0200
>>>>
>>>>       ubi: fastmap: Implement produce_free_peb()
>>>>       If fastmap requests a free PEB for a pool and UBI is busy
>>>>       with erasing PEBs we need to offer a function to wait for one.
>>>>       We can reuse produce_free_peb() from the non-fastmap WL code
>>>>       but with different locking semantics.
>>>>       Cc: stable at vger.kernel.org # 4.1.x-
>>>>       Reported-and-tested-by: Jörg Krause <joerg.krause@embedded.rocks>
>>>>       Signed-off-by: Richard Weinberger <richard@nod.at
>>>>
>>>> Heiko, I'm currently working on Fastmap in Linux[0].
>>>> I suggest to re-syncing with Linux if my changes go upstream, I can 
>>>> ping you them. :-)
>>>> Fastmap gained many improvements since the last sync.
>>>
>>> Yes, but I am currently under load, and doing a rebase is a bigger
>>> task I fear (I speculate currently to automate this task with tbot,
>>> let me think a little bit about this, if this would work, U-Boot UBI
>>> would autaomtically follow linux tree, and I immediately would see
>>> merge errors ...)
>>
>> Ok. :-)
>>
>>> Also before a rebase can go mainline, we need deep testing. So no
>>> fast chance here for me ... sorry. If we can find commits, which
>>> fix this regression, it may makes sense to apply them before such a
>>> rebase ... ?
>>
>> As I said, commit 1cb8f9776c7dcadc57885c6653943511d282633b should do 
>> the trick.
>> Patrice, can you please give it a try?

I confirm this patch fixes the issue.

Thanks for your reactivity ;-)

Patrice

> 
> Yes sure,
> 
> Thanks
> 
> Patrice
> 
>>
>> Thanks,
>> //richard
>>

[U-Boot] UBI: regression since "mtd: ubi: Fix worker handling"

[Heiko Schocher]

Hello Patrice,

Am 22.05.2018 um 09:21 schrieb Patrice CHOTARD:
> Hi Richard, Heiko
> 
> On 05/22/2018 08:51 AM, Patrice CHOTARD wrote:
>> Hi
>>
>> On 05/22/2018 08:37 AM, Richard Weinberger wrote:
>>> Am Dienstag, 22. Mai 2018, 08:30:45 CEST schrieb Heiko Schocher:
>>>> Hello Richard,
>>>>
>>>> Am 21.05.2018 um 21:31 schrieb Richard Weinberger:
>>>>> Patrice,
>>>>>
>>>>> Am Montag, 21. Mai 2018, 16:07:41 CEST schrieb Richard Weinberger:
>>>>>>>              e->pnum = aeb->pnum;
>>>>>>>              e->ec = aeb->ec;
>>>>>>>              ubi->lookuptbl[e->pnum] = e;
>>>>>>> +        ubi->thread_enabled = 1;
>>>>>>
>>>>>> This is not correct. At this point the UBI thread is not ready.
>>>>>> I know, I know, U-Boot has no threads but some data structures might
>>>>>> not be ready.
>>>>>>
>>>>>> Let me think how to work around this properly.
>>>>>
>>>>> The root cause seems to be that U-Boot misses this fix from Linux:
>>>>>
>>>>> commit 1cb8f9776c7dcadc57885c6653943511d282633b
>>>>> Author: Richard Weinberger <richard@nod.at>
>>>>> Date:   Tue Aug 11 23:27:44 2015 +0200
>>>>>
>>>>>        ubi: fastmap: Implement produce_free_peb()
>>>>>        If fastmap requests a free PEB for a pool and UBI is busy
>>>>>        with erasing PEBs we need to offer a function to wait for one.
>>>>>        We can reuse produce_free_peb() from the non-fastmap WL code
>>>>>        but with different locking semantics.
>>>>>        Cc: stable at vger.kernel.org # 4.1.x-
>>>>>        Reported-and-tested-by: Jörg Krause <joerg.krause@embedded.rocks>
>>>>>        Signed-off-by: Richard Weinberger <richard@nod.at
>>>>>
>>>>> Heiko, I'm currently working on Fastmap in Linux[0].
>>>>> I suggest to re-syncing with Linux if my changes go upstream, I can
>>>>> ping you them. :-)
>>>>> Fastmap gained many improvements since the last sync.
>>>>
>>>> Yes, but I am currently under load, and doing a rebase is a bigger
>>>> task I fear (I speculate currently to automate this task with tbot,
>>>> let me think a little bit about this, if this would work, U-Boot UBI
>>>> would autaomtically follow linux tree, and I immediately would see
>>>> merge errors ...)
>>>
>>> Ok. :-)
>>>
>>>> Also before a rebase can go mainline, we need deep testing. So no
>>>> fast chance here for me ... sorry. If we can find commits, which
>>>> fix this regression, it may makes sense to apply them before such a
>>>> rebase ... ?
>>>
>>> As I said, commit 1cb8f9776c7dcadc57885c6653943511d282633b should do
>>> the trick.
>>> Patrice, can you please give it a try?
> 
> I confirm this patch fixes the issue.
> 
> Thanks for your reactivity ;-)

Thanks for testing! Could you may send a formal patch to the U-Boot ML ?

Thanks!

bye,
Heiko
> 
> Patrice
> 
>>
>> Yes sure,
>>
>> Thanks
>>
>> Patrice
>>
>>>
>>> Thanks,
>>> //richard
>> >

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de

[U-Boot] UBI: regression since "mtd: ubi: Fix worker handling"

[Patrice CHOTARD]

On 05/22/2018 09:29 AM, Heiko Schocher wrote:
> Hello Patrice,
> 
> Am 22.05.2018 um 09:21 schrieb Patrice CHOTARD:
>> Hi Richard, Heiko
>>
>> On 05/22/2018 08:51 AM, Patrice CHOTARD wrote:
>>> Hi
>>>
>>> On 05/22/2018 08:37 AM, Richard Weinberger wrote:
>>>> Am Dienstag, 22. Mai 2018, 08:30:45 CEST schrieb Heiko Schocher:
>>>>> Hello Richard,
>>>>>
>>>>> Am 21.05.2018 um 21:31 schrieb Richard Weinberger:
>>>>>> Patrice,
>>>>>>
>>>>>> Am Montag, 21. Mai 2018, 16:07:41 CEST schrieb Richard Weinberger:
>>>>>>>>              e->pnum = aeb->pnum;
>>>>>>>>              e->ec = aeb->ec;
>>>>>>>>              ubi->lookuptbl[e->pnum] = e;
>>>>>>>> +        ubi->thread_enabled = 1;
>>>>>>>
>>>>>>> This is not correct. At this point the UBI thread is not ready.
>>>>>>> I know, I know, U-Boot has no threads but some data structures might
>>>>>>> not be ready.
>>>>>>>
>>>>>>> Let me think how to work around this properly.
>>>>>>
>>>>>> The root cause seems to be that U-Boot misses this fix from Linux:
>>>>>>
>>>>>> commit 1cb8f9776c7dcadc57885c6653943511d282633b
>>>>>> Author: Richard Weinberger <richard@nod.at>
>>>>>> Date:   Tue Aug 11 23:27:44 2015 +0200
>>>>>>
>>>>>>        ubi: fastmap: Implement produce_free_peb()
>>>>>>        If fastmap requests a free PEB for a pool and UBI is busy
>>>>>>        with erasing PEBs we need to offer a function to wait for one.
>>>>>>        We can reuse produce_free_peb() from the non-fastmap WL code
>>>>>>        but with different locking semantics.
>>>>>>        Cc: stable at vger.kernel.org # 4.1.x-
>>>>>>        Reported-and-tested-by: Jörg Krause 
>>>>>> <joerg.krause@embedded.rocks>
>>>>>>        Signed-off-by: Richard Weinberger <richard@nod.at
>>>>>>
>>>>>> Heiko, I'm currently working on Fastmap in Linux[0].
>>>>>> I suggest to re-syncing with Linux if my changes go upstream, I can
>>>>>> ping you them. :-)
>>>>>> Fastmap gained many improvements since the last sync.
>>>>>
>>>>> Yes, but I am currently under load, and doing a rebase is a bigger
>>>>> task I fear (I speculate currently to automate this task with tbot,
>>>>> let me think a little bit about this, if this would work, U-Boot UBI
>>>>> would autaomtically follow linux tree, and I immediately would see
>>>>> merge errors ...)
>>>>
>>>> Ok. :-)
>>>>
>>>>> Also before a rebase can go mainline, we need deep testing. So no
>>>>> fast chance here for me ... sorry. If we can find commits, which
>>>>> fix this regression, it may makes sense to apply them before such a
>>>>> rebase ... ?
>>>>
>>>> As I said, commit 1cb8f9776c7dcadc57885c6653943511d282633b should do
>>>> the trick.
>>>> Patrice, can you please give it a try?
>>
>> I confirm this patch fixes the issue.
>>
>> Thanks for your reactivity ;-)
> 
> Thanks for testing! Could you may send a formal patch to the U-Boot ML ?

Ok no problem

Patrice

> 
> Thanks!
> 
> bye,
> Heiko
>>
>> Patrice
>>
>>>
>>> Yes sure,
>>>
>>> Thanks
>>>
>>> Patrice
>>>
>>>>
>>>> Thanks,
>>>> //richard
>>> >
>