[PATCH] mtd: ubi: Fix endless loop when moving PEB

[PATCH] mtd: ubi: Fix endless loop when moving PEB

[Teresa Remmet]

When moving a PEB the leb_write_trylock() function is called.
As the function never returns 0 UBI will end up in an endless
loop.

Noticed the issue when fastmap has been enabled and data is beeing copied
several times to a UBI volume. When UBI tries to move the anchor PEB,
the issue comes up.

The leb_write_trylock() is now equal to the leb_write_lock().
But kept it for easier maintaince in future when syncing with
kernel.

Signed-off-by: Teresa Remmet <t.remmet@phytec.de>
---
 drivers/mtd/ubi/eba.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/drivers/mtd/ubi/eba.c b/drivers/mtd/ubi/eba.c
index a7af247..31dbcd2 100644
--- a/drivers/mtd/ubi/eba.c
+++ b/drivers/mtd/ubi/eba.c
@@ -257,16 +257,7 @@ static int leb_write_trylock(struct ubi_device *ubi, int vol_id, int lnum)
 	le = ltree_add_entry(ubi, vol_id, lnum);
 	if (IS_ERR(le))
 		return PTR_ERR(le);
-
-	/* Contention, cancel */
-	le->users -= 1;
-	ubi_assert(le->users >= 0);
-	if (le->users == 0) {
-		rb_erase(&le->rb, &ubi->ltree);
-		kfree(le);
-	}
-
-	return 1;
+	return 0;
 }
 
 /**
-- 
1.9.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] mtd: ubi: Fix endless loop when moving PEB

[Sascha Hauer]

Hi Teresa,

On Wed, Jun 22, 2016 at 11:01:57AM +0200, Teresa Remmet wrote:
> When moving a PEB the leb_write_trylock() function is called.
> As the function never returns 0 UBI will end up in an endless
> loop.
> 
> Noticed the issue when fastmap has been enabled and data is beeing copied
> several times to a UBI volume. When UBI tries to move the anchor PEB,
> the issue comes up.
> 
> The leb_write_trylock() is now equal to the leb_write_lock().
> But kept it for easier maintaince in future when syncing with
> kernel.
> 
> Signed-off-by: Teresa Remmet <t.remmet@phytec.de>
> ---
>  drivers/mtd/ubi/eba.c | 11 +----------
>  1 file changed, 1 insertion(+), 10 deletions(-)
> 
> diff --git a/drivers/mtd/ubi/eba.c b/drivers/mtd/ubi/eba.c
> index a7af247..31dbcd2 100644
> --- a/drivers/mtd/ubi/eba.c
> +++ b/drivers/mtd/ubi/eba.c
> @@ -257,16 +257,7 @@ static int leb_write_trylock(struct ubi_device *ubi, int vol_id, int lnum)
>  	le = ltree_add_entry(ubi, vol_id, lnum);
>  	if (IS_ERR(le))
>  		return PTR_ERR(le);
> -
> -	/* Contention, cancel */
> -	le->users -= 1;
> -	ubi_assert(le->users >= 0);
> -	if (le->users == 0) {
> -		rb_erase(&le->rb, &ubi->ltree);
> -		kfree(le);
> -	}
> -
> -	return 1;
> +	return 0;
>  }

This seems to be the missing link to make the following patch work. I
have tried this several times but so far never found this bug. Could you
give it a try? With this patch it should be now possible to ubiformat a
device with an image and to generate a fastmap when the device is
detached first.

Sascha

------------------------8<---------------------------------

From 94cea61ade69f712383e282d6aa573d3a12f11d0 Mon Sep 17 00:00:00 2001
From: Sascha Hauer <s.hauer@pengutronix.de>
Date: Fri, 24 Jun 2016 11:38:24 +0200
Subject: [PATCH] mtd: ubi: actually do work in wear leveling code

The actual work in the wear leveling code is done in a separate thread.
Since we do not have threading so far we did not do any of the queued
work. Change this by calling the worker function synchronously.

