[edk2-platforms: PATCH 0/1] Handle RPi device trees using "ethernet0"

[edk2-platforms: PATCH 0/1] Handle RPi device trees using "ethernet0"

[Michael Brown]

This standalone patch fixes FdtDxe.c to handle newer Raspberry Pi
device trees using the "ethernet0" alias (while retaining
compatibility with older device trees).

Michael Brown (1):
  Platform/RPi3: Accept "ethernet" or "ethernet0" aliases in device tree

 Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

-- 
2.20.1

[edk2-platforms: PATCH 1/1] Platform/RPi3: Accept "ethernet" or "ethernet0" aliases in device tree

[Michael Brown]

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").

Accept either "ethernet" or "ethernet0" as being the alias to the node
for which the MAC address should be updated.

Signed-off-by: Michael Brown <mbrown@fensystems.co.uk>
---
 Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c b/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c
index 83446e3e45..e80bc2c391 100644
--- a/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c
+++ b/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c
@@ -35,11 +35,14 @@ UpdateMacAddress (
   UINT8         MacAddress[6];
 
   //
-  // Locate the node that the 'ethernet' alias refers to
+  // Locate the node that the 'ethernet[0]' alias refers to
   //
   Node = fdt_path_offset (mFdtImage, "ethernet");
   if (Node < 0) {
-    DEBUG ((DEBUG_ERROR, "%a: failed to locate 'ethernet' alias\n", __FUNCTION__));
+    Node = fdt_path_offset (mFdtImage, "ethernet0");
+  }
+  if (Node < 0) {
+    DEBUG ((DEBUG_ERROR, "%a: failed to locate 'ethernet[0]' alias\n", __FUNCTION__));
     return;
   }
 
-- 
2.20.1

Re: [edk2-platforms: PATCH 1/1] Platform/RPi3: Accept "ethernet" or "ethernet0" aliases in device tree

[Leif Lindholm]

On Fri, Jul 19, 2019 at 06:29:07PM +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").
> 
> Accept either "ethernet" or "ethernet0" as being the alias to the node
> for which the MAC address should be updated.

Why is this patch useful?
I understand the problem, but we include the .dtb from our own
edk2-non-osi tree. And it seems that device tree already provides an
alias to support both:
https://github.com/tianocore/edk2-non-osi/blob/master/Platform/RaspberryPi/RPi3/DeviceTree/bcm2710-rpi-3-b.dts

Would it be more useful to warn loudly if "ethernet" isn't present?
(And just *replace* ethernet[0] with ethernet below.)

Regards,

Leif

> Signed-off-by: Michael Brown <mbrown@fensystems.co.uk>
> ---
>  Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c b/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c
> index 83446e3e45..e80bc2c391 100644
> --- a/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c
> +++ b/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c
> @@ -35,11 +35,14 @@ UpdateMacAddress (
>    UINT8         MacAddress[6];
>  
>    //
> -  // Locate the node that the 'ethernet' alias refers to
> +  // Locate the node that the 'ethernet[0]' alias refers to
>    //
>    Node = fdt_path_offset (mFdtImage, "ethernet");
>    if (Node < 0) {
> -    DEBUG ((DEBUG_ERROR, "%a: failed to locate 'ethernet' alias\n", __FUNCTION__));
> +    Node = fdt_path_offset (mFdtImage, "ethernet0");
> +  }
> +  if (Node < 0) {
> +    DEBUG ((DEBUG_ERROR, "%a: failed to locate 'ethernet[0]' alias\n", __FUNCTION__));
>      return;
>    }
>  
> -- 
> 2.20.1
> 

Re: [edk2-platforms: PATCH 1/1] Platform/RPi3: Accept "ethernet" or "ethernet0" aliases in device tree

[Michael Brown]

On 23/07/2019 11:34, Leif Lindholm wrote:
> On Fri, Jul 19, 2019 at 06:29:07PM +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").
>>
>> Accept either "ethernet" or "ethernet0" as being the alias to the node
>> for which the MAC address should be updated.
> 
> Why is this patch useful?
> I understand the problem, but we include the .dtb from our own
> edk2-non-osi tree. And it seems that device tree already provides an
> alias to support both:
> https://github.com/tianocore/edk2-non-osi/blob/master/Platform/RaspberryPi/RPi3/DeviceTree/bcm2710-rpi-3-b.dts
> 
> Would it be more useful to warn loudly if "ethernet" isn't present?
> (And just *replace* ethernet[0] with ethernet below.)

The device tree can also be provided by the VC4 firmware, e.g. with a 
config.txt containing e.g.

   device_tree_address=0x10000
   device_tree_end=0x20000
   device_tree=bcm2710-rpi-3-b-plus.dtb

It is also possible to omit the explicit "device_tree=..." and have just

   device_tree_address=0x10000
   device_tree_end=0x20000

This configuration will cause the VC4 firmware to automatically load the 
appropriate device tree file.  I have tested that this allows me to boot 
from a 3B or 3B+ using a single SD card image that contains all of the 
.dtb files from

   https://github.com/raspberrypi/firmware/tree/master/boot

These "official" raspberrypi-firmware .dtb files are built from the 
stock Linux kernel .dts files, and contain only "ethernet0".

To address your specific questions:

 > Why is this patch useful?

It allows us to boot using the "official" .dtb files without any 
unexpected behaviour.

This in turn allows us to boot with the VC4 firmware automatically 
selecting the appropriate .dtb file for the hardware.  This is extremely 
useful when trying to provide a single SD card image that will work on a 
wide range of hardware.

 > Would it be more useful to warn loudly if "ethernet" isn't present?
 > (And just *replace* ethernet[0] with ethernet below.)

This would also solve the problem.  I would find it mildly surprising 
behaviour if I were debugging around it, since the "official" .dtb files 
have moved in the other direction (going from "ethernet" to 
"ethernet0"), but I would not expect it to break anything.

Another option would be to have UpdateMacAddress() ensure that both 
"ethernet" and "ethernet0" aliases exist.

I am happy to work up a new version of the patch: just let me know which 
option you prefer.

Many thanks,

Michael

Re: [edk2-platforms: PATCH 1/1] Platform/RPi3: Accept "ethernet" or "ethernet0" aliases in device tree

[Leif Lindholm]

On Tue, Jul 23, 2019 at 12:00:04PM +0100, Michael Brown wrote:
> > Why is this patch useful?
> > I understand the problem, but we include the .dtb from our own
> > edk2-non-osi tree. And it seems that device tree already provides an
> > alias to support both:
> > https://github.com/tianocore/edk2-non-osi/blob/master/Platform/RaspberryPi/RPi3/DeviceTree/bcm2710-rpi-3-b.dts
> > 
> > Would it be more useful to warn loudly if "ethernet" isn't present?
> > (And just *replace* ethernet[0] with ethernet below.)
> 
> The device tree can also be provided by the VC4 firmware, e.g. with a
> config.txt containing e.g.
> 
>   device_tree_address=0x10000
>   device_tree_end=0x20000
>   device_tree=bcm2710-rpi-3-b-plus.dtb

Ah, right.

> It is also possible to omit the explicit "device_tree=..." and have just
> 
>   device_tree_address=0x10000
>   device_tree_end=0x20000
> 
> This configuration will cause the VC4 firmware to automatically load the
> appropriate device tree file.  I have tested that this allows me to boot
> from a 3B or 3B+ using a single SD card image that contains all of the .dtb
> files from
> 
>   https://github.com/raspberrypi/firmware/tree/master/boot
> 
> These "official" raspberrypi-firmware .dtb files are built from the stock
> Linux kernel .dts files, and contain only "ethernet0".

*facepalm*

> To address your specific questions:
> 
> > Why is this patch useful?
> 
> It allows us to boot using the "official" .dtb files without any unexpected
> behaviour.
> 
> This in turn allows us to boot with the VC4 firmware automatically selecting
> the appropriate .dtb file for the hardware.  This is extremely useful when
> trying to provide a single SD card image that will work on a wide range of
> hardware.

Yeah, understood. I just have a poor understanding of the esoteric boot
architecture of the Pis.

> > Would it be more useful to warn loudly if "ethernet" isn't present?
> > (And just *replace* ethernet[0] with ethernet below.)
> 
> This would also solve the problem.  I would find it mildly surprising
> behaviour if I were debugging around it, since the "official" .dtb files
> have moved in the other direction (going from "ethernet" to "ethernet0"),
> but I would not expect it to break anything.
> 
> Another option would be to have UpdateMacAddress() ensure that both
> "ethernet" and "ethernet0" aliases exist.

Yeah, that would probably be my preferred option.

> I am happy to work up a new version of the patch: just let me know which
> option you prefer.

As mentioned.
And thank you for the explanation.

Best Regards,

Leif

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

[Michael Brown]

This standalone patch fixes FdtDxe.c to handle newer Raspberry Pi
device trees using the "ethernet0" alias (while retaining
compatibility with older device trees).

Changes from v1: as per Leif's request, the patch now ensures that
both "ethernet" and "ethernet0" aliases exist within the device tree.
The code is unfortunately much more verbose than v1, since there is no
convenient libfdt helper function for creating an alias.

I have attempted to match the existing coding style, but I apologise
in advance for any errors in so doing.

Michael Brown (1):
  Platform/RPi3: Provide "ethernet[0]" aliases in device tree

 .../RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c  | 64 +++++++++++++++++++
 1 file changed, 64 insertions(+)

-- 
2.21.0

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

[Michael Brown]

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__));
+    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 ();
   UpdateMacAddress ();
   if (Internal) {
     /*
-- 
2.21.0

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

[Leif Lindholm]

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
> 

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

[Michael Brown]

On 24/07/2019 19:53, Leif Lindholm wrote:
>>     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?

Yes, I think that would be very sensible, but it would also be 
inconsistent with the rest of the code: all of the other 
device-tree-modifying functions in FdtDxe.c are declared as VOID and use 
only DEBUG to report errors.

Changing this pattern throughout FdtDxe.c should, I think, be a 
completely separate patch.  Would it be possible to merge the current 
patch, and I can then follow up with a second patch to improve the error 
visibility in non-debug builds?

Thanks,

Michael

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

[Leif Lindholm]

On Wed, Jul 24, 2019 at 09:53:00PM +0100, Michael Brown wrote:
> On 24/07/2019 19:53, Leif Lindholm wrote:
> > >     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?
> 
> Yes, I think that would be very sensible, but it would also be inconsistent
> with the rest of the code: all of the other device-tree-modifying functions
> in FdtDxe.c are declared as VOID and use only DEBUG to report errors.
> 
> Changing this pattern throughout FdtDxe.c should, I think, be a completely
> separate patch.  Would it be possible to merge the current patch, and I can
> then follow up with a second patch to improve the error visibility in
> non-debug builds?

Yeah, that works for me.
I'll push it tomorrow when I'm more awake, but for now:
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

/
    Leif

[edk2-platforms: PATCH 0/1] Report device tree modification errors using Print()

[Michael Brown]

>>> ...would it be worth having a return value here and Print()ing a
>>> message visible regardless of build profile if this function fails?
>>
>> Yes, I think that would be very sensible, but it would also be inconsistent
>> with the rest of the code: all of the other device-tree-modifying functions
>> in FdtDxe.c are declared as VOID and use only DEBUG to report errors.
>>
>> Changing this pattern throughout FdtDxe.c should, I think, be a completely
>> separate patch.  Would it be possible to merge the current patch, and I can
>> then follow up with a second patch to improve the error visibility in
>> non-debug builds?
> 
> Yeah, that works for me.

As promised, here is the subsequent patch to improve error visibility
throughout FdtDxe.

Michael Brown (1):
  Platform/RPi3: Report device tree modification errors using Print()

 .../RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c  | 102 ++++++++++++------
 1 file changed, 70 insertions(+), 32 deletions(-)

-- 
2.21.0

[edk2-platforms: PATCH 1/1] Platform/RPi3: Report device tree modification errors using Print()

[Michael Brown]

Most functions in FdtDxe currently return VOID and report errors using
only DEBUG.  Update functions to return EFI_STATUS and report errors
using Print() so that errors are at least visible in non-debug builds.

Signed-off-by: Michael Brown <mbrown@fensystems.co.uk>
---
 .../RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c  | 102 ++++++++++++------
 1 file changed, 70 insertions(+), 32 deletions(-)

diff --git a/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c b/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c
index 57e4bee8da..17d3bbd82d 100644
--- a/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c
+++ b/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c
@@ -13,6 +13,7 @@
 #include <Library/DxeServicesLib.h>
 #include <Library/MemoryAllocationLib.h>
 #include <Library/UefiBootServicesTableLib.h>
+#include <Library/UefiLib.h>
 #include <libfdt.h>
 
 #include <Protocol/RpiFirmware.h>
@@ -24,7 +25,7 @@ STATIC VOID                             *mFdtImage;
 STATIC RASPBERRY_PI_FIRMWARE_PROTOCOL   *mFwProtocol;
 
 STATIC
-VOID
+EFI_STATUS
 FixEthernetAliases (
   VOID
 )
@@ -36,6 +37,7 @@ FixEthernetAliases (
   UINTN         CopySize;
   CHAR8         *Copy;
   INTN          Retval;
+  EFI_STATUS    Status;
 
   //
   // Look up the 'ethernet[0]' aliases
@@ -43,14 +45,14 @@ FixEthernetAliases (
   Aliases = fdt_path_offset (mFdtImage, "/aliases");
   if (Aliases < 0) {
     DEBUG ((DEBUG_ERROR, "%a: failed to locate '/aliases'\n", __FUNCTION__));
-    return;
+    return EFI_NOT_FOUND;
   }
   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;
+    return EFI_NOT_FOUND;
   }
 
   //
@@ -60,15 +62,17 @@ FixEthernetAliases (
   Copy = AllocateCopyPool (CopySize, Alias);
   if (!Copy) {
     DEBUG ((DEBUG_ERROR, "%a: failed to copy '%a'\n", __FUNCTION__, Alias));
-    return;
+    return EFI_OUT_OF_RESOURCES;
   }
 
   //
   // Create missing aliases
   //
+  Status = EFI_SUCCESS;
   if (!Ethernet) {
     Retval = fdt_setprop (mFdtImage, Aliases, "ethernet", Copy, CopySize);
     if (Retval != 0) {
+      Status = EFI_DEVICE_ERROR;
       DEBUG ((DEBUG_ERROR, "%a: failed to create 'ethernet' alias (%d)\n",
         __FUNCTION__, Retval));
     }
@@ -77,6 +81,7 @@ FixEthernetAliases (
   if (!Ethernet0) {
     Retval = fdt_setprop (mFdtImage, Aliases, "ethernet0", Copy, CopySize);
     if (Retval != 0) {
+      Status = EFI_DEVICE_ERROR;
       DEBUG ((DEBUG_ERROR, "%a: failed to create 'ethernet0' alias (%d)\n",
         __FUNCTION__, Retval));
     }
@@ -84,10 +89,11 @@ FixEthernetAliases (
   }
 
   FreePool (Copy);
+  return Status;
 }
 
 STATIC
-VOID
+EFI_STATUS
 UpdateMacAddress (
   VOID
   )
@@ -103,7 +109,7 @@ UpdateMacAddress (
   Node = fdt_path_offset (mFdtImage, "ethernet");
   if (Node < 0) {
     DEBUG ((DEBUG_ERROR, "%a: failed to locate 'ethernet' alias\n", __FUNCTION__));
-    return;
+    return EFI_NOT_FOUND;
   }
 
   //
@@ -112,7 +118,7 @@ UpdateMacAddress (
   Status = mFwProtocol->GetMacAddress (MacAddress);
   if (EFI_ERROR (Status)) {
     DEBUG ((DEBUG_ERROR, "%a: failed to retrieve MAC address\n", __FUNCTION__));
-    return;
+    return Status;
   }
 
   Retval = fdt_setprop (mFdtImage, Node, "mac-address", MacAddress,
@@ -120,16 +126,17 @@ UpdateMacAddress (
   if (Retval != 0) {
     DEBUG ((DEBUG_ERROR, "%a: failed to create 'mac-address' property (%d)\n",
       __FUNCTION__, Retval));
-    return;
+    return EFI_DEVICE_ERROR;
   }
 
   DEBUG ((DEBUG_INFO, "%a: setting MAC address to %02x:%02x:%02x:%02x:%02x:%02x\n",
     __FUNCTION__, MacAddress[0], MacAddress[1], MacAddress[2], MacAddress[3],
     MacAddress[4], MacAddress[5]));
+  return EFI_SUCCESS;
 }
 
 STATIC
-VOID
+EFI_STATUS
 CleanMemoryNodes (
   VOID
   )
@@ -139,7 +146,7 @@ CleanMemoryNodes (
 
   Node = fdt_path_offset (mFdtImage, "/memory");
   if (Node < 0) {
-    return;
+    return EFI_SUCCESS;
   }
 
   /*
@@ -150,11 +157,14 @@ CleanMemoryNodes (
   Retval = fdt_del_node (mFdtImage, Node);
   if (Retval != 0) {
     DEBUG ((DEBUG_ERROR, "Failed to remove /memory\n"));
+    return EFI_DEVICE_ERROR;
   }
+
+  return EFI_SUCCESS;
 }
 
 STATIC
-VOID
+EFI_STATUS
 SanitizePSCI (
   VOID
   )
@@ -166,7 +176,7 @@ SanitizePSCI (
   Root = fdt_path_offset (mFdtImage, "/");
   ASSERT (Root >= 0);
   if (Root < 0) {
-    return;
+    return EFI_NOT_FOUND;
   }
 
   Node = fdt_path_offset (mFdtImage, "/psci");
@@ -177,41 +187,42 @@ SanitizePSCI (
   ASSERT (Node >= 0);
   if (Node < 0) {
     DEBUG ((DEBUG_ERROR, "Couldn't find/create /psci\n"));
-    return;
+    return EFI_DEVICE_ERROR;
   }
 
   Retval = fdt_setprop_string (mFdtImage, Node, "compatible", "arm,psci-1.0");
   if (Retval != 0) {
     DEBUG ((DEBUG_ERROR, "Couldn't set /psci compatible property\n"));
-    return;
+    return EFI_DEVICE_ERROR;
   }
 
   Retval = fdt_setprop_string (mFdtImage, Node, "method", "smc");
   if (Retval != 0) {
     DEBUG ((DEBUG_ERROR, "Couldn't set /psci method property\n"));
-    return;
+    return EFI_DEVICE_ERROR;
   }
 
   Root = fdt_path_offset (mFdtImage, "/cpus");
   if (Root < 0) {
     DEBUG ((DEBUG_ERROR, "No CPUs to update with PSCI enable-method?\n"));
-    return;
+    return EFI_NOT_FOUND;
   }
 
   Node = fdt_first_subnode (mFdtImage, Root);
   while (Node >= 0) {
     if (fdt_setprop_string (mFdtImage, Node, "enable-method", "psci") != 0) {
       DEBUG ((DEBUG_ERROR, "Failed to update enable-method for a CPU\n"));
-      return;
+      return EFI_DEVICE_ERROR;
     }
 
     fdt_delprop (mFdtImage, Node, "cpu-release-addr");
     Node = fdt_next_subnode (mFdtImage, Node);
   }
+  return EFI_SUCCESS;
 }
 
 STATIC
-VOID
+EFI_STATUS
 CleanSimpleFramebuffer (
   VOID
   )
@@ -225,7 +236,7 @@ CleanSimpleFramebuffer (
    */
   Node = fdt_path_offset (mFdtImage, "display0");
   if (Node < 0) {
-    return;
+    return EFI_SUCCESS;
   }
 
   /*
@@ -236,25 +247,28 @@ CleanSimpleFramebuffer (
   Retval = fdt_del_node (mFdtImage, Node);
   if (Retval != 0) {
     DEBUG ((DEBUG_ERROR, "Failed to remove display0\n"));
-    return;
+    return EFI_DEVICE_ERROR;
   }
 
   Node = fdt_path_offset (mFdtImage, "/aliases");
   if (Node < 0) {
     DEBUG ((DEBUG_ERROR, "Couldn't find /aliases to remove display0\n"));
-    return;
+    return EFI_NOT_FOUND;
   }
 
   Retval = fdt_delprop (mFdtImage, Node, "display0");
   if (Retval != 0) {
     DEBUG ((DEBUG_ERROR, "Failed to remove display0 alias\n"));
+    return EFI_DEVICE_ERROR;
   }
+
+  return EFI_SUCCESS;
 }
 
 #define MAX_CMDLINE_SIZE    512
 
 STATIC
-VOID
+EFI_STATUS
 UpdateBootArgs (
   VOID
   )
@@ -270,7 +284,7 @@ UpdateBootArgs (
   Node = fdt_path_offset (mFdtImage, "/chosen");
   if (Node < 0) {
     DEBUG ((DEBUG_ERROR, "%a: failed to locate /chosen node\n", __FUNCTION__));
-    return;
+    return EFI_NOT_FOUND;
   }
 
   //
@@ -282,7 +296,7 @@ UpdateBootArgs (
   CommandLine = AllocatePool (MAX_CMDLINE_SIZE) + 4;
   if (!CommandLine) {
     DEBUG ((DEBUG_ERROR, "%a: failed to allocate memory\n", __FUNCTION__));
-    return;
+    return EFI_OUT_OF_RESOURCES;
   }
 
   //
@@ -291,12 +305,12 @@ UpdateBootArgs (
   Status = mFwProtocol->GetCommandLine (MAX_CMDLINE_SIZE, CommandLine + 4);
   if (EFI_ERROR (Status)) {
     DEBUG ((DEBUG_ERROR, "%a: failed to retrieve command line\n", __FUNCTION__));
-    return;
+    return Status;
   }
 
   if (AsciiStrLen (CommandLine + 4) == 0) {
     DEBUG ((DEBUG_INFO, "%a: empty command line received\n", __FUNCTION__));
-    return;
+    return EFI_SUCCESS;
   }
 
   CommandLine[3] = ' ';
@@ -331,6 +345,7 @@ UpdateBootArgs (
   DEBUG_CODE_END ();
 
   FreePool (CommandLine - 4);
+  return EFI_SUCCESS;
 }
 
 
@@ -402,16 +417,39 @@ FdtDxeInitialize (
   Retval = fdt_open_into (FdtImage, mFdtImage, FdtSize);
   ASSERT (Retval == 0);
 
-  SanitizePSCI ();
-  CleanMemoryNodes ();
-  CleanSimpleFramebuffer ();
-  FixEthernetAliases ();
-  UpdateMacAddress ();
+  Status = SanitizePSCI ();
+  if (EFI_ERROR (Status)) {
+    Print (L"Failed to sanitize PSCI (error %d)\n", Status);
+  }
+
+  Status = CleanMemoryNodes ();
+  if (EFI_ERROR (Status)) {
+    Print (L"Failed to clean memory nodes (error %d)\n", Status);
+  }
+
+  Status = CleanSimpleFramebuffer ();
+  if (EFI_ERROR (Status)) {
+    Print (L"Failed to clean frame buffer (error %d)\n", Status);
+  }
+
+  Status = FixEthernetAliases ();
+  if (EFI_ERROR (Status)) {
+    Print (L"Failed to fix ethernet aliases (error %d)\n", Status);
+  }
+
+  Status = UpdateMacAddress ();
+  if (EFI_ERROR (Status)) {
+    Print (L"Failed to update MAC address (error %d)\n", Status);
+  }
+
   if (Internal) {
     /*
      * A GPU-provided DTB already has the full command line.
      */
-    UpdateBootArgs ();
+    Status = UpdateBootArgs ();
+    if (EFI_ERROR (Status)) {
+      Print (L"Failed to update boot arguments (error %d)\n", Status);
+    }
   }
 
   DEBUG ((DEBUG_INFO, "Installed FDT is at %p\n", mFdtImage));
