[PATCH] mtd/ubi: fix initialization order of ubi subsystems

[PATCH] mtd/ubi: fix initialization order of ubi subsystems

[Mikhail Kshevetskiy]

during ubi initialization we have a following calling sequence

1) ubi_attach()

   ----------------------------------------------------------------
   err = ubi_wl_init(ubi, ai);
   if (err) goto out_vtbl;

   err = ubi_eba_init(ubi, ai);
   if (err) goto out_wl;
   ----------------------------------------------------------------

   As we can see "eba" subsytem is NOT initialized at the moment of
   initializing of "wl" subsystem

2) ubi_wl_init()

   it call ensure_wear_leveling() at some moment

3) ensure_wear_leveling()

   ---------------------------------------------------------------
   e1 = rb_entry(rb_first(&ubi->used), struct ubi_wl_entry, u.rb);
   e2 = find_wl_entry(ubi, &ubi->free, WL_FREE_MAX_DIFF);
   if (!(e2->ec - e1->ec >= UBI_WL_THRESHOLD)) goto out_unlock;
   dbg_wl("schedule wear-leveling");
   ---------------------------------------------------------------

   so, if no wear-leveling is scheduled than everything is OK

   and a little bit below

   ---------------------------------------------------------------
   wrk->anchor = 0;
   wrk->func = &wear_leveling_worker;
   if (nested) __schedule_ubi_work(ubi, wrk);
   else schedule_ubi_work(ubi, wrk);
   ---------------------------------------------------------------

   as result we enter to wear_leveling_worker() function

4) wear_leveling_worker()

   ---------------------------------------------------------------
   /*
    * Now pick the least worn-out used physical eraseblock and a
    * highly worn-out free physical eraseblock. If the erase
    * counters differ much enough, start wear-leveling.
    */
   e1 = rb_entry(rb_first(&ubi->used), struct ubi_wl_entry, u.rb);
   e2 = get_peb_for_wl(ubi);
   if (!e2) goto out_cancel;

   if (!(e2->ec - e1->ec >= UBI_WL_THRESHOLD)) {
       dbg_wl("no WL needed: min used EC %d, max free EC %d", e1->ec, e2->ec);
       /* Give the unused PEB back */
       wl_tree_add(e2, &ubi->free);
       ubi->free_count++;
       goto out_cancel;
   }
   ---------------------------------------------------------------

   so, if no WL needed than everything is OK

   and a little bit below

   ---------------------------------------------------------------
   err = ubi_eba_copy_leb(ubi, e1->pnum, e2->pnum, vid_hdr);
   ---------------------------------------------------------------

   OPS, eba sybsystem is not initialized yet (see (1))

From the other side, it looks like eba sybsystem does not require wl sybsystem
during initialization, so just fix ordering and proper handle error path.
---
 drivers/mtd/ubi/attach.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/ubi/attach.c b/drivers/mtd/ubi/attach.c
index 10b2459f8951..8c1d629c0e1d 100644
--- a/drivers/mtd/ubi/attach.c
+++ b/drivers/mtd/ubi/attach.c
@@ -1602,13 +1602,13 @@ int ubi_attach(struct ubi_device *ubi, int force_scan)
 	if (err)
 		goto out_ai;
 
-	err = ubi_wl_init(ubi, ai);
+	err = ubi_eba_init(ubi, ai);
 	if (err)
 		goto out_vtbl;
 
-	err = ubi_eba_init(ubi, ai);
+	err = ubi_wl_init(ubi, ai);
 	if (err)
-		goto out_wl;
+		goto out_vtbl;
 
 #ifdef CONFIG_MTD_UBI_FASTMAP
 	if (ubi->fm && ubi_dbg_chk_fastmap(ubi)) {
-- 
2.20.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] mtd/ubi: fix initialization order of ubi subsystems

[Artem Bityutskiy]

On Thu, 2019-06-20 at 16:27 +0300, Mikhail Kshevetskiy wrote:
> -	err = ubi_wl_init(ubi, ai);
> +	err = ubi_eba_init(ubi, ai);
>  	if (err)
>  		goto out_vtbl;
>  
> -	err = ubi_eba_init(ubi, ai);
> +	err = ubi_wl_init(ubi, ai);
>  	if (err)
> -		goto out_wl;
> +		goto out_vtbl;

Looks good to me, thanks.

Reviewed-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] mtd/ubi: fix initialization order of ubi subsystems

[Richard Weinberger]

