[PATCH 0/5] UBI: fastmap: add few todos

[PATCH 0/5] UBI: fastmap: add few todos

[Artem Bityutskiy]

From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>

Hi Richard,

thanks for the patches. I have am sorry for bugging you with my nitpicks, but
I would read your code and review it with much more pleasure if you could bear
with me and address them. It should not be too difficult to do. Here are some
patches with a bunch of my nit-picks mostly about coding style, but there are
also few serious things like ignoring endiannes.

I've pushed these to the fastmap branch. Please, complain of send patches on
top.

Artem.

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

[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. */
 	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.
+ *
+ * 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).
+ *
+ * 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? */
 	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? */
 		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. */
 	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. */
 		ubi_err("super block magic does not match");
 		ret = UBI_BAD_FASTMAP;
 		kfree(fmsb);
-- 
1.7.10

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

[Richard Weinberger]

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

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


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

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

[Artem Bityutskiy]

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

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

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Richard Weinberger]

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

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


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

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

[Artem Bityutskiy]

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

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.

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH 2/5] UBI: fastmap: kill junk newlines and add a TODO about that

[Artem Bityutskiy]

From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>

Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
---
 drivers/mtd/ubi/fastmap.c |   45 ++++++++++++++++++++++++---------------------
 1 file changed, 24 insertions(+), 21 deletions(-)

diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
index 09629c2..b2ee872 100644
--- a/drivers/mtd/ubi/fastmap.c
+++ b/drivers/mtd/ubi/fastmap.c
@@ -751,7 +751,6 @@ int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info *ai)
 	fmsb = kmalloc(sizeof(*fmsb), GFP_KERNEL);
 	if (!fmsb) {
 		ret = -ENOMEM;
-
 		goto out;
 	}
 
@@ -764,9 +763,7 @@ int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info *ai)
 		/* TODO: what are the error codes > 0 ? Why is this check? */
 		if (ret > 0)
 			ret = UBI_BAD_FASTMAP;
-
 		kfree(fmsb);
-
 		goto out;
 	}
 
@@ -784,7 +781,6 @@ int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info *ai)
 		ubi_err("super block magic does not match");
 		ret = UBI_BAD_FASTMAP;
 		kfree(fmsb);
-
 		goto out;
 	}
 
@@ -792,17 +788,14 @@ int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info *ai)
 		ubi_err("unknown fastmap format version!");
 		ret = UBI_BAD_FASTMAP;
 		kfree(fmsb);
-
 		goto out;
 	}
 
 	used_blocks = be32_to_cpu(fmsb->used_blocks);
-
 	if (used_blocks > UBI_FM_MAX_BLOCKS || used_blocks < 1) {
 		ubi_err("number of fastmap blocks is invalid");
 		ret = UBI_BAD_FASTMAP;
 		kfree(fmsb);
-
 		goto out;
 	}
 
@@ -812,7 +805,6 @@ int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info *ai)
 	if (!fm_raw) {
 		ret = -ENOMEM;
 		kfree(fmsb);
-
 		goto out;
 	}
 
@@ -820,7 +812,6 @@ int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info *ai)
 	if (!ech) {
 		ret = -ENOMEM;
 		kfree(fmsb);
-
 		goto free_raw;
 	}
 
@@ -829,7 +820,6 @@ int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info *ai)
 		ret = -ENOMEM;
 		kfree(fmsb);
 		kfree(ech);
-
 		goto free_raw;
 	}
 
@@ -839,7 +829,6 @@ int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info *ai)
 		if (ubi_io_is_bad(ubi, pnum)) {
 			ret = UBI_BAD_FASTMAP;
 			kfree(fmsb);
-
 			goto free_hdr;
 		}
 
@@ -850,7 +839,6 @@ int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info *ai)
 			if (ret > 0)
 				ret = UBI_BAD_FASTMAP;
 			kfree(fmsb);
-
 			goto free_hdr;
 		}
 
