disk: part_dos: Fix part_test_dos() regression

From: Fabio Estevam <[email protected]>

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 <[email protected]>

---
 disk/part_dos.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

disk/part_dos.c +2 -9

index 1a36be0..213a8d8 100644
@@ -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

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

Quoted 25 lines · Fabio Estevam

From: Fabio Estevam <[email protected]>

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 <[email protected]>

---
 disk/part_dos.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

disk/part_dos.c +0 -0

index 1a36be0..213a8d8 100644
@@ -89,20 +89,14 @@ static int test_block_type(unsigned char *buffer)
 

disk/part_dos.c static int test_block_type(unsigned char *buffer) · line 92

Quoted 4 lines

disk/part_dos.c +1 -1

 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);

Quoted 20 lines

disk/part_dos.c +1 -8

        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

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

Quoted 4 lines · Rob Clark

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:

disk/part_dos.c +3 -1

@@ -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 :-)

Quoted 22 lines · Fabio Estevam

Quoted 4 lines · Rob Clark

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:

disk/part_dos.c +3 -1

@@ -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));
 

Quoted 4 lines

disk/part_dos.c +0 -0

        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..