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

Ben Gardiner
2010-08-31T13:51:15Z

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.