@@ -860,7 +848,6 @@ int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info *ai)
 		if (be32_to_cpu(ech->image_seq) != ubi->image_seq) {
 			ret = UBI_BAD_FASTMAP;
 			kfree(fmsb);
-
 			goto free_hdr;
 		}
 
@@ -871,7 +858,6 @@ int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info *ai)
 			if (ret > 0)
 				ret = UBI_BAD_FASTMAP;
 			kfree(fmsb);
-
 			goto free_hdr;
 		}
 
@@ -886,7 +872,6 @@ int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info *ai)
 			if (be32_to_cpu(vh->vol_id) != UBI_FM_DATA_VOLUME_ID) {
 				ret = UBI_BAD_FASTMAP;
 				kfree(fmsb);
-
 				goto free_hdr;
 			}
 		}
@@ -903,7 +888,6 @@ int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info *ai)
 			if (ret > 0)
 				ret = UBI_BAD_FASTMAP;
 			kfree(fmsb);
-
 			goto free_hdr;
 		}
 	}
@@ -927,14 +911,12 @@ int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info *ai)
 	if (ret) {
 		if (ret > 0)
 			ret = UBI_BAD_FASTMAP;
-
 		goto free_hdr;
 	}
 
 	ai->fm = kzalloc(sizeof(*ai->fm), GFP_KERNEL);
 	if (!ai->fm) {
 		ret = -ENOMEM;
-
 		goto free_hdr;
 	}
 
@@ -952,12 +934,11 @@ int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info *ai)
 			kfree(ai->fm);
 			ai->fm = NULL;
 			ret = -ENOMEM;
-
 			goto free_hdr;
 		}
+
 		e->pnum = be32_to_cpu(fmsb->block_loc[i]);
 		e->ec = be32_to_cpu(fmsb->block_ec[i]);
-
 		ai->fm->e[i] = e;
 	}
 
@@ -969,7 +950,6 @@ free_raw:
 out:
 	if (ret == UBI_BAD_FASTMAP)
 		ubi_err("Attach by fastmap failed, doing a full scan!");
-
 	return ret;
 }
 
@@ -1005,6 +985,29 @@ static int ubi_write_fastmap(struct ubi_device *ubi,
 	if (!fm_raw) {
 		ret = -ENOMEM;
 
+		/* TODO: nitpick, sorry for being annoying, but this should not
+		 * be difficult to fix. I find it very irritating when there
+		 * are useless blank lines like this - they only make less code
+		 * fit my screen and make large functions even larger.
+		 *
+		 * Why we use blank lines inside a single function? To make
+		 * code more readable. How we make it more readable? By
+		 * separating logically different blocks of code with a
+		 * newline.
+		 *
+		 * What is the perbose of these newlines before goto's? This is
+		 * a single piece of error-handling code - you assing the
+		 * return value and go to the further error processing. These
+		 * newlines serve no purpose just blow the lines number in the
+		 * code. Could we please kill them globally?
+		 *
+		 * I am again sorry - probably this is not completely
+		 * technical, but I gave the explanation why these newlines are
+		 * annying. And as a person who spent a lot of personal
+		 * (non-paid) time maintaining this code and who will keep
+		 * doing this - I think I have right to require to do such
+		 * cosmetic things :-) If you do not agree with my reasoning,
+		 * may be this is better - you'll keep me happier :-) */
 		goto out;
 	}
 
-- 
1.7.10

Re: [PATCH 2/5] UBI: fastmap: kill junk newlines and add a TODO about that

[Richard Weinberger]

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

Am 05.06.2012 17:11, schrieb Artem Bityutskiy:
> +		/* TODO: nitpick, sorry for being annoying, but this should not
> +		 * be difficult to fix. I find it very irritating when there
> +		 * are useless blank lines like this - they only make less code
> +		 * fit my screen and make large functions even larger.
> +		 *
> +		 * Why we use blank lines inside a single function? To make
> +		 * code more readable. How we make it more readable? By
> +		 * separating logically different blocks of code with a
> +		 * newline.
> +		 *
> +		 * What is the perbose of these newlines before goto's? This is
> +		 * a single piece of error-handling code - you assing the
> +		 * return value and go to the further error processing. These
> +		 * newlines serve no purpose just blow the lines number in the
> +		 * code. Could we please kill them globally?
> +		 *
> +		 * I am again sorry - probably this is not completely
> +		 * technical, but I gave the explanation why these newlines are
> +		 * annying. And as a person who spent a lot of personal
> +		 * (non-paid) time maintaining this code and who will keep
> +		 * doing this - I think I have right to require to do such
> +		 * cosmetic things :-) If you do not agree with my reasoning,
> +		 * may be this is better - you'll keep me happier :-) */
>  		goto out;

Your and mine coding style are quite different. :-)
I *hate* it when statements like goto and return are glued to a code block.
E.g.

