[U-Boot] [PATCH] disk: Don't assume blksz > legacy_mbr

[U-Boot] [PATCH] disk: Don't assume blksz > legacy_mbr

[Rob Clark]

On some devices, this does not appear to be a valid assumption.  So
figure out the number of blocks we actually need to read.

Signed-off-by: Rob Clark <robdclark@gmail.com>
---
Sorry, took a bit longer to get to a point of testing this.. somehow
himport_r() of the default_environment is clobbering my usb keyboard's
usb_device, which I'm still tracking down.  Good thing that pointers
overwritten with ascii are fairly obvious.

 disk/part_dos.c | 6 ++++--
 disk/part_efi.c | 6 ++++--
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/disk/part_dos.c b/disk/part_dos.c
index 1a36be0446..a9d23e121c 100644
--- a/disk/part_dos.c
+++ b/disk/part_dos.c
@@ -89,9 +89,11 @@ 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);
+	/* blksz *should* be a PoT, but to be safe use DIV_ROUND_UP: */
+	lbaint_t blkcnt = DIV_ROUND_UP(sizeof(legacy_mbr), dev_desc->blksz);
+	ALLOC_CACHE_ALIGN_BUFFER(legacy_mbr, mbr, blkcnt * dev_desc->blksz);
 
-	if (blk_dread(dev_desc, 0, 1, (ulong *)mbr) != 1)
+	if (blk_dread(dev_desc, 0, blkcnt, (ulong *)mbr) != 1)
 		return -1;
 
 	if (test_block_type((unsigned char *)mbr) != DOS_MBR)
