UBI fastmap updates

UBI fastmap updates

[Richard Weinberger]

this series is a collection of all pending UBI fastmap updates.

Richard Genoud found issues in combination with U-Boot.
These issues also uncovered other minor issues in some error paths.
Only one fix is not really fastmap specific and touches generic 
UBI code, "UBI: simplify image sequence test".

Thanks,
//richard

Richard Genoud (2):
      UBI: fastmap: fix backward compatibility with image_seq
      UBI: simplify image sequence test

Richard Weinberger (5):
      UBI: fix refill_wl_user_pool()
      UBI: Fix error path in scan_pool()
      UBI: Call scan_all() with correct offset in error case
      UBI: Fix memory leak in ubi_attach_fastmap() error path
      UBI: Add some asserts to ubi_attach_fastmap()

 drivers/mtd/ubi/attach.c  | 11 ++++++-----
 drivers/mtd/ubi/fastmap.c | 41 +++++++++++++++++++++++++++++++++++++----
 drivers/mtd/ubi/wl.c      |  4 ----
 3 files changed, 43 insertions(+), 13 deletions(-)

[PATCH 1/7] UBI: fix refill_wl_user_pool()

[Richard Weinberger]

If no free PEBs are available refill_wl_user_pool() must not
return with -ENOSPC immediately.
It has to block till produce_free_peb() produced a free PEB.

Reported-and-Tested-by: Richard Genoud <richard.genoud@gmail.com>
Signed-off-by: Richard Weinberger <richard@nod.at>
---
 drivers/mtd/ubi/wl.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
