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

From: Artem Bityutskiy <artem.bityutskiy@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@linux.intel.com>
---
 TODO                      |    1 -
 drivers/mtd/ubi/attach.c  |   21 +++++++++++++++------
 drivers/mtd/ubi/fastmap.c |   26 ++++++++++++++++++++++++++
 3 files changed, 41 insertions(+), 7 deletions(-)

Am 05.06.2012 17:11, schrieb Artem Bityutskiy:
> From: Artem Bityutskiy <artem.bityutskiy@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@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

On Wed, 2012-06-06 at 23:30 +0200, Richard Weinberger wrote:
> I'm confused to find bad PEBs I'd still need a full scan, right?

If full scan means call mtd_block_isbad() for each PEB, then yes.

> >  	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.

Which are never returned by ubi_io_read().

> AFAIK ->magic is the only __b32 field where I not use be32_to_cpu() because it's useless here.

Why is it useless?

if we have the following bytes starting from address 0xC0000000:
7B 11 D6 9F

then we do:

unsigned int *p = 0xC0000000;
printk("%#x\n", *p);

we should see:

0x7B11D69F on BE system (e.g., powerpc)
and
0x9FD6117B on LE system (e.g., x86).

On Thu, 2012-06-07 at 01:46 +0200, Richard Weinberger wrote:
> >> To catch UBI_IO_BAD_HDR, UBI_IO_BAD_HDR_EBADMSG and friends.
> > 
> > Which are never returned by ubi_io_read().
> 
> Also never ever in future?
> If so, I'll drop this check. 

Well, if you read those constants they suggest that they are specific to
functions which read the UBI headers. ubi_io_read() function is just a
general read function which is just a simple wrapper over mtd_read()
plus re-trying, nice error messaging, debugging prints, some error codes
handling, nothing else.

So no, it is not going to ever check UBI headers and return those codes.

/me goes to sleep.

Am 07.06.2012 01:38, schrieb Artem Bityutskiy:
> On Wed, 2012-06-06 at 23:30 +0200, Richard Weinberger wrote:
>> I'm confused to find bad PEBs I'd still need a full scan, right?
> 
> If full scan means call mtd_block_isbad() for each PEB, then yes.
> 
>>>  	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.
> 
> Which are never returned by ubi_io_read().

Also never ever in future?
If so, I'll drop this check.

>> AFAIK ->magic is the only __b32 field where I not use be32_to_cpu() because it's useless here.
> 
> Why is it useless?
> 
> if we have the following bytes starting from address 0xC0000000:
> 7B 11 D6 9F
> 
> then we do:
> 
> unsigned int *p = 0xC0000000;
> printk("%#x\n", *p);
> 
> we should see:
> 
> 0x7B11D69F on BE system (e.g., powerpc)
> and
> 0x9FD6117B on LE system (e.g., x86).
> 

Damit, you are right!
Sorry,
//richard