On Thu, Jun 20, 2019 at 3:28 PM Mikhail Kshevetskiy
<mikhail.kshevetskiy@gmail.com> wrote:
>
> during ubi initialization we have a following calling sequence
>
> 1) ubi_attach()
>
>    ----------------------------------------------------------------
>    err = ubi_wl_init(ubi, ai);
>    if (err) goto out_vtbl;
>
>    err = ubi_eba_init(ubi, ai);
>    if (err) goto out_wl;
>    ----------------------------------------------------------------
>
>    As we can see "eba" subsytem is NOT initialized at the moment of
>    initializing of "wl" subsystem
>
> 2) ubi_wl_init()
>
>    it call ensure_wear_leveling() at some moment
>
> 3) ensure_wear_leveling()
>
>    ---------------------------------------------------------------
>    e1 = rb_entry(rb_first(&ubi->used), struct ubi_wl_entry, u.rb);
>    e2 = find_wl_entry(ubi, &ubi->free, WL_FREE_MAX_DIFF);
>    if (!(e2->ec - e1->ec >= UBI_WL_THRESHOLD)) goto out_unlock;
>    dbg_wl("schedule wear-leveling");
>    ---------------------------------------------------------------
>
>    so, if no wear-leveling is scheduled than everything is OK
>
>    and a little bit below
>
>    ---------------------------------------------------------------
>    wrk->anchor = 0;
>    wrk->func = &wear_leveling_worker;
>    if (nested) __schedule_ubi_work(ubi, wrk);
>    else schedule_ubi_work(ubi, wrk);
>    ---------------------------------------------------------------
>
>    as result we enter to wear_leveling_worker() function

Well, we schedule work, but don't execute it since the ubi-thread
is still disabled.

Can you please share a little more about the problem you are facing?
Also produce_free_peb() should not get called at this point.
So before we flip the order of initialization I'd like to understand the problem
better.

Thanks,
//richard

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] mtd/ubi: fix initialization order of ubi subsystems

[Mikhail Kshevetskiy]

On Fri, 21 Jun 2019 19:26:37 +0200
Richard Weinberger <richard.weinberger@gmail.com> wrote:

> On Thu, Jun 20, 2019 at 3:28 PM Mikhail Kshevetskiy
> <mikhail.kshevetskiy@gmail.com> wrote:
> >
> > during ubi initialization we have a following calling sequence
> >
> > 1) ubi_attach()
> >
> >    ----------------------------------------------------------------
> >    err = ubi_wl_init(ubi, ai);
> >    if (err) goto out_vtbl;
> >
> >    err = ubi_eba_init(ubi, ai);
> >    if (err) goto out_wl;
> >    ----------------------------------------------------------------
> >
> >    As we can see "eba" subsytem is NOT initialized at the moment of
> >    initializing of "wl" subsystem
> >
> > 2) ubi_wl_init()
> >
> >    it call ensure_wear_leveling() at some moment
> >
> > 3) ensure_wear_leveling()
> >
> >    ---------------------------------------------------------------
> >    e1 = rb_entry(rb_first(&ubi->used), struct ubi_wl_entry, u.rb);
> >    e2 = find_wl_entry(ubi, &ubi->free, WL_FREE_MAX_DIFF);
> >    if (!(e2->ec - e1->ec >= UBI_WL_THRESHOLD)) goto out_unlock;
> >    dbg_wl("schedule wear-leveling");
> >    ---------------------------------------------------------------
> >
> >    so, if no wear-leveling is scheduled than everything is OK
> >
> >    and a little bit below
> >
> >    ---------------------------------------------------------------
> >    wrk->anchor = 0;
> >    wrk->func = &wear_leveling_worker;
> >    if (nested) __schedule_ubi_work(ubi, wrk);
> >    else schedule_ubi_work(ubi, wrk);
> >    ---------------------------------------------------------------
> >
> >    as result we enter to wear_leveling_worker() function
> 
> Well, we schedule work, but don't execute it since the ubi-thread
> is still disabled.
> 
> Can you please share a little more about the problem you are facing?
> Also produce_free_peb() should not get called at this point.
> So before we flip the order of initialization I'd like to understand the
> problem better.

We faced a cycle rebooting in u-boot during ubi initialization. The problem
appears approximately once per week on a random router from our test farm.
We never trigger this problem in linux (only in u-boot).

From the other side ubi code in u-boot is almost the same as ubi code in linux
kernel (it backported from linux periodically), so it make sense to fix it in
linux as well to help with future porting.

PS we send the same patch to u-boot.

Mikhail

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] mtd/ubi: fix initialization order of ubi subsystems

[Richard Weinberger]

----- Ursprüngliche Mail -----
>> Can you please share a little more about the problem you are facing?
>> Also produce_free_peb() should not get called at this point.
>> So before we flip the order of initialization I'd like to understand the
>> problem better.
> 
> We faced a cycle rebooting in u-boot during ubi initialization. The problem
> appears approximately once per week on a random router from our test farm.
> We never trigger this problem in linux (only in u-boot).
> 
> From the other side ubi code in u-boot is almost the same as ubi code in linux
> kernel (it backported from linux periodically), so it make sense to fix it in
> linux as well to help with future porting.
> 
> PS we send the same patch to u-boot.