index c95bfb1..02317c1 100644
--- a/drivers/mtd/ubi/wl.c
+++ b/drivers/mtd/ubi/wl.c
@@ -599,10 +599,6 @@ static void refill_wl_user_pool(struct ubi_device *ubi)
 	return_unused_pool_pebs(ubi, pool);
 
 	for (pool->size = 0; pool->size < pool->max_size; pool->size++) {
-		if (!ubi->free.rb_node ||
-		   (ubi->free_count - ubi->beb_rsvd_pebs < 1))
-			break;
-
 		pool->pebs[pool->size] = __wl_get_peb(ubi);
 		if (pool->pebs[pool->size] < 0)
 			break;
-- 
1.8.3.1

[PATCH 1/7] UBI: fix refill_wl_user_pool()

[Richard Weinberger]

If no free PEBs are available refill_wl_user_pool() must not
return with -ENOSPC immediately.
It has to block till produce_free_peb() produced a free PEB.

Reported-and-Tested-by: Richard Genoud <richard.genoud@gmail.com>
Signed-off-by: Richard Weinberger <richard@nod.at>
---
 drivers/mtd/ubi/wl.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
index c95bfb1..02317c1 100644
--- a/drivers/mtd/ubi/wl.c
+++ b/drivers/mtd/ubi/wl.c
@@ -599,10 +599,6 @@ static void refill_wl_user_pool(struct ubi_device *ubi)
 	return_unused_pool_pebs(ubi, pool);
 
 	for (pool->size = 0; pool->size < pool->max_size; pool->size++) {
-		if (!ubi->free.rb_node ||
-		   (ubi->free_count - ubi->beb_rsvd_pebs < 1))
-			break;
-
 		pool->pebs[pool->size] = __wl_get_peb(ubi);
 		if (pool->pebs[pool->size] < 0)
 			break;
-- 
1.8.3.1

Re: [PATCH 1/7] UBI: fix refill_wl_user_pool()

[Artem Bityutskiy]

On Sat, 2013-09-28 at 15:55 +0200, Richard Weinberger wrote:
> If no free PEBs are available refill_wl_user_pool() must not
> return with -ENOSPC immediately.
> It has to block till produce_free_peb() produced a free PEB.
> 
> Reported-and-Tested-by: Richard Genoud <richard.genoud@gmail.com>
> Signed-off-by: Richard Weinberger <richard@nod.at>

What is pool size, I wonder?

-- 
Best Regards,
Artem Bityutskiy

Re: [PATCH 1/7] UBI: fix refill_wl_user_pool()

[Richard Weinberger]

Am 03.10.2013 17:00, schrieb Artem Bityutskiy:
> On Sat, 2013-09-28 at 15:55 +0200, Richard Weinberger wrote:
>> If no free PEBs are available refill_wl_user_pool() must not
>> return with -ENOSPC immediately.
>> It has to block till produce_free_peb() produced a free PEB.
>>
>> Reported-and-Tested-by: Richard Genoud <richard.genoud@gmail.com>
>> Signed-off-by: Richard Weinberger <richard@nod.at>
> 
> What is pool size, I wonder?

Currently it's 25 (UBI_FM_WL_POOL_SIZE).
If experience shows that 25 is too low/big we can change this constant.
Maybe it's also worth making them configurable...

Thanks,
//richard

Re: [PATCH 1/7] UBI: fix refill_wl_user_pool()

[Artem Bityutskiy]

On Thu, 2013-10-03 at 17:08 +0200, Richard Weinberger wrote:
> Am 03.10.2013 17:00, schrieb Artem Bityutskiy:
> > On Sat, 2013-09-28 at 15:55 +0200, Richard Weinberger wrote:
> >> If no free PEBs are available refill_wl_user_pool() must not
> >> return with -ENOSPC immediately.
> >> It has to block till produce_free_peb() produced a free PEB.
> >>
> >> Reported-and-Tested-by: Richard Genoud <richard.genoud@gmail.com>
> >> Signed-off-by: Richard Weinberger <richard@nod.at>
> > 
> > What is pool size, I wonder?
> 
> Currently it's 25 (UBI_FM_WL_POOL_SIZE).
> If experience shows that 25 is too low/big we can change this constant.
> Maybe it's also worth making them configurable...

I if it is a possible scenario that this function will not return until
25 (or even 10) PEBs are erased? 

-- 
Best Regards,
Artem Bityutskiy

Re: [PATCH 1/7] UBI: fix refill_wl_user_pool()

[Richard Weinberger]

Am 03.10.2013 17:27, schrieb Artem Bityutskiy:
> On Thu, 2013-10-03 at 17:08 +0200, Richard Weinberger wrote:
>> Am 03.10.2013 17:00, schrieb Artem Bityutskiy:
>>> On Sat, 2013-09-28 at 15:55 +0200, Richard Weinberger wrote:
>>>> If no free PEBs are available refill_wl_user_pool() must not
>>>> return with -ENOSPC immediately.
>>>> It has to block till produce_free_peb() produced a free PEB.
>>>>
>>>> Reported-and-Tested-by: Richard Genoud <richard.genoud@gmail.com>
>>>> Signed-off-by: Richard Weinberger <richard@nod.at>
>>>
>>> What is pool size, I wonder?
>>
>> Currently it's 25 (UBI_FM_WL_POOL_SIZE).
>> If experience shows that 25 is too low/big we can change this constant.
>> Maybe it's also worth making them configurable...
> 
> I if it is a possible scenario that this function will not return until
> 25 (or even 10) PEBs are erased? 
> 

Sure. It will try to gain up to 25 PEBs but return if it gets less.
That's why a pool has a current and a max size.

Thanks,
//richard

Re: [PATCH 1/7] UBI: fix refill_wl_user_pool()

[Artem Bityutskiy]

On Thu, 2013-10-03 at 17:53 +0200, Richard Weinberger wrote:
> Am 03.10.2013 17:27, schrieb Artem Bityutskiy:
> > On Thu, 2013-10-03 at 17:08 +0200, Richard Weinberger wrote:
> >> Am 03.10.2013 17:00, schrieb Artem Bityutskiy:
> >>> On Sat, 2013-09-28 at 15:55 +0200, Richard Weinberger wrote:
> >>>> If no free PEBs are available refill_wl_user_pool() must not
> >>>> return with -ENOSPC immediately.
> >>>> It has to block till produce_free_peb() produced a free PEB.
> >>>>
> >>>> Reported-and-Tested-by: Richard Genoud <richard.genoud@gmail.com>
> >>>> Signed-off-by: Richard Weinberger <richard@nod.at>
> >>>
> >>> What is pool size, I wonder?
> >>
> >> Currently it's 25 (UBI_FM_WL_POOL_SIZE).
> >> If experience shows that 25 is too low/big we can change this constant.
> >> Maybe it's also worth making them configurable...
> > 
> > I if it is a possible scenario that this function will not return until
> > 25 (or even 10) PEBs are erased? 
> > 
> 
> Sure. It will try to gain up to 25 PEBs but return if it gets less.
> That's why a pool has a current and a max size.

So if erasing speed is say, 250ms, then it would take 6.25 seconds?

-- 
Best Regards,
Artem Bityutskiy

Re: [PATCH 1/7] UBI: fix refill_wl_user_pool()

[Richard Weinberger]

Am 03.10.2013 18:00, schrieb Artem Bityutskiy:
> On Thu, 2013-10-03 at 17:53 +0200, Richard Weinberger wrote:
>> Am 03.10.2013 17:27, schrieb Artem Bityutskiy:
>>> On Thu, 2013-10-03 at 17:08 +0200, Richard Weinberger wrote:
>>>> Am 03.10.2013 17:00, schrieb Artem Bityutskiy:
>>>>> On Sat, 2013-09-28 at 15:55 +0200, Richard Weinberger wrote:
>>>>>> If no free PEBs are available refill_wl_user_pool() must not
>>>>>> return with -ENOSPC immediately.
>>>>>> It has to block till produce_free_peb() produced a free PEB.
>>>>>>
>>>>>> Reported-and-Tested-by: Richard Genoud <richard.genoud@gmail.com>
>>>>>> Signed-off-by: Richard Weinberger <richard@nod.at>
>>>>>
>>>>> What is pool size, I wonder?
>>>>
>>>> Currently it's 25 (UBI_FM_WL_POOL_SIZE).
>>>> If experience shows that 25 is too low/big we can change this constant.
>>>> Maybe it's also worth making them configurable...
>>>
>>> I if it is a possible scenario that this function will not return until
>>> 25 (or even 10) PEBs are erased? 
>>>
>>
>> Sure. It will try to gain up to 25 PEBs but return if it gets less.
>> That's why a pool has a current and a max size.
> 
> So if erasing speed is say, 250ms, then it would take 6.25 seconds?

Only in the very worst case if we have to call 25 times produce_free_peb().

Of course we could add a check to return immediately if produce_free_peb()
got called a few times in series.
But I really would like to wait with such performance tweaks until fastmap
is more mature.

Thanks,
//richard

Re: [PATCH 1/7] UBI: fix refill_wl_user_pool()

[Artem Bityutskiy]

On Thu, 2013-10-03 at 18:35 +0200, Richard Weinberger wrote:
> Am 03.10.2013 18:00, schrieb Artem Bityutskiy:
> > On Thu, 2013-10-03 at 17:53 +0200, Richard Weinberger wrote:
> >> Am 03.10.2013 17:27, schrieb Artem Bityutskiy:
> >>> On Thu, 2013-10-03 at 17:08 +0200, Richard Weinberger wrote:
> >>>> Am 03.10.2013 17:00, schrieb Artem Bityutskiy:
> >>>>> On Sat, 2013-09-28 at 15:55 +0200, Richard Weinberger wrote:
> >>>>>> If no free PEBs are available refill_wl_user_pool() must not
> >>>>>> return with -ENOSPC immediately.
> >>>>>> It has to block till produce_free_peb() produced a free PEB.
> >>>>>>
> >>>>>> Reported-and-Tested-by: Richard Genoud <richard.genoud@gmail.com>
> >>>>>> Signed-off-by: Richard Weinberger <richard@nod.at>
> >>>>>
> >>>>> What is pool size, I wonder?
> >>>>
> >>>> Currently it's 25 (UBI_FM_WL_POOL_SIZE).
> >>>> If experience shows that 25 is too low/big we can change this constant.
> >>>> Maybe it's also worth making them configurable...
> >>>
> >>> I if it is a possible scenario that this function will not return until
> >>> 25 (or even 10) PEBs are erased? 
> >>>
> >>
> >> Sure. It will try to gain up to 25 PEBs but return if it gets less.
> >> That's why a pool has a current and a max size.
> > 
> > So if erasing speed is say, 250ms, then it would take 6.25 seconds?
> 
> Only in the very worst case if we have to call 25 times produce_free_peb().
> 
> Of course we could add a check to return immediately if produce_free_peb()
> got called a few times in series.
> But I really would like to wait with such performance tweaks until fastmap
> is more mature.

OK, but how about at least adding a comment talking about this unlikely
scenario?

-- 
Best Regards,
Artem Bityutskiy

Re: [PATCH 1/7] UBI: fix refill_wl_user_pool()

[Richard Weinberger]

Am 03.10.2013 18:41, schrieb Artem Bityutskiy:
> On Thu, 2013-10-03 at 18:35 +0200, Richard Weinberger wrote:
>> Am 03.10.2013 18:00, schrieb Artem Bityutskiy:
>>> On Thu, 2013-10-03 at 17:53 +0200, Richard Weinberger wrote:
>>>> Am 03.10.2013 17:27, schrieb Artem Bityutskiy:
>>>>> On Thu, 2013-10-03 at 17:08 +0200, Richard Weinberger wrote:
>>>>>> Am 03.10.2013 17:00, schrieb Artem Bityutskiy:
>>>>>>> On Sat, 2013-09-28 at 15:55 +0200, Richard Weinberger wrote:
>>>>>>>> If no free PEBs are available refill_wl_user_pool() must not
>>>>>>>> return with -ENOSPC immediately.
>>>>>>>> It has to block till produce_free_peb() produced a free PEB.
>>>>>>>>
>>>>>>>> Reported-and-Tested-by: Richard Genoud <richard.genoud@gmail.com>
>>>>>>>> Signed-off-by: Richard Weinberger <richard@nod.at>
>>>>>>>
>>>>>>> What is pool size, I wonder?
>>>>>>
>>>>>> Currently it's 25 (UBI_FM_WL_POOL_SIZE).
>>>>>> If experience shows that 25 is too low/big we can change this constant.
>>>>>> Maybe it's also worth making them configurable...
>>>>>
>>>>> I if it is a possible scenario that this function will not return until
>>>>> 25 (or even 10) PEBs are erased? 
>>>>>
>>>>
>>>> Sure. It will try to gain up to 25 PEBs but return if it gets less.
>>>> That's why a pool has a current and a max size.
>>>
>>> So if erasing speed is say, 250ms, then it would take 6.25 seconds?
>>
>> Only in the very worst case if we have to call 25 times produce_free_peb().
>>
>> Of course we could add a check to return immediately if produce_free_peb()
>> got called a few times in series.
>> But I really would like to wait with such performance tweaks until fastmap
>> is more mature.
> 
> OK, but how about at least adding a comment talking about this unlikely
> scenario?

I'm fine with this. Will send a patch. :-)

Thanks,
//richard

[PATCH 2/7] UBI: Fix error path in scan_pool()

We have to set "ret", not "err" in case of an error.

Reported-and-tested-by: Richard Genoud <richard.genoud@gmail.com>
Signed-off-by: Richard Weinberger <richard@nod.at>
---
 drivers/mtd/ubi/fastmap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
index f5aa4b0..9b42add 100644
--- a/drivers/mtd/ubi/fastmap.c
+++ b/drivers/mtd/ubi/fastmap.c
@@ -428,7 +428,7 @@ static int scan_pool(struct ubi_device *ubi, struct ubi_attach_info *ai,
 		if (be32_to_cpu(ech->image_seq) != ubi->image_seq) {
 			ubi_err("bad image seq: 0x%x, expected: 0x%x",
 				be32_to_cpu(ech->image_seq), ubi->image_seq);
-			err = UBI_BAD_FASTMAP;
+			ret = UBI_BAD_FASTMAP;
 			goto out;
 		}
 
-- 
1.8.3.1

[PATCH 2/7] UBI: Fix error path in scan_pool()

[Richard Weinberger]

We have to set "ret", not "err" in case of an error.

Reported-and-tested-by: Richard Genoud <richard.genoud@gmail.com>
Signed-off-by: Richard Weinberger <richard@nod.at>
---
 drivers/mtd/ubi/fastmap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
index f5aa4b0..9b42add 100644
--- a/drivers/mtd/ubi/fastmap.c
+++ b/drivers/mtd/ubi/fastmap.c
@@ -428,7 +428,7 @@ static int scan_pool(struct ubi_device *ubi, struct ubi_attach_info *ai,
 		if (be32_to_cpu(ech->image_seq) != ubi->image_seq) {
 			ubi_err("bad image seq: 0x%x, expected: 0x%x",
 				be32_to_cpu(ech->image_seq), ubi->image_seq);
-			err = UBI_BAD_FASTMAP;
+			ret = UBI_BAD_FASTMAP;
 			goto out;
 		}
 
-- 
1.8.3.1

[PATCH 3/7] UBI: Call scan_all() with correct offset in error case

[Richard Weinberger]

If we find an invalid fastmap we have to scan from the very beginning.
Otherwise we leak the first 64 PEBs.

Reported-and-tested-by: Richard Genoud <richard.genoud@gmail.com>
Signed-off-by: Richard Weinberger <richard@nod.at>
---
 drivers/mtd/ubi/attach.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/ubi/attach.c b/drivers/mtd/ubi/attach.c
index c071d41..03b32b0 100644
--- a/drivers/mtd/ubi/attach.c
+++ b/drivers/mtd/ubi/attach.c
@@ -1417,9 +1417,11 @@ int ubi_attach(struct ubi_device *ubi, int force_scan)
 				ai = alloc_ai("ubi_aeb_slab_cache2");
 				if (!ai)
 					return -ENOMEM;
-			}
 
-			err = scan_all(ubi, ai, UBI_FM_MAX_START);
+				err = scan_all(ubi, ai, 0);
+			} else {
+				err = scan_all(ubi, ai, UBI_FM_MAX_START);
+			}
 		}
 	}
 #else
