| Message ID | 20171003153015.13458-1-robdclark@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | [U-Boot] disk: Don't assume blksz > legacy_mbr | expand |
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
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 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(-)