With this barebox now can write a fastmap on a freshly ubiformated
device.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/mtd/ubi/wl.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
index 4535f2d..f24c219 100644
--- a/drivers/mtd/ubi/wl.c
+++ b/drivers/mtd/ubi/wl.c
@@ -183,7 +183,6 @@ static void wl_entry_destroy(struct ubi_device *ubi, struct ubi_wl_entry *e)
 	kfree(e);
 }
 
-#ifndef CONFIG_MTD_UBI_FASTMAP
 /**
  * do_work - do one pending work.
  * @ubi: UBI device description object
@@ -221,7 +220,6 @@ static int do_work(struct ubi_device *ubi)
 
 	return err;
 }
-#endif
 
 /**
  * in_wl_tree - check if wear-leveling entry is present in a WL RB-tree.
@@ -523,8 +521,9 @@ static void __schedule_ubi_work(struct ubi_device *ubi, struct ubi_work *wrk)
 	list_add_tail(&wrk->list, &ubi->works);
 	ubi_assert(ubi->works_count >= 0);
 	ubi->works_count += 1;
-	if (ubi->thread_enabled && !ubi_dbg_is_bgt_disabled(ubi))
-		wake_up_process(ubi->bgt_thread);
+
+	/* No threading in barebox, so do work synchronously */
+	do_work(ubi);
 }
 
 /**
-- 
2.8.1



-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] mtd: ubi: Fix endless loop when moving PEB

[Teresa Remmet]

Hello Sascha,

Am Freitag, den 24.06.2016, 13:20 +0200 schrieb Sascha Hauer:
> Hi Teresa,
> 
> On Wed, Jun 22, 2016 at 11:01:57AM +0200, Teresa Remmet wrote:
> > When moving a PEB the leb_write_trylock() function is called.
> > As the function never returns 0 UBI will end up in an endless
> > loop.
> > 
> > Noticed the issue when fastmap has been enabled and data is beeing copied
> > several times to a UBI volume. When UBI tries to move the anchor PEB,
> > the issue comes up.
> > 
> > The leb_write_trylock() is now equal to the leb_write_lock().
> > But kept it for easier maintaince in future when syncing with
> > kernel.
> > 
> > Signed-off-by: Teresa Remmet <t.remmet@phytec.de>
> > ---
> >  drivers/mtd/ubi/eba.c | 11 +----------
> >  1 file changed, 1 insertion(+), 10 deletions(-)
> > 
> > diff --git a/drivers/mtd/ubi/eba.c b/drivers/mtd/ubi/eba.c
> > index a7af247..31dbcd2 100644
> > --- a/drivers/mtd/ubi/eba.c
> > +++ b/drivers/mtd/ubi/eba.c
> > @@ -257,16 +257,7 @@ static int leb_write_trylock(struct ubi_device *ubi, int vol_id, int lnum)
> >  	le = ltree_add_entry(ubi, vol_id, lnum);
> >  	if (IS_ERR(le))
> >  		return PTR_ERR(le);
> > -
> > -	/* Contention, cancel */
> > -	le->users -= 1;
> > -	ubi_assert(le->users >= 0);
> > -	if (le->users == 0) {
> > -		rb_erase(&le->rb, &ubi->ltree);
> > -		kfree(le);
> > -	}
> > -
> > -	return 1;
> > +	return 0;
> >  }
> 
> This seems to be the missing link to make the following patch work. I
> have tried this several times but so far never found this bug. Could you
> give it a try? With this patch it should be now possible to ubiformat a
> device with an image and to generate a fastmap when the device is
> detached first.

I tested it, and it is working. Ubi now successfully writes an anchor
PEB.

Teresa

