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

[Scott Wood <scottwood@freescale.com>]

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