[U-Boot] [PATCH] Fix some bugs in the EFI support

[U-Boot] [PATCH] Fix some bugs in the EFI support

[Gabe Black]

The ALLOC_CACHE_ALIGN_BUFFER macro allocates a pointer to a buffer, and the
address of the pointer and not the buffer was being passed to a function
that expected the other. This change fixes a few places that bug crops up.
Also, it provides a default definition for the CONFIG_SYS_CACHELINE_SIZE
macro to avoid compiler errors.

Signed-off-by: Gabe Black <gabeblack@chromium.org>
---
 disk/part_efi.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/disk/part_efi.c b/disk/part_efi.c
index e7f2714..c94d808 100644
--- a/disk/part_efi.c
+++ b/disk/part_efi.c
@@ -37,6 +37,10 @@
 #include "part_efi.h"
 #include <linux/ctype.h>
 
+#ifndef CONFIG_SYS_CACHELINE_SIZE
+#define CONFIG_SYS_CACHELINE_SIZE __BIGGEST_ALIGNMENT__
+#endif
+
 #if defined(CONFIG_CMD_IDE) || \
     defined(CONFIG_CMD_MG_DISK) || \
     defined(CONFIG_CMD_SATA) || \
@@ -130,7 +134,7 @@ void print_part_efi(block_dev_desc_t * dev_desc)
 	}
 	/* This function validates AND fills in the GPT header and PTE */
 	if (is_gpt_valid(dev_desc, GPT_PRIMARY_PARTITION_TABLE_LBA,
-			 &(gpt_head), &gpt_pte) != 1) {
+			 gpt_head, &gpt_pte) != 1) {
 		printf("%s: *** ERROR: Invalid GPT ***\n", __func__);
 		return;
 	}
@@ -169,7 +173,7 @@ int get_partition_info_efi(block_dev_desc_t * dev_desc, int part,
 
 	/* This function validates AND fills in the GPT header and PTE */
 	if (is_gpt_valid(dev_desc, GPT_PRIMARY_PARTITION_TABLE_LBA,
-			&(gpt_head), &gpt_pte) != 1) {
+			gpt_head, &gpt_pte) != 1) {
 		printf("%s: *** ERROR: Invalid GPT ***\n", __func__);
 		return -1;
 	}
-- 
1.7.3.1

[U-Boot] [PATCH] Fix some bugs in the EFI support

[Graeme Russ]

Hi Anton,

On 30/11/11 10:39, Gabe Black wrote:
> The ALLOC_CACHE_ALIGN_BUFFER macro allocates a pointer to a buffer, and the
> address of the pointer and not the buffer was being passed to a function
> that expected the other. This change fixes a few places that bug crops up.
> Also, it provides a default definition for the CONFIG_SYS_CACHELINE_SIZE
> macro to avoid compiler errors.
> 
> Signed-off-by: Gabe Black <gabeblack@chromium.org>
> ---
>  disk/part_efi.c |    8 ++++++--
>  1 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/disk/part_efi.c b/disk/part_efi.c
> index e7f2714..c94d808 100644
> --- a/disk/part_efi.c
> +++ b/disk/part_efi.c
> @@ -37,6 +37,10 @@
>  #include "part_efi.h"
>  #include <linux/ctype.h>
>  
> +#ifndef CONFIG_SYS_CACHELINE_SIZE
> +#define CONFIG_SYS_CACHELINE_SIZE __BIGGEST_ALIGNMENT__
> +#endif
> +
>  #if defined(CONFIG_CMD_IDE) || \
>      defined(CONFIG_CMD_MG_DISK) || \
>      defined(CONFIG_CMD_SATA) || \
> @@ -130,7 +134,7 @@ void print_part_efi(block_dev_desc_t * dev_desc)
>  	}
>  	/* This function validates AND fills in the GPT header and PTE */
>  	if (is_gpt_valid(dev_desc, GPT_PRIMARY_PARTITION_TABLE_LBA,
> -			 &(gpt_head), &gpt_pte) != 1) {
> +			 gpt_head, &gpt_pte) != 1) {
>  		printf("%s: *** ERROR: Invalid GPT ***\n", __func__);
>  		return;
>  	}
> @@ -169,7 +173,7 @@ int get_partition_info_efi(block_dev_desc_t * dev_desc, int part,
>  
>  	/* This function validates AND fills in the GPT header and PTE */
>  	if (is_gpt_valid(dev_desc, GPT_PRIMARY_PARTITION_TABLE_LBA,
> -			&(gpt_head), &gpt_pte) != 1) {
> +			gpt_head, &gpt_pte) != 1) {
>  		printf("%s: *** ERROR: Invalid GPT ***\n", __func__);
>  		return -1;
>  	}

