[PATCH] Check mac-address first in fsl_soc.c

[PATCH] Check mac-address first in fsl_soc.c

[Timur Tabi]

The mac-address property in the device tree should be checked first,
before local-mac-address.  This is because mac-address contains the most
recent MAC address, whereas local-mac-address is the default address.
Depending on the platform and the version of U-Boot, U-Boot will set
one or the other, or both.

This patch updates gfar_of_init() and fs_enet_of_init() to conform to
this order.  It skips a property if it doesn't exist or if it contains
an all-zero MAC address.  This patch also adds some NULL-pointer checking
to make sure there are no panics if no MAC address has been passed.

Signed-off-by: Timur Tabi <timur@freescale.com>
---
 arch/powerpc/sysdev/fsl_soc.c |   39 +++++++++++++++++++++++++++++----------
 1 files changed, 29 insertions(+), 10 deletions(-)

diff --git a/arch/powerpc/sysdev/fsl_soc.c b/arch/powerpc/sysdev/fsl_soc.c
index 9f2a9a4..a0586ea 100644
--- a/arch/powerpc/sysdev/fsl_soc.c
+++ b/arch/powerpc/sysdev/fsl_soc.c
@@ -233,12 +233,13 @@ static int __init gfar_of_init(void)
 			goto err;
 		}
 
-		mac_addr = get_property(np, "local-mac-address", NULL);
-		if (mac_addr == NULL)
-			mac_addr = get_property(np, "mac-address", NULL);
-		if (mac_addr == NULL) {
-			/* Obsolete */
-			mac_addr = get_property(np, "address", NULL);
+		mac_addr = get_property(np, "mac-address", NULL);
+		if (!mac_addr || (memcmp(mac_addr, "\0\0\0\0\0", 6) == 0)) {
+			mac_addr = get_property(np, "local-mac-address", NULL);
+			if (!mac_addr || (memcmp(mac_addr, "\0\0\0\0\0", 6) == 0)) {
+				/* Obsolete */
+				mac_addr = get_property(np, "address", NULL);
+			}
 		}
 
 		if (mac_addr)
@@ -607,7 +608,16 @@ static int __init fs_enet_of_init(void)
 		}
 
 		mac_addr = get_property(np, "mac-address", NULL);
-		memcpy(fs_enet_data.macaddr, mac_addr, 6);
+		if (!mac_addr || (memcmp(mac_addr, "\0\0\0\0\0", 6) == 0)) {
+			mac_addr = get_property(np, "local-mac-address", NULL);
+			if (!mac_addr || (memcmp(mac_addr, "\0\0\0\0\0", 6) == 0)) {
+				/* Obsolete */
+				mac_addr = get_property(np, "address", NULL);
+			}
+		}
+
+		if (mac_addr)
+			memcpy(fs_enet_data.macaddr, mac_addr, 6);
 
 		ph = get_property(np, "phy-handle", NULL);
 		phy = of_find_node_by_phandle(*ph);
@@ -699,7 +709,7 @@ static int __init fs_enet_of_init(void)
 				if (ret)
 					goto unreg;
 			}
-			
+
 			of_node_put(phy);
 			of_node_put(mdio);
 
@@ -891,8 +901,17 @@ static int __init fs_enet_of_init(void)
 			goto err;
 		r[0].name = enet_regs;
 
-		mac_addr = (void *)get_property(np, "mac-address", NULL);
-		memcpy(fs_enet_data.macaddr, mac_addr, 6);
+		mac_addr = get_property(np, "mac-address", NULL);
+		if (!mac_addr || (memcmp(mac_addr, "\0\0\0\0\0", 6) == 0)) {
+			mac_addr = get_property(np, "local-mac-address", NULL);
+			if (!mac_addr || (memcmp(mac_addr, "\0\0\0\0\0", 6) == 0)) {
+				/* Obsolete */
+				mac_addr = get_property(np, "address", NULL);
+			}
+		}
+
+		if (mac_addr)
+			memcpy(fs_enet_data.macaddr, mac_addr, 6);
 
 		ph = (phandle *) get_property(np, "phy-handle", NULL);
 		if (ph != NULL)
