| Message ID | 1506995402-415-1-git-send-email-festevam@gmail.com |
|---|---|
| State | RFC |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | [U-Boot,RFC] disk: part_dos: Fix part_test_dos() regression | expand |
personally I think we should try to figure out what is wrong on imx6 rather than blindly reverting.. without this change MBR partitioned disks might not generate unique device-paths for EFI boot. If you can't get any debug logs from SPL build, perhaps you can try an old SPL image but main u-boot image with the logs I suggested? I guess the same issue should happen with the main u-boot image. BR, -R On Mon, Oct 2, 2017 at 9:50 PM, Fabio Estevam <festevam@gmail.com> wrote: > From: Fabio Estevam <fabio.estevam@nxp.com> > > Since commit ff98cb90514d ("part: extract MBR signature from partitions") > SPL boot on i.MX6 starts to fail: > > U-Boot SPL 2017.09-00221-g0d6ab32 (Oct 02 2017 - 15:13:19) > Trying to boot from MMC1 > (hangs here) > > Revert the part_test_dos() changes from this commit, so that > SPL boot can be functional again. > > Tested on a imx6q-cuboxi board. > > Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com> > --- > disk/part_dos.c | 10 ++-------- > 1 file changed, 2 insertions(+), 8 deletions(-) > > diff --git a/disk/part_dos.c b/disk/part_dos.c > index 1a36be0..213a8d8 100644 > --- a/disk/part_dos.c > +++ b/disk/part_dos.c > @@ -89,20 +89,14 @@ static int test_block_type(unsigned char *buffer) > > static int part_test_dos(struct blk_desc *dev_desc) > { > - ALLOC_CACHE_ALIGN_BUFFER(legacy_mbr, mbr, dev_desc->blksz); > + ALLOC_CACHE_ALIGN_BUFFER(unsigned char, mbr, dev_desc->blksz); > > if (blk_dread(dev_desc, 0, 1, (ulong *)mbr) != 1) > return -1; > > - if (test_block_type((unsigned char *)mbr) != DOS_MBR) > + if (test_block_type(mbr) != DOS_MBR) > return -1; > > - if (dev_desc->sig_type == SIG_TYPE_NONE && > - mbr->unique_mbr_signature != 0) { > - dev_desc->sig_type = SIG_TYPE_MBR; > - dev_desc->mbr_sig = mbr->unique_mbr_signature; > - } > - > return 0; > } > > -- > 2.7.4 >
On Tue, Oct 3, 2017 at 6:47 AM, Rob Clark <robdclark@gmail.com> wrote: > personally I think we should try to figure out what is wrong on imx6 > rather than blindly reverting.. without this change MBR partitioned > disks might not generate unique device-paths for EFI boot. > > If you can't get any debug logs from SPL build, perhaps you can try an > old SPL image but main u-boot image with the logs I suggested? I > guess the same issue should happen with the main u-boot image. > > BR, > -R > > On Mon, Oct 2, 2017 at 9:50 PM, Fabio Estevam <festevam@gmail.com> wrote: >> From: Fabio Estevam <fabio.estevam@nxp.com> >> >> Since commit ff98cb90514d ("part: extract MBR signature from partitions") >> SPL boot on i.MX6 starts to fail: >> >> U-Boot SPL 2017.09-00221-g0d6ab32 (Oct 02 2017 - 15:13:19) >> Trying to boot from MMC1 >> (hangs here) >> >> Revert the part_test_dos() changes from this commit, so that >> SPL boot can be functional again. >> >> Tested on a imx6q-cuboxi board. >> >> Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com> >> --- >> disk/part_dos.c | 10 ++-------- >> 1 file changed, 2 insertions(+), 8 deletions(-) >> >> diff --git a/disk/part_dos.c b/disk/part_dos.c >> index 1a36be0..213a8d8 100644 >> --- a/disk/part_dos.c >> +++ b/disk/part_dos.c >> @@ -89,20 +89,14 @@ static int test_block_type(unsigned char *buffer) >> >> static int part_test_dos(struct blk_desc *dev_desc) >> { >> - ALLOC_CACHE_ALIGN_BUFFER(legacy_mbr, mbr, dev_desc->blksz); >> + ALLOC_CACHE_ALIGN_BUFFER(unsigned char, mbr, dev_desc->blksz); btw, if I had to take a guess, I'd say that perhaps blksz is smaller than 'legacy_mbr', so maybe rather than allocating blksize, it should be DIV_ROUND_UP(sizeof(legacy_mbr), dev_desc->blksz).. or I guess that could be simplified to not use division if blksz is a power of two BR, -R >> >> if (blk_dread(dev_desc, 0, 1, (ulong *)mbr) != 1) >> return -1; >> >> - if (test_block_type((unsigned char *)mbr) != DOS_MBR) >> + if (test_block_type(mbr) != DOS_MBR) >> return -1; >> >> - if (dev_desc->sig_type == SIG_TYPE_NONE && >> - mbr->unique_mbr_signature != 0) { >> - dev_desc->sig_type = SIG_TYPE_MBR; >> - dev_desc->mbr_sig = mbr->unique_mbr_signature; >> - } >> - >> return 0; >> } >> >> -- >> 2.7.4 >>
On Tue, Oct 3, 2017 at 7:57 AM, Rob Clark <robdclark@gmail.com> wrote: > btw, if I had to take a guess, I'd say that perhaps blksz is smaller > than 'legacy_mbr', so maybe rather than allocating blksize, it should > be DIV_ROUND_UP(sizeof(legacy_mbr), dev_desc->blksz).. or I guess that > could be simplified to not use division if blksz is a power of two Yes, it does seem to be size related as we are size constraint in SPL. Just tried your suggestion: --- a/disk/part_dos.c +++ b/disk/part_dos.c @@ -89,7 +89,9 @@ static int test_block_type(unsigned char *buffer) static int part_test_dos(struct blk_desc *dev_desc) { - ALLOC_CACHE_ALIGN_BUFFER(legacy_mbr, mbr, dev_desc->blksz); + ALLOC_CACHE_ALIGN_BUFFER(legacy_mbr, mbr, + DIV_ROUND_UP(sizeof(legacy_mbr), + dev_desc->blksz)); if (blk_dread(dev_desc, 0, 1, (ulong *)mbr) != 1) return -1; and it does work for me :-)
On Tue, Oct 3, 2017 at 7:04 AM, Fabio Estevam <festevam@gmail.com> wrote: > On Tue, Oct 3, 2017 at 7:57 AM, Rob Clark <robdclark@gmail.com> wrote: > >> btw, if I had to take a guess, I'd say that perhaps blksz is smaller >> than 'legacy_mbr', so maybe rather than allocating blksize, it should >> be DIV_ROUND_UP(sizeof(legacy_mbr), dev_desc->blksz).. or I guess that >> could be simplified to not use division if blksz is a power of two > > Yes, it does seem to be size related as we are size constraint in SPL. > > Just tried your suggestion: > > --- a/disk/part_dos.c > +++ b/disk/part_dos.c > @@ -89,7 +89,9 @@ static int test_block_type(unsigned char *buffer) > > static int part_test_dos(struct blk_desc *dev_desc) > { > - ALLOC_CACHE_ALIGN_BUFFER(legacy_mbr, mbr, dev_desc->blksz); > + ALLOC_CACHE_ALIGN_BUFFER(legacy_mbr, mbr, > + DIV_ROUND_UP(sizeof(legacy_mbr), > + dev_desc->blksz)); > > if (blk_dread(dev_desc, 0, 1, (ulong *)mbr) != 1) > return -1; > > and it does work for me :-) Ok, I guess if blksz can actually be less than the mbr, we probably also need a similar fix in is_gpt_valid() (and also to pass the correct # of blks to blk_dread()).. I'll make a patch in a few.. BR, -R
On Tue, Oct 03, 2017 at 08:44:31AM -0400, Rob Clark wrote: > On Tue, Oct 3, 2017 at 7:04 AM, Fabio Estevam <festevam@gmail.com> wrote: > > On Tue, Oct 3, 2017 at 7:57 AM, Rob Clark <robdclark@gmail.com> wrote: > > > >> btw, if I had to take a guess, I'd say that perhaps blksz is smaller > >> than 'legacy_mbr', so maybe rather than allocating blksize, it should > >> be DIV_ROUND_UP(sizeof(legacy_mbr), dev_desc->blksz).. or I guess that > >> could be simplified to not use division if blksz is a power of two > > > > Yes, it does seem to be size related as we are size constraint in SPL. > > > > Just tried your suggestion: > > > > --- a/disk/part_dos.c > > +++ b/disk/part_dos.c > > @@ -89,7 +89,9 @@ static int test_block_type(unsigned char *buffer) > > > > static int part_test_dos(struct blk_desc *dev_desc) > > { > > - ALLOC_CACHE_ALIGN_BUFFER(legacy_mbr, mbr, dev_desc->blksz); > > + ALLOC_CACHE_ALIGN_BUFFER(legacy_mbr, mbr, > > + DIV_ROUND_UP(sizeof(legacy_mbr), > > + dev_desc->blksz)); > > > > if (blk_dread(dev_desc, 0, 1, (ulong *)mbr) != 1) > > return -1; > > > > and it does work for me :-) > > Ok, I guess if blksz can actually be less than the mbr, we probably > also need a similar fix in is_gpt_valid() (and also to pass the > correct # of blks to blk_dread()).. I'll make a patch in a few.. If you want to re-work / include my changes in https://patchwork.ozlabs.org/patch/820884/ in v2 of https://patchwork.ozlabs.org/patch/820920/ please feel free (and make sure we don't have / introduce more similar ones), thanks!
diff --git a/disk/part_dos.c b/disk/part_dos.c index 1a36be0..213a8d8 100644 --- a/disk/part_dos.c +++ b/disk/part_dos.c @@ -89,20 +89,14 @@ static int test_block_type(unsigned char *buffer) static int part_test_dos(struct blk_desc *dev_desc) { - ALLOC_CACHE_ALIGN_BUFFER(legacy_mbr, mbr, dev_desc->blksz); + ALLOC_CACHE_ALIGN_BUFFER(unsigned char, mbr, dev_desc->blksz); if (blk_dread(dev_desc, 0, 1, (ulong *)mbr) != 1) return -1; - if (test_block_type((unsigned char *)mbr) != DOS_MBR) + if (test_block_type(mbr) != DOS_MBR) return -1; - if (dev_desc->sig_type == SIG_TYPE_NONE && - mbr->unique_mbr_signature != 0) { - dev_desc->sig_type = SIG_TYPE_MBR; - dev_desc->mbr_sig = mbr->unique_mbr_signature; - } - return 0; }