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 sunil@amarulasolutions.com Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@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