> 
> Sascha
> 
> ------------------------8<---------------------------------
> 
> From 94cea61ade69f712383e282d6aa573d3a12f11d0 Mon Sep 17 00:00:00 2001
> From: Sascha Hauer <s.hauer@pengutronix.de>
> Date: Fri, 24 Jun 2016 11:38:24 +0200
> Subject: [PATCH] mtd: ubi: actually do work in wear leveling code
> 
> The actual work in the wear leveling code is done in a separate thread.
> Since we do not have threading so far we did not do any of the queued
> work. Change this by calling the worker function synchronously.
> 
> With this barebox now can write a fastmap on a freshly ubiformated
> device.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  drivers/mtd/ubi/wl.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
> index 4535f2d..f24c219 100644
> --- a/drivers/mtd/ubi/wl.c
> +++ b/drivers/mtd/ubi/wl.c
> @@ -183,7 +183,6 @@ static void wl_entry_destroy(struct ubi_device *ubi, struct ubi_wl_entry *e)
>  	kfree(e);
>  }
>  
> -#ifndef CONFIG_MTD_UBI_FASTMAP
>  /**
>   * do_work - do one pending work.
>   * @ubi: UBI device description object
> @@ -221,7 +220,6 @@ static int do_work(struct ubi_device *ubi)
>  
>  	return err;
>  }
> -#endif
>  
>  /**
>   * in_wl_tree - check if wear-leveling entry is present in a WL RB-tree.
> @@ -523,8 +521,9 @@ static void __schedule_ubi_work(struct ubi_device *ubi, struct ubi_work *wrk)
>  	list_add_tail(&wrk->list, &ubi->works);
>  	ubi_assert(ubi->works_count >= 0);
>  	ubi->works_count += 1;
> -	if (ubi->thread_enabled && !ubi_dbg_is_bgt_disabled(ubi))
> -		wake_up_process(ubi->bgt_thread);
> +
> +	/* No threading in barebox, so do work synchronously */
> +	do_work(ubi);
>  }
>  
>  /**
> -- 
> 2.8.1
> 
> 
> 



_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] mtd: ubi: Fix endless loop when moving PEB

[Sascha Hauer]

On Mon, Jun 27, 2016 at 10:22:38AM +0200, Teresa Remmet wrote:
> Hello Sascha,
> 
> Am Freitag, den 24.06.2016, 13:20 +0200 schrieb Sascha Hauer:
> > Hi Teresa,
> > 
> > On Wed, Jun 22, 2016 at 11:01:57AM +0200, Teresa Remmet wrote:
> > > When moving a PEB the leb_write_trylock() function is called.
> > > As the function never returns 0 UBI will end up in an endless
> > > loop.
> > > 
> > > Noticed the issue when fastmap has been enabled and data is beeing copied
> > > several times to a UBI volume. When UBI tries to move the anchor PEB,
> > > the issue comes up.
> > > 
> > > The leb_write_trylock() is now equal to the leb_write_lock().
> > > But kept it for easier maintaince in future when syncing with
> > > kernel.
> > > 
> > > Signed-off-by: Teresa Remmet <t.remmet@phytec.de>
> > > ---
> > >  drivers/mtd/ubi/eba.c | 11 +----------
> > >  1 file changed, 1 insertion(+), 10 deletions(-)
> > > 
> > > diff --git a/drivers/mtd/ubi/eba.c b/drivers/mtd/ubi/eba.c
> > > index a7af247..31dbcd2 100644
> > > --- a/drivers/mtd/ubi/eba.c
> > > +++ b/drivers/mtd/ubi/eba.c
> > > @@ -257,16 +257,7 @@ static int leb_write_trylock(struct ubi_device *ubi, int vol_id, int lnum)
> > >  	le = ltree_add_entry(ubi, vol_id, lnum);
> > >  	if (IS_ERR(le))
> > >  		return PTR_ERR(le);
> > > -
> > > -	/* Contention, cancel */
> > > -	le->users -= 1;
> > > -	ubi_assert(le->users >= 0);
> > > -	if (le->users == 0) {
> > > -		rb_erase(&le->rb, &ubi->ltree);
> > > -		kfree(le);
> > > -	}
> > > -
> > > -	return 1;
> > > +	return 0;
> > >  }
> > 
> > This seems to be the missing link to make the following patch work. I
> > have tried this several times but so far never found this bug. Could you
> > give it a try? With this patch it should be now possible to ubiformat a
> > device with an image and to generate a fastmap when the device is
> > detached first.
> 
> I tested it, and it is working. Ubi now successfully writes an anchor
> PEB.

Thanks for testing. Applied both patches.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox