| Message ID | 20170318012651.GB19897@bill-the-cat |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
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.
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!
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!
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;