In u-boot the story is a little different because it has no concept of
threads and executes work immediately.

Do you see this on a recent u-boot?
With this commit in u-boot this problem should not happen:
f82290afc847 ("mtd: ubi: Fix worker handling")

Thanks,
//richard

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] mtd/ubi: fix initialization order of ubi subsystems

[Mikhail Kshevetskiy]

On Fri, 21 Jun 2019 20:47:50 +0200 (CEST)
Richard Weinberger <richard@nod.at> wrote:

> ----- Ursprüngliche Mail -----
> >> Can you please share a little more about the problem you are facing?
> >> Also produce_free_peb() should not get called at this point.
> >> So before we flip the order of initialization I'd like to understand the
> >> problem better.
> > 
> > We faced a cycle rebooting in u-boot during ubi initialization. The problem
> > appears approximately once per week on a random router from our test farm.
> > We never trigger this problem in linux (only in u-boot).
> > 
> > From the other side ubi code in u-boot is almost the same as ubi code in
> > linux kernel (it backported from linux periodically), so it make sense to
> > fix it in linux as well to help with future porting.
> > 
> > PS we send the same patch to u-boot.
> 
> In u-boot the story is a little different because it has no concept of
> threads and executes work immediately.
> 
> Do you see this on a recent u-boot?
> With this commit in u-boot this problem should not happen:
> f82290afc847 ("mtd: ubi: Fix worker handling")

no we use 201.07 based bootloader. I'll look on it. Thanks.

> 
> Thanks,
> //richard

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] mtd/ubi: fix initialization order of ubi subsystems

[Richard Weinberger]

----- Ursprüngliche Mail -----
>> > PS we send the same patch to u-boot.
>> 
>> In u-boot the story is a little different because it has no concept of
>> threads and executes work immediately.
>> 
>> Do you see this on a recent u-boot?
>> With this commit in u-boot this problem should not happen:
>> f82290afc847 ("mtd: ubi: Fix worker handling")
> 
> no we use 201.07 based bootloader. I'll look on it. Thanks.

Please backport the said fix and communicate this on the
u-boot mailinglist.

Your patch fixes the issue only partially, you will still face
issues if ubi sees bitflips at attach time.

Thanks,
//richard

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] mtd/ubi: fix initialization order of ubi subsystems

[Mikhail Kshevetskiy]

On Fri, 21 Jun 2019 21:33:01 +0200 (CEST)
Richard Weinberger <richard@nod.at> wrote:

> ----- Ursprüngliche Mail -----
> >> > PS we send the same patch to u-boot.
> >> 
> >> In u-boot the story is a little different because it has no concept of
> >> threads and executes work immediately.
> >> 
> >> Do you see this on a recent u-boot?
> >> With this commit in u-boot this problem should not happen:
> >> f82290afc847 ("mtd: ubi: Fix worker handling")
> > 
> > no we use 201.07 based bootloader. I'll look on it. Thanks.
> 
> Please backport the said fix and communicate this on the
> u-boot mailinglist.
> 
> Your patch fixes the issue only partially, you will still face
> issues if ubi sees bitflips at attach time.

thanks a lot.


> Thanks,
> //richard

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] mtd/ubi: fix initialization order of ubi subsystems

[Mikhail Kshevetskiy]

On Fri, 21 Jun 2019 20:47:50 +0200 (CEST)
Richard Weinberger <richard@nod.at> wrote:

> ----- Ursprüngliche Mail -----
> >> Can you please share a little more about the problem you are facing?
> >> Also produce_free_peb() should not get called at this point.
> >> So before we flip the order of initialization I'd like to understand the
> >> problem better.
> > 
> > We faced a cycle rebooting in u-boot during ubi initialization. The problem
> > appears approximately once per week on a random router from our test farm.
> > We never trigger this problem in linux (only in u-boot).
> > 
> > From the other side ubi code in u-boot is almost the same as ubi code in
> > linux kernel (it backported from linux periodically), so it make sense to
> > fix it in linux as well to help with future porting.
> > 
> > PS we send the same patch to u-boot.
> 
> In u-boot the story is a little different because it has no concept of
> threads and executes work immediately.
> 
> Do you see this on a recent u-boot?
> With this commit in u-boot this problem should not happen:
> f82290afc847 ("mtd: ubi: Fix worker handling")

no we use 2016.07 based bootloader. I'll look on it. Thanks.

> 
> Thanks,
> //richard

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/