This appears to be a fix for your "part_efi: dcache: allocate cacheline
aligned buffers" commit - Can you have a look at it please? If you ack, I
will apply to staging

Regards,

Graeme

[U-Boot] [PATCH] Fix some bugs in the EFI support

[Simon Glass]

Hi Graeme,

On Fri, Dec 2, 2011 at 1:28 PM, Graeme Russ <graeme.russ@gmail.com> wrote:
> Hi Anton,
>
> On 30/11/11 10:39, Gabe Black wrote:
>> The ALLOC_CACHE_ALIGN_BUFFER macro allocates a pointer to a buffer, and the
>> address of the pointer and not the buffer was being passed to a function
>> that expected the other. This change fixes a few places that bug crops up.
>> Also, it provides a default definition for the CONFIG_SYS_CACHELINE_SIZE
>> macro to avoid compiler errors.
>>
>> Signed-off-by: Gabe Black <gabeblack@chromium.org>
>> ---
>> ?disk/part_efi.c | ? ?8 ++++++--
>> ?1 files changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/disk/part_efi.c b/disk/part_efi.c
>> index e7f2714..c94d808 100644
>> --- a/disk/part_efi.c
>> +++ b/disk/part_efi.c
>> @@ -37,6 +37,10 @@
>> ?#include "part_efi.h"
>> ?#include <linux/ctype.h>
>>
>> +#ifndef CONFIG_SYS_CACHELINE_SIZE
>> +#define CONFIG_SYS_CACHELINE_SIZE __BIGGEST_ALIGNMENT__
>> +#endif
>> +
>> ?#if defined(CONFIG_CMD_IDE) || \
>> ? ? ?defined(CONFIG_CMD_MG_DISK) || \
>> ? ? ?defined(CONFIG_CMD_SATA) || \
>> @@ -130,7 +134,7 @@ void print_part_efi(block_dev_desc_t * dev_desc)
>> ? ? ? }
>> ? ? ? /* This function validates AND fills in the GPT header and PTE */
>> ? ? ? if (is_gpt_valid(dev_desc, GPT_PRIMARY_PARTITION_TABLE_LBA,
>> - ? ? ? ? ? ? ? ? ? ? ?&(gpt_head), &gpt_pte) != 1) {
>> + ? ? ? ? ? ? ? ? ? ? ?gpt_head, &gpt_pte) != 1) {
>> ? ? ? ? ? ? ? printf("%s: *** ERROR: Invalid GPT ***\n", __func__);
>> ? ? ? ? ? ? ? return;
>> ? ? ? }
>> @@ -169,7 +173,7 @@ int get_partition_info_efi(block_dev_desc_t * dev_desc, int part,
>>
>> ? ? ? /* This function validates AND fills in the GPT header and PTE */
>> ? ? ? if (is_gpt_valid(dev_desc, GPT_PRIMARY_PARTITION_TABLE_LBA,
>> - ? ? ? ? ? ? ? ? ? ? &(gpt_head), &gpt_pte) != 1) {
>> + ? ? ? ? ? ? ? ? ? ? gpt_head, &gpt_pte) != 1) {
>> ? ? ? ? ? ? ? printf("%s: *** ERROR: Invalid GPT ***\n", __func__);
>> ? ? ? ? ? ? ? return -1;
>> ? ? ? }
>
> This appears to be a fix for your "part_efi: dcache: allocate cacheline
> aligned buffers" commit - Can you have a look at it please? If you ack, I
> will apply to staging

A fix for this is apparently already in the ARM tree. Perhaps it could
be applied so everyone can see it?

Regards,
Simon


>
> Regards,
>
> Graeme
>
>

