[U-Boot] [PATCH] disk: Don't assume blksz > legacy_mbr
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; }
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.
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
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
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
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
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
participants (2)
-
Fabio Estevam -
Rob Clark