[PATCH 1/5] UBI: fastmap: add more TODOs

[Richard Weinberger]

Am 05.06.2012 17:11, schrieb Artem Bityutskiy:
> From: Artem Bityutskiy <artem.bityutskiy at linux.intel.com>
> 
> I've spent 10 minutes looking at the code and added few TODOs. Many of them are
> nit-picks, though, but I'd like them to be fixed - should not be difficult.
> Some are more than just nit-picks.
> 
> Signed-off-by: Artem Bityutskiy <artem.bityutskiy at linux.intel.com>
> ---
>  TODO                      |    1 -
>  drivers/mtd/ubi/attach.c  |   21 +++++++++++++++------
>  drivers/mtd/ubi/fastmap.c |   26 ++++++++++++++++++++++++++
>  3 files changed, 41 insertions(+), 7 deletions(-)
> 
> diff --git a/TODO b/TODO
> index 63a22b9..17e30b6 100644
> --- a/TODO
> +++ b/TODO
> @@ -9,4 +9,3 @@ to the ubi-utils.git repository, to a separate branch at the beginning
>     test UBI + fastmap with it.
>  3. Test the autoresize feature
>  4. Test 'ubi_flush()'
> -5. Test
> diff --git a/drivers/mtd/ubi/attach.c b/drivers/mtd/ubi/attach.c
> index acf7db3..2a0c1ba 100644
> --- a/drivers/mtd/ubi/attach.c
> +++ b/drivers/mtd/ubi/attach.c
> @@ -1228,12 +1228,21 @@ int ubi_attach(struct ubi_device *ubi)
>  	} else if (err < 0)
>  		return err;
>  
> -	/* TODO: When you create an image with ubinize - you do not know the
> -	 * amount of PEBs. So you need to initialize this field with '-1' at
> -	 * ubinize time. And here you need to check for -1 and initialize it if
> -	 * needed. Then store it at fastmap. This special value has to be also
> -	 * documented at ubi-media.h. You also have to amend 'nused' etc.
> -	 * Probably this can be done later. */
> +	/* TODO: currently the fastmap code assumes that the fastmap data
> +	 * structures are created only by the kernel when the kernel attaches
> +	 * an fastmap-less image. However, this assumption is too limiting and
> +	 * for sure people will want to pre-create UBI images with fastmap
> +	 * using the ubinize tool. Then they wont have to waste a lot of time
> +	 * waiting for full scan and fastmap initializetion during the first
> +	 * boot. This is a very important feature for the factory production
> +	 * line where every additional minute per device costs a lot.
> +	 *
> +	 * When you are attaching an MTD device which contains an image
> +	 * generated by ubinize with a fastmap, you will not know the
> +	 * 'bad_peb_count' value. Most probably it will contain something like
> +	 * -1. The same is true for the per-PEB information in the fastmap - it
> +	 * won't tell which PEBs are bad. So we need to detect this and iterate
> +	 * over all PEBs, find out which are bad, and update 'ai' here. */

I'm confused to find bad PEBs I'd still need a full scan, right?

>  	ubi->bad_peb_count = ai->bad_peb_count;
>  	ubi->good_peb_count = ubi->peb_count - ubi->bad_peb_count;
>  	ubi->corr_peb_count = ai->corr_peb_count;
> diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
> index a8143da..09629c2 100644
> --- a/drivers/mtd/ubi/fastmap.c
> +++ b/drivers/mtd/ubi/fastmap.c
> @@ -718,6 +718,17 @@ out:
>   * ubi_scan_fastmap - scan the fastmap
>   * @ubi: UBI device object
>   * @ai: UBI attach info to be filled
> + *
> + * TODO: not urgent, but at some point - check the code with kernel doc and fix
> + * its complaints.

Okay.

> + * TODO: not urgent, but for consistency, follow the UBI/UBIFS style and put a
> + * dot at the end of the first short description sentence (globally):
> + *    ubi_scan_fastmap - scan the fastmap. (<-dot).

Will fix!

> + * TODO: not urgent, but it is desireble to document error codes in the header
> + * comments and probably describe what the function does, if there is something
> + * to say (globally).
>   */
>  int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info *ai)
>  {
> @@ -744,8 +755,13 @@ int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info *ai)
>  		goto out;
>  	}
>  
> +	/* TODO: If 'ubi_io_read()' returns you UBI_IO_BITFLIP, this means that
> +	 * the PEB has a bit-flip and has to be scrubbed. How will the
> +	 * superblock be scrubbed or how is the bit-flip guaranteed to be taken
> +	 * care of? */

At this stage it's a bit difficult.
But I can add this information to the in-memory fastmap structure such that the PEB will be
scrubbed upon it's returned to the WL sub-system.

>  	ret = ubi_io_read(ubi, fmsb, sb_pnum, ubi->leb_start, sizeof(*fmsb));
>  	if (ret && ret != UBI_IO_BITFLIPS) {
> +		/* TODO: what are the error codes > 0 ? Why is this check? */

To catch UBI_IO_BAD_HDR, UBI_IO_BAD_HDR_EBADMSG and friends.
If we fail to read from a fastmap PEB we have to fail with UBI_BAD_FASTMAP.

>  		if (ret > 0)
>  			ret = UBI_BAD_FASTMAP;
>  
> @@ -754,7 +770,17 @@ int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info *ai)
>  		goto out;
>  	}
>  
> +	/* TODO: please, use 'be32_to_cpu()' _every_ time you access a __be32 /
> +	 * etc field. Please, look how things are done in io.c. Please, check
> +	 * and fix globally. */

AFAIK ->magic is the only __b32 field where I not use be32_to_cpu() because it's useless here.
The magic value will be always the same. (It's fixed)
I know UBI does also use be32_to_cpu for the EC and HDR magic.
Anyway, I'll change it. :-)

>  	if (fmsb->magic != UBI_FM_SB_MAGIC) {
> +		/* TODO: not urgent, but examine all the error messages and
> +		 * print more information there. Here you should print what was
> +		 * read and what was expected. See io.c and do similarly or
> +		 * better.
> +		 * Please, change globally. E.g., when you print about bad
> +		 * version - print what was expected and what was actually
> +		 * found. */

Okay, this is a good idea!

Thanks,
//richard

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 490 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-mtd/attachments/20120606/6011ec2d/attachment.sig>