-- 
2.21.0

Re: [edk2-platforms: PATCH 1/1] Platform/RPi3: Report device tree modification errors using Print()

[Leif Lindholm]

On Thu, Jul 25, 2019 at 11:27:15AM +0100, Michael Brown wrote:
> Most functions in FdtDxe currently return VOID and report errors using
> only DEBUG.  Update functions to return EFI_STATUS and report errors
> using Print() so that errors are at least visible in non-debug builds.
> 
> Signed-off-by: Michael Brown <mbrown@fensystems.co.uk>

Apologies for slow review.

Happy with all of this, except for the use of EFI_DEVICE_ERROR.

Generally, EFI_DEVICE_ERROR is used as an i/o error type status.
Either EFI_NOT_FOUND, EFI_INVALID_PARAMETER or EFI_OUT_OF_RECOURCES
would be more appropriate when referring to device-tree manipulation.

Could you update these and spin a v2 please?

Best Regards,

Leif

> ---
>  .../RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c  | 102 ++++++++++++------
>  1 file changed, 70 insertions(+), 32 deletions(-)
> 
> diff --git a/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c b/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c
> index 57e4bee8da..17d3bbd82d 100644
> --- a/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c
> +++ b/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c
> @@ -13,6 +13,7 @@
>  #include <Library/DxeServicesLib.h>
>  #include <Library/MemoryAllocationLib.h>
>  #include <Library/UefiBootServicesTableLib.h>
> +#include <Library/UefiLib.h>
>  #include <libfdt.h>
>  
>  #include <Protocol/RpiFirmware.h>
> @@ -24,7 +25,7 @@ STATIC VOID                             *mFdtImage;
>  STATIC RASPBERRY_PI_FIRMWARE_PROTOCOL   *mFwProtocol;
>  
>  STATIC
> -VOID
> +EFI_STATUS
>  FixEthernetAliases (
>    VOID
>  )
> @@ -36,6 +37,7 @@ FixEthernetAliases (
>    UINTN         CopySize;
>    CHAR8         *Copy;
>    INTN          Retval;
> +  EFI_STATUS    Status;
>  
>    //
>    // Look up the 'ethernet[0]' aliases
> @@ -43,14 +45,14 @@ FixEthernetAliases (
>    Aliases = fdt_path_offset (mFdtImage, "/aliases");
>    if (Aliases < 0) {
>      DEBUG ((DEBUG_ERROR, "%a: failed to locate '/aliases'\n", __FUNCTION__));
> -    return;
> +    return EFI_NOT_FOUND;
>    }
>    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;
> +    return EFI_NOT_FOUND;
>    }
>  
>    //
> @@ -60,15 +62,17 @@ FixEthernetAliases (
>    Copy = AllocateCopyPool (CopySize, Alias);
>    if (!Copy) {
>      DEBUG ((DEBUG_ERROR, "%a: failed to copy '%a'\n", __FUNCTION__, Alias));
> -    return;
> +    return EFI_OUT_OF_RESOURCES;
>    }
>  
>    //
>    // Create missing aliases
>    //
> +  Status = EFI_SUCCESS;
>    if (!Ethernet) {
>      Retval = fdt_setprop (mFdtImage, Aliases, "ethernet", Copy, CopySize);
>      if (Retval != 0) {
> +      Status = EFI_DEVICE_ERROR;
>        DEBUG ((DEBUG_ERROR, "%a: failed to create 'ethernet' alias (%d)\n",
>          __FUNCTION__, Retval));
>      }
> @@ -77,6 +81,7 @@ FixEthernetAliases (
>    if (!Ethernet0) {
>      Retval = fdt_setprop (mFdtImage, Aliases, "ethernet0", Copy, CopySize);
>      if (Retval != 0) {
> +      Status = EFI_DEVICE_ERROR;
>        DEBUG ((DEBUG_ERROR, "%a: failed to create 'ethernet0' alias (%d)\n",
>          __FUNCTION__, Retval));
>      }
> @@ -84,10 +89,11 @@ FixEthernetAliases (
>    }
>  
>    FreePool (Copy);
> +  return Status;
>  }
>  
>  STATIC
> -VOID
> +EFI_STATUS
>  UpdateMacAddress (
>    VOID
>    )
> @@ -103,7 +109,7 @@ UpdateMacAddress (
>    Node = fdt_path_offset (mFdtImage, "ethernet");
>    if (Node < 0) {
>      DEBUG ((DEBUG_ERROR, "%a: failed to locate 'ethernet' alias\n", __FUNCTION__));
> -    return;
> +    return EFI_NOT_FOUND;
>    }
>  
>    //
> @@ -112,7 +118,7 @@ UpdateMacAddress (
>    Status = mFwProtocol->GetMacAddress (MacAddress);
>    if (EFI_ERROR (Status)) {
>      DEBUG ((DEBUG_ERROR, "%a: failed to retrieve MAC address\n", __FUNCTION__));
> -    return;
> +    return Status;
>    }
>  
>    Retval = fdt_setprop (mFdtImage, Node, "mac-address", MacAddress,
> @@ -120,16 +126,17 @@ UpdateMacAddress (
>    if (Retval != 0) {
>      DEBUG ((DEBUG_ERROR, "%a: failed to create 'mac-address' property (%d)\n",
>        __FUNCTION__, Retval));
> -    return;
> +    return EFI_DEVICE_ERROR;
>    }
>  
>    DEBUG ((DEBUG_INFO, "%a: setting MAC address to %02x:%02x:%02x:%02x:%02x:%02x\n",
>      __FUNCTION__, MacAddress[0], MacAddress[1], MacAddress[2], MacAddress[3],
>      MacAddress[4], MacAddress[5]));
> +  return EFI_SUCCESS;
>  }
>  
>  STATIC
> -VOID
> +EFI_STATUS
>  CleanMemoryNodes (
>    VOID
>    )
> @@ -139,7 +146,7 @@ CleanMemoryNodes (
>  
>    Node = fdt_path_offset (mFdtImage, "/memory");
>    if (Node < 0) {
> -    return;
> +    return EFI_SUCCESS;
>    }
>  
>    /*
> @@ -150,11 +157,14 @@ CleanMemoryNodes (
>    Retval = fdt_del_node (mFdtImage, Node);
>    if (Retval != 0) {
>      DEBUG ((DEBUG_ERROR, "Failed to remove /memory\n"));
> +    return EFI_DEVICE_ERROR;
>    }
> +
> +  return EFI_SUCCESS;
>  }
>  
>  STATIC
> -VOID
> +EFI_STATUS
>  SanitizePSCI (
>    VOID
>    )
> @@ -166,7 +176,7 @@ SanitizePSCI (
>    Root = fdt_path_offset (mFdtImage, "/");
>    ASSERT (Root >= 0);
>    if (Root < 0) {
> -    return;
> +    return EFI_NOT_FOUND;
>    }
>  
>    Node = fdt_path_offset (mFdtImage, "/psci");
> @@ -177,41 +187,42 @@ SanitizePSCI (
>    ASSERT (Node >= 0);
>    if (Node < 0) {
>      DEBUG ((DEBUG_ERROR, "Couldn't find/create /psci\n"));
> -    return;
> +    return EFI_DEVICE_ERROR;
>    }
>  
>    Retval = fdt_setprop_string (mFdtImage, Node, "compatible", "arm,psci-1.0");
>    if (Retval != 0) {
>      DEBUG ((DEBUG_ERROR, "Couldn't set /psci compatible property\n"));
> -    return;
> +    return EFI_DEVICE_ERROR;
>    }
>  
>    Retval = fdt_setprop_string (mFdtImage, Node, "method", "smc");
>    if (Retval != 0) {
>      DEBUG ((DEBUG_ERROR, "Couldn't set /psci method property\n"));
> -    return;
> +    return EFI_DEVICE_ERROR;
>    }
>  
>    Root = fdt_path_offset (mFdtImage, "/cpus");
>    if (Root < 0) {
>      DEBUG ((DEBUG_ERROR, "No CPUs to update with PSCI enable-method?\n"));
> -    return;
> +    return EFI_NOT_FOUND;
>    }
>  
>    Node = fdt_first_subnode (mFdtImage, Root);
>    while (Node >= 0) {
>      if (fdt_setprop_string (mFdtImage, Node, "enable-method", "psci") != 0) {
>        DEBUG ((DEBUG_ERROR, "Failed to update enable-method for a CPU\n"));
> -      return;
> +      return EFI_DEVICE_ERROR;
>      }
>  
>      fdt_delprop (mFdtImage, Node, "cpu-release-addr");
>      Node = fdt_next_subnode (mFdtImage, Node);
>    }
> +  return EFI_SUCCESS;
>  }
>  
>  STATIC
> -VOID
> +EFI_STATUS
>  CleanSimpleFramebuffer (
>    VOID
>    )
> @@ -225,7 +236,7 @@ CleanSimpleFramebuffer (
>     */
>    Node = fdt_path_offset (mFdtImage, "display0");
>    if (Node < 0) {
> -    return;
> +    return EFI_SUCCESS;
>    }
>  
>    /*
> @@ -236,25 +247,28 @@ CleanSimpleFramebuffer (
>    Retval = fdt_del_node (mFdtImage, Node);
>    if (Retval != 0) {
>      DEBUG ((DEBUG_ERROR, "Failed to remove display0\n"));
> -    return;
> +    return EFI_DEVICE_ERROR;
>    }
>  
>    Node = fdt_path_offset (mFdtImage, "/aliases");
>    if (Node < 0) {
>      DEBUG ((DEBUG_ERROR, "Couldn't find /aliases to remove display0\n"));
> -    return;
> +    return EFI_NOT_FOUND;
>    }
>  
>    Retval = fdt_delprop (mFdtImage, Node, "display0");
>    if (Retval != 0) {
>      DEBUG ((DEBUG_ERROR, "Failed to remove display0 alias\n"));
> +    return EFI_DEVICE_ERROR;
>    }
> +
> +  return EFI_SUCCESS;
>  }
>  
>  #define MAX_CMDLINE_SIZE    512
>  
>  STATIC
> -VOID
> +EFI_STATUS
>  UpdateBootArgs (
>    VOID
>    )
> @@ -270,7 +284,7 @@ UpdateBootArgs (
>    Node = fdt_path_offset (mFdtImage, "/chosen");
>    if (Node < 0) {
>      DEBUG ((DEBUG_ERROR, "%a: failed to locate /chosen node\n", __FUNCTION__));
> -    return;
> +    return EFI_NOT_FOUND;
>    }
>  
>    //
> @@ -282,7 +296,7 @@ UpdateBootArgs (
>    CommandLine = AllocatePool (MAX_CMDLINE_SIZE) + 4;
>    if (!CommandLine) {
>      DEBUG ((DEBUG_ERROR, "%a: failed to allocate memory\n", __FUNCTION__));
> -    return;
> +    return EFI_OUT_OF_RESOURCES;
>    }
>  
>    //
> @@ -291,12 +305,12 @@ UpdateBootArgs (
>    Status = mFwProtocol->GetCommandLine (MAX_CMDLINE_SIZE, CommandLine + 4);
>    if (EFI_ERROR (Status)) {
>      DEBUG ((DEBUG_ERROR, "%a: failed to retrieve command line\n", __FUNCTION__));
> -    return;
> +    return Status;
>    }
>  
>    if (AsciiStrLen (CommandLine + 4) == 0) {
>      DEBUG ((DEBUG_INFO, "%a: empty command line received\n", __FUNCTION__));
> -    return;
> +    return EFI_SUCCESS;
>    }
>  
>    CommandLine[3] = ' ';
> @@ -331,6 +345,7 @@ UpdateBootArgs (
>    DEBUG_CODE_END ();
>  
>    FreePool (CommandLine - 4);
> +  return EFI_SUCCESS;
>  }
>  
>  
> @@ -402,16 +417,39 @@ FdtDxeInitialize (
>    Retval = fdt_open_into (FdtImage, mFdtImage, FdtSize);
>    ASSERT (Retval == 0);
>  
> -  SanitizePSCI ();
> -  CleanMemoryNodes ();
> -  CleanSimpleFramebuffer ();
> -  FixEthernetAliases ();
> -  UpdateMacAddress ();
> +  Status = SanitizePSCI ();
> +  if (EFI_ERROR (Status)) {
> +    Print (L"Failed to sanitize PSCI (error %d)\n", Status);
> +  }
> +
> +  Status = CleanMemoryNodes ();
> +  if (EFI_ERROR (Status)) {
> +    Print (L"Failed to clean memory nodes (error %d)\n", Status);
> +  }
> +
> +  Status = CleanSimpleFramebuffer ();
> +  if (EFI_ERROR (Status)) {
> +    Print (L"Failed to clean frame buffer (error %d)\n", Status);
> +  }
> +
> +  Status = FixEthernetAliases ();
> +  if (EFI_ERROR (Status)) {
> +    Print (L"Failed to fix ethernet aliases (error %d)\n", Status);
> +  }
> +
> +  Status = UpdateMacAddress ();
> +  if (EFI_ERROR (Status)) {
> +    Print (L"Failed to update MAC address (error %d)\n", Status);
> +  }
> +
>    if (Internal) {
>      /*
>       * A GPU-provided DTB already has the full command line.
>       */
> -    UpdateBootArgs ();
> +    Status = UpdateBootArgs ();
> +    if (EFI_ERROR (Status)) {
> +      Print (L"Failed to update boot arguments (error %d)\n", Status);
> +    }
>    }
>  
>    DEBUG ((DEBUG_INFO, "Installed FDT is at %p\n", mFdtImage));
> -- 
> 2.21.0
> 

[edk2-platforms: PATCH v2 0/1] Platform/RPi3: Report device tree

[Michael Brown]

On 06/08/2019 18:07, Leif Lindholm wrote:
> Happy with all of this, except for the use of EFI_DEVICE_ERROR.
> 
> Generally, EFI_DEVICE_ERROR is used as an i/o error type status.
> Either EFI_NOT_FOUND, EFI_INVALID_PARAMETER or EFI_OUT_OF_RECOURCES
> would be more appropriate when referring to device-tree manipulation.
> 
> Could you update these and spin a v2 please?

Here you go.  I have switched all of the EFI_DEVICE_ERROR uses to
EFI_NOT_FOUND, because I could not find any more appropriate
EFI_STATUS value to represent "device tree appears to be corrupted".

(EFI_INVALID_PARAMETER seemed misleading to me, since there is nothing
wrong with the parameters provided by the caller.)

Michael Brown (1):
  Platform/RPi3: Report device tree modification errors using Print()

 .../RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c  | 102 ++++++++++++------
 1 file changed, 70 insertions(+), 32 deletions(-)

-- 
2.21.0

Re: [edk2-platforms: PATCH v2 0/1] Platform/RPi3: Report device tree

[Leif Lindholm]

On Fri, Aug 09, 2019 at 12:16:24AM +0100, Michael Brown wrote:
> On 06/08/2019 18:07, Leif Lindholm wrote:
> > Happy with all of this, except for the use of EFI_DEVICE_ERROR.
> > 
> > Generally, EFI_DEVICE_ERROR is used as an i/o error type status.
> > Either EFI_NOT_FOUND, EFI_INVALID_PARAMETER or EFI_OUT_OF_RECOURCES
> > would be more appropriate when referring to device-tree manipulation.
> > 
> > Could you update these and spin a v2 please?
> 
> Here you go.  I have switched all of the EFI_DEVICE_ERROR uses to
> EFI_NOT_FOUND, because I could not find any more appropriate
> EFI_STATUS value to represent "device tree appears to be corrupted".
> 
> (EFI_INVALID_PARAMETER seemed misleading to me, since there is nothing
> wrong with the parameters provided by the caller.)

Fair point, and agree with the choice.
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
Pushed as 812b291c5589.

Thanks!

> Michael Brown (1):
>   Platform/RPi3: Report device tree modification errors using Print()
> 
>  .../RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c  | 102 ++++++++++++------
>  1 file changed, 70 insertions(+), 32 deletions(-)
> 
> -- 
> 2.21.0
> 

[edk2-platforms: PATCH v2 1/1] Platform/RPi3: Report device tree modification errors using Print()

[Michael Brown]

Most functions in FdtDxe currently return VOID and report errors using
only DEBUG.  Update functions to return EFI_STATUS and report errors
using Print() so that errors are at least visible in non-debug builds.

Signed-off-by: Michael Brown <mbrown@fensystems.co.uk>
---
 .../RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c  | 102 ++++++++++++------
 1 file changed, 70 insertions(+), 32 deletions(-)

diff --git a/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c b/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c
index 57e4bee8da..c84e5b2767 100644
--- a/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c
+++ b/Platform/RaspberryPi/RPi3/Drivers/FdtDxe/FdtDxe.c
@@ -13,6 +13,7 @@
 #include <Library/DxeServicesLib.h>
 #include <Library/MemoryAllocationLib.h>
 #include <Library/UefiBootServicesTableLib.h>
+#include <Library/UefiLib.h>
 #include <libfdt.h>
 
 #include <Protocol/RpiFirmware.h>
@@ -24,7 +25,7 @@ STATIC VOID                             *mFdtImage;
 STATIC RASPBERRY_PI_FIRMWARE_PROTOCOL   *mFwProtocol;
 
 STATIC
-VOID
+EFI_STATUS
 FixEthernetAliases (
   VOID
 )
@@ -36,6 +37,7 @@ FixEthernetAliases (
   UINTN         CopySize;
   CHAR8         *Copy;
   INTN          Retval;
+  EFI_STATUS    Status;
 
   //
   // Look up the 'ethernet[0]' aliases
@@ -43,14 +45,14 @@ FixEthernetAliases (
   Aliases = fdt_path_offset (mFdtImage, "/aliases");
   if (Aliases < 0) {
     DEBUG ((DEBUG_ERROR, "%a: failed to locate '/aliases'\n", __FUNCTION__));
-    return;
+    return EFI_NOT_FOUND;
   }
   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;
+    return EFI_NOT_FOUND;
   }
 
   //
@@ -60,15 +62,17 @@ FixEthernetAliases (
   Copy = AllocateCopyPool (CopySize, Alias);
   if (!Copy) {
     DEBUG ((DEBUG_ERROR, "%a: failed to copy '%a'\n", __FUNCTION__, Alias));
-    return;
+    return EFI_OUT_OF_RESOURCES;
   }
 
   //
   // Create missing aliases
   //
+  Status = EFI_SUCCESS;
   if (!Ethernet) {
     Retval = fdt_setprop (mFdtImage, Aliases, "ethernet", Copy, CopySize);
     if (Retval != 0) {
+      Status = EFI_NOT_FOUND;
       DEBUG ((DEBUG_ERROR, "%a: failed to create 'ethernet' alias (%d)\n",
         __FUNCTION__, Retval));
     }
@@ -77,6 +81,7 @@ FixEthernetAliases (
   if (!Ethernet0) {
     Retval = fdt_setprop (mFdtImage, Aliases, "ethernet0", Copy, CopySize);
     if (Retval != 0) {
+      Status = EFI_NOT_FOUND;
       DEBUG ((DEBUG_ERROR, "%a: failed to create 'ethernet0' alias (%d)\n",
         __FUNCTION__, Retval));
     }
@@ -84,10 +89,11 @@ FixEthernetAliases (
   }
 
   FreePool (Copy);
+  return Status;
 }
 
 STATIC
-VOID
+EFI_STATUS
 UpdateMacAddress (
   VOID
   )
@@ -103,7 +109,7 @@ UpdateMacAddress (
   Node = fdt_path_offset (mFdtImage, "ethernet");
   if (Node < 0) {
     DEBUG ((DEBUG_ERROR, "%a: failed to locate 'ethernet' alias\n", __FUNCTION__));
-    return;
+    return EFI_NOT_FOUND;
   }
 
   //
@@ -112,7 +118,7 @@ UpdateMacAddress (
   Status = mFwProtocol->GetMacAddress (MacAddress);
   if (EFI_ERROR (Status)) {
     DEBUG ((DEBUG_ERROR, "%a: failed to retrieve MAC address\n", __FUNCTION__));
-    return;
+    return Status;
   }
 
   Retval = fdt_setprop (mFdtImage, Node, "mac-address", MacAddress,
@@ -120,16 +126,17 @@ UpdateMacAddress (
   if (Retval != 0) {
     DEBUG ((DEBUG_ERROR, "%a: failed to create 'mac-address' property (%d)\n",
       __FUNCTION__, Retval));
-    return;
+    return EFI_NOT_FOUND;
   }
 
   DEBUG ((DEBUG_INFO, "%a: setting MAC address to %02x:%02x:%02x:%02x:%02x:%02x\n",
     __FUNCTION__, MacAddress[0], MacAddress[1], MacAddress[2], MacAddress[3],
     MacAddress[4], MacAddress[5]));
+  return EFI_SUCCESS;
 }
 
 STATIC
-VOID
+EFI_STATUS
 CleanMemoryNodes (
   VOID
   )
@@ -139,7 +146,7 @@ CleanMemoryNodes (
 
   Node = fdt_path_offset (mFdtImage, "/memory");
   if (Node < 0) {
-    return;
+    return EFI_SUCCESS;
   }
 
   /*
@@ -150,11 +157,14 @@ CleanMemoryNodes (
   Retval = fdt_del_node (mFdtImage, Node);
   if (Retval != 0) {
     DEBUG ((DEBUG_ERROR, "Failed to remove /memory\n"));
+    return EFI_NOT_FOUND;
   }
+
+  return EFI_SUCCESS;
 }
 
 STATIC
-VOID
+EFI_STATUS
 SanitizePSCI (
   VOID
   )
@@ -166,7 +176,7 @@ SanitizePSCI (
   Root = fdt_path_offset (mFdtImage, "/");
   ASSERT (Root >= 0);
   if (Root < 0) {
-    return;
+    return EFI_NOT_FOUND;
   }
 
   Node = fdt_path_offset (mFdtImage, "/psci");
@@ -177,41 +187,42 @@ SanitizePSCI (
   ASSERT (Node >= 0);
   if (Node < 0) {
     DEBUG ((DEBUG_ERROR, "Couldn't find/create /psci\n"));
-    return;
+    return EFI_NOT_FOUND;
   }
 
   Retval = fdt_setprop_string (mFdtImage, Node, "compatible", "arm,psci-1.0");
   if (Retval != 0) {
     DEBUG ((DEBUG_ERROR, "Couldn't set /psci compatible property\n"));
-    return;
+    return EFI_NOT_FOUND;
   }
 
   Retval = fdt_setprop_string (mFdtImage, Node, "method", "smc");
   if (Retval != 0) {
     DEBUG ((DEBUG_ERROR, "Couldn't set /psci method property\n"));
-    return;
+    return EFI_NOT_FOUND;
   }
 
   Root = fdt_path_offset (mFdtImage, "/cpus");
   if (Root < 0) {
     DEBUG ((DEBUG_ERROR, "No CPUs to update with PSCI enable-method?\n"));
-    return;
+    return EFI_NOT_FOUND;
   }
 
   Node = fdt_first_subnode (mFdtImage, Root);
   while (Node >= 0) {
     if (fdt_setprop_string (mFdtImage, Node, "enable-method", "psci") != 0) {
       DEBUG ((DEBUG_ERROR, "Failed to update enable-method for a CPU\n"));
-      return;
+      return EFI_NOT_FOUND;
     }
 
     fdt_delprop (mFdtImage, Node, "cpu-release-addr");
     Node = fdt_next_subnode (mFdtImage, Node);
   }
+  return EFI_SUCCESS;
 }
 
 STATIC
-VOID
+EFI_STATUS
 CleanSimpleFramebuffer (
   VOID
   )
@@ -225,7 +236,7 @@ CleanSimpleFramebuffer (
    */
   Node = fdt_path_offset (mFdtImage, "display0");
   if (Node < 0) {
-    return;
+    return EFI_SUCCESS;
   }
 
   /*
@@ -236,25 +247,28 @@ CleanSimpleFramebuffer (
   Retval = fdt_del_node (mFdtImage, Node);
   if (Retval != 0) {
     DEBUG ((DEBUG_ERROR, "Failed to remove display0\n"));
-    return;
+    return EFI_NOT_FOUND;
   }
 
   Node = fdt_path_offset (mFdtImage, "/aliases");
   if (Node < 0) {
     DEBUG ((DEBUG_ERROR, "Couldn't find /aliases to remove display0\n"));
-    return;
+    return EFI_NOT_FOUND;
   }
 
   Retval = fdt_delprop (mFdtImage, Node, "display0");
   if (Retval != 0) {
     DEBUG ((DEBUG_ERROR, "Failed to remove display0 alias\n"));
+    return EFI_NOT_FOUND;
   }
+
+  return EFI_SUCCESS;
 }
 
 #define MAX_CMDLINE_SIZE    512
 
 STATIC
-VOID
+EFI_STATUS
 UpdateBootArgs (
   VOID
   )
@@ -270,7 +284,7 @@ UpdateBootArgs (
   Node = fdt_path_offset (mFdtImage, "/chosen");
   if (Node < 0) {
     DEBUG ((DEBUG_ERROR, "%a: failed to locate /chosen node\n", __FUNCTION__));
-    return;
+    return EFI_NOT_FOUND;
   }
 
   //
@@ -282,7 +296,7 @@ UpdateBootArgs (
   CommandLine = AllocatePool (MAX_CMDLINE_SIZE) + 4;
   if (!CommandLine) {
     DEBUG ((DEBUG_ERROR, "%a: failed to allocate memory\n", __FUNCTION__));
-    return;
+    return EFI_OUT_OF_RESOURCES;
   }
 
   //
@@ -291,12 +305,12 @@ UpdateBootArgs (
   Status = mFwProtocol->GetCommandLine (MAX_CMDLINE_SIZE, CommandLine + 4);
   if (EFI_ERROR (Status)) {
     DEBUG ((DEBUG_ERROR, "%a: failed to retrieve command line\n", __FUNCTION__));
-    return;
+    return Status;
   }
 
   if (AsciiStrLen (CommandLine + 4) == 0) {
     DEBUG ((DEBUG_INFO, "%a: empty command line received\n", __FUNCTION__));
-    return;
+    return EFI_SUCCESS;
   }
 
   CommandLine[3] = ' ';
@@ -331,6 +345,7 @@ UpdateBootArgs (
   DEBUG_CODE_END ();
 
   FreePool (CommandLine - 4);
+  return EFI_SUCCESS;
 }
 
 
@@ -402,16 +417,39 @@ FdtDxeInitialize (
   Retval = fdt_open_into (FdtImage, mFdtImage, FdtSize);
   ASSERT (Retval == 0);
 
-  SanitizePSCI ();
-  CleanMemoryNodes ();
-  CleanSimpleFramebuffer ();
-  FixEthernetAliases ();
-  UpdateMacAddress ();
+  Status = SanitizePSCI ();
+  if (EFI_ERROR (Status)) {
+    Print (L"Failed to sanitize PSCI (error %d)\n", Status);
+  }
+
+  Status = CleanMemoryNodes ();
+  if (EFI_ERROR (Status)) {
+    Print (L"Failed to clean memory nodes (error %d)\n", Status);
+  }
+
+  Status = CleanSimpleFramebuffer ();
+  if (EFI_ERROR (Status)) {
+    Print (L"Failed to clean frame buffer (error %d)\n", Status);
+  }
+
+  Status = FixEthernetAliases ();
+  if (EFI_ERROR (Status)) {
+    Print (L"Failed to fix ethernet aliases (error %d)\n", Status);
+  }
+
+  Status = UpdateMacAddress ();
+  if (EFI_ERROR (Status)) {
+    Print (L"Failed to update MAC address (error %d)\n", Status);
+  }
+
   if (Internal) {
     /*
      * A GPU-provided DTB already has the full command line.
      */
-    UpdateBootArgs ();
+    Status = UpdateBootArgs ();
+    if (EFI_ERROR (Status)) {
+      Print (L"Failed to update boot arguments (error %d)\n", Status);
+    }
   }
 
   DEBUG ((DEBUG_INFO, "Installed FDT is at %p\n", mFdtImage));
-- 
2.21.0

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

[Leif Lindholm]

On Wed, Jul 24, 2019 at 10:43:42PM +0100, Leif Lindholm wrote:
> On Wed, Jul 24, 2019 at 09:53:00PM +0100, Michael Brown wrote:
> > On 24/07/2019 19:53, Leif Lindholm wrote:
> > > >     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?
> > 
> > Yes, I think that would be very sensible, but it would also be inconsistent
> > with the rest of the code: all of the other device-tree-modifying functions
> > in FdtDxe.c are declared as VOID and use only DEBUG to report errors.
> > 
> > Changing this pattern throughout FdtDxe.c should, I think, be a completely
> > separate patch.  Would it be possible to merge the current patch, and I can
> > then follow up with a second patch to improve the error visibility in
> > non-debug builds?
> 
> Yeah, that works for me.
> I'll push it tomorrow when I'm more awake, but for now:
> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

Pushed as 9181684081bf.

Thanks!

/
    Leif

Re: [edk2-platforms: PATCH 1/1] Platform/RPi3: Accept "ethernet" or "ethernet0" aliases in device tree

[Pete Batard]

Just adding a comment for one item, that isn't directly relevant to what 
direction we should take with this patch, but that clarifies where we 
got our .dtb's from.

On 2019.07.23 12:00, Michael Brown wrote:
> On 23/07/2019 11:34, Leif Lindholm wrote:
>> On Fri, Jul 19, 2019 at 06:29:07PM +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").
>>>
>>> Accept either "ethernet" or "ethernet0" as being the alias to the node
>>> for which the MAC address should be updated.
>>
>> Why is this patch useful?
>> I understand the problem, but we include the .dtb from our own
>> edk2-non-osi tree. And it seems that device tree already provides an
>> alias to support both:
>> https://github.com/tianocore/edk2-non-osi/blob/master/Platform/RaspberryPi/RPi3/DeviceTree/bcm2710-rpi-3-b.dts 
>>
>>
>> Would it be more useful to warn loudly if "ethernet" isn't present?
>> (And just *replace* ethernet[0] with ethernet below.)
> 
> The device tree can also be provided by the VC4 firmware, e.g. with a 
> config.txt containing e.g.
> 
>    device_tree_address=0x10000
>    device_tree_end=0x20000
>    device_tree=bcm2710-rpi-3-b-plus.dtb
> 
> It is also possible to omit the explicit "device_tree=..." and have just
> 
>    device_tree_address=0x10000
>    device_tree_end=0x20000
> 
> This configuration will cause the VC4 firmware to automatically load the 
> appropriate device tree file.  I have tested that this allows me to boot 
> from a 3B or 3B+ using a single SD card image that contains all of the 
> .dtb files from
> 
>    https://github.com/raspberrypi/firmware/tree/master/boot
> 
> These "official" raspberrypi-firmware .dtb files are built from the 
> stock Linux kernel .dts files, and contain only "ethernet0".

Note that the .dtb's we use for this platform (in edk2-non-osi) were 
actually picked up from the URL above, then decompiled to produce a .dts 
which was then edited to fix a couple issues, as per:
https://github.com/tianocore/edk2-non-osi/blob/master/Platform/RaspberryPi/RPi3/DeviceTree/bcm2710-rpi-3-b-plus.dts#L6-L15

Most notably, unless you change
   compatible = "brcm,bcm2708-usb";
to
   compatible = "brcm,bcm2835-usb";
in the usb@7e980000 section you will find your USB keyboard may not work 
during a Linux install (Tested with both the Ubuntu 18.04 and Debian 10 
installers).

Now, I am also aware that the Raspberry Pi Foundation has updated its 
official .dtb's for the Pi 3 with the release of the Pi 4, and I have 
some plans to send a patch to also update our Device Tree accordingly at 
some stage. For the record, I've been testing these updated official 
versions last week and confirmed that the keyboard issue was still present.

Regards,

/Pete

> To address your specific questions:
> 
>  > Why is this patch useful?
> 
> It allows us to boot using the "official" .dtb files without any 
> unexpected behaviour.
> 
> This in turn allows us to boot with the VC4 firmware automatically 
> selecting the appropriate .dtb file for the hardware.  This is extremely 
> useful when trying to provide a single SD card image that will work on a 
> wide range of hardware.
> 
>  > Would it be more useful to warn loudly if "ethernet" isn't present?
>  > (And just *replace* ethernet[0] with ethernet below.)
> 
> This would also solve the problem.  I would find it mildly surprising 
> behaviour if I were debugging around it, since the "official" .dtb files 
> have moved in the other direction (going from "ethernet" to 
> "ethernet0"), but I would not expect it to break anything.
> 
> Another option would be to have UpdateMacAddress() ensure that both 
> "ethernet" and "ethernet0" aliases exist.
> 
> I am happy to work up a new version of the patch: just let me know which 
> option you prefer.
> 
> Many thanks,
> 
> Michael