disk: Don't assume blksz > legacy_mbr
PATCH 1/1 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.
---
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(-)
disk/part_dos.c +4 -2
@@ -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 )
disk/part_efi.c +4 -2
@@ -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 ; }
Signature · 1 line
2.13.5
PATCH 1/1 disk: Don't assume blksz > legacy_mbr
Quoted 4 lines · 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 <[email protected]>
This version does not solve the mx6 spl boot issue.
PATCH 1/1 disk: Don't assume blksz > legacy_mbr
Quoted 7 lines · Fabio Estevam
Quoted 4 lines · 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 <[email protected]>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
PATCH 1/1 disk: Don't assume blksz > legacy_mbr
Quoted 13 lines · Rob Clark
Quoted 7 lines · Fabio Estevam
Quoted 4 lines · 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 <[email protected]>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)..
Quoted 6 lines
*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
PATCH 1/1 disk: Don't assume blksz > legacy_mbr
Hi Rob,
Quoted 2 lines · Rob Clark
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.
Quoted 10 lines
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 breakefi_bootmgrplus 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
PATCH 1/1 disk: Don't assume blksz > legacy_mbr
Quoted 28 lines · Fabio Estevam
Hi Rob,
Quoted 2 lines · Rob Clark
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.
Quoted 10 lines
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 breakefi_bootmgrplus boards with multiple "disks".. having the proper partition signatures I guess doesn't matter in the SPL stage..
Quoted 3 lines
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
PATCH 1/1 disk: Don't assume blksz > legacy_mbr
Quoted 3 lines · Rob Clark
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