Patchwork [U-Boot,v3] hwconfig: Fix dummy initialization of {board, cpu}_hwconfig

login
register
mail settings
Submitter Kumar Gala
Date Nov. 30, 2010, 9:01 p.m.
Message ID <1291150888-20937-1-git-send-email-galak@kernel.crashing.org>
Download mbox | patch
Permalink /patch/73656/
State Accepted
Commit b194577b2995eacbd2a3769269f0af1bc5e22530
Delegated to: Wolfgang Denk
Headers show

Comments

Kumar Gala - Nov. 30, 2010, 9:01 p.m.
Since board_hwconfig & cpu_hwconfig are defined as weak and dont have a
default value they will get put into the BSS if they aren't defined
elsewhere.  This is problematic as we try to utilize hwconfig before
we've relocated and thus BSS isn't setup.

Instead of giving dummy values in the board files that utilize this
feature, we can just initialize the variables to an empty string and
thus move them out of the BSS if they aren't defined elsewhere.

Also made board_hwconfig & cpu_hwconfig arrays to reduce size associated
with string pointers vs arrays.

Signed-off-by: Kumar Gala <galak@kernel.crashing.org>
---
* Removed null checks for board_hwconfig & cpu_hwconfig 

 board/freescale/mpc8641hpcn/mpc8641hpcn.c |    3 ---
 board/freescale/p2020ds/p2020ds.c         |    3 ---
 common/hwconfig.c                         |   14 ++++++--------
 3 files changed, 6 insertions(+), 14 deletions(-)
Wolfgang Denk - Nov. 30, 2010, 9:11 p.m.
Dear Kumar Gala,

In message <1291150888-20937-1-git-send-email-galak@kernel.crashing.org> you wrote:
> Since board_hwconfig & cpu_hwconfig are defined as weak and dont have a
> default value they will get put into the BSS if they aren't defined
> elsewhere.  This is problematic as we try to utilize hwconfig before
> we've relocated and thus BSS isn't setup.
> 
> Instead of giving dummy values in the board files that utilize this
> feature, we can just initialize the variables to an empty string and
> thus move them out of the BSS if they aren't defined elsewhere.
> 
> Also made board_hwconfig & cpu_hwconfig arrays to reduce size associated
> with string pointers vs arrays.
> 
> Signed-off-by: Kumar Gala <galak@kernel.crashing.org>
> ---
> * Removed null checks for board_hwconfig & cpu_hwconfig 
> 
>  board/freescale/mpc8641hpcn/mpc8641hpcn.c |    3 ---
>  board/freescale/p2020ds/p2020ds.c         |    3 ---
>  common/hwconfig.c                         |   14 ++++++--------
>  3 files changed, 6 insertions(+), 14 deletions(-)

Applied, thanks.

Best regards,

Wolfgang Denk
Scott Wood - Nov. 30, 2010, 9:12 p.m.
On Tue, 30 Nov 2010 15:01:28 -0600
Kumar Gala <galak@kernel.crashing.org> wrote:

> diff --git a/common/hwconfig.c b/common/hwconfig.c
> index 3c9759f..da8d3ed 100644
> --- a/common/hwconfig.c
> +++ b/common/hwconfig.c
> @@ -68,8 +68,8 @@ next:
>  	return NULL;
>  }
>  
> -const char *cpu_hwconfig __attribute__((weak));
> -const char *board_hwconfig __attribute__((weak));
> +const char cpu_hwconfig[] __attribute__((weak)) = "";
> +const char board_hwconfig[] __attribute__((weak)) = "";
>  
>  #define HWCONFIG_PRE_RELOC_BUF_SIZE	128
>  
> @@ -96,13 +96,11 @@ static const char *__hwconfig(const char *opt, size_t *arglen)
>  		return hwconfig_parse(env_hwconfig, strlen(env_hwconfig),
>  				      opt, ";", ':', arglen);
>  
> -	if (board_hwconfig)
> -		return hwconfig_parse(board_hwconfig, strlen(board_hwconfig),
> -				      opt, ";", ':', arglen);
> +	return hwconfig_parse(board_hwconfig, strlen(board_hwconfig),
> +			opt, ";", ':', arglen);
>  
> -	if (cpu_hwconfig)
> -		return hwconfig_parse(cpu_hwconfig, strlen(cpu_hwconfig),
> -				      opt, ";", ':', arglen);
> +	return hwconfig_parse(cpu_hwconfig, strlen(cpu_hwconfig),
> +			opt, ";", ':', arglen);
>  
>  	return NULL;
>  }

Hmm.  "return x; return y; return NULL;"

Was the presence of a board hwconfig really intended to override, rather
than add to, the cpu hwconfig?  Should we check the return of the first
hwconfig_parse to see if it found anything?

-Scott
Kumar Gala - Nov. 30, 2010, 9:24 p.m.
On Nov 30, 2010, at 3:12 PM, Scott Wood wrote:

