Skip to contentSkip to conversation

cmd: gpt: add - partition size parsing

RFC PATCH In review Michael Trimarchi 4 participants 5 messages Jun 8, 2016 – Jul 27, 2016

lore
Share & tools
lore
#0 lore raw

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
index 8ffaef3..3d9706b 100644
@@ -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
#2 lore raw
In reply to Michael Trimarchi
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 $partitions

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>
#3 lore raw

RFC PATCH 1/1 cmd: gpt: add - partition size parsing

In reply to Michael Trimarchi

Hi 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 $partitions

---
 cmd/gpt.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)
cmd/gpt.c +1 -0
index 8ffaef3..3d9706b 100644
@@ -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

#4 lore raw

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 $partitions

---
 cmd/gpt.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)
cmd/gpt.c +1 -0
index 8ffaef3..3d9706b 100644
@@ -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;
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.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

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               |