Re: [PATCH 1/1] efi_loader: support all uclasses in device path

[Heinrich Schuchardt]

On 19.07.23 07:00, AKASHI Takahiro wrote:
On Wed, Jul 19, 2023 at 06:43:08AM +0200, Heinrich Schuchardt wrote:
On devices with multiple USB mass storage devices errors like
     Path /../USB(0x0,0x0)/USB(0x1,0x0)/Ctrl(0x0)
     already installed.

are seen. This is due to creating non-unique device paths. To uniquely
identify devices we must provide path nodes for all devices on the path
from the root device.

Add support for generating device path nodes for all uclasses.

I think that this is the last resort for devices that cannot be classified/
identified by any other means. If possible, should we provide a more concrete
device path?
I have submitted a patch specifically for USB devices in the past:
https://lists.denx.de/pipermail/u-boot/2021-November/468216.html

# I'm no longer working on this patch, though.

UCLASS_USB models USB host controllers. The UEFI specification has this 
example:

    PciRoot(0)/PCI(31,2)/USB(0,0)

Here PCI(31,2) is the host controller and USB(0,0) is a device attached 
to its port 0.

As can be seen from the example USB host controllers are not meant to be 
modelled as USB() nodes. The UEFI specification has no specific node 
type for devices in the SoC or directly attached to the SoC without PCIe 
bus.


-Takahiro Akashi

My patch creates VenHW() nodes for all uclasses that have no specific 
device path node type assigned in our code. This does not exclude 
providing a more specific modelling in future.

With my patch the PCI(31,2) node from the example above would be 
represented as two nodes

   VenHW()/USB(0,0)

where VenHW would be the PCIe device (UCLASS_USB) and USB(0,0) the 
embedded USB root hub (UCLASS_USB_HUB).

Specifically PCIe devices need further work.

Best regards

Heinrich



Reported-by: Suniel Mahesh <su...@amarulasolutions.com>
Signed-off-by: Heinrich Schuchardt <heinrich.schucha...@canonical.com>
---
  include/efi_api.h                |  7 ++++
  lib/efi_loader/efi_device_path.c | 56 ++++++++++++++------------------
  2 files changed, 32 insertions(+), 31 deletions(-)

diff --git a/include/efi_api.h b/include/efi_api.h
index 55a4c989fc..8f5ef5f680 100644
--- a/include/efi_api.h
+++ b/include/efi_api.h
@@ -579,6 +579,13 @@ struct efi_device_path_vendor {
        u8 vendor_data[];
  } __packed;
  
+struct efi_device_path_udevice {
+       struct efi_device_path dp;
+       efi_guid_t guid;
+       int uclass_id;
+       int dev_number;
+} __packed;
+
  struct efi_device_path_controller {
        struct efi_device_path dp;
        u32 controller_number;
diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c
index 04ebb449ca..d0be869d94 100644
--- a/lib/efi_loader/efi_device_path.c
+++ b/lib/efi_loader/efi_device_path.c
@@ -10,6 +10,7 @@
  #include <common.h>
  #include <blk.h>
  #include <dm.h>
+#include <dm/root.h>
  #include <log.h>
  #include <net.h>
  #include <usb.h>
@@ -38,16 +39,6 @@ const struct efi_device_path END = {
        .length   = sizeof(END),
  };
  
-/* template ROOT node: */
-static const struct efi_device_path_vendor ROOT = {
-       .dp = {
-               .type     = DEVICE_PATH_TYPE_HARDWARE_DEVICE,
-               .sub_type = DEVICE_PATH_SUB_TYPE_VENDOR,
-               .length   = sizeof(ROOT),
-       },
-       .guid = U_BOOT_GUID,
-};
-
  #if defined(CONFIG_MMC)
  /*
   * Determine if an MMC device is an SD card.
@@ -497,13 +488,12 @@ bool efi_dp_is_multi_instance(const struct 
efi_device_path *dp)
  __maybe_unused static unsigned int dp_size(struct udevice *dev)
  {
        if (!dev || !dev->driver)
-               return sizeof(ROOT);
+               return sizeof(struct efi_device_path_udevice);
  
  	switch (device_get_uclass_id(dev)) {
        case UCLASS_ROOT:
-       case UCLASS_SIMPLE_BUS:
                /* stop traversing parents at this point: */
-               return sizeof(ROOT);
+               return sizeof(struct efi_device_path_udevice);
        case UCLASS_ETH:
                return dp_size(dev->parent) +
                        sizeof(struct efi_device_path_mac_addr);
@@ -582,8 +572,8 @@ __maybe_unused static unsigned int dp_size(struct udevice 
*dev)
                return dp_size(dev->parent) +
                        sizeof(struct efi_device_path_usb);
        default:
-               /* just skip over unknown classes: */
-               return dp_size(dev->parent);
+               return dp_size(dev->parent) +
+                       sizeof(struct efi_device_path_udevice);
        }
  }
  
@@ -600,13 +590,6 @@ __maybe_unused static void *dp_fill(void *buf, struct udevice *dev)
                return buf;
  
  	switch (device_get_uclass_id(dev)) {
-       case UCLASS_ROOT:
-       case UCLASS_SIMPLE_BUS: {
-               /* stop traversing parents at this point: */
-               struct efi_device_path_vendor *vdp = buf;
-               *vdp = ROOT;
-               return &vdp[1];
-       }
  #ifdef CONFIG_NETDEVICES
        case UCLASS_ETH: {
                struct efi_device_path_mac_addr *dp =
@@ -811,11 +794,24 @@ __maybe_unused static void *dp_fill(void *buf, struct 
udevice *dev)
  
  		return &udp[1];
        }
-       default:
-               /* If the uclass driver is missing, this will show NULL */
-               log_debug("unhandled device class: %s (%s)\n", dev->name,
-                         dev_get_uclass_name(dev));
-               return dp_fill(buf, dev->parent);
+       default: {
+               struct efi_device_path_udevice *vdp;
+               enum uclass_id uclass_id = device_get_uclass_id(dev);
+
+               if (uclass_id == UCLASS_ROOT)
+                       vdp = buf;
+               else
+                       vdp = dp_fill(buf, dev->parent);
+
+               vdp->dp.type = DEVICE_PATH_TYPE_HARDWARE_DEVICE;
+               vdp->dp.sub_type = DEVICE_PATH_SUB_TYPE_VENDOR;
+               vdp->dp.length = sizeof(*vdp);
+               memcpy(&vdp->guid, &efi_u_boot_guid, sizeof(efi_guid_t));
+               vdp->uclass_id = uclass_id;
+               vdp->dev_number = dev->seq_;
+
+               return &vdp[1];
+           }
        }
  }
  
@@ -1052,14 +1048,12 @@ struct efi_device_path *efi_dp_from_uart(void)
  {
        void *buf, *pos;
        struct efi_device_path_uart *uart;
-       size_t dpsize = sizeof(ROOT) + sizeof(*uart) + sizeof(END);
+       size_t dpsize = dp_size(dm_root()) + sizeof(*uart) + sizeof(END);
  
  	buf = efi_alloc(dpsize);
        if (!buf)
                return NULL;
-       pos = buf;
-       memcpy(pos, &ROOT, sizeof(ROOT));
-       pos += sizeof(ROOT);
+       pos = dp_fill(buf, dm_root());
        uart = pos;
        uart->dp.type = DEVICE_PATH_TYPE_MESSAGING_DEVICE;
        uart->dp.sub_type = DEVICE_PATH_SUB_TYPE_MSG_UART;
--
2.40.1