[U-Boot] [PATCH] Fix some bugs in the EFI support

[Anton Staaf]

On Fri, Dec 2, 2011 at 1:59 PM, Simon Glass <sjg@chromium.org> wrote:
> Hi Graeme,
>
> On Fri, Dec 2, 2011 at 1:28 PM, Graeme Russ <graeme.russ@gmail.com> wrote:
>> Hi Anton,
>>
>> On 30/11/11 10:39, Gabe Black wrote:
>>> The ALLOC_CACHE_ALIGN_BUFFER macro allocates a pointer to a buffer, and the
>>> address of the pointer and not the buffer was being passed to a function
>>> that expected the other. This change fixes a few places that bug crops up.
>>> Also, it provides a default definition for the CONFIG_SYS_CACHELINE_SIZE
>>> macro to avoid compiler errors.
>>>
>>> Signed-off-by: Gabe Black <gabeblack@chromium.org>
>>> ---
>>> ?disk/part_efi.c | ? ?8 ++++++--
>>> ?1 files changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/disk/part_efi.c b/disk/part_efi.c
>>> index e7f2714..c94d808 100644
>>> --- a/disk/part_efi.c
>>> +++ b/disk/part_efi.c
>>> @@ -37,6 +37,10 @@
>>> ?#include "part_efi.h"
>>> ?#include <linux/ctype.h>
>>>
>>> +#ifndef CONFIG_SYS_CACHELINE_SIZE
>>> +#define CONFIG_SYS_CACHELINE_SIZE __BIGGEST_ALIGNMENT__
>>> +#endif
>>> +
>>> ?#if defined(CONFIG_CMD_IDE) || \
>>> ? ? ?defined(CONFIG_CMD_MG_DISK) || \
>>> ? ? ?defined(CONFIG_CMD_SATA) || \
>>> @@ -130,7 +134,7 @@ void print_part_efi(block_dev_desc_t * dev_desc)
>>> ? ? ? }
>>> ? ? ? /* This function validates AND fills in the GPT header and PTE */
>>> ? ? ? if (is_gpt_valid(dev_desc, GPT_PRIMARY_PARTITION_TABLE_LBA,
>>> - ? ? ? ? ? ? ? ? ? ? ?&(gpt_head), &gpt_pte) != 1) {
>>> + ? ? ? ? ? ? ? ? ? ? ?gpt_head, &gpt_pte) != 1) {
>>> ? ? ? ? ? ? ? printf("%s: *** ERROR: Invalid GPT ***\n", __func__);
>>> ? ? ? ? ? ? ? return;
>>> ? ? ? }
>>> @@ -169,7 +173,7 @@ int get_partition_info_efi(block_dev_desc_t * dev_desc, int part,
>>>
>>> ? ? ? /* This function validates AND fills in the GPT header and PTE */
>>> ? ? ? if (is_gpt_valid(dev_desc, GPT_PRIMARY_PARTITION_TABLE_LBA,
>>> - ? ? ? ? ? ? ? ? ? ? &(gpt_head), &gpt_pte) != 1) {
>>> + ? ? ? ? ? ? ? ? ? ? gpt_head, &gpt_pte) != 1) {
>>> ? ? ? ? ? ? ? printf("%s: *** ERROR: Invalid GPT ***\n", __func__);
>>> ? ? ? ? ? ? ? return -1;
>>> ? ? ? }
>>
>> This appears to be a fix for your "part_efi: dcache: allocate cacheline
>> aligned buffers" commit - Can you have a look at it please? If you ack, I
>> will apply to staging
>
> A fix for this is apparently already in the ARM tree. Perhaps it could
> be applied so everyone can see it?
>
> Regards,
> Simon
>
>
>>
>> Regards,
>>
>> Graeme
>>
>>

Whimper whimper.  This is silly, what happened was that two different
patches collided when committed.  Mine had the correct change to this
file.  Doug Andersons also touched this file but reverted the change.
So when both changes went in the result was that these lines didn't
change.  Since then there have been no fewer than four attempts to get
this simple fix committed.  I have acked and pinged at least two of
them.  One finally made it into the ARM tree.  Someone should just put
this into the main tree because that is where my changes to add the
MACRO went in initially.  Oh, and we really don't want to have a
default for CONFIG_SYS_CACHELINE_SIZE, especially not in this file.

