Re: [U-Boot] [PATCH v5 3/5] mtdparts: show net size in mtdparts list

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;
  

  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?

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