a = 1 + b;
res = c + a;

return res;

vs:

a = 1 + b;
res = c + a;
return res;

Anyway, I'll change this to match the UBI style.

Thanks,
//richard

P.s: Can we please have a ubi_checkpatch.pl? *SCNR*


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

Re: [PATCH 2/5] UBI: fastmap: kill junk newlines and add a TODO about that

[Artem Bityutskiy]

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

On Wed, 2012-06-06 at 23:30 +0200, Richard Weinberger wrote:
> a = 1 + b;
> res = c + a;
> 
> return res;
> 
> vs:
> 
> a = 1 + b;
> res = c + a;
> return res;

Sorry, but I think the above example is not quite suitable because it
looks too general, while I was asking only about a specific thing, not
for any return or goto.

ret = func();
if (ret) {
	free(x);

	goto out;
}

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH 3/5] UBI: fastmap: more nitpicks

[Artem Bityutskiy]

From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>

Again, I feel that this may be annoing for others, but it will make me happier
if I see UBI code having consistent style. After all, people come and go but I
stay maintaining this stuff so I want reading the code to be pleasant for me.
So please, bear with my strange requirements, I will not add this long
explanation anymore, ok? :-)

Anyway, in UBI/UBIFS we try to put identifiers of the same type on one line, if
it fits them.

Also, we do not add blank lines to the identifiers declaration block. If your
function is so huge that it has too many local variables and you need to group
it - fix the function - split it instead.

I've also added a TODO about this. If you could change this globally - it would
make me happier and I'd read your code with more pleasure.

Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
---
 drivers/mtd/ubi/fastmap.c |   14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