-- 
1.4.4

Re: [PATCH] Check mac-address first in fsl_soc.c

[Sergei Shtylyov]

Hello.

Timur Tabi wrote:
> The mac-address property in the device tree should be checked first,
> before local-mac-address.  This is because mac-address contains the most
> recent MAC address, whereas local-mac-address is the default address.
> Depending on the platform and the version of U-Boot, U-Boot will set
> one or the other, or both.

    Argh, *when* it will be setting both?

> This patch updates gfar_of_init() and fs_enet_of_init() to conform to
> this order.  It skips a property if it doesn't exist or if it contains
> an all-zero MAC address.  This patch also adds some NULL-pointer checking
> to make sure there are no panics if no MAC address has been passed.

> diff --git a/arch/powerpc/sysdev/fsl_soc.c b/arch/powerpc/sysdev/fsl_soc.c
> index 9f2a9a4..a0586ea 100644
> --- a/arch/powerpc/sysdev/fsl_soc.c
> +++ b/arch/powerpc/sysdev/fsl_soc.c
> @@ -233,12 +233,13 @@ static int __init gfar_of_init(void)
>  			goto err;
>  		}
>  
> -		mac_addr = get_property(np, "local-mac-address", NULL);
> -		if (mac_addr == NULL)
> -			mac_addr = get_property(np, "mac-address", NULL);
> -		if (mac_addr == NULL) {
> -			/* Obsolete */
> -			mac_addr = get_property(np, "address", NULL);
> +		mac_addr = get_property(np, "mac-address", NULL);
> +		if (!mac_addr || (memcmp(mac_addr, "\0\0\0\0\0", 6) == 0)) {
> +			mac_addr = get_property(np, "local-mac-address", NULL);
> +			if (!mac_addr || (memcmp(mac_addr, "\0\0\0\0\0", 6) == 0)) {
> +				/* Obsolete */
> +				mac_addr = get_property(np, "address", NULL);
> +			}
>  		}
>  
>  		if (mac_addr)
> @@ -607,7 +608,16 @@ static int __init fs_enet_of_init(void)
>  		}
>  
>  		mac_addr = get_property(np, "mac-address", NULL);
> -		memcpy(fs_enet_data.macaddr, mac_addr, 6);
> +		if (!mac_addr || (memcmp(mac_addr, "\0\0\0\0\0", 6) == 0)) {
> +			mac_addr = get_property(np, "local-mac-address", NULL);
> +			if (!mac_addr || (memcmp(mac_addr, "\0\0\0\0\0", 6) == 0)) {
> +				/* Obsolete */
> +				mac_addr = get_property(np, "address", NULL);
> +			}
> +		}
> +
> +		if (mac_addr)
> +			memcpy(fs_enet_data.macaddr, mac_addr, 6);
>  
>  		ph = get_property(np, "phy-handle", NULL);
>  		phy = of_find_node_by_phandle(*ph);
[...]
> @@ -891,8 +901,17 @@ static int __init fs_enet_of_init(void)
>  			goto err;
>  		r[0].name = enet_regs;
>  
> -		mac_addr = (void *)get_property(np, "mac-address", NULL);
> -		memcpy(fs_enet_data.macaddr, mac_addr, 6);
> +		mac_addr = get_property(np, "mac-address", NULL);
> +		if (!mac_addr || (memcmp(mac_addr, "\0\0\0\0\0", 6) == 0)) {
> +			mac_addr = get_property(np, "local-mac-address", NULL);
> +			if (!mac_addr || (memcmp(mac_addr, "\0\0\0\0\0", 6) == 0)) {
> +				/* Obsolete */
> +				mac_addr = get_property(np, "address", NULL);
> +			}
> +		}
> +
> +		if (mac_addr)
> +			memcpy(fs_enet_data.macaddr, mac_addr, 6);

    These are just asking to be put into a separate function... :-)

>  
>  		ph = (phandle *) get_property(np, "phy-handle", NULL);
>  		if (ph != NULL)

WBR, Sergei

Re: [PATCH] Check mac-address first in fsl_soc.c

[Timur Tabi]

