Re: [PATCH v6 12/15] ubi: fastmap: Add all fastmap pebs into 'ai->fastmap' when fm->used_blocks>=2

[Richard Weinberger]

----- Ursprüngliche Mail -----
> Von: "chengzhihao1" <chengzhihao1@xxxxxxxxxx>
> An: "richard" <richard@xxxxxx>, "Miquel Raynal" <miquel.raynal@xxxxxxxxxxx>, "Vignesh Raghavendra" <vigneshr@xxxxxx>,
> "mcoquelin stm32" <mcoquelin.stm32@xxxxxxxxx>, "kirill shutemov" <kirill.shutemov@xxxxxxxxxxxxxxx>, "Sascha Hauer"
> <s.hauer@xxxxxxxxxxxxxx>
> CC: "linux-mtd" <linux-mtd@xxxxxxxxxxxxxxxxxxx>, "linux-kernel" <linux-kernel@xxxxxxxxxxxxxxx>
> Gesendet: Montag, 27. Dezember 2021 04:22:43
> Betreff: [PATCH v6 12/15] ubi: fastmap: Add all fastmap pebs into 'ai->fastmap' when fm->used_blocks>=2

> Fastmap pebs(pnum >= UBI_FM_MAX_START) won't be added into 'ai->fastmap'
> while attaching ubi device if 'fm->used_blocks' is greater than 2, which
> may cause warning from 'ubi_assert(ubi->good_peb_count == found_pebs)':
> 
>  UBI assert failed in ubi_wl_init at 1878 (pid 2409)
>  Call Trace:
>    ubi_wl_init.cold+0xae/0x2af [ubi]
>    ubi_attach+0x1b0/0x780 [ubi]
>    ubi_init+0x23a/0x3ad [ubi]
>    load_module+0x22d2/0x2430
> 
> Reproduce:
>  ID="0x20,0x33,0x00,0x00" # 16M 16KB PEB, 512 page
>  modprobe nandsim id_bytes=$ID
>  modprobe ubi mtd="0,0" fm_autoconvert  # Fastmap takes 2 pebs
>  rmmod ubi
>  modprobe ubi mtd="0,0" fm_autoconvert  # Attach by fastmap
> 
> Add all used fastmap pebs into list 'ai->fastmap' to make sure they can
> be counted into 'found_pebs'.
> 
> Fixes: fdf10ed710c0aa ("ubi: Rework Fastmap attach base code")
> Signed-off-by: Zhihao Cheng <chengzhihao1@xxxxxxxxxx>
> ---
> drivers/mtd/ubi/fastmap.c | 35 +++++------------------------------
> 1 file changed, 5 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
> index 6b5f1ffd961b..01dcdd94c9d2 100644
> --- a/drivers/mtd/ubi/fastmap.c
> +++ b/drivers/mtd/ubi/fastmap.c
> /**
>  * ubi_scan_fastmap - scan the fastmap.
>  * @ubi: UBI device object
> @@ -865,7 +847,6 @@ int ubi_scan_fastmap(struct ubi_device *ubi, struct
> ubi_attach_info *ai,
> 	struct ubi_vid_hdr *vh;
> 	struct ubi_ec_hdr *ech;
> 	struct ubi_fastmap_layout *fm;
> -	struct ubi_ainf_peb *aeb;
> 	int i, used_blocks, pnum, fm_anchor, ret = 0;
> 	size_t fm_size;
> 	__be32 crc, tmp_crc;
> @@ -875,17 +856,6 @@ int ubi_scan_fastmap(struct ubi_device *ubi, struct
> ubi_attach_info *ai,
> 	if (fm_anchor < 0)
> 		return UBI_NO_FASTMAP;
> 
> -	/* Copy all (possible) fastmap blocks into our new attach structure. */
> -	list_for_each_entry(aeb, &scan_ai->fastmap, u.list) {
> -		struct ubi_ainf_peb *new;
> -
> -		new = clone_aeb(ai, aeb);
> -		if (!new)
> -			return -ENOMEM;
> -
> -		list_add(&new->u.list, &ai->fastmap);
> -	}
> -

scan_ai->fastmap may contain also old fastmap PEBs.
In the area < UBI_FM_MAX_START you can find outdated fastmap PEBs.
e.g. after power-cut.
That's why scan_ai->fastmap is copied into ai->fastmap.
Later in ubi_wl_init() these outdated PEBs will get erased.
So, you cannot remove this code.

But I fully agree with you that the fm->used_blocks > 1 case is not correct.
I fear if scan_ai->fastmap contains old fastmap PEBs and fm->used_blocks is > 1
we need to fall back to scanning mode while attaching.

Thanks,
//richard