* [U-Boot] [RFC] disk: part_dos: Fix part_test_dos() regression
@ 2017-10-03 1:50 Fabio Estevam
2017-10-03 10:47 ` Rob Clark
0 siblings, 1 reply; 6+ messages in thread
From: Fabio Estevam @ 2017-10-03 1:50 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* [U-Boot] [RFC] disk: part_dos: Fix part_test_dos() regression 2017-10-03 1:50 [U-Boot] [RFC] disk: part_dos: Fix part_test_dos() regression Fabio Estevam @ 2017-10-03 10:47 ` Rob Clark 2017-10-03 10:57 ` Rob Clark 0 siblings, 1 reply; 6+ messages in thread From: Rob Clark @ 2017-10-03 10:47 UTC (permalink / raw) To: u-boot 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 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [RFC] disk: part_dos: Fix part_test_dos() regression 2017-10-03 10:47 ` Rob Clark @ 2017-10-03 10:57 ` Rob Clark 2017-10-03 11:04 ` Fabio Estevam 0 siblings, 1 reply; 6+ messages in thread From: Rob Clark @ 2017-10-03 10:57 UTC (permalink / raw) To: u-boot 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 >> ^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [RFC] disk: part_dos: Fix part_test_dos() regression 2017-10-03 10:57 ` Rob Clark @ 2017-10-03 11:04 ` Fabio Estevam 2017-10-03 12:44 ` Rob Clark 0 siblings, 1 reply; 6+ messages in thread From: Fabio Estevam @ 2017-10-03 11:04 UTC (permalink / raw) To: u-boot 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 :-) ^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [RFC] disk: part_dos: Fix part_test_dos() regression 2017-10-03 11:04 ` Fabio Estevam @ 2017-10-03 12:44 ` Rob Clark 2017-10-03 16:10 ` Tom Rini 0 siblings, 1 reply; 6+ messages in thread From: Rob Clark @ 2017-10-03 12:44 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [RFC] disk: part_dos: Fix part_test_dos() regression 2017-10-03 12:44 ` Rob Clark @ 2017-10-03 16:10 ` Tom Rini 0 siblings, 0 replies; 6+ messages in thread From: Tom Rini @ 2017-10-03 16:10 UTC (permalink / raw) To: u-boot 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! -- Tom -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: not available URL: <http://lists.denx.de/pipermail/u-boot/attachments/20171003/8f9f264b/attachment.sig> ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-10-03 16:10 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-10-03 1:50 [U-Boot] [RFC] disk: part_dos: Fix part_test_dos() regression Fabio Estevam 2017-10-03 10:47 ` Rob Clark 2017-10-03 10:57 ` Rob Clark 2017-10-03 11:04 ` Fabio Estevam 2017-10-03 12:44 ` Rob Clark 2017-10-03 16:10 ` Tom Rini
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox