[U-Boot] [PATCH v2] part_efi: fix protective mbr struct allocation

The calloc() call was allocating space for the sizeof the struct pointer rather than for the struct contents. Besides, since this buffer is passed to mmc for writing and some platforms may use cache, the legacy_mbr struct should be cache-aligned.
Signed-off-by: Hector Palacios hector.palacios@digi.com ---
Notes: Changes since V1: - Cache-align declaration of p_mbr pointer - Use *p_mbr in sizeof() to match kernel CodingStyle
disk/part_efi.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/disk/part_efi.c b/disk/part_efi.c index 5dfaf490c89a..42936e04fb67 100644 --- a/disk/part_efi.c +++ b/disk/part_efi.c @@ -229,10 +229,10 @@ int test_part_efi(block_dev_desc_t * dev_desc) */ static int set_protective_mbr(block_dev_desc_t *dev_desc) { - legacy_mbr *p_mbr; - /* Setup the Protective MBR */ - p_mbr = calloc(1, sizeof(p_mbr)); + ALLOC_CACHE_ALIGN_BUFFER(legacy_mbr, p_mbr, 1); + memset(p_mbr, 0, sizeof(*p_mbr)); + if (p_mbr == NULL) { printf("%s: calloc failed!\n", __func__); return -1; @@ -247,11 +247,9 @@ static int set_protective_mbr(block_dev_desc_t *dev_desc) if (dev_desc->block_write(dev_desc->dev, 0, 1, p_mbr) != 1) { printf("** Can't write to device %d **\n", dev_desc->dev); - free(p_mbr); return -1; }
- free(p_mbr); return 0; }

Hi Hector,
The calloc() call was allocating space for the sizeof the struct pointer rather than for the struct contents. Besides, since this buffer is passed to mmc for writing and some platforms may use cache, the legacy_mbr struct should be cache-aligned.
Thanks for preparing v2. Everything seems OK.
Test HW: Exynos4210 - Trats.
Tested-by: Lukasz Majewski l.majewski@samsung.com
Signed-off-by: Hector Palacios hector.palacios@digi.com
Notes: Changes since V1: - Cache-align declaration of p_mbr pointer - Use *p_mbr in sizeof() to match kernel CodingStyle
disk/part_efi.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/disk/part_efi.c b/disk/part_efi.c index 5dfaf490c89a..42936e04fb67 100644 --- a/disk/part_efi.c +++ b/disk/part_efi.c @@ -229,10 +229,10 @@ int test_part_efi(block_dev_desc_t * dev_desc) */ static int set_protective_mbr(block_dev_desc_t *dev_desc) {
- legacy_mbr *p_mbr;
- /* Setup the Protective MBR */
- p_mbr = calloc(1, sizeof(p_mbr));
- ALLOC_CACHE_ALIGN_BUFFER(legacy_mbr, p_mbr, 1);
- memset(p_mbr, 0, sizeof(*p_mbr));
- if (p_mbr == NULL) { printf("%s: calloc failed!\n", __func__); return -1;
@@ -247,11 +247,9 @@ static int set_protective_mbr(block_dev_desc_t *dev_desc) if (dev_desc->block_write(dev_desc->dev, 0, 1, p_mbr) !=
- { printf("** Can't write to device %d **\n", dev_desc->dev);
free(p_mbr);
return -1; }
free(p_mbr); return 0;
}

Hi Tom,
The calloc() call was allocating space for the sizeof the struct pointer rather than for the struct contents. Besides, since this buffer is passed to mmc for writing and some platforms may use cache, the legacy_mbr struct should be cache-aligned.
Is there any problem with this patch?
Signed-off-by: Hector Palacios hector.palacios@digi.com
Notes: Changes since V1: - Cache-align declaration of p_mbr pointer - Use *p_mbr in sizeof() to match kernel CodingStyle
disk/part_efi.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/disk/part_efi.c b/disk/part_efi.c index 5dfaf490c89a..42936e04fb67 100644 --- a/disk/part_efi.c +++ b/disk/part_efi.c @@ -229,10 +229,10 @@ int test_part_efi(block_dev_desc_t * dev_desc) */ static int set_protective_mbr(block_dev_desc_t *dev_desc) {
- legacy_mbr *p_mbr;
- /* Setup the Protective MBR */
- p_mbr = calloc(1, sizeof(p_mbr));
- ALLOC_CACHE_ALIGN_BUFFER(legacy_mbr, p_mbr, 1);
- memset(p_mbr, 0, sizeof(*p_mbr));
- if (p_mbr == NULL) { printf("%s: calloc failed!\n", __func__); return -1;
@@ -247,11 +247,9 @@ static int set_protective_mbr(block_dev_desc_t *dev_desc) if (dev_desc->block_write(dev_desc->dev, 0, 1, p_mbr) !=
- { printf("** Can't write to device %d **\n", dev_desc->dev);
free(p_mbr);
return -1; }
free(p_mbr); return 0;
}

On Mon, Feb 24, 2014 at 04:46:05PM +0100, Lukasz Majewski wrote:
Hi Tom,
The calloc() call was allocating space for the sizeof the struct pointer rather than for the struct contents. Besides, since this buffer is passed to mmc for writing and some platforms may use cache, the legacy_mbr struct should be cache-aligned.
Is there any problem with this patch?
Just started re-reviewing this one today in fact, good timing.

On Thu, Feb 13, 2014 at 09:48:24AM +0100, Hector Palacios wrote:
The calloc() call was allocating space for the sizeof the struct pointer rather than for the struct contents. Besides, since this buffer is passed to mmc for writing and some platforms may use cache, the legacy_mbr struct should be cache-aligned.
Signed-off-by: Hector Palacios hector.palacios@digi.com Tested-by: Lukasz Majewski l.majewski@samsung.com
Applied to u-boot/master, thanks!
participants (3)
-
Hector Palacios
-
Lukasz Majewski
-
Tom Rini