> On Tue, 30 Nov 2010 15:01:28 -0600
> Kumar Gala <galak@kernel.crashing.org> wrote:
> 
>> diff --git a/common/hwconfig.c b/common/hwconfig.c
>> index 3c9759f..da8d3ed 100644
>> --- a/common/hwconfig.c
>> +++ b/common/hwconfig.c
>> @@ -68,8 +68,8 @@ next:
>> 	return NULL;
>> }
>> 
>> -const char *cpu_hwconfig __attribute__((weak));
>> -const char *board_hwconfig __attribute__((weak));
>> +const char cpu_hwconfig[] __attribute__((weak)) = "";
>> +const char board_hwconfig[] __attribute__((weak)) = "";
>> 
>> #define HWCONFIG_PRE_RELOC_BUF_SIZE	128
>> 
>> @@ -96,13 +96,11 @@ static const char *__hwconfig(const char *opt, size_t *arglen)
>> 		return hwconfig_parse(env_hwconfig, strlen(env_hwconfig),
>> 				      opt, ";", ':', arglen);
>> 
>> -	if (board_hwconfig)
>> -		return hwconfig_parse(board_hwconfig, strlen(board_hwconfig),
>> -				      opt, ";", ':', arglen);
>> +	return hwconfig_parse(board_hwconfig, strlen(board_hwconfig),
>> +			opt, ";", ':', arglen);
>> 
>> -	if (cpu_hwconfig)
>> -		return hwconfig_parse(cpu_hwconfig, strlen(cpu_hwconfig),
>> -				      opt, ";", ':', arglen);
>> +	return hwconfig_parse(cpu_hwconfig, strlen(cpu_hwconfig),
>> +			opt, ";", ':', arglen);
>> 
>> 	return NULL;
>> }
> 
> Hmm.  "return x; return y; return NULL;"
> 
> Was the presence of a board hwconfig really intended to override, rather
> than add to, the cpu hwconfig?  Should we check the return of the first
> hwconfig_parse to see if it found anything?

Yeah, I'll fix this - didn't even notice it before.

I'm going to make it:
	ret = hwconfig_parse(env_hwconfig, ...)
	if (ret)
		return ret;
	ret = hwconfig_parse(board_hwconfig, ...)
	if (ret)
		return ret;

	return hwconfig_parse(cpu_hwconfig, ...);

- k

Patch

diff --git a/board/freescale/mpc8641hpcn/mpc8641hpcn.c b/board/freescale/mpc8641hpcn/mpc8641hpcn.c
index 812111d..882ff0b 100644
--- a/board/freescale/mpc8641hpcn/mpc8641hpcn.c
+++ b/board/freescale/mpc8641hpcn/mpc8641hpcn.c
@@ -60,9 +60,6 @@  int checkboard(void)
 	return 0;
 }
 
-const char *board_hwconfig = "foo:bar=baz";
-const char *cpu_hwconfig = "foo:bar=baz";
-
 phys_size_t
 initdram(int board_type)
 {
diff --git a/board/freescale/p2020ds/p2020ds.c b/board/freescale/p2020ds/p2020ds.c
index b507677..b05ef98 100644
--- a/board/freescale/p2020ds/p2020ds.c
+++ b/board/freescale/p2020ds/p2020ds.c
@@ -69,9 +69,6 @@  int checkboard(void)
 	return 0;
 }
 
-const char *board_hwconfig = "foo:bar=baz";
-const char *cpu_hwconfig = "foo:bar=baz";
-
 phys_size_t initdram(int board_type)
 {
 	phys_size_t dram_size = 0;
diff --git a/common/hwconfig.c b/common/hwconfig.c
index 3c9759f..da8d3ed 100644
--- a/common/hwconfig.c
+++ b/common/hwconfig.c
@@ -68,8 +68,8 @@  next:
 	return NULL;
 }
 
-const char *cpu_hwconfig __attribute__((weak));
-const char *board_hwconfig __attribute__((weak));
+const char cpu_hwconfig[] __attribute__((weak)) = "";
+const char board_hwconfig[] __attribute__((weak)) = "";
 
 #define HWCONFIG_PRE_RELOC_BUF_SIZE	128
 
@@ -96,13 +96,11 @@  static const char *__hwconfig(const char *opt, size_t *arglen)
 		return hwconfig_parse(env_hwconfig, strlen(env_hwconfig),
 				      opt, ";", ':', arglen);
 
-	if (board_hwconfig)
-		return hwconfig_parse(board_hwconfig, strlen(board_hwconfig),
-				      opt, ";", ':', arglen);
+	return hwconfig_parse(board_hwconfig, strlen(board_hwconfig),
+			opt, ";", ':', arglen);
 
-	if (cpu_hwconfig)
-		return hwconfig_parse(cpu_hwconfig, strlen(cpu_hwconfig),
-				      opt, ";", ':', arglen);
+	return hwconfig_parse(cpu_hwconfig, strlen(cpu_hwconfig),
+			opt, ";", ':', arglen);
 
 	return NULL;
 }