| Message ID | 20160608081811.GA2648@panicking |
|---|---|
| State | Accepted |
| Commit | 666362356e1ccc0df91c03b1d3f97939968b9c04 |
| Delegated to: | Tom Rini |
| Headers | show |
On 8 June 2016 at 02:18, Michael Trimarchi <michael@amarulasolutions.com> 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@amarulasolutions.com> > --- > cmd/gpt.c | 13 +++++++++++-- > 1 file changed, 11 insertions(+), 2 deletions(-) Reviewed-by: Simon Glass <sjg@chromium.org>
On Wed, Jun 08, 2016 at 10:18:16AM +0200, 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@amarulasolutions.com> > Reviewed-by: Simon Glass <sjg@chromium.org> Applied to u-boot/master, thanks!
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@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? Regards, Julian
Hi On Wed, Jul 27, 2016 at 2:57 PM, Julian Scheel <julian@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@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
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; + } 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;
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@amarulasolutions.com> --- cmd/gpt.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)