On Mon, Aug 30, 2010 at 4:50 PM, Scott Wood <scottw...@freescale.com> wrote:
> On Mon, 30 Aug 2010 13:38:58 -0400
> Ben Gardiner <bengardi...@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 <w...@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 <w...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
