[U-Boot] fdt_support: Fixup 'ethernet' aliases not ending in digits

Message ID 20170318012651.GB19897@bill-the-cat
State Changes Requested
Delegated to: Tom Rini
Headers show

Commit Message

Tom Rini March 18, 2017, 1:26 a.m.
On Fri, Mar 17, 2017 at 09:44:59PM +0200, Tuomas Tynkkynen wrote:

> The Raspberry Pi device tree files since Linux v4.9 have a "ethernet"
> alias pointing to the on-board Ethernet device node. However,
> U-Boot's fdt_fixup_ethernet() (and the kernel's of_alias_scan()) only
> looks at ethernet aliases ending in digits. Make it also check the
> "ethernet" alias.
> 
> Without this Linux isn't told of the MAC address provided by the
> RPI firmware and the ethernet device is always assigned a random MAC
> address.
> 
> The device trees themselves can't be fixed as someone is already
> depending on the "ethernet" alias:
> https://github.com/raspberrypi/firmware/issues/613
> 
> Signed-off-by: Tuomas Tynkkynen <tuomas@tuxera.com>

I looked into this a bit and there's a few other (much older) examples
of 'ethernet' rather than 'ethernet0' and the spec doesn't mandate
aliases end in a number.

That said, looking at the code, I think we can do this in a more clear
manner.  Can you test this please?

Comments

Tuomas Tynkkynen March 18, 2017, 6:20 p.m. | #1
On Fri, 17 Mar 2017 21:26:51 -0400
Tom Rini <trini@konsulko.com> wrote:

> On Fri, Mar 17, 2017 at 09:44:59PM +0200, Tuomas Tynkkynen wrote:
> 
> > The Raspberry Pi device tree files since Linux v4.9 have a "ethernet"
> > alias pointing to the on-board Ethernet device node. However,
> > U-Boot's fdt_fixup_ethernet() (and the kernel's of_alias_scan()) only
> > looks at ethernet aliases ending in digits. Make it also check the
> > "ethernet" alias.
> > 
> > Without this Linux isn't told of the MAC address provided by the
> > RPI firmware and the ethernet device is always assigned a random MAC
> > address.
> > 
> > The device trees themselves can't be fixed as someone is already
> > depending on the "ethernet" alias:
> > https://github.com/raspberrypi/firmware/issues/613
> > 
> > Signed-off-by: Tuomas Tynkkynen <tuomas@tuxera.com>  
> 
> I looked into this a bit and there's a few other (much older) examples
> of 'ethernet' rather than 'ethernet0' and the spec doesn't mandate
> aliases end in a number.

Ah, good to know.

