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

[Ben Gardiner]

On Mon, Aug 30, 2010 at 4:50 PM, Scott Wood <scottwood at freescale.com> wrote:
> On Mon, 30 Aug 2010 13:38:58 -0400
> Ben Gardiner <bengardiner at 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 at 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 at 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