Sergei Shtylyov wrote:
> Hello.
> 
> Timur Tabi wrote:
>> The mac-address property in the device tree should be checked first,
>> before local-mac-address.  This is because mac-address contains the most
>> recent MAC address, whereas local-mac-address is the default address.
>> Depending on the platform and the version of U-Boot, U-Boot will set
>> one or the other, or both.
> 
>    Argh, *when* it will be setting both?

I'm working on a patch now to implement that for 83xx.  Whether/when it will be 
applied is anyone's guess.

>> -        mac_addr = (void *)get_property(np, "mac-address", NULL);
>> -        memcpy(fs_enet_data.macaddr, mac_addr, 6);
>> +        mac_addr = get_property(np, "mac-address", NULL);
>> +        if (!mac_addr || (memcmp(mac_addr, "\0\0\0\0\0", 6) == 0)) {
>> +            mac_addr = get_property(np, "local-mac-address", NULL);
>> +            if (!mac_addr || (memcmp(mac_addr, "\0\0\0\0\0", 6) == 0)) {
>> +                /* Obsolete */
>> +                mac_addr = get_property(np, "address", NULL);
>> +            }
>> +        }
>> +
>> +        if (mac_addr)
>> +            memcpy(fs_enet_data.macaddr, mac_addr, 6);
> 
>    These are just asking to be put into a separate function... :-)

I wanted to keep my patch simple.


-- 
Timur Tabi
Linux Kernel Developer @ Freescale

Re: [PATCH] Check mac-address first in fsl_soc.c

[Sergei Shtylyov]

Hello.

Timur Tabi wrote:

>>> The mac-address property in the device tree should be checked first,
>>> before local-mac-address.  This is because mac-address contains the most
>>> recent MAC address, whereas local-mac-address is the default address.
>>> Depending on the platform and the version of U-Boot, U-Boot will set
>>> one or the other, or both.

>>    Argh, *when* it will be setting both?

> I'm working on a patch now to implement that for 83xx.  Whether/when it 
> will be applied is anyone's guess.

    I'm mainly concerned about 85xx. I guess the needed change should be very 
alike...

>>> -        mac_addr = (void *)get_property(np, "mac-address", NULL);
>>> -        memcpy(fs_enet_data.macaddr, mac_addr, 6);
>>> +        mac_addr = get_property(np, "mac-address", NULL);
>>> +        if (!mac_addr || (memcmp(mac_addr, "\0\0\0\0\0", 6) == 0)) {
>>> +            mac_addr = get_property(np, "local-mac-address", NULL);
>>> +            if (!mac_addr || (memcmp(mac_addr, "\0\0\0\0\0", 6) == 
>>> 0)) {
>>> +                /* Obsolete */
>>> +                mac_addr = get_property(np, "address", NULL);
>>> +            }
>>> +        }
>>> +
>>> +        if (mac_addr)
>>> +            memcpy(fs_enet_data.macaddr, mac_addr, 6);
>>
>>
>>    These are just asking to be put into a separate function... :-)

> I wanted to keep my patch simple.

    That's clearly an over-simplification to repeat the same code thrice.

WBR, Sergei

Re: [PATCH] Check mac-address first in fsl_soc.c

[Timur Tabi]

Sergei Shtylyov wrote:

>    I'm mainly concerned about 85xx. I guess the needed change should be 
> very alike...

Each family is maintained separately, so even if the resulting code is the same, 
the patches need to be created individually.  I'll do them for 85xx and 86xx, too.

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

Re: [PATCH] Check mac-address first in fsl_soc.c

[Kumar Gala]

On Feb 9, 2007, at 3:00 PM, Timur Tabi wrote:

> Sergei Shtylyov wrote:
>
>>    I'm mainly concerned about 85xx. I guess the needed change  
>> should be
>> very alike...
>
> Each family is maintained separately, so even if the resulting code  
> is the same,
> the patches need to be created individually.  I'll do them for 85xx  
> and 86xx, too.

I assume you're talking about the u-boot side.

- k

Re: [PATCH] Check mac-address first in fsl_soc.c

[Timur Tabi]

Kumar Gala wrote:

