cmd: gpt: add - partition size parsing
RFC PATCH 1/1 cmd: gpt: add - partition size parsing
This patch try to parse name=userdata,size=-,uuid=${uuid_gpt_userdata};
gpt mmc write 0 $partitions
gpt mmc verify 0 $partitions
--- cmd/gpt.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)
cmd/gpt.c +11 -2
@@ -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; + }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; +/* bootable */ if (found_key (tok,"bootable" )) parts[i].bootable =1 ;
Signature · 1 line
2.8.3
RFC PATCH 1/1 cmd: gpt: add - partition size parsing
Quoted 9 lines · Michael Trimarchi
This patch try to parse name=userdata,size=-,uuid=${uuid_gpt_userdata};
gpt mmc write 0 $partitions
gpt mmc verify 0 $partitionsSigned-off-by Michael Trimarchi <[email protected]>--- cmd/gpt.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)
Quoted 7 lines · Michael Trimarchi
This patch try to parse name=userdata,size=-,uuid=${uuid_gpt_userdata};
gpt mmc write 0 $partitions
gpt mmc verify 0 $partitionsSigned-off-by Michael Trimarchi <[email protected]>Reviewed-by Simon Glass <[email protected]>
Applied to u-boot/master, thanks!
Signature · 8 lines
Tom -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: Digital signature URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160619/ff5c93a7/attachment.sig>
RFC PATCH 1/1 cmd: gpt: add - partition size parsing
In reply to Michael TrimarchiHi Michael,
Quoted 23 lines · Michael Trimarchi
This patch try to parse name=userdata,size=-,uuid=${uuid_gpt_userdata};
gpt mmc write 0 $partitions
gpt mmc verify 0 $partitionsSigned-off-by Michael Trimarchi <[email protected]>--- cmd/gpt.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)cmd/gpt.c +1
-0@@ -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);
cmd/gpt.c static int set_gpt_info(struct blk_desc *dev_desc, · line 302
Quoted 8 lines
cmd/gpt.c +3
-2@@ -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
cmd/gpt.c static int set_gpt_info(struct blk_desc *dev_desc, · line 320
Quoted 13 lines
cmd/gpt.c +6
-0+ }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;cmd/gpt.c static int set_gpt_info(struct blk_desc *dev_desc, · line 324
Quoted 5 lines
cmd/gpt.c +1
-0+/* 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?
Regards,
Julian
RFC PATCH 1/1 cmd: gpt: add - partition size parsing
Hi
Quoted 93 lines · Julian Scheel
Hi Michael,
Quoted 24 lines · Michael Trimarchi
This patch try to parse name=userdata,size=-,uuid=${uuid_gpt_userdata};
gpt mmc write 0 $partitions
gpt mmc verify 0 $partitionsSigned-off-by Michael Trimarchi <[email protected]>--- cmd/gpt.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)cmd/gpt.c +1
-0@@ -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);cmd/gpt.c static int set_gpt_info(struct blk_desc *dev_desc, · line 302
Quoted 8 lines
cmd/gpt.c +3
-2@@ -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.cthelast_usableblock isdev_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 inpart_efi.ccmd/gpt.c static int set_gpt_info(struct blk_desc *dev_desc, · line 320
Quoted 13 lines
cmd/gpt.c +6
-0+ }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 offsetsb) start is specified for a partition i, example table:
part[0].size = 10
part[1].size = 10
part[1].start = 10part[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-40So 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;cmd/gpt.c static int set_gpt_info(struct blk_desc *dev_desc, · line 324
Quoted 5 lines
cmd/gpt.c +1
-0+/* bootable */ if (found_key (tok,"bootable" )) parts[i].bootable =1 ;
Quoted 9 lines
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.cautomatically 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
Quoted 3 lines
Regards,
Julian
Signature · 4 lines
| Michael Nazzareno Trimarchi Amarula Solutions BV | | COO - Founder Cruquiuskade 47 | | +31(0)851119172 Amsterdam 1018 AM NL | | [`as] http://www.amarulasolutions.com |