Thanks,
    Anton

[U-Boot] [PATCH] Fix some bugs in the EFI support

[Anatolij Gustschin]

Hi Anton,

On Tue, 6 Dec 2011 10:50:14 -0800
Anton Staaf <robotboy@chromium.org> wrote:
...
> Whimper whimper.  This is silly, what happened was that two different
> patches collided when committed.  Mine had the correct change to this
> file.  Doug Andersons also touched this file but reverted the change.
> So when both changes went in the result was that these lines didn't
> change.  Since then there have been no fewer than four attempts to get
> this simple fix committed.  I have acked and pinged at least two of
> them.  One finally made it into the ARM tree.  Someone should just put
> this into the main tree because that is where my changes to add the
> MACRO went in initially.  Oh, and we really don't want to have a
> default for CONFIG_SYS_CACHELINE_SIZE, especially not in this file.

One of the acked patches [1] is in mainline u-boot.git/master now [2].

We will have to double-check after Wolfgang pulls from Albert's ARM
tree since there is another patch [3] addressing this and it
additionally changes CONFIG_SYS_CACHELINE_SIZE to ARCH_DMA_MINALIGN.

Thanks,
Anatolij

[1] http://patchwork.ozlabs.org/patch/122485/
[2] http://git.denx.de/?p=u-boot.git;a=commit;h=4715a81136049167160230efd28eb9d48eeded1f
[3] http://git.denx.de/?p=u-boot/u-boot-arm.git;a=commit;h=46ccae75e9f9c497a3e0e3c55a3173c2801db41d

[U-Boot] [PATCH] Fix some bugs in the EFI support

[Wolfgang Denk]

Dear Anton Staaf,

In message <CAF6FioWeoF1RWckrfWUsoBcybwU4g4xp84pROirb5HcbfJKEZQ@mail.gmail.com> you wrote:
>
> Whimper whimper.  This is silly, what happened was that two different
> patches collided when committed.  Mine had the correct change to this
> file.  Doug Andersons also touched this file but reverted the change.
> So when both changes went in the result was that these lines didn't
> change.  Since then there have been no fewer than four attempts to get
> this simple fix committed.  I have acked and pinged at least two of
> them.  One finally made it into the ARM tree.  Someone should just put
> this into the main tree because that is where my changes to add the
> MACRO went in initially.  Oh, and we really don't want to have a
> default for CONFIG_SYS_CACHELINE_SIZE, especially not in this file.

What exactly is wrong - in your opinion - in the current master?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
SW engineering is a race between programmers trying  to  make  better
idiot-proof programs and the universe producing greater idiots.

[U-Boot] [PATCH] Fix some bugs in the EFI support

[Anton Staaf]

On Tue, Dec 6, 2011 at 11:35 AM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Anton Staaf,
>
> In message <CAF6FioWeoF1RWckrfWUsoBcybwU4g4xp84pROirb5HcbfJKEZQ@mail.gmail.com> you wrote:
>>
>> Whimper whimper. ?This is silly, what happened was that two different
>> patches collided when committed. ?Mine had the correct change to this
>> file. ?Doug Andersons also touched this file but reverted the change.
>> So when both changes went in the result was that these lines didn't
>> change. ?Since then there have been no fewer than four attempts to get
>> this simple fix committed. ?I have acked and pinged at least two of
>> them. ?One finally made it into the ARM tree. ?Someone should just put
>> this into the main tree because that is where my changes to add the
>> MACRO went in initially. ?Oh, and we really don't want to have a
>> default for CONFIG_SYS_CACHELINE_SIZE, especially not in this file.
>
> What exactly is wrong - in your opinion - in the current master?

I don't believe that there is anything wrong right now.  My whimpering
was more about the sheer number of messages have been generated by
this patch collision.  Not a big deal since as far as I can tell
master is OK.

Thanks,
    Anton

> Best regards,
>
> Wolfgang Denk
>
> --
> DENX Software Engineering GmbH, ? ? MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
> SW engineering is a race between programmers trying ?to ?make ?better
> idiot-proof programs and the universe producing greater idiots.