-- 
1.8.3.1

[PATCH 3/7] UBI: Call scan_all() with correct offset in error case

[Richard Weinberger]

If we find an invalid fastmap we have to scan from the very beginning.
Otherwise we leak the first 64 PEBs.

Reported-and-tested-by: Richard Genoud <richard.genoud@gmail.com>
Signed-off-by: Richard Weinberger <richard@nod.at>
---
 drivers/mtd/ubi/attach.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/ubi/attach.c b/drivers/mtd/ubi/attach.c
index c071d41..03b32b0 100644
--- a/drivers/mtd/ubi/attach.c
+++ b/drivers/mtd/ubi/attach.c
@@ -1417,9 +1417,11 @@ int ubi_attach(struct ubi_device *ubi, int force_scan)
 				ai = alloc_ai("ubi_aeb_slab_cache2");
 				if (!ai)
 					return -ENOMEM;
-			}
 
-			err = scan_all(ubi, ai, UBI_FM_MAX_START);
+				err = scan_all(ubi, ai, 0);
+			} else {
+				err = scan_all(ubi, ai, UBI_FM_MAX_START);
+			}
 		}
 	}
 #else
-- 
1.8.3.1

[PATCH 4/7] UBI: fastmap: fix backward compatibility with image_seq

[Richard Weinberger]

