Re: [edk2-platforms: PATCHv2 1/1] Platform/RPi3: Provide "ethernet[0]" aliases in device tree

["Leif Lindholm" <leif.lindholm@linaro.org>]

Hi Michael,

Thanks for this.
One comment below.

On Wed, Jul 24, 2019 at 03:31:14PM +0100, Michael Brown wrote:
> Older device trees tend to use the alias "ethernet".  Newer device
> trees tend to use "ethernet0" since older versions of U-Boot would
> skip aliases that do not include an index number.  See, for example,
> Linux kernel commit 10b6c0c ("ARM: dts: bcm2835: add index to the
> ethernet alias") and U-Boot commit f8e57c6 ("fdt_support: Fixup
> 'ethernet' aliases not ending in digits").
> 
> Ensure that both "ethernet" and "ethernet0" aliases are present within
> the device tree, to provide compatibility with operating systems that
> expect either alias, and with device trees that contain either (or
> both) aliases.
> 
> Signed-off-by: Michael Brown <mbrown@fensystems.co.uk>
> ---
>  .../RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c  | 64 +++++++++++++++++++
>  1 file changed, 64 insertions(+)
> 
> diff --git a/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c b/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c
> index 83446e3e45..57e4bee8da 100644
> --- a/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c
> +++ b/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c
> @@ -23,6 +23,69 @@ STATIC VOID                             *mFdtImage;
>  
>  STATIC RASPBERRY_PI_FIRMWARE_PROTOCOL   *mFwProtocol;
>  
> +STATIC
> +VOID
> +FixEthernetAliases (
> +  VOID
> +)
> +{
> +  INTN          Aliases;
> +  CONST CHAR8   *Ethernet;
> +  CONST CHAR8   *Ethernet0;
> +  CONST CHAR8   *Alias;
> +  UINTN         CopySize;
> +  CHAR8         *Copy;
> +  INTN          Retval;
> +
> +  //
> +  // Look up the 'ethernet[0]' aliases
> +  //
> +  Aliases = fdt_path_offset (mFdtImage, "/aliases");
> +  if (Aliases < 0) {
> +    DEBUG ((DEBUG_ERROR, "%a: failed to locate '/aliases'\n", __FUNCTION__));

The DEBUG statements are only included in DEBUG builds.
And I think that's fine for these very detailed ones, but...

> +    return;
> +  }
> +  Ethernet = fdt_getprop (mFdtImage, Aliases, "ethernet", NULL);
> +  Ethernet0 = fdt_getprop (mFdtImage, Aliases, "ethernet0", NULL);
> +  Alias = Ethernet ? Ethernet : Ethernet0;
> +  if (!Alias) {
> +    DEBUG ((DEBUG_ERROR, "%a: failed to locate 'ethernet[0]' alias\n", __FUNCTION__));
> +    return;
> +  }
> +
> +  //
> +  // Create copy for fdt_setprop
> +  //
> +  CopySize = AsciiStrSize (Alias);
> +  Copy = AllocateCopyPool (CopySize, Alias);
> +  if (!Copy) {
> +    DEBUG ((DEBUG_ERROR, "%a: failed to copy '%a'\n", __FUNCTION__, Alias));
> +    return;
> +  }
> +
> +  //
> +  // Create missing aliases
> +  //
> +  if (!Ethernet) {
> +    Retval = fdt_setprop (mFdtImage, Aliases, "ethernet", Copy, CopySize);
> +    if (Retval != 0) {
> +      DEBUG ((DEBUG_ERROR, "%a: failed to create 'ethernet' alias (%d)\n",
> +        __FUNCTION__, Retval));
> +    }
> +    DEBUG ((DEBUG_INFO, "%a: created 'ethernet' alias '%a'\n", __FUNCTION__, Copy));
> +  }
> +  if (!Ethernet0) {
> +    Retval = fdt_setprop (mFdtImage, Aliases, "ethernet0", Copy, CopySize);
> +    if (Retval != 0) {
> +      DEBUG ((DEBUG_ERROR, "%a: failed to create 'ethernet0' alias (%d)\n",
> +        __FUNCTION__, Retval));
> +    }
> +    DEBUG ((DEBUG_INFO, "%a: created 'ethernet0' alias '%a'\n", __FUNCTION__, Copy));
> +  }
> +
> +  FreePool (Copy);
> +}
> +
>  STATIC
>  VOID
>  UpdateMacAddress (
> @@ -342,6 +405,7 @@ FdtDxeInitialize (
>    SanitizePSCI ();
>    CleanMemoryNodes ();
>    CleanSimpleFramebuffer ();
> +  FixEthernetAliases ();

...would it be worth having a return value here and Print()ing a
message visible regardless of build profile if this function fails?

/
    Leif

>    UpdateMacAddress ();
>    if (Internal) {
>      /*
> -- 
> 2.21.0
>