index b2ee872..e6900e4 100644
--- a/drivers/mtd/ubi/fastmap.c
+++ b/drivers/mtd/ubi/fastmap.c
@@ -447,6 +447,7 @@ static int ubi_attach_fastmap(struct ubi_device *ubi,
 {
 	struct list_head used;
 	struct list_head eba_orphans;
+	/* TODO: please, try to declare variables of the same time on one line */
 	struct ubi_ainf_volume *av;
 	struct ubi_ainf_peb *aeb, *tmp_aeb, *_tmp_aeb;
 	struct ubi_ec_hdr *ech;
@@ -458,6 +459,8 @@ static int ubi_attach_fastmap(struct ubi_device *ubi,
 	struct ubi_fm_volhdr *fmvhdr;
 	struct ubi_fm_eba *fm_eba;
 
+	/* TODO: no blank lines in the local variable declaration block
+	 * please. */
 	int ret, i, j;
 	size_t fm_pos = 0;
 	unsigned long long max_sqnum = 0;
@@ -735,8 +738,7 @@ int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info *ai)
 	struct ubi_fm_sb *fmsb;
 	struct ubi_vid_hdr *vh;
 	struct ubi_ec_hdr *ech;
-	int ret, i, used_blocks, pnum;
-	int sb_pnum = 0;
+	int ret, i, used_blocks, pnum, sb_pnum = 0;
 	char *fm_raw;
 	size_t fm_size;
 	__be32 crc, tmp_crc;
@@ -961,25 +963,19 @@ out:
 static int ubi_write_fastmap(struct ubi_device *ubi,
 			     struct ubi_fastmap_layout *new_fm)
 {
-	int ret;
 	size_t fm_pos = 0;
 	char *fm_raw;
-	int i, j;
-
 	struct ubi_fm_sb *fmsb;
 	struct ubi_fm_hdr *fmh;
 	struct ubi_fm_scan_pool *fmpl;
 	struct ubi_fm_ec *fec;
 	struct ubi_fm_volhdr *fvh;
 	struct ubi_fm_eba *feba;
-
 	struct rb_node *node;
 	struct ubi_wl_entry *wl_e;
 	struct ubi_volume *vol;
-
 	struct ubi_vid_hdr *avhdr, *dvhdr;
-
-	int free_peb_count, used_peb_count, vol_count;
+	int ret, i, j, free_peb_count, used_peb_count, vol_count;
 
 	fm_raw = vzalloc(new_fm->size);
 	if (!fm_raw) {
-- 
1.7.10

Re: [PATCH 3/5] UBI: fastmap: more nitpicks

[Richard Weinberger]

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

Am 05.06.2012 17:11, schrieb Artem Bityutskiy:
> From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
> 
> Again, I feel that this may be annoing for others, but it will make me happier
> if I see UBI code having consistent style. After all, people come and go but I
> stay maintaining this stuff so I want reading the code to be pleasant for me.
> So please, bear with my strange requirements, I will not add this long
> explanation anymore, ok? :-)

No problem. :-)

Thanks,
//richard


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

[PATCH 4/5] UBI: fastmap: more annoying TODOs

[Artem Bityutskiy]

From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>

Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
---
 drivers/mtd/ubi/fastmap.c |   14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
index e6900e4..f938507 100644
--- a/drivers/mtd/ubi/fastmap.c
+++ b/drivers/mtd/ubi/fastmap.c
@@ -356,6 +356,14 @@ static int scan_pool(struct ubi_device *ubi, struct ubi_attach_info *ai,
 
 		err = ubi_io_read_vid_hdr(ubi, pnum, vh, 0);
 
+		/* TODO: so here again - calling the function and checking what
+		 * it returns I consider to be one logical block which should
+		 * not contain newlines inbetween. You are inconsistent about
+		 * this and in some places you have a newline, in some you
+		 * don't. Could we please remove them globally in cases like
+		 * this?
+		 * Again my disclaimer: sorry, I hope this is not too annoying
+		 * for you. */
 		if (err == UBI_IO_FF || err == UBI_IO_FF_BITFLIPS)
 			continue;
 		else if (err == 0 || err == UBI_IO_BITFLIPS) {
@@ -394,6 +402,12 @@ static int scan_pool(struct ubi_device *ubi, struct ubi_attach_info *ai,
 				list_del(&tmp_aeb->u.list);
 			}
 
+			/* TODO: could you please follow UBI style of how we
+			 * split lines? Notice that we aling arguments WRT the
+			 * bracket. We use few tabs, and then at the and use
+			 * few spaces for fine-tuning the alignment. Please,
+			 * let's be consistent. Take a look at any UBI file for
+			 * example. */
 			new_aeb = kmem_cache_alloc(ai->aeb_slab_cache,
 				GFP_KERNEL);
 			if (!new_aeb) {
-- 
1.7.10

[PATCH 5/5] UBI: fastmap: more tiny TODOs

[Artem Bityutskiy]

From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>

Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
---
 TODO                      |    2 ++
 drivers/mtd/ubi/fastmap.c |    7 +++++++
 2 files changed, 9 insertions(+)

diff --git a/TODO b/TODO
index 17e30b6..a944159 100644
--- a/TODO
+++ b/TODO
@@ -9,3 +9,5 @@ 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 that the same UBI image works fine on both LE and BE machines. I guess
+   we can do this using sime kind of emulators?
diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
index f938507..d446fc3 100644
--- a/drivers/mtd/ubi/fastmap.c
+++ b/drivers/mtd/ubi/fastmap.c
@@ -455,6 +455,9 @@ out:
  * @fm_raw: the fastmap it self as byte array
  * @fm_size: size of the fastmap in bytes
  */
+/* TODO: please, make all pointers like 'fm_raw' of type void. It is indeed a
+ * pointer to data blob, it is not a pointer to a string of characters. Note,
+ * that in pointer arithmetics void * is the same as char *. */
 static int ubi_attach_fastmap(struct ubi_device *ubi,
 			      struct ubi_attach_info *ai,
 			      char *fm_raw, size_t fm_size)
@@ -503,6 +506,10 @@ static int ubi_attach_fastmap(struct ubi_device *ubi,
 	if (fm_pos >= fm_size)
 		goto fail_bad;
 
+	/* TODO: this is difficult to read. Can we please have instead an
+	 * aggregate data structure? I did not think hard on it may be you have
+	 * a good reason for this difficult style, but on the first glance it
+	 * does not look like. And where are all the endiness stuff?  */
 	fmhdr = (struct ubi_fm_hdr *)(fm_raw + fm_pos);
 	fm_pos += sizeof(*fmhdr);
 	if (fm_pos >= fm_size)
-- 
1.7.10

Re: [PATCH 5/5] UBI: fastmap: more tiny TODOs

[Richard Weinberger]

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

Am 05.06.2012 17:11, schrieb Artem Bityutskiy:
> From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
> 
> Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
> ---
>  TODO                      |    2 ++
>  drivers/mtd/ubi/fastmap.c |    7 +++++++
>  2 files changed, 9 insertions(+)
> 
> diff --git a/TODO b/TODO
> index 17e30b6..a944159 100644
> --- a/TODO
> +++ b/TODO
> @@ -9,3 +9,5 @@ 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 that the same UBI image works fine on both LE and BE machines. I guess
> +   we can do this using sime kind of emulators?

Okay, I'll test the same image on x86 and ppc.

> diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
> index f938507..d446fc3 100644
> --- a/drivers/mtd/ubi/fastmap.c
> +++ b/drivers/mtd/ubi/fastmap.c
> @@ -455,6 +455,9 @@ out:
>   * @fm_raw: the fastmap it self as byte array
>   * @fm_size: size of the fastmap in bytes
>   */
> +/* TODO: please, make all pointers like 'fm_raw' of type void. It is indeed a
> + * pointer to data blob, it is not a pointer to a string of characters. Note,
> + * that in pointer arithmetics void * is the same as char *. */

I know that char * and void * are equivalent.
That's why I've chosen char *.
IMHO a blob is a string of chars. (Of course it's not a C sting).
Anyway, if you prefer void * I'll change it. :-)

>  static int ubi_attach_fastmap(struct ubi_device *ubi,
>  			      struct ubi_attach_info *ai,
>  			      char *fm_raw, size_t fm_size)
> @@ -503,6 +506,10 @@ static int ubi_attach_fastmap(struct ubi_device *ubi,
>  	if (fm_pos >= fm_size)
>  		goto fail_bad;
>  
> +	/* TODO: this is difficult to read. Can we please have instead an
> +	 * aggregate data structure? I did not think hard on it may be you have
> +	 * a good reason for this difficult style, but on the first glance it
> +	 * does not look like. And where are all the endiness stuff?  */

Yes, it's difficult to read but an aggregate data structure is not really doable.
Please let me explain why:

The fastmap data blob has not a fixed length and it's internal field don't have fixed positions.

1 struct ubi_fm_sb followed by
1 struct ubi_fm_hdr followed by
1 struct ubi_fm_scan_pool followed by
free_peb_count+used_peb_count struct ubi_fm_ec followed by
vol_count (
 struct ubi_fm_volhdr
 struct ubi_fm_eba
)

You see that only the first three structs have fixed positions.
So, the whole blob cannot be mapped into one aggregate data structure.
And I really don't want to play nasty games with variable-length arrays in stucts...

Why do you mean by "where are all the endiness stuff"?
The position calculation takes care about endiness.

Thanks,
//richard


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