[U-Boot] [PATCH v5 3/5] mtdparts: show net size in mtdparts list
This patch adds an additional column to the output of list_partitions. The additional column will contain the net size and a '(!)' beside it if the net size is not equal to the partition size.
Signed-off-by: Ben Gardiner bengardiner@nanometrics.ca CC: Wolfgang Denk wd@denx.de CC: Scott Wood scottwood@freescale.com ---
V2: * formatting: spaces after 'if' and 'for' * the entire new feature is conditional on a macro, there is now a zero-byte binary size impact when the macro is not defined. * return the net parition size directly from net_part_size instead of using an output variable
V3: * rebased to 54841ab50c20d6fa6c9cc3eb826989da3a22d934 of git://git.denx.de/u-boot.git * fix line length over 80 chars * update copyright of cmd_mtdparts.c
V4: * rebased to b417260d871d4d8d336c160d95ed40cc8c0fb0fa of git://git.denx.de/u-boot.git * removed copyright statement and changelog from file header * re-grouped list_partition #ifdefs into one * fixed multi-line comment style
V5: * rebased to 962ad59e25640e586e2bceabf67a628a27f8f508 of git://git.denx.de/u-boot.git * renumbered from 2/4 to 3/5 * return uint64_t instead of u32 for net_size * do a quick if((cond) return * calculate net_size by adding-up good blocks instead of subtracting bad blocks * try to strike a balance; reuse more code between the branches of #if defined(CONFIG_CMD_MTDPARTS_SHOW_NET_SIZES) in print_partition_table --- common/cmd_mtdparts.c | 74 +++++++++++++++++++++++++++++++++++++++++++----- drivers/mtd/mtdcore.c | 5 ++- 2 files changed, 69 insertions(+), 10 deletions(-)
diff --git a/common/cmd_mtdparts.c b/common/cmd_mtdparts.c index 772ad54..a8912ed 100644 --- a/common/cmd_mtdparts.c +++ b/common/cmd_mtdparts.c @@ -1215,38 +1215,96 @@ static int generate_mtdparts_save(char *buf, u32 buflen) return ret; }
+#if defined(CONFIG_CMD_MTDPARTS_SHOW_NET_SIZES) /** - * Format and print out a partition list for each device from global device - * list. + * Get the net size (w/o bad blocks) of the given partition. + * + * @param mtd the mtd info + * @param part the partition + * @return the calculated net size of this partition */ -static void list_partitions(void) +static uint64_t net_part_size(struct mtd_info *mtd, struct part_info *part) +{ + uint64_t gross_size, trailing_bad_size = 0; + int truncated = 0; + + mtd_get_len_incl_bad(mtd, part->offset, part->size, &gross_size, + &truncated); + + if (!truncated) { + mtd_get_len_incl_bad(mtd, part->offset + part->size, + mtd->erasesize, &trailing_bad_size, + &truncated); + trailing_bad_size -= mtd->erasesize; + } + + return part->size - (gross_size - trailing_bad_size - part->size); +} +#endif + +static void print_partition_table(void) { struct list_head *dentry, *pentry; struct part_info *part; struct mtd_device *dev; int part_num;
- debug("\n---list_partitions---\n"); - list_for_each(dentry, &devices) { +list_for_each(dentry, &devices) { dev = list_entry(dentry, struct mtd_device, link); + /* list partitions for given device */ + part_num = 0; +#if defined(CONFIG_CMD_MTDPARTS_SHOW_NET_SIZES) + struct mtd_info *mtd; + + if (get_mtd_info(dev->id->type, dev->id->num, &mtd)) + return; + + printf("\ndevice %s%d <%s>, # parts = %d\n", + MTD_DEV_TYPE(dev->id->type), dev->id->num, + dev->id->mtd_id, dev->num_parts); + printf(" #: name\t\tsize\t\tnet size\toffset\t\tmask_flags\n"); + + list_for_each(pentry, &dev->parts) { + u32 net_size; + char *size_note; + + part = list_entry(pentry, struct part_info, link); + net_size = net_part_size(mtd, part); + size_note = part->size == net_size ? " " : " (!)"; + printf("%2d: %-20s0x%08x\t0x%08x%s\t0x%08x\t%d\n", + part_num, part->name, part->size, + net_size, size_note, part->offset, + part->mask_flags); +#else /* !defined(CONFIG_CMD_MTDPARTS_SHOW_NET_SIZES) */ printf("\ndevice %s%d <%s>, # parts = %d\n", MTD_DEV_TYPE(dev->id->type), dev->id->num, dev->id->mtd_id, dev->num_parts); printf(" #: name\t\tsize\t\toffset\t\tmask_flags\n");
- /* list partitions for given device */ - part_num = 0; list_for_each(pentry, &dev->parts) { part = list_entry(pentry, struct part_info, link); printf("%2d: %-20s0x%08x\t0x%08x\t%d\n", part_num, part->name, part->size, part->offset, part->mask_flags); - +#endif /* defined(CONFIG_CMD_MTDPARTS_SHOW_NET_SIZES) */ part_num++; } } + if (list_empty(&devices)) printf("no partitions defined\n"); +} + +/** + * Format and print out a partition list for each device from global device + * list. + */ +static void list_partitions(void) +{ + struct part_info *part; + + debug("\n---list_partitions---\n"); + print_partition_table();
/* current_mtd_dev is not NULL only when we have non empty device list */ if (current_mtd_dev) { diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c index cb86657..211b993 100644 --- a/drivers/mtd/mtdcore.c +++ b/drivers/mtd/mtdcore.c @@ -143,7 +143,8 @@ void put_mtd_device(struct mtd_info *mtd) BUG_ON(c < 0); }
-#if defined(CONFIG_CMD_MTDPARTS_SPREAD) +#if defined(CONFIG_CMD_MTDPARTS_SPREAD) || \ + defined(CONFIG_CMD_MTDPARTS_SHOW_NET_SIZES) /** * mtd_get_len_incl_bad * @@ -185,4 +186,4 @@ void mtd_get_len_incl_bad(struct mtd_info *mtd, uint64_t offset, } } } -#endif /* defined(CONFIG_CMD_MTDPARTS_SPREAD) */ +#endif
On Mon, 30 Aug 2010 13:38:58 -0400 Ben Gardiner bengardiner@nanometrics.ca wrote:
+#if defined(CONFIG_CMD_MTDPARTS_SHOW_NET_SIZES) /**
- Format and print out a partition list for each device from global device
- list.
- Get the net size (w/o bad blocks) of the given partition.
- @param mtd the mtd info
- @param part the partition
*/
- @return the calculated net size of this partition
-static void list_partitions(void) +static uint64_t net_part_size(struct mtd_info *mtd, struct part_info *part) +{
- uint64_t gross_size, trailing_bad_size = 0;
- int truncated = 0;
- mtd_get_len_incl_bad(mtd, part->offset, part->size, &gross_size,
&truncated);- if (!truncated) {
mtd_get_len_incl_bad(mtd, part->offset + part->size,mtd->erasesize, &trailing_bad_size,&truncated);trailing_bad_size -= mtd->erasesize;- }
- return part->size - (gross_size - trailing_bad_size - part->size);
I'm not sure I follow the logic here...
You're trying to find net size given gross size, but you first treat the gross size as a net size and get the gross size of *that*.
If it was truncated, then you'll return a value that is still probably greater than the partition's true net size. For example, suppose you called this on the final partition, which includes at least one bad block (or the BBT, which is marked bad). mtd_get_len_incl_bad() will return the full partition size and set truncated. You'll end up with part->size - (part->size - 0 - part->size), which evaluates to part->size. The function should have returned something less than part->size.
If it was not truncated, suppose the partition had two bad blocks, and the next partition had its second block bad. mtd_get_len_incl_bad() will return part->size plus 3, since it ran into the next partition's bad block. The second call to mtd_get_len_incl_bad() will return one block, since it never got to the next partition's second block. Thus net_part_size() will return part->size - ((part->size + 3) - 0 - part->size), or part->size - 3. The right answer was part->size - 2.
I don't think a net-to-gross transformation is useful as a base for a gross-to-net transformation.
+} +#endif
+static void print_partition_table(void) { struct list_head *dentry, *pentry; struct part_info *part; struct mtd_device *dev; int part_num;
- debug("\n---list_partitions---\n");
- list_for_each(dentry, &devices) {
+list_for_each(dentry, &devices) {
Wrong indentation.
dev = list_entry(dentry, struct mtd_device, link);
/* list partitions for given device */part_num = 0;+#if defined(CONFIG_CMD_MTDPARTS_SHOW_NET_SIZES)
struct mtd_info *mtd;if (get_mtd_info(dev->id->type, dev->id->num, &mtd))return;printf("\ndevice %s%d <%s>, # parts = %d\n",MTD_DEV_TYPE(dev->id->type), dev->id->num,dev->id->mtd_id, dev->num_parts);printf(" #: name\t\tsize\t\tnet size\toffset\t\tmask_flags\n");list_for_each(pentry, &dev->parts) {u32 net_size;char *size_note;part = list_entry(pentry, struct part_info, link);net_size = net_part_size(mtd, part);size_note = part->size == net_size ? " " : " (!)";printf("%2d: %-20s0x%08x\t0x%08x%s\t0x%08x\t%d\n",part_num, part->name, part->size,net_size, size_note, part->offset,part->mask_flags);+#else /* !defined(CONFIG_CMD_MTDPARTS_SHOW_NET_SIZES) */ printf("\ndevice %s%d <%s>, # parts = %d\n", MTD_DEV_TYPE(dev->id->type), dev->id->num, dev->id->mtd_id, dev->num_parts); printf(" #: name\t\tsize\t\toffset\t\tmask_flags\n");
/* list partitions for given device */part_num = 0;+#endif /* defined(CONFIG_CMD_MTDPARTS_SHOW_NET_SIZES) */
I'll let Wolfgang speak up if this really is how he wants it done, but this seems like too much duplication to me. And what if someone else wants to add another optional field, do we end up with 4 versions? Then 8 versions the next time?
Is this really worth ifdeffing at all?
diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c index cb86657..211b993 100644 --- a/drivers/mtd/mtdcore.c +++ b/drivers/mtd/mtdcore.c @@ -143,7 +143,8 @@ void put_mtd_device(struct mtd_info *mtd) BUG_ON(c < 0); }
-#if defined(CONFIG_CMD_MTDPARTS_SPREAD) +#if defined(CONFIG_CMD_MTDPARTS_SPREAD) || \
- defined(CONFIG_CMD_MTDPARTS_SHOW_NET_SIZES)
/**
- mtd_get_len_incl_bad
Let's avoid stuff like this -- I'd define the function unconditionally. IMHO, the right solution to saving space from unused functions is function-sections/gc-sections.
-Scott
On Mon, Aug 30, 2010 at 4:50 PM, Scott Wood scottwood@freescale.com wrote:
On Mon, 30 Aug 2010 13:38:58 -0400 Ben Gardiner bengardiner@nanometrics.ca wrote:
+#if defined(CONFIG_CMD_MTDPARTS_SHOW_NET_SIZES) /**
- Format and print out a partition list for each device from global device
- list.
- Get the net size (w/o bad blocks) of the given partition.
- @param mtd the mtd info
- @param part the partition
- @return the calculated net size of this partition
*/ -static void list_partitions(void) +static uint64_t net_part_size(struct mtd_info *mtd, struct part_info *part) +{
- uint64_t gross_size, trailing_bad_size = 0;
- int truncated = 0;
- mtd_get_len_incl_bad(mtd, part->offset, part->size, &gross_size,
- &truncated);
- if (!truncated) {
- mtd_get_len_incl_bad(mtd, part->offset + part->size,
- mtd->erasesize, &trailing_bad_size,
- &truncated);
- trailing_bad_size -= mtd->erasesize;
- }
- return part->size - (gross_size - trailing_bad_size - part->size);
I'm not sure I follow the logic here...
You're trying to find net size given gross size, but you first treat the gross size as a net size and get the gross size of *that*.
If it was truncated, then you'll return a value that is still probably greater than the partition's true net size. For example, suppose you called this on the final partition, which includes at least one bad block (or the BBT, which is marked bad). mtd_get_len_incl_bad() will return the full partition size and set truncated. You'll end up with part->size - (part->size - 0 - part->size), which evaluates to part->size. The function should have returned something less than part->size.
If it was not truncated, suppose the partition had two bad blocks, and the next partition had its second block bad. mtd_get_len_incl_bad() will return part->size plus 3, since it ran into the next partition's bad block. The second call to mtd_get_len_incl_bad() will return one block, since it never got to the next partition's second block. Thus net_part_size() will return part->size - ((part->size + 3) - 0 - part->size), or part->size - 3. The right answer was part->size - 2.
I don't think a net-to-gross transformation is useful as a base for a gross-to-net transformation.
Right -- I was trying to maximize reuse of the new function but failed. But you're right that it just isn't suitable for reuse here. I'll go back to counting good blocks.
+} +#endif
+static void print_partition_table(void) { struct list_head *dentry, *pentry; struct part_info *part; struct mtd_device *dev; int part_num;
- debug("\n---list_partitions---\n");
- list_for_each(dentry, &devices) {
+list_for_each(dentry, &devices) {
Wrong indentation.
Sorry.
dev = list_entry(dentry, struct mtd_device, link);
- /* list partitions for given device */
- part_num = 0;
+#if defined(CONFIG_CMD_MTDPARTS_SHOW_NET_SIZES)
- struct mtd_info *mtd;
- if (get_mtd_info(dev->id->type, dev->id->num, &mtd))
- return;
- printf("\ndevice %s%d <%s>, # parts = %d\n",
- MTD_DEV_TYPE(dev->id->type), dev->id->num,
- dev->id->mtd_id, dev->num_parts);
- printf(" #: name\t\tsize\t\tnet size\toffset\t\tmask_flags\n");
- list_for_each(pentry, &dev->parts) {
- u32 net_size;
- char *size_note;
- part = list_entry(pentry, struct part_info, link);
- net_size = net_part_size(mtd, part);
- size_note = part->size == net_size ? " " : " (!)";
- printf("%2d: %-20s0x%08x\t0x%08x%s\t0x%08x\t%d\n",
- part_num, part->name, part->size,
- net_size, size_note, part->offset,
- part->mask_flags);
+#else /* !defined(CONFIG_CMD_MTDPARTS_SHOW_NET_SIZES) */ printf("\ndevice %s%d <%s>, # parts = %d\n", MTD_DEV_TYPE(dev->id->type), dev->id->num, dev->id->mtd_id, dev->num_parts); printf(" #: name\t\tsize\t\toffset\t\tmask_flags\n");
- /* list partitions for given device */
- part_num = 0;
list_for_each(pentry, &dev->parts) { part = list_entry(pentry, struct part_info, link); printf("%2d: %-20s0x%08x\t0x%08x\t%d\n", part_num, part->name, part->size, part->offset, part->mask_flags);
+#endif /* defined(CONFIG_CMD_MTDPARTS_SHOW_NET_SIZES) */
I'll let Wolfgang speak up if this really is how he wants it done, but this seems like too much duplication to me. And what if someone else wants to add another optional field, do we end up with 4 versions? Then 8 versions the next time?
Yes it doesn't scale well with the number of fields printed -- see below.
diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c index cb86657..211b993 100644 --- a/drivers/mtd/mtdcore.c +++ b/drivers/mtd/mtdcore.c @@ -143,7 +143,8 @@ void put_mtd_device(struct mtd_info *mtd) BUG_ON(c < 0); }
-#if defined(CONFIG_CMD_MTDPARTS_SPREAD) +#if defined(CONFIG_CMD_MTDPARTS_SPREAD) || \
- defined(CONFIG_CMD_MTDPARTS_SHOW_NET_SIZES)
/** * mtd_get_len_incl_bad *
Let's avoid stuff like this -- I'd define the function unconditionally. IMHO, the right solution to saving space from unused functions is function-sections/gc-sections.
I'm really glad for all your input on these patches -- your patience with my endless revisions appears equally as endless.
But I'm not sure what I'm supposed to do with this block now since Wolfgang Denk wd@denx.de has asked for me to 1) not use a bunch of #ifdefs to define the field [1] 2) not introduce changes that increase the code size on boards that do not enable this feature [2] ... and you have asked me to do the opposite in both cases here.
I will definitely re-spin a v6 with all of the mistakes you have pointed out fixed. But I think I have to stick with zero-impact-on-size patch and the minimal #ifdefs there since Wolfgang Denk wd@denx.de has asked for that specifically.
Best Regards, Ben Gardiner
[1] http://article.gmane.org/gmane.comp.boot-loaders.u-boot/82208 [2] http://article.gmane.org/gmane.comp.boot-loaders.u-boot/79419
--- Nanometrics Inc. http://www.nanometrics.ca
On Tue, 31 Aug 2010 09:51:15 -0400 Ben Gardiner bengardiner@nanometrics.ca wrote:
But I'm not sure what I'm supposed to do with this block now since Wolfgang Denk wd@denx.de has asked for me to 1) not use a bunch of #ifdefs to define the field [1] 2) not introduce changes that increase the code size on boards that do not enable this feature [2] ... and you have asked me to do the opposite in both cases here.
Sometimes a request seems perfectly reasonable until you see what the consequences are. :-)
Wolfgang, how do you want this to be handled?
-Scott
This patch adds an additional column to the output of list_partitions. The additional column will contain the net size and a '(!)' beside it if the net size is not equal to the partition size.
Signed-off-by: Ben Gardiner bengardiner@nanometrics.ca CC: Wolfgang Denk wd@denx.de CC: Scott Wood scottwood@freescale.com
---
V2: * formatting: spaces after 'if' and 'for' * the entire new feature is conditional on a macro, there is now a zero-byte binary size impact when the macro is not defined. * return the net parition size directly from net_part_size instead of using an output variable
V3: * rebased to 54841ab50c20d6fa6c9cc3eb826989da3a22d934 of git://git.denx.de/u-boot.git * fix line length over 80 chars * update copyright of cmd_mtdparts.c
V4: * rebased to b417260d871d4d8d336c160d95ed40cc8c0fb0fa of git://git.denx.de/u-boot.git * removed copyright statement and changelog from file header * re-grouped list_partition #ifdefs into one * fixed multi-line comment style
V5: * rebased to 962ad59e25640e586e2bceabf67a628a27f8f508 of git://git.denx.de/u-boot.git * renumbered from 2/4 to 3/5 * return uint64_t instead of u32 for net_size * do a quick if((cond) return * calculate net_size by adding-up good blocks instead of subtracting bad blocks * try to strike a balance; reuse more code between the branches of #if defined(CONFIG_CMD_MTDPARTS_SHOW_NET_SIZES) in print_partition_table
V6: * fix indentation on list_entry * don't use mtd_get_len_incl_bad to get net_size anymore as it was a bad idea. Just count up the good blocks.
--- common/cmd_mtdparts.c | 68 +++++++++++++++++++++++++++++++++++++++++++----- 1 files changed, 61 insertions(+), 7 deletions(-)
diff --git a/common/cmd_mtdparts.c b/common/cmd_mtdparts.c index 772ad54..266844f 100644 --- a/common/cmd_mtdparts.c +++ b/common/cmd_mtdparts.c @@ -1215,38 +1215,92 @@ static int generate_mtdparts_save(char *buf, u32 buflen) return ret; }
+#if defined(CONFIG_CMD_MTDPARTS_SHOW_NET_SIZES) /** - * Format and print out a partition list for each device from global device - * list. + * Get the net size (w/o bad blocks) of the given partition. + * + * @param mtd the mtd info + * @param part the partition + * @return the calculated net size of this partition */ -static void list_partitions(void) +static uint64_t net_part_size(struct mtd_info *mtd, struct part_info *part) +{ + if (!mtd->block_isbad) + return part->size; + + uint64_t i, net_size = 0; + + for (i = 0; i < part->size; i += mtd->erasesize) { + if (!mtd->block_isbad(mtd, part->offset + i)) + net_size += mtd->erasesize; + } + return net_size; +} +#endif + +static void print_partition_table(void) { struct list_head *dentry, *pentry; struct part_info *part; struct mtd_device *dev; int part_num;
- debug("\n---list_partitions---\n"); list_for_each(dentry, &devices) { dev = list_entry(dentry, struct mtd_device, link); + /* list partitions for given device */ + part_num = 0; +#if defined(CONFIG_CMD_MTDPARTS_SHOW_NET_SIZES) + struct mtd_info *mtd; + + if (get_mtd_info(dev->id->type, dev->id->num, &mtd)) + return; + + printf("\ndevice %s%d <%s>, # parts = %d\n", + MTD_DEV_TYPE(dev->id->type), dev->id->num, + dev->id->mtd_id, dev->num_parts); + printf(" #: name\t\tsize\t\tnet size\toffset\t\tmask_flags\n"); + + list_for_each(pentry, &dev->parts) { + u32 net_size; + char *size_note; + + part = list_entry(pentry, struct part_info, link); + net_size = net_part_size(mtd, part); + size_note = part->size == net_size ? " " : " (!)"; + printf("%2d: %-20s0x%08x\t0x%08x%s\t0x%08x\t%d\n", + part_num, part->name, part->size, + net_size, size_note, part->offset, + part->mask_flags); +#else /* !defined(CONFIG_CMD_MTDPARTS_SHOW_NET_SIZES) */ printf("\ndevice %s%d <%s>, # parts = %d\n", MTD_DEV_TYPE(dev->id->type), dev->id->num, dev->id->mtd_id, dev->num_parts); printf(" #: name\t\tsize\t\toffset\t\tmask_flags\n");
- /* list partitions for given device */ - part_num = 0; list_for_each(pentry, &dev->parts) { part = list_entry(pentry, struct part_info, link); printf("%2d: %-20s0x%08x\t0x%08x\t%d\n", part_num, part->name, part->size, part->offset, part->mask_flags); - +#endif /* defined(CONFIG_CMD_MTDPARTS_SHOW_NET_SIZES) */ part_num++; } } + if (list_empty(&devices)) printf("no partitions defined\n"); +} + +/** + * Format and print out a partition list for each device from global device + * list. + */ +static void list_partitions(void) +{ + struct part_info *part; + + debug("\n---list_partitions---\n"); + print_partition_table();
/* current_mtd_dev is not NULL only when we have non empty device list */ if (current_mtd_dev) {
participants (2)
-
Ben Gardiner -
Scott Wood