diff --git a/disk/part_efi.c b/disk/part_efi.c
index 208bb14ee8..e00f6c9d24 100644
--- a/disk/part_efi.c
+++ b/disk/part_efi.c
@@ -923,7 +923,9 @@ static int is_pmbr_valid(legacy_mbr * mbr)
 static int is_gpt_valid(struct blk_desc *dev_desc, u64 lba,
 			gpt_header *pgpt_head, gpt_entry **pgpt_pte)
 {
-	ALLOC_CACHE_ALIGN_BUFFER(legacy_mbr, mbr, dev_desc->blksz);
+	/* blksz *should* be a PoT, but to be safe use DIV_ROUND_UP: */
+	lbaint_t blkcnt = DIV_ROUND_UP(sizeof(legacy_mbr), dev_desc->blksz);
+	ALLOC_CACHE_ALIGN_BUFFER(legacy_mbr, mbr, blkcnt * dev_desc->blksz);
 
 	if (!dev_desc || !pgpt_head) {
 		printf("%s: Invalid Argument(s)\n", __func__);
@@ -931,7 +933,7 @@ static int is_gpt_valid(struct blk_desc *dev_desc, u64 lba,
 	}
 
 	/* Read MBR Header from device */
-	if (blk_dread(dev_desc, 0, 1, (ulong *)mbr) != 1) {
+	if (blk_dread(dev_desc, 0, blkcnt, (ulong *)mbr) != 1) {
 		printf("*** ERROR: Can't read MBR header ***\n");
 		return 0;
 	}
-- 
2.13.5

[U-Boot] [PATCH] disk: Don't assume blksz > legacy_mbr

[Fabio Estevam]

On Tue, Oct 3, 2017 at 12:30 PM, Rob Clark <robdclark@gmail.com> wrote:
> On some devices, this does not appear to be a valid assumption.  So
> figure out the number of blocks we actually need to read.
>
> Signed-off-by: Rob Clark <robdclark@gmail.com>

This version does not solve the mx6 spl boot issue.

[U-Boot] [PATCH] disk: Don't assume blksz > legacy_mbr

[Rob Clark]

On Tue, Oct 3, 2017 at 12:04 PM, Fabio Estevam <festevam@gmail.com> wrote:
> On Tue, Oct 3, 2017 at 12:30 PM, Rob Clark <robdclark@gmail.com> wrote:
>> On some devices, this does not appear to be a valid assumption.  So
>> figure out the number of blocks we actually need to read.
>>
>> Signed-off-by: Rob Clark <robdclark@gmail.com>
>
> This version does not solve the mx6 spl boot issue.

Hmm, the change you had tested earlier is not correct, since it would
allocate potentially less than blksz (and then read blksz bytes into
it)..

*maybe* the memory allocation is failing?  I'm not sure, I'm kind of
just guessing here.  It would be nice to know what blksz is on your
device.  (That said, before the original patch, the allocation was for
blksz bytes too, so if this were the issue I think it would have been
failing before.)

BR,
-R

[U-Boot] [PATCH] disk: Don't assume blksz > legacy_mbr

[Rob Clark]

On Tue, Oct 3, 2017 at 12:32 PM, Rob Clark <robdclark@gmail.com> wrote:
> On Tue, Oct 3, 2017 at 12:04 PM, Fabio Estevam <festevam@gmail.com> wrote:
>> On Tue, Oct 3, 2017 at 12:30 PM, Rob Clark <robdclark@gmail.com> wrote:
>>> On some devices, this does not appear to be a valid assumption.  So
>>> figure out the number of blocks we actually need to read.
>>>
>>> Signed-off-by: Rob Clark <robdclark@gmail.com>
>>
>> This version does not solve the mx6 spl boot issue.
>
> Hmm, the change you had tested earlier is not correct, since it would
> allocate potentially less than blksz (and then read blksz bytes into
> it)..
>
> *maybe* the memory allocation is failing?  I'm not sure, I'm kind of
> just guessing here.  It would be nice to know what blksz is on your
> device.  (That said, before the original patch, the allocation was for
> blksz bytes too, so if this were the issue I think it would have been
> failing before.)
>

btw, I'm still hoping someone can get me some logs so I can see what
blksz is, etc, on these boards..

in the worst case I guess we can change part_test_dos() to:

#ifdef SPL
  .. old code ..
#else
  .. new code ..
#endif

a bit ugly, but at least that wouldn't break efi_bootmgr plus boards
with multiple "disks".. having the proper partition signatures I guess
doesn't matter in the SPL stage..

BR,
-R

[U-Boot] [PATCH] disk: Don't assume blksz > legacy_mbr

[Fabio Estevam]

Hi Rob,

On Wed, Oct 4, 2017 at 11:11 AM, Rob Clark <robdclark@gmail.com> wrote:

> btw, I'm still hoping someone can get me some logs so I can see what
> blksz is, etc, on these boards..

Sorry, I haven't been able to get you the logs yet.

I will probably be able to work on this next week only, unfortunately.

I am adding some mx6 folks in case they could help collecting such
logs in the meantime.

Just to provide them some context: we are investigating the mx6 SPL
regression in mainline.

> in the worst case I guess we can change part_test_dos() to:
>
> #ifdef SPL
>   .. old code ..
> #else
>   .. new code ..
> #endif
> a bit ugly, but at least that wouldn't break efi_bootmgr plus boards
> with multiple "disks".. having the proper partition signatures I guess
> doesn't matter in the SPL stage..

Yes, this is what I have locally to workaround the issue:
https://pastebin.com/XxBfpwh7

Thanks

[U-Boot] [PATCH] disk: Don't assume blksz > legacy_mbr

[Rob Clark]

On Wed, Oct 4, 2017 at 10:27 AM, Fabio Estevam <festevam@gmail.com> wrote:
> Hi Rob,
>
> On Wed, Oct 4, 2017 at 11:11 AM, Rob Clark <robdclark@gmail.com> wrote:
>
>> btw, I'm still hoping someone can get me some logs so I can see what
>> blksz is, etc, on these boards..
>
> Sorry, I haven't been able to get you the logs yet.
>
> I will probably be able to work on this next week only, unfortunately.
>
> I am adding some mx6 folks in case they could help collecting such
> logs in the meantime.
>
> Just to provide them some context: we are investigating the mx6 SPL
> regression in mainline.
>
>> in the worst case I guess we can change part_test_dos() to:
>>
>> #ifdef SPL
>>   .. old code ..
>> #else
>>   .. new code ..
>> #endif
>> a bit ugly, but at least that wouldn't break efi_bootmgr plus boards
>> with multiple "disks".. having the proper partition signatures I guess
>> doesn't matter in the SPL stage..
>
> Yes, this is what I have locally to workaround the issue:
> https://pastebin.com/XxBfpwh7
>

Not sure Tom's opinion, but I'd be ok with applying this patch, even
if just a temporary fix, to unblock folks trying to test latest u-boot
on these devices

BR,
-R

[U-Boot] [PATCH] disk: Don't assume blksz > legacy_mbr

[Fabio Estevam]

On Wed, Oct 4, 2017 at 11:51 AM, Rob Clark <robdclark@gmail.com> wrote:

> Not sure Tom's opinion, but I'd be ok with applying this patch, even
> if just a temporary fix, to unblock folks trying to test latest u-boot
> on these devices

Yes, sounds like an option given we are at -rc1 now.

Will submit it and let's see what Tom says about it.

Thanks