Re: UBI fastmap updates

[Richard Weinberger <richard@nod.at>]

[-- Attachment #1: Type: text/plain, Size: 3814 bytes --]

Hi Shmulik!

Am 08.07.2012 13:47, schrieb Shmulik Ladkani:
> +
> +		/* TODO: if find_fastmap==1, we do not enter this block at all.
> +		 * shouldn't we? shouldn't we care of compatability of unknown
> +		 * internal volumes OTHER than the fastmap ones, even if
> +		 * find_fastmap==1?
> +		 */
> +

If find_fastmap=1 we scan only the first 64 PEBs (now by using scan_peb()).
When using fastmap can do not care about compatibility of unknown internal volumes
at all.
Fastmap keeps only track of known (and compatible volumes).
Taking care of unknown internal volumes would imply a full scan.

		int lnum = be32_to_cpu(vidh->lnum);
>  
>  		/* Unsupported internal volume */
> @@ -1221,6 +1228,7 @@ static void destroy_ai(struct ubi_attach_info *ai)
>   * scan_all - scan entire MTD device.
>   * @ubi: UBI device description object
>   * @ai: attach info object
> + * TODO: document @start

Tomorrow I'll send another kernel-doc update.
There more tags missing.

>   * This function does full scanning of an MTD device and returns complete
>   * information about it in form of a "struct ubi_attach_info" object. In case
> @@ -1350,6 +1358,11 @@ out_vidh:
>  out_ech:
>  	kfree(ech);
>  out_ai:
> +	/* TODO: doesn't seem clean to destroy 'ai' as it was allocated by the
> +	 * caller, its his responsibility.
> +	 * also looks like it leads to double freee in case 'err' returned is
> +	 * negative
> +	 */

I have to look closer into this.
It looks like the call to destroy_ai() in scan_fast() has to go.

>  	destroy_ai(ai);
>  	return err;
>  }
> @@ -1441,6 +1454,7 @@ int ubi_attach(struct ubi_device *ubi, int force_scan)
>  
>  		err = scan_all(ubi, scan_ai, 0);
>  		if (err) {
> +			/* TODO: hmm... kfree or destroy_ai ? */

True. Must be desroy_ai().

>  			kfree(scan_ai);
>  			goto out_wl;
>  		}
> diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
> index 50b7590..f769c22 100644
> --- a/drivers/mtd/ubi/build.c
> +++ b/drivers/mtd/ubi/build.c
> @@ -1053,6 +1053,7 @@ int ubi_detach_mtd_dev(int ubi_num, int anyway)
>  	ubi_notify_all(ubi, UBI_VOLUME_REMOVED, NULL);
>  	dbg_msg("detaching mtd%d from ubi%d", ubi->mtd->index, ubi_num);
>  
> +	/* TODO: any action on failure? */

What do you expect on failure?
At this point we have either no fastmap or a valid fastmap on flash.
If ubi_update_fastmap() fails it will ensure that no invalid fastmap remains on flash.
See invalidate_fastmap().

> --- a/drivers/mtd/ubi/wl.c
> +++ b/drivers/mtd/ubi/wl.c
> @@ -272,6 +272,10 @@ static int produce_free_peb(struct ubi_device *ubi)
>  		dbg_wl("do one work synchronously");
>  		err = do_work(ubi);
>  		if (err)
> +			/* TODO: in the new locking scheme, produce_free_peb is
> +			 * called under wl_lock taken.
> +			 * so when returning, should reacquire the lock
> +			 */

Which new locking scheme?

>  			return err;
>  
>  		spin_lock(&ubi->wl_lock);
> @@ -377,6 +381,7 @@ static struct ubi_wl_entry *find_wl_entry(struct ubi_device *ubi,
>  	 * as anchor PEB, hold it back and return the second best WL entry
>  	 * such that fastmap can use the anchor PEB later. */
>  	if (!ubi->fm_disabled && !ubi->fm && e->pnum < UBI_FM_MAX_START)
> +		/* TODO: do we have a risk returning NULL here? */

How?

>  		return prev_e;
>  
>  	return e;
> @@ -405,6 +410,7 @@ static struct ubi_wl_entry *find_mean_wl_entry(struct ubi_device *ubi,
>  		/* If no fastmap has been written and this WL entry can be used
>  		 * as anchor PEB, hold it back and return the second best
>  		 * WL entry such that fastmap can use the anchor PEB later. */
> +		/* TODO: why this is specific just to < WL_FREE_MAX_DIFF case? */

Because find_wl_entry() already takes care of this.

Thanks,
//richard


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]