[U-Boot] [RFC PATCH] cmd: gpt: add - partition size parsing

[Michael Trimarchi]

Hi

On Wed, Jul 27, 2016 at 2:57 PM, Julian Scheel <julian at jusst.de> wrote:
> Hi Michael,
>
>
> On 08.06.2016 10:18, Michael Trimarchi wrote:
>>
>> This patch try to parse name=userdata,size=-,uuid=${uuid_gpt_userdata};
>>
>> gpt mmc write 0 $partitions
>> gpt mmc verify 0 $partitions
>>
>> Signed-off-by: Michael Trimarchi <michael at amarulasolutions.com>
>> ---
>>  cmd/gpt.c | 13 +++++++++++--
>>  1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/cmd/gpt.c b/cmd/gpt.c
>> index 8ffaef3..3d9706b 100644
>> --- a/cmd/gpt.c
>> +++ b/cmd/gpt.c
>> @@ -181,6 +181,7 @@ static int set_gpt_info(struct blk_desc *dev_desc,
>>         disk_partition_t *parts;
>>         int errno = 0;
>>         uint64_t size_ll, start_ll;
>> +       lbaint_t offset = 0;
>>
>>         debug("%s:  lba num: 0x%x %d\n", __func__,
>>               (unsigned int)dev_desc->lba, (unsigned int)dev_desc->lba);
>> @@ -296,8 +297,14 @@ static int set_gpt_info(struct blk_desc *dev_desc,
>>                 }
>>                 if (extract_env(val, &p))
>>                         p = val;
>> -               size_ll = ustrtoull(p, &p, 0);
>> -               parts[i].size = lldiv(size_ll, dev_desc->blksz);
>> +               if ((strcmp(p, "-") == 0)) {
>> +                       /* remove first usable lba and last block */
>> +                       parts[i].size = dev_desc->lba - 34  - 1 - offset;
>
>
> Looking into part_efi.c the last_usable block is dev_desc->lba - 34 and the
> first usable block is 34. So 34 needs to be substracted twice, otherwise you
> hit the "Partitions layout exceds disk size" error case in part_efi.c
>
>> +               } else {
>> +                       size_ll = ustrtoull(p, &p, 0);
>> +                       parts[i].size = lldiv(size_ll, dev_desc->blksz);
>> +               }
>> +
>>                 free(val);
>>
>>                 /* start address */
>> @@ -310,6 +317,8 @@ static int set_gpt_info(struct blk_desc *dev_desc,
>>                         free(val);
>>                 }
>>
>> +               offset += parts[i].size + parts[i].start;
>
>
> This appears to be wrong. We have two cases here:
>
> a) start is not specified for parition i
> In this case the code works as parts[i].start is 0 and size accumulates to
> the proper offsets
>
> b) start is specified for a partition i, example table:
>
> part[0].size = 10
>
> part[1].size = 10
> part[1].start = 10
>
> part[2].size = 10
>
> This table should end up in the same table as if part[1].start = 10 would
> have been omited. In fact it will lead to:
> part[0] = 0-10
> part[1] = 10-20
> part[2] = 30-40
>
> So it introduced a gap, because start is assumed to be relative to previous
> offset, which is not the case.
>
> I think the proper handling would be:
> if (parts[i].start)
>     offset = parts[i].size + parts[i].start;
> else
>     offset += parts[i].size;
>
>> +
>>                 /* bootable */
>>                 if (found_key(tok, "bootable"))
>>                         parts[i].bootable = 1;
>>
>
> While preparing a patch to fix the previously mentioned issues, so that gpt
> writing becomes usable again, I started to wonder why you added this patch
> at all. In fact the usecase you describe in the commit message did work
> perfectly without this patch, because part_efi.c automatically scales a
> partition without given size to the available space, in case it is the last
> partition. (See:
> http://git.denx.de/?p=u-boot.git;a=blob;f=disk/part_efi.c;h=01f71bee79e2656a295ee1cd3505185efc9c5cf7;hb=HEAD#l447)
> So imho this patch should simply be reverted or did I miss something
> important?

The idea was to properly handle the verification part in an easy way,
to make command consistent. Your fix look
correct. I have some try on my side and I miss this point. I'm working
on imx7d platform and and fastboot on latest
uboot but I was busy more on other part.

Michael

>
> Regards,
> Julian



-- 
| Michael Nazzareno Trimarchi                     Amarula Solutions BV |
| COO  -  Founder                                      Cruquiuskade 47 |
| +31(0)851119172                                 Amsterdam 1018 AM NL |
|                  [`as] http://www.amarulasolutions.com               |