* [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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.