>> Each family is maintained separately, so even if the resulting code is 
>> the same,
>> the patches need to be created individually.  I'll do them for 85xx 
>> and 86xx, too.
> 
> I assume you're talking about the u-boot side.

Yes.


-- 
Timur Tabi
Linux Kernel Developer @ Freescale

Re: [PATCH] Check mac-address first in fsl_soc.c

[Jerry Van Baren]

Timur Tabi wrote:
> The mac-address property in the device tree should be checked first,
> before local-mac-address.  This is because mac-address contains the most
> recent MAC address, whereas local-mac-address is the default address.
> Depending on the platform and the version of U-Boot, U-Boot will set
> one or the other, or both.
> 
> This patch updates gfar_of_init() and fs_enet_of_init() to conform to
> this order.  It skips a property if it doesn't exist or if it contains
> an all-zero MAC address.  This patch also adds some NULL-pointer checking
> to make sure there are no panics if no MAC address has been passed.
> 
> Signed-off-by: Timur Tabi <timur@freescale.com>
> ---
>  arch/powerpc/sysdev/fsl_soc.c |   39 +++++++++++++++++++++++++++++----------
>  1 files changed, 29 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/powerpc/sysdev/fsl_soc.c b/arch/powerpc/sysdev/fsl_soc.c
> index 9f2a9a4..a0586ea 100644
> --- a/arch/powerpc/sysdev/fsl_soc.c
> +++ b/arch/powerpc/sysdev/fsl_soc.c
> @@ -233,12 +233,13 @@ static int __init gfar_of_init(void)
>  			goto err;
>  		}
>  
> -		mac_addr = get_property(np, "local-mac-address", NULL);
> -		if (mac_addr == NULL)
> -			mac_addr = get_property(np, "mac-address", NULL);
> -		if (mac_addr == NULL) {
> -			/* Obsolete */
> -			mac_addr = get_property(np, "address", NULL);
> +		mac_addr = get_property(np, "mac-address", NULL);
> +		if (!mac_addr || (memcmp(mac_addr, "\0\0\0\0\0", 6) == 0)) {
> +			mac_addr = get_property(np, "local-mac-address", NULL);
> +			if (!mac_addr || (memcmp(mac_addr, "\0\0\0\0\0", 6) == 0)) {
> +				/* Obsolete */
> +				mac_addr = get_property(np, "address", NULL);
> +			}

If you want to check for common errors, add a check that the first byte 
does not have the multicast flag set.  Note that this will catch the 
error of setting the MAC to the broadcast address of all 0xFFs as well 
as the naive users that set it to 11:22:33:44:55:66 (remarkable number 
of them out there).

if (!mac_addr || (memcmp(mac_addr, "\0\0\0\0\0", 6) == 0)
     || ((*mac_addr & 0x01) == 0x01)) {

or, if you are a terse, excuse me, tight C programmer:

if (!mac_addr || (memcmp(mac_addr, "\0\0\0\0\0", 6) == 0)
     || (*mac_addr & 0x01)) {

gvb

Re: [PATCH] Check mac-address first in fsl_soc.c

[Timur Tabi]

Jerry Van Baren wrote:

> If you want to check for common errors, add a check that the first byte 
> does not have the multicast flag set.  

Actually, I'm not checking for errors when I check for all zeros.  I'm checking 
for a situation where the property exists in the device tree, but it has not 
been initialized by U-Boot.

Some versions of U-Boot only update the local-mac-address property, but some 
device trees have both mac-address and local-mac-address in them.  In this case, 
mac-address will be all zeros, and the MAC address will be in local-mac-address.

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

Re: [PATCH] Check mac-address first in fsl_soc.c

[Timur Tabi]

Sergei Shtylyov wrote:

>    I'm mainly concerned about 85xx. I guess the needed change should be 
> very alike...

I've submitted U-Boot patches for 83xx, 85xx, 86xx, and 5xxx to the maintainers. 
  You'll have to ask them when the patches will be applied.

>> I wanted to keep my patch simple.
> 
>    That's clearly an over-simplification to repeat the same code thrice.

Actually, the 2nd and third instances are conditionally compiled, so in most 
cases only one instance of the code will actually be compiled.  gfar_of_init() 
should probably also be conditionally compiled, so if I make it a separate 
function, I'd have to prefix it with something like this:

#if defined(CONFIG_GIANFAR) || defined(CONFIG_CPM2) || defined(CONFIG_8xx)

and then I would need to pass a bunch of parameters.  I don't really think 
that's an improvement.

Paul, if there are no serious objections, please apply this patch.

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

Re: [PATCH] Check mac-address first in fsl_soc.c

[Sergei Shtylyov]

Hello.

Timur Tabi wrote:

>>    I'm mainly concerned about 85xx. I guess the needed change should 
>> be very alike...

> I've submitted U-Boot patches for 83xx, 85xx, 86xx, and 5xxx to the 
> maintainers.  You'll have to ask them when the patches will be applied.

    In fact, I/'m regretting not fixing this myself -- Andy said it'll be 
fixed RSN and it took about 2 months (actually, more since the issue was first 
noticed by me in October)...

>>> I wanted to keep my patch simple.

>>    That's clearly an over-simplification to repeat the same code thrice.

> Actually, the 2nd and third instances are conditionally compiled, so in 

    I think that's a bad excuse. You could have coded inline function at least.

> most cases only one instance of the code will actually be compiled.  
> gfar_of_init() should probably also be conditionally compiled, so if I 

    But it's not?

> make it a separate function, I'd have to prefix it with something like 
> this:

> #if defined(CONFIG_GIANFAR) || defined(CONFIG_CPM2) || defined(CONFIG_8xx)

> and then I would need to pass a bunch of parameters.  I don't really 
> think that's an improvement.

> Paul, if there are no serious objections, please apply this patch.

    I object -- such practice produces the code that is harder to change and 
which is more likely to get out of sync between the 3 instances later. :-/

WBR, Sergei

Re: [PATCH] Check mac-address first in fsl_soc.c

[Kumar Gala]

On Feb 13, 2007, at 11:25 AM, Timur Tabi wrote:

> Sergei Shtylyov wrote:
>
>>    I'm mainly concerned about 85xx. I guess the needed change  
>> should be very alike...
>
> I've submitted U-Boot patches for 83xx, 85xx, 86xx, and 5xxx to the  
> maintainers.  You'll have to ask them when the patches will be  
> applied.
>
>>> I wanted to keep my patch simple.
>>    That's clearly an over-simplification to repeat the same code  
>> thrice.
>
> Actually, the 2nd and third instances are conditionally compiled,  
> so in most cases only one instance of the code will actually be  
> compiled.  gfar_of_init() should probably also be conditionally  
> compiled, so if I make it a separate function, I'd have to prefix  
> it with something like this:
>
> #if defined(CONFIG_GIANFAR) || defined(CONFIG_CPM2) || defined 
> (CONFIG_8xx)
>
> and then I would need to pass a bunch of parameters.  I don't  
> really think that's an improvement.
>
> Paul, if there are no serious objections, please apply this patch.

Paul asked me to look at this.

I'd ask one style change.

	if (!mac_addr || (memcmp(mac_addr, "\0\0\0\0\0", 6) == 0)) {

How about something like comparing against a local null array  
instead.  Something like:

	u8 null_mac_addr[6] = { 0 };
	if (!mac_addr || (memcmp(mac_addr, null_mac_addr, 6) == 0)) {

- k

Re: [PATCH] Check mac-address first in fsl_soc.c

[Timur Tabi]

Sergei Shtylyov wrote:

>> Actually, the 2nd and third instances are conditionally compiled, so in 
> 
>    I think that's a bad excuse. You could have coded inline function at 
> least.

Fair enough.  I'll submit a new patch in a few minutes that makes the code an 
inline function.

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

Re: [PATCH] Check mac-address first in fsl_soc.c

[Timur Tabi]

Kumar Gala wrote:

> How about something like comparing against a local null array instead.  
> Something like:
> 
>     u8 null_mac_addr[6] = { 0 };
>     if (!mac_addr || (memcmp(mac_addr, null_mac_addr, 6) == 0)) {

I believe this will cause the compiler to generate more code, not less.  When 
you declare an initialized array as a local variable, the compiler reserves 
space on the stack, and then it generates code to copy the data from some global 
storage to the stack.

If I inline a string instead, however, the compiler will just put the string in 
global storage and reference it directly.  The compiler should be able to detect 
multiple instances of the same string, and store them as a single string.

If you still want null_mac_addr[], you would need to do this:

	static const u8 null_mac_addr[6] = { 0 };

Although frankly it would be nice if the Ethernet module had static globals and 
functions for this sort of thing.

Do you still want me to create null_mac_addr[], as above?  I was going to put 
the code inside an inline function anyway, per Sergei's request.

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

Re: [PATCH] Check mac-address first in fsl_soc.c

[Kumar Gala]

On Feb 13, 2007, at 11:45 AM, Timur Tabi wrote:

> Kumar Gala wrote:
>
>> How about something like comparing against a local null array  
>> instead.  Something like:
>>     u8 null_mac_addr[6] = { 0 };
>>     if (!mac_addr || (memcmp(mac_addr, null_mac_addr, 6) == 0)) {
>
> I believe this will cause the compiler to generate more code, not  
> less.  When you declare an initialized array as a local variable,  
> the compiler reserves space on the stack, and then it generates  
> code to copy the data from some global storage to the stack.

Actually it just does two stores in this case to the stack space, so  
we talking about 4 instructions:
* stack update
* load 0
* stw 0
* sth 0

vs

the static array that we never get ride of.

I'd prefer the instructions/local variable.

> If I inline a string instead, however, the compiler will just put  
> the string in global storage and reference it directly.  The  
> compiler should be able to detect multiple instances of the same  
> string, and store them as a single string.
>
> If you still want null_mac_addr[], you would need to do this:
>
> 	static const u8 null_mac_addr[6] = { 0 };

Let's drop the static const part.

- k

Re: [PATCH] Check mac-address first in fsl_soc.c

[Timur Tabi]

Kumar Gala wrote:

>> If you still want null_mac_addr[], you would need to do this:
>>
>>     static const u8 null_mac_addr[6] = { 0 };
> 
> Let's drop the static const part.

Ok.

Now all I need to do is fix this error, that has nothing to do with my changes:

arch/powerpc/sysdev/fsl_soc.c: In function 'gfar_of_init':
arch/powerpc/sysdev/fsl_soc.c:305: warning: ISO C90 forbids mixed declarations 
and code
arch/powerpc/sysdev/fsl_soc.c:308: error: invalid storage class for function 
'fsl_i2c_of_init'
arch/powerpc/sysdev/fsl_soc.c:361: error: initializer element is not constant
arch/powerpc/sysdev/fsl_soc.c:365: error: invalid storage class for function 
'mpc83xx_wdt_init'
arch/powerpc/sysdev/fsl_soc.c:423: error: initializer element is not constant
arch/powerpc/sysdev/fsl_soc.c:427: error: invalid storage class for function 
'determine_usb_phy'
arch/powerpc/sysdev/fsl_soc.c:443: error: invalid storage class for function 
'fsl_usb_of_init'
arch/powerpc/sysdev/fsl_soc.c:586: error: initializer element is not constant
arch/powerpc/sysdev/fsl_soc.c:586: error: syntax error at end of input
make[1]: *** [arch/powerpc/sysdev/fsl_soc.o] Error 1

Looks like someone broke arch_initcall() or something.

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

Re: [PATCH] Check mac-address first in fsl_soc.c

[Kumar Gala]

On Feb 13, 2007, at 12:51 PM, Timur Tabi wrote:

> Kumar Gala wrote:
>
>>> If you still want null_mac_addr[], you would need to do this:
>>>
>>>     static const u8 null_mac_addr[6] = { 0 };
>> Let's drop the static const part.
>
> Ok.
>
> Now all I need to do is fix this error, that has nothing to do with  
> my changes:
>
> arch/powerpc/sysdev/fsl_soc.c: In function 'gfar_of_init':
> arch/powerpc/sysdev/fsl_soc.c:305: warning: ISO C90 forbids mixed  
> declarations and code
> arch/powerpc/sysdev/fsl_soc.c:308: error: invalid storage class for  
> function 'fsl_i2c_of_init'
> arch/powerpc/sysdev/fsl_soc.c:361: error: initializer element is  
> not constant
> arch/powerpc/sysdev/fsl_soc.c:365: error: invalid storage class for  
> function 'mpc83xx_wdt_init'
> arch/powerpc/sysdev/fsl_soc.c:423: error: initializer element is  
> not constant
> arch/powerpc/sysdev/fsl_soc.c:427: error: invalid storage class for  
> function 'determine_usb_phy'
> arch/powerpc/sysdev/fsl_soc.c:443: error: invalid storage class for  
> function 'fsl_usb_of_init'
> arch/powerpc/sysdev/fsl_soc.c:586: error: initializer element is  
> not constant
> arch/powerpc/sysdev/fsl_soc.c:586: error: syntax error at end of input
> make[1]: *** [arch/powerpc/sysdev/fsl_soc.o] Error 1
>
> Looks like someone broke arch_initcall() or something.

Odd, what config is this? (powerpc.git or linux.git)

Just did a build of my powerpc.git tree w/o this issue.

- k

Re: [PATCH] Check mac-address first in fsl_soc.c

[Timur Tabi]

Kumar Gala wrote:

>> Looks like someone broke arch_initcall() or something.
> 
> Odd, what config is this? (powerpc.git or linux.git)

powerpc.git, with mpc8360emds_defconfig

I just did a git-pull of Paul's tree a couple hours ago, "make 
mpc8360emds_defconfig", and then "make uImage".

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

Re: [PATCH] Check mac-address first in fsl_soc.c

[Timur Tabi]

Kumar Gala wrote:

>> Looks like someone broke arch_initcall() or something.
> 
> Odd, what config is this? (powerpc.git or linux.git)
> 
> Just did a build of my powerpc.git tree w/o this issue.

Never mind.  I just did git-clone again and rebuilt, and now it works.  I guess 
there was stray keystroke somewhere that I didn't notice.

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

Re: [PATCH] Check mac-address first in fsl_soc.c

[Timur Tabi]

I just had an idea.  The mac-address change should really be extended to all 
OF-enabled Ethernet drivers.  How about I put get_mac_address() in 
arch/powerpc/kernel/prom.c, right after get_property(), instead of making it an 
inline function just for fsl_soc.c?

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

Re: [PATCH] Check mac-address first in fsl_soc.c

[Timur Tabi]

Timur Tabi wrote:
> I just had an idea.  The mac-address change should really be extended to all 
> OF-enabled Ethernet drivers.  How about I put get_mac_address() in 
> arch/powerpc/kernel/prom.c, right after get_property(), instead of making it an 
> inline function just for fsl_soc.c?

Hmmm... if I do that, then get_mac_address() is no longer an init-only function. 
  That probably is a good thing, because then device trees can call it when 
they're loaded.  In that case, should I make null_mac_address[] static again?

And what about the name?  Should I call it of_get_mac_address()?  Plain old 
"get_mac_address" sounds like a namespace collision waiting to happen.

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

Re: [PATCH] Check mac-address first in fsl_soc.c

[Kumar Gala]

On Feb 13, 2007, at 3:23 PM, Timur Tabi wrote:

> Timur Tabi wrote:
>> I just had an idea.  The mac-address change should really be  
>> extended to all OF-enabled Ethernet drivers.  How about I put  
>> get_mac_address() in arch/powerpc/kernel/prom.c, right after  
>> get_property(), instead of making it an inline function just for  
>> fsl_soc.c?
>
> Hmmm... if I do that, then get_mac_address() is no longer an init- 
> only function.  That probably is a good thing, because then device  
> trees can call it when they're loaded.  In that case, should I make  
> null_mac_address[] static again?
>
> And what about the name?  Should I call it of_get_mac_address()?   
> Plain old "get_mac_address" sounds like a namespace collision  
> waiting to happen.


of_get_mac_address() seems reasonable, you can always stick it in asm- 
powerpc/prom.h

- k