> That said, looking at the code, I think we can do this in a more clear
> manner.  Can you test this please?
> 
> diff --git a/common/fdt_support.c b/common/fdt_support.c
> index 55d4d6f6d444..80186a95baf0 100644
> --- a/common/fdt_support.c
> +++ b/common/fdt_support.c
> @@ -482,7 +482,6 @@ void fdt_fixup_ethernet(void *fdt)
>  	/* Cycle through all aliases */
>  	for (prop = 0; ; prop++) {
>  		const char *name;
> -		int len = strlen("ethernet");
>  
>  		/* FDT might have been edited, recompute the offset */
>  		offset = fdt_first_property_offset(fdt,
> @@ -495,16 +494,13 @@ void fdt_fixup_ethernet(void *fdt)
>  			break;
>  
>  		path = fdt_getprop_by_offset(fdt, offset, &name, NULL);
> -		if (!strncmp(name, "ethernet", len)) {
> +		if (!strncmp(name, "ethernet", 8)) {
>  			i = trailing_strtol(name);
> -			if (i != -1) {
> -				if (i == 0)
> -					strcpy(mac, "ethaddr");
> -				else
> -					sprintf(mac, "eth%daddr", i);
> -			} else {
> -				continue;
> -			}
> +			/* Handle 'ethernet0' (0) and 'ethernet' (-1) */
> +			if (i <= 0)
> +				strcpy(mac, "ethaddr");
> +			else
> +				sprintf(mac, "eth%daddr", i);
>  			tmp = getenv(mac);
>  			if (!tmp)
>  				continue;
> 

That one accepts everything starting with "ethernet", which sounds undesirable
if one wants to have e.g. an "ethernet-phy0" alias for some different purpose.
Tom Rini March 18, 2017, 6:34 p.m. | #2
On Sat, Mar 18, 2017 at 08:20:07PM +0200, Tuomas Tynkkynen wrote:
> On Fri, 17 Mar 2017 21:26:51 -0400
> Tom Rini <trini@konsulko.com> wrote:
> 
> > On Fri, Mar 17, 2017 at 09:44:59PM +0200, Tuomas Tynkkynen wrote:
> > 
> > > The Raspberry Pi device tree files since Linux v4.9 have a "ethernet"
> > > alias pointing to the on-board Ethernet device node. However,
> > > U-Boot's fdt_fixup_ethernet() (and the kernel's of_alias_scan()) only
> > > looks at ethernet aliases ending in digits. Make it also check the
> > > "ethernet" alias.
> > > 
> > > Without this Linux isn't told of the MAC address provided by the
> > > RPI firmware and the ethernet device is always assigned a random MAC
> > > address.
> > > 
> > > The device trees themselves can't be fixed as someone is already
> > > depending on the "ethernet" alias:
> > > https://github.com/raspberrypi/firmware/issues/613
> > > 
> > > Signed-off-by: Tuomas Tynkkynen <tuomas@tuxera.com>  
> > 
> > I looked into this a bit and there's a few other (much older) examples
> > of 'ethernet' rather than 'ethernet0' and the spec doesn't mandate
> > aliases end in a number.
> 
> Ah, good to know.
> 
> > That said, looking at the code, I think we can do this in a more clear
> > manner.  Can you test this please?
> > 
> > diff --git a/common/fdt_support.c b/common/fdt_support.c
> > index 55d4d6f6d444..80186a95baf0 100644
> > --- a/common/fdt_support.c
> > +++ b/common/fdt_support.c
> > @@ -482,7 +482,6 @@ void fdt_fixup_ethernet(void *fdt)
> >  	/* Cycle through all aliases */
> >  	for (prop = 0; ; prop++) {
> >  		const char *name;
> > -		int len = strlen("ethernet");
> >  
> >  		/* FDT might have been edited, recompute the offset */
> >  		offset = fdt_first_property_offset(fdt,
> > @@ -495,16 +494,13 @@ void fdt_fixup_ethernet(void *fdt)
> >  			break;
> >  
> >  		path = fdt_getprop_by_offset(fdt, offset, &name, NULL);
> > -		if (!strncmp(name, "ethernet", len)) {
> > +		if (!strncmp(name, "ethernet", 8)) {
> >  			i = trailing_strtol(name);
> > -			if (i != -1) {
> > -				if (i == 0)
> > -					strcpy(mac, "ethaddr");
> > -				else
> > -					sprintf(mac, "eth%daddr", i);
> > -			} else {
> > -				continue;
> > -			}
> > +			/* Handle 'ethernet0' (0) and 'ethernet' (-1) */
> > +			if (i <= 0)
> > +				strcpy(mac, "ethaddr");
> > +			else
> > +				sprintf(mac, "eth%daddr", i);
> >  			tmp = getenv(mac);
> >  			if (!tmp)
> >  				continue;
> > 
> 
> That one accepts everything starting with "ethernet", which sounds undesirable
> if one wants to have e.g. an "ethernet-phy0" alias for some different purpose.

True.  Would you mind doing a v2 of your patch then (and grab my part to
just use the length of ethernet in the strncmp) where we don't add a
comment implying RPi is "wrong" here and just that we sometimes have
"ethernet" as the alias for the first and only ethernet device?  Thanks!
Tuomas Tynkkynen March 18, 2017, 11:17 p.m. | #3
On Sat, 18 Mar 2017 14:34:34 -0400
Tom Rini <trini@konsulko.com> wrote:

> On Sat, Mar 18, 2017 at 08:20:07PM +0200, Tuomas Tynkkynen wrote:
...
> > 
> > That one accepts everything starting with "ethernet", which sounds undesirable
> > if one wants to have e.g. an "ethernet-phy0" alias for some different purpose.  
> 
> True.  Would you mind doing a v2 of your patch then (and grab my part to
> just use the length of ethernet in the strncmp) where we don't add a
> comment implying RPi is "wrong" here and just that we sometimes have
> "ethernet" as the alias for the first and only ethernet device?  Thanks!
> 

Sure, will do it on Monday when I have access to my Pi again!

Patch

diff --git a/common/fdt_support.c b/common/fdt_support.c
index 55d4d6f6d444..80186a95baf0 100644
--- a/common/fdt_support.c
+++ b/common/fdt_support.c
@@ -482,7 +482,6 @@  void fdt_fixup_ethernet(void *fdt)
 	/* Cycle through all aliases */
 	for (prop = 0; ; prop++) {
 		const char *name;
-		int len = strlen("ethernet");
 
 		/* FDT might have been edited, recompute the offset */
 		offset = fdt_first_property_offset(fdt,
@@ -495,16 +494,13 @@  void fdt_fixup_ethernet(void *fdt)
 			break;
 
 		path = fdt_getprop_by_offset(fdt, offset, &name, NULL);
-		if (!strncmp(name, "ethernet", len)) {
+		if (!strncmp(name, "ethernet", 8)) {
 			i = trailing_strtol(name);
-			if (i != -1) {
-				if (i == 0)
-					strcpy(mac, "ethaddr");
-				else
-					sprintf(mac, "eth%daddr", i);
-			} else {
-				continue;
-			}
+			/* Handle 'ethernet0' (0) and 'ethernet' (-1) */
+			if (i <= 0)
+				strcpy(mac, "ethaddr");
+			else
+				sprintf(mac, "eth%daddr", i);
 			tmp = getenv(mac);
 			if (!tmp)
 				continue;