[10/14] ubi: fastmap: Change a WARN_ON() to ubi_err()

Message ID 20180613212344.11608-11-richard@nod.at
State Needs Review / ACK
Delegated to: Richard Weinberger
Headers show
Series
  • ubi: Fastmap updates
Related show

Commit Message

Richard Weinberger June 13, 2018, 9:23 p.m.
This WARN_ON() was used while development of fastmap, now
it can be a regular ubi_err().

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

Comments

Boris Brezillon June 24, 2018, 1:04 p.m. | #1
On Wed, 13 Jun 2018 23:23:40 +0200
Richard Weinberger <richard@nod.at> wrote:

> This WARN_ON() was used while development of fastmap, now
> it can be a regular ubi_err().
> 
> Signed-off-by: Richard Weinberger <richard@nod.at>
> ---
>  drivers/mtd/ubi/fastmap.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
> index dabeb01af24a..d5506152c9f7 100644
> --- a/drivers/mtd/ubi/fastmap.c
> +++ b/drivers/mtd/ubi/fastmap.c
> @@ -949,14 +949,16 @@ static int ubi_attach_fastmap(struct ubi_device *ubi,
>  	ubi_assert(list_empty(&free));
>  
>  	/*
> -	 * If fastmap is leaking PEBs (must not happen), raise a
> -	 * fat warning and fall back to scanning mode.
> -	 * We do this here because in ubi_wl_init() it's too late
> -	 * and we cannot fall back to scanning.
> +	 * The orginal purpose of this check was detecting fastmap bugs were
> +	 * it missed blocks. Now it helps to detect mis-flashed UBIs.
> +	 * E.g. when a fastmap enabled UBI is copied to another target with
> +	 * different bad blocks.
>  	 */
> -	if (WARN_ON(count_fastmap_pebs(ai) != ubi->peb_count -
> -		    ai->bad_peb_count - fm->used_blocks))
> +	if (count_fastmap_pebs(ai) != ubi->peb_count - ai->bad_peb_count -
> +		fm->used_blocks) {

Nitpick, but can you align things like that:

	if (count_fastmap_pebs(ai) != ubi->peb_count - ai->bad_peb_count -
				      fm->used_blocks) {

or

	if (count_fastmap_pebs(ai) !=
	    ubi->peb_count - ai->bad_peb_count - fm->used_blocks) {

or even better, have an avail_peb_count variable:

	avail_peb_count = ubi->peb_count - ai->bad_peb_count - fm->used_blocks;
	if (count_fastmap_pebs(ai) != avail_peb_count)

> +		ubi_err(ubi, "number of PEBs referenced by fastmap does not match MTD!");
>  		goto fail_bad;

Also, I'm not sure I get it. Why is that an error now that we have
preseeded fastmap? Isn't it expected to have a potential mismatch the
first time we boot, or is it already taken into account before we reach
this point?



> +	}
>  
>  	return 0;
>

Patch

diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
index dabeb01af24a..d5506152c9f7 100644
--- a/drivers/mtd/ubi/fastmap.c
+++ b/drivers/mtd/ubi/fastmap.c
@@ -949,14 +949,16 @@  static int ubi_attach_fastmap(struct ubi_device *ubi,
 	ubi_assert(list_empty(&free));
 
 	/*
-	 * If fastmap is leaking PEBs (must not happen), raise a
-	 * fat warning and fall back to scanning mode.
-	 * We do this here because in ubi_wl_init() it's too late
-	 * and we cannot fall back to scanning.
+	 * The orginal purpose of this check was detecting fastmap bugs were
+	 * it missed blocks. Now it helps to detect mis-flashed UBIs.
+	 * E.g. when a fastmap enabled UBI is copied to another target with
+	 * different bad blocks.
 	 */
-	if (WARN_ON(count_fastmap_pebs(ai) != ubi->peb_count -
-		    ai->bad_peb_count - fm->used_blocks))
+	if (count_fastmap_pebs(ai) != ubi->peb_count - ai->bad_peb_count -
+		fm->used_blocks) {
+		ubi_err(ubi, "number of PEBs referenced by fastmap does not match MTD!");
 		goto fail_bad;
+	}
 
 	return 0;