From: Richard Genoud <richard.genoud@gmail.com>

Some old UBI implementations (e.g. U-Boot) have not implemented the image
sequence feature.
So, when erase blocks are written, the image sequence in the ec header
is lost (set to zero).
UBI scan_all() takes this case into account (commits
32bc4820287a1a03982979515949e8ea56eac641 and
2eadaad67b2b6bd132eda105128d2d466298b8e3)

But fastmap scan functions (ubi_scan_fastmap() and scan_pool()) didn't.

This patch fixes the issue.

Signed-off-by: Richard Genoud <richard.genoud@gmail.com>
Signed-off-by: Richard Weinberger <richard@nod.at>
---
 drivers/mtd/ubi/fastmap.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
index 9b42add..05067f5 100644
--- a/drivers/mtd/ubi/fastmap.c
+++ b/drivers/mtd/ubi/fastmap.c
@@ -407,6 +407,7 @@ static int scan_pool(struct ubi_device *ubi, struct ubi_attach_info *ai,
 	 */
 	for (i = 0; i < pool_size; i++) {
 		int scrub = 0;
+		int image_seq;
 
 		pnum = be32_to_cpu(pebs[i]);
 
@@ -425,7 +426,13 @@ static int scan_pool(struct ubi_device *ubi, struct ubi_attach_info *ai,
 		} else if (ret == UBI_IO_BITFLIPS)
 			scrub = 1;
 
-		if (be32_to_cpu(ech->image_seq) != ubi->image_seq) {
+		/*
+		 * Older UBI implementations have image_seq set to zero, so
+		 * we shouldn't fail if image_seq == 0.
+		 */
+		image_seq = be32_to_cpu(ech->image_seq);
+
+		if (image_seq && (image_seq != ubi->image_seq)) {
 			ubi_err("bad image seq: 0x%x, expected: 0x%x",
 				be32_to_cpu(ech->image_seq), ubi->image_seq);
 			ret = UBI_BAD_FASTMAP;
@@ -923,6 +930,8 @@ int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info *ai,
 	}
 
 	for (i = 0; i < used_blocks; i++) {
+		int image_seq;
+
 		pnum = be32_to_cpu(fmsb->block_loc[i]);
 
 		if (ubi_io_is_bad(ubi, pnum)) {
@@ -940,10 +949,17 @@ int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info *ai,
 		} else if (ret == UBI_IO_BITFLIPS)
 			fm->to_be_tortured[i] = 1;
 
+		image_seq = be32_to_cpu(ech->image_seq);
 		if (!ubi->image_seq)
-			ubi->image_seq = be32_to_cpu(ech->image_seq);
+			ubi->image_seq = image_seq;
 
-		if (be32_to_cpu(ech->image_seq) != ubi->image_seq) {
+		/*
+		 * Older UBI implementations have image_seq set to zero, so
+		 * we shouldn't fail if image_seq == 0.
+		 */
+		if (image_seq && (image_seq != ubi->image_seq)) {
+			ubi_err("wrong image seq:%d instead of %d",
+				be32_to_cpu(ech->image_seq), ubi->image_seq);
 			ret = UBI_BAD_FASTMAP;
 			goto free_hdr;
 		}
-- 
1.8.3.1

[PATCH 4/7] UBI: fastmap: fix backward compatibility with image_seq

[Richard Weinberger]

From: Richard Genoud <richard.genoud@gmail.com>

Some old UBI implementations (e.g. U-Boot) have not implemented the image
sequence feature.
So, when erase blocks are written, the image sequence in the ec header
is lost (set to zero).
UBI scan_all() takes this case into account (commits
32bc4820287a1a03982979515949e8ea56eac641 and
2eadaad67b2b6bd132eda105128d2d466298b8e3)

But fastmap scan functions (ubi_scan_fastmap() and scan_pool()) didn't.

This patch fixes the issue.

Signed-off-by: Richard Genoud <richard.genoud@gmail.com>
Signed-off-by: Richard Weinberger <richard@nod.at>
---
 drivers/mtd/ubi/fastmap.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
index 9b42add..05067f5 100644
--- a/drivers/mtd/ubi/fastmap.c
+++ b/drivers/mtd/ubi/fastmap.c
@@ -407,6 +407,7 @@ static int scan_pool(struct ubi_device *ubi, struct ubi_attach_info *ai,
 	 */
 	for (i = 0; i < pool_size; i++) {
 		int scrub = 0;
+		int image_seq;
 
 		pnum = be32_to_cpu(pebs[i]);
 
@@ -425,7 +426,13 @@ static int scan_pool(struct ubi_device *ubi, struct ubi_attach_info *ai,
 		} else if (ret == UBI_IO_BITFLIPS)
 			scrub = 1;
 
-		if (be32_to_cpu(ech->image_seq) != ubi->image_seq) {
+		/*
+		 * Older UBI implementations have image_seq set to zero, so
+		 * we shouldn't fail if image_seq == 0.
+		 */
+		image_seq = be32_to_cpu(ech->image_seq);
+
+		if (image_seq && (image_seq != ubi->image_seq)) {
 			ubi_err("bad image seq: 0x%x, expected: 0x%x",
 				be32_to_cpu(ech->image_seq), ubi->image_seq);
 			ret = UBI_BAD_FASTMAP;
@@ -923,6 +930,8 @@ int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info *ai,
 	}
 
 	for (i = 0; i < used_blocks; i++) {
+		int image_seq;
+
 		pnum = be32_to_cpu(fmsb->block_loc[i]);
 
 		if (ubi_io_is_bad(ubi, pnum)) {
@@ -940,10 +949,17 @@ int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info *ai,
 		} else if (ret == UBI_IO_BITFLIPS)
 			fm->to_be_tortured[i] = 1;
 
+		image_seq = be32_to_cpu(ech->image_seq);
 		if (!ubi->image_seq)
-			ubi->image_seq = be32_to_cpu(ech->image_seq);
+			ubi->image_seq = image_seq;
 
-		if (be32_to_cpu(ech->image_seq) != ubi->image_seq) {
+		/*
+		 * Older UBI implementations have image_seq set to zero, so
+		 * we shouldn't fail if image_seq == 0.
+		 */
+		if (image_seq && (image_seq != ubi->image_seq)) {
+			ubi_err("wrong image seq:%d instead of %d",
+				be32_to_cpu(ech->image_seq), ubi->image_seq);
 			ret = UBI_BAD_FASTMAP;
 			goto free_hdr;
 		}
-- 
1.8.3.1

[PATCH 5/7] UBI: simplify image sequence test

[Richard Weinberger]

From: Richard Genoud <richard.genoud@gmail.com>

The test:
if (!a && b)
  a = b;
can be symplified in:
if (!a)
  a = b;

And there's no need to test if ubi->image_seq is not null, because if it is,
it is set to image_seq.
So, we just test if image_seq is not null.

Signed-off-by: Richard Genoud <richard.genoud@gmail.com>
Signed-off-by: Richard Weinberger <richard@nod.at>
---
 drivers/mtd/ubi/attach.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/ubi/attach.c b/drivers/mtd/ubi/attach.c
index 03b32b0..33bb1f2 100644
--- a/drivers/mtd/ubi/attach.c
+++ b/drivers/mtd/ubi/attach.c
@@ -900,10 +900,9 @@ static int scan_peb(struct ubi_device *ubi, struct ubi_attach_info *ai,
 		 * number.
 		 */
 		image_seq = be32_to_cpu(ech->image_seq);
-		if (!ubi->image_seq && image_seq)
+		if (!ubi->image_seq)
 			ubi->image_seq = image_seq;
-		if (ubi->image_seq && image_seq &&
-		    ubi->image_seq != image_seq) {
+		if (image_seq && ubi->image_seq != image_seq) {
 			ubi_err("bad image sequence number %d in PEB %d, expected %d",
 				image_seq, pnum, ubi->image_seq);
 			ubi_dump_ec_hdr(ech);
-- 
1.8.3.1

[PATCH 5/7] UBI: simplify image sequence test

[Richard Weinberger]

From: Richard Genoud <richard.genoud@gmail.com>

The test:
if (!a && b)
  a = b;
can be symplified in:
if (!a)
  a = b;

And there's no need to test if ubi->image_seq is not null, because if it is,
it is set to image_seq.
So, we just test if image_seq is not null.

Signed-off-by: Richard Genoud <richard.genoud@gmail.com>
Signed-off-by: Richard Weinberger <richard@nod.at>
---
 drivers/mtd/ubi/attach.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/ubi/attach.c b/drivers/mtd/ubi/attach.c
index 03b32b0..33bb1f2 100644
--- a/drivers/mtd/ubi/attach.c
+++ b/drivers/mtd/ubi/attach.c
@@ -900,10 +900,9 @@ static int scan_peb(struct ubi_device *ubi, struct ubi_attach_info *ai,
 		 * number.
 		 */
 		image_seq = be32_to_cpu(ech->image_seq);
-		if (!ubi->image_seq && image_seq)
+		if (!ubi->image_seq)
 			ubi->image_seq = image_seq;
-		if (ubi->image_seq && image_seq &&
-		    ubi->image_seq != image_seq) {
+		if (image_seq && ubi->image_seq != image_seq) {
 			ubi_err("bad image sequence number %d in PEB %d, expected %d",
 				image_seq, pnum, ubi->image_seq);
 			ubi_dump_ec_hdr(ech);
-- 
1.8.3.1

[PATCH 6/7] UBI: Fix memory leak in ubi_attach_fastmap() error path

[Richard Weinberger]

On error we have to free all three temporary lists.

Reported-by: Richard Genoud <richard.genoud@gmail.com>
Signed-off-by: Richard Weinberger <richard@nod.at>
---
 drivers/mtd/ubi/fastmap.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
index 05067f5..4cfc8da 100644
--- a/drivers/mtd/ubi/fastmap.c
+++ b/drivers/mtd/ubi/fastmap.c
@@ -841,6 +841,19 @@ static int ubi_attach_fastmap(struct ubi_device *ubi,
 fail_bad:
 	ret = UBI_BAD_FASTMAP;
 fail:
+	list_for_each_entry_safe(tmp_aeb, _tmp_aeb, &used, u.list) {
+		kmem_cache_free(ai->aeb_slab_cache, tmp_aeb);
+		list_del(&tmp_aeb->u.list);
+	}
+	list_for_each_entry_safe(tmp_aeb, _tmp_aeb, &eba_orphans, u.list) {
+		kmem_cache_free(ai->aeb_slab_cache, tmp_aeb);
+		list_del(&tmp_aeb->u.list);
+	}
+	list_for_each_entry_safe(tmp_aeb, _tmp_aeb, &free, u.list) {
+		kmem_cache_free(ai->aeb_slab_cache, tmp_aeb);
+		list_del(&tmp_aeb->u.list);
+	}
+
 	return ret;
 }
 
-- 
1.8.3.1

[PATCH 6/7] UBI: Fix memory leak in ubi_attach_fastmap() error path

[Richard Weinberger]

On error we have to free all three temporary lists.

Reported-by: Richard Genoud <richard.genoud@gmail.com>
Signed-off-by: Richard Weinberger <richard@nod.at>
---
 drivers/mtd/ubi/fastmap.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
index 05067f5..4cfc8da 100644
--- a/drivers/mtd/ubi/fastmap.c
+++ b/drivers/mtd/ubi/fastmap.c
@@ -841,6 +841,19 @@ static int ubi_attach_fastmap(struct ubi_device *ubi,
 fail_bad:
 	ret = UBI_BAD_FASTMAP;
 fail:
+	list_for_each_entry_safe(tmp_aeb, _tmp_aeb, &used, u.list) {
+		kmem_cache_free(ai->aeb_slab_cache, tmp_aeb);
+		list_del(&tmp_aeb->u.list);
+	}
+	list_for_each_entry_safe(tmp_aeb, _tmp_aeb, &eba_orphans, u.list) {
+		kmem_cache_free(ai->aeb_slab_cache, tmp_aeb);
+		list_del(&tmp_aeb->u.list);
+	}
+	list_for_each_entry_safe(tmp_aeb, _tmp_aeb, &free, u.list) {
+		kmem_cache_free(ai->aeb_slab_cache, tmp_aeb);
+		list_del(&tmp_aeb->u.list);
+	}
+
 	return ret;
 }
 
-- 
1.8.3.1

Re: [PATCH 6/7] UBI: Fix memory leak in ubi_attach_fastmap() error path

[Richard Genoud]

2013/9/28 Richard Weinberger <richard@nod.at>:
> On error we have to free all three temporary lists.
>
> Reported-by: Richard Genoud <richard.genoud@gmail.com>
> Signed-off-by: Richard Weinberger <richard@nod.at>
> ---
>  drivers/mtd/ubi/fastmap.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
> index 05067f5..4cfc8da 100644
> --- a/drivers/mtd/ubi/fastmap.c
> +++ b/drivers/mtd/ubi/fastmap.c
> @@ -841,6 +841,19 @@ static int ubi_attach_fastmap(struct ubi_device *ubi,
>  fail_bad:
>         ret = UBI_BAD_FASTMAP;
>  fail:
> +       list_for_each_entry_safe(tmp_aeb, _tmp_aeb, &used, u.list) {
> +               kmem_cache_free(ai->aeb_slab_cache, tmp_aeb);
> +               list_del(&tmp_aeb->u.list);
> +       }
> +       list_for_each_entry_safe(tmp_aeb, _tmp_aeb, &eba_orphans, u.list) {
> +               kmem_cache_free(ai->aeb_slab_cache, tmp_aeb);
> +               list_del(&tmp_aeb->u.list);
> +       }
> +       list_for_each_entry_safe(tmp_aeb, _tmp_aeb, &free, u.list) {
> +               kmem_cache_free(ai->aeb_slab_cache, tmp_aeb);
> +               list_del(&tmp_aeb->u.list);
> +       }
> +
>         return ret;
>  }
>
> --
> 1.8.3.1
>

Works like a charm !
Tested-by: Richard Genoud <richard.genoud@gmail.com>


Thanks !

Re: [PATCH 6/7] UBI: Fix memory leak in ubi_attach_fastmap() error path

[Richard Genoud]

2013/9/28 Richard Weinberger <richard@nod.at>:
> On error we have to free all three temporary lists.
>
> Reported-by: Richard Genoud <richard.genoud@gmail.com>
> Signed-off-by: Richard Weinberger <richard@nod.at>
> ---
>  drivers/mtd/ubi/fastmap.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
> index 05067f5..4cfc8da 100644
> --- a/drivers/mtd/ubi/fastmap.c
> +++ b/drivers/mtd/ubi/fastmap.c
> @@ -841,6 +841,19 @@ static int ubi_attach_fastmap(struct ubi_device *ubi,
>  fail_bad:
>         ret = UBI_BAD_FASTMAP;
>  fail:
> +       list_for_each_entry_safe(tmp_aeb, _tmp_aeb, &used, u.list) {
> +               kmem_cache_free(ai->aeb_slab_cache, tmp_aeb);
> +               list_del(&tmp_aeb->u.list);
> +       }
> +       list_for_each_entry_safe(tmp_aeb, _tmp_aeb, &eba_orphans, u.list) {
> +               kmem_cache_free(ai->aeb_slab_cache, tmp_aeb);
> +               list_del(&tmp_aeb->u.list);
> +       }
> +       list_for_each_entry_safe(tmp_aeb, _tmp_aeb, &free, u.list) {
> +               kmem_cache_free(ai->aeb_slab_cache, tmp_aeb);
> +               list_del(&tmp_aeb->u.list);
> +       }
> +
>         return ret;
>  }
>
> --
> 1.8.3.1
>

Works like a charm !
Tested-by: Richard Genoud <richard.genoud@gmail.com>


Thanks !

[PATCH 7/7] UBI: Add some asserts to ubi_attach_fastmap()

[Richard Weinberger]

Add more paranioa asserts to make it easier to detect
implementation errors.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 drivers/mtd/ubi/fastmap.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
index 4cfc8da..ead8613 100644
--- a/drivers/mtd/ubi/fastmap.c
+++ b/drivers/mtd/ubi/fastmap.c
@@ -826,6 +826,10 @@ static int ubi_attach_fastmap(struct ubi_device *ubi,
 	list_for_each_entry_safe(tmp_aeb, _tmp_aeb, &free, u.list)
 		list_move_tail(&tmp_aeb->u.list, &ai->free);
 
+	ubi_assert(list_empty(&used));
+	ubi_assert(list_empty(&eba_orphans));
+	ubi_assert(list_empty(&free));
+
 	/*
 	 * If fastmap is leaking PEBs (must not happen), raise a
 	 * fat warning and fall back to scanning mode.
-- 
1.8.3.1

[PATCH 7/7] UBI: Add some asserts to ubi_attach_fastmap()

[Richard Weinberger]

Add more paranioa asserts to make it easier to detect
implementation errors.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 drivers/mtd/ubi/fastmap.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
index 4cfc8da..ead8613 100644
--- a/drivers/mtd/ubi/fastmap.c
+++ b/drivers/mtd/ubi/fastmap.c
@@ -826,6 +826,10 @@ static int ubi_attach_fastmap(struct ubi_device *ubi,
 	list_for_each_entry_safe(tmp_aeb, _tmp_aeb, &free, u.list)
 		list_move_tail(&tmp_aeb->u.list, &ai->free);
 
+	ubi_assert(list_empty(&used));
+	ubi_assert(list_empty(&eba_orphans));
+	ubi_assert(list_empty(&free));
+
 	/*
 	 * If fastmap is leaking PEBs (must not happen), raise a
 	 * fat warning and fall back to scanning mode.
-- 
1.8.3.1

Re: [PATCH 7/7] UBI: Add some asserts to ubi_attach_fastmap()

[Richard Genoud]

2013/9/28 Richard Weinberger <richard@nod.at>:
> Add more paranioa asserts to make it easier to detect
> implementation errors.
>
> Signed-off-by: Richard Weinberger <richard@nod.at>
> ---
>  drivers/mtd/ubi/fastmap.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
> index 4cfc8da..ead8613 100644
> --- a/drivers/mtd/ubi/fastmap.c
> +++ b/drivers/mtd/ubi/fastmap.c
> @@ -826,6 +826,10 @@ static int ubi_attach_fastmap(struct ubi_device *ubi,
>         list_for_each_entry_safe(tmp_aeb, _tmp_aeb, &free, u.list)
>                 list_move_tail(&tmp_aeb->u.list, &ai->free);
>
> +       ubi_assert(list_empty(&used));
> +       ubi_assert(list_empty(&eba_orphans));
> +       ubi_assert(list_empty(&free));
> +
>         /*
>          * If fastmap is leaking PEBs (must not happen), raise a
>          * fat warning and fall back to scanning mode.
> --
> 1.8.3.1
>

Tested-by: Richard Genoud <richard.genoud@gmail.com>

Re: [PATCH 7/7] UBI: Add some asserts to ubi_attach_fastmap()

[Richard Genoud]

2013/9/28 Richard Weinberger <richard@nod.at>:
> Add more paranioa asserts to make it easier to detect
> implementation errors.
>
> Signed-off-by: Richard Weinberger <richard@nod.at>
> ---
>  drivers/mtd/ubi/fastmap.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
> index 4cfc8da..ead8613 100644
> --- a/drivers/mtd/ubi/fastmap.c
> +++ b/drivers/mtd/ubi/fastmap.c
> @@ -826,6 +826,10 @@ static int ubi_attach_fastmap(struct ubi_device *ubi,
>         list_for_each_entry_safe(tmp_aeb, _tmp_aeb, &free, u.list)
>                 list_move_tail(&tmp_aeb->u.list, &ai->free);
>
> +       ubi_assert(list_empty(&used));
> +       ubi_assert(list_empty(&eba_orphans));
> +       ubi_assert(list_empty(&free));
> +
>         /*
>          * If fastmap is leaking PEBs (must not happen), raise a
>          * fat warning and fall back to scanning mode.
> --
> 1.8.3.1
>

Tested-by: Richard Genoud <richard.genoud@gmail.com>

Re: [PATCH 7/7] UBI: Add some asserts to ubi_attach_fastmap()

[Bill Pringlemeir]

On 30 Sep 2013, richard.genoud at gmail.com wrote:

> 2013/9/28 Richard Weinberger <richard at nod.at>:
>> Add more paranioa asserts to make it easier to detect
>> implementation errors.
>>
>> Signed-off-by: Richard Weinberger <richard at nod.at>
>
> Tested-by: Richard Genoud <richard.genoud at gmail.com>

As I reported here,

 http://lists.infradead.org/pipermail/linux-mtd/2013-August/048102.html

This patch-set seems to fix the issue I encountered as well.  I
reflashed my saved image with the updated patch set and the file system
successfully mounts.  Although in my case, it was back-ported.

Fwiw,
Bill Pringlemeir.

Re: UBI fastmap updates

[Artem Bityutskiy]

On Sat, 2013-09-28 at 15:55 +0200, Richard Weinberger wrote:
> this series is a collection of all pending UBI fastmap updates.
> 
> Richard Genoud found issues in combination with U-Boot.
> These issues also uncovered other minor issues in some error paths.
> Only one fix is not really fastmap specific and touches generic 
> UBI code, "UBI: simplify image sequence test".
> 

Pushed to linux-ubi, thanks!

-- 
Best Regards,
Artem Bityutskiy