On 11/25/21 06:44, AKASHI Takahiro wrote:
Heinrich,

On Wed, Nov 24, 2021 at 12:10:32PM +0900, AKASHI Takahiro wrote:
On Sat, Nov 20, 2021 at 01:54:30PM +0100, Heinrich Schuchardt wrote:
Hello Takahiro,

in a prior mail we have discussed the creation of device paths for USB
mass storage devices.

On the sand boxyou get the following devices after 'usb start':

  Class     Index  Probed  Driver                Name
-----------------------------------------------------------
  usb           0  [ + ]   usb_sandbox           |-- usb@1
  usb_hub       0  [ + ]   usb_hub               |   `-- hub
  usb_mass_s    0  [ + ]   usb_mass_storage      |       |--
usb_mass_storage
  blk           3  [   ]   usb_storage_blk       |       |   `--
usb_mass_storage.lun0
  usb_mass_s    1  [ + ]   usb_mass_storage      |       |--
usb_mass_storage
  blk           4  [   ]   usb_storage_blk       |       |   `--
usb_mass_storage.lun0
  usb_mass_s    2  [ + ]   usb_mass_storage      |       `--
usb_mass_storage
  blk           5  [   ]   usb_storage_blk       |           `--
usb_mass_storage.lun0

For of these storage devices we try to create the same device path which
is not allowable:

=> usb start
starting USB...
Bus usb@1: scanning bus usb@1 for devices... 5 USB Device(s) found
        scanning usb for storage devices... 3 Storage Device(s) found
=>
=> efidebug dh
Scanning disk mmc2.blk...
handle 0000000015e34f00,
/VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(2)/SD(0)
Scanning disk mmc1.blk...
handle 0000000015e36b30,
/VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(1)/SD(1)
Scanning disk mmc0.blk...
handle 0000000015e35b00,
/VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(2)
Scanning disk usb_mass_storage.lun0...
handle 0000000015e35c10,
/VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/UsbClass(0x1234,0x5678,0x9,0x0,0x0)/UsbClass(0x1234,0x5678,0x0,0x0,0x0)
fs_devread read outside partition 2
Failed to mount ext2 filesystem...
BTRFS: superblock end 69632 is larger than device size 512
Scanning disk usb_mass_storage.lun0...
handle 0000000015e361f0,
/VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/UsbClass(0x1234,0x5678,0x9,0x0,0x0)/UsbClass(0x1234,0x5678,0x0,0x0,0x0)
ERROR: failure to add disk device usb_mass_storage.lun0, r = 20
Error: Cannot initialize UEFI sub-system, r = 20

I will provide a patch that will allow the first USB device to be used
and avoids stopping the boot process. But we really have to walk the dm
tree to create a device patch for USB devices based on port IDs.

We should add a new field to struct uclass_driver:

struct efi_device_path *get_node(udevice *dev);

This function shall return a pointer to an freshly allocated buffer with
the device node for the device. To build the devicepath we can then walk
the dm tree.

I'm not sure this is an acceptable solution.

Let me make sure:
The goal would be to create a device path like
    .../USB(0x1,0x0)/HD(1,...)
instead of
    .../UsbHub(0x0,0x0,0x0,0x3)/UsbMassStorage(0x46f4,0x1,0x0,0x0)/HD(1,...)
as we already see this format for SCSI:
    .../Scsi(0,0)/HD(1,..)

Right?

Please try the tweak attached below.
(I think what I did here is trivial.)

If you like, I will post it as a patch.
(See "10.3.4.5.1 USB Device Path Example".)

-Takahiro Akashi

 From cda91e9d8144f89f0d73738b338289a7940bbe0e Mon Sep 17 00:00:00 2001
From: AKASHI Takahiro <takahiro.aka...@linaro.org>
Date: Thu, 25 Nov 2021 14:20:08 +0900
Subject: [PATCH] efi_loader: disk: usb mass-storage

- use path_usb instead of path_usb_class for existing USB dp nodes
- add a dp node for UCLASS_USB

=> usb start
starting USB...
Bus xhci_pci: Register 8001040 NbrPorts 8
Starting the controller
USB XHCI 1.00
scanning bus xhci_pci for devices... 4 USB Device(s) found
        scanning usb for storage devices... 2 Storage Device(s) found
=> dm tree
  Class     Index  Probed  Driver                Name
-----------------------------------------------------------
  root          0  [ + ]   root_driver           root_driver
  ...
  pci           0  [ + ]   pci_generic_ecam      |-- pcie@10000000
  pci_generi    0  [   ]   pci_generic_drv       |   |-- pci_0:0.0
  virtio       32  [ + ]   virtio-pci.l          |   |-- virtio-pci.l#0
  ethernet      0  [ + ]   virtio-net            |   |   `-- virtio-net#32
  usb           0  [ + ]   xhci_pci              |   `-- xhci_pci
  usb_hub       0  [ + ]   usb_hub               |       `-- usb_hub
  usb_dev_ge    0  [ + ]   usb_dev_generic_drv   |           |-- 
generic_bus_0_dev_2
  usb_mass_s    0  [ + ]   usb_mass_storage      |           |-- 
usb_mass_storage
  blk           0  [   ]   usb_storage_blk       |           |   `-- 
usb_mass_storage.lun0
  usb_mass_s    1  [ + ]   usb_mass_storage      |           `-- 
usb_mass_storage
  blk           1  [   ]   usb_storage_blk       |               `-- 
usb_mass_storage.lun0
  ...
=> efidebug devices
Scanning disk usb_mass_storage.lun0...
Scanning disk usb_mass_storage.lun0...
Found 6 disks
Missing RNG device for EFI_RNG_PROTOCOL
** Unable to read file ubootefi.var **
Failed to load EFI variables
Unable to find TPMv2 device
Device           Device Path
================ ====================
000000013eef3a50 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)
000000013eefe840 
/VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/USB(0xf3,0x0)/USB(0x0,0x0)/USB(0x2,0x0)
000000013eefe990 
/VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/USB(0xf3,0x0)/USB(0x0,0x0)/USB(0x2,0x0)/HD(1,0x01,0,0x0,0x99800)
000000013eefeae0 
/VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/USB(0xf3,0x0)/USB(0x0,0x0)/USB(0x2,0x0)/HD(2,0x01,0,0x99800,0x1800)
000000013eeff5f0 
/VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/USB(0xf3,0x0)/USB(0x0,0x0)/USB(0x3,0x0)
000000013eeff990 
/VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/USB(0xf3,0x0)/USB(0x0,0x0)/USB(0x3,0x0)/HD(1,GPT,ce86c5a7-b32a-488f-a346-88fe698e0edc,0x22,0x4c2a)
000000013eeffda0 
/VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/USB(0xf3,0x0)/USB(0x0,0x0)/USB(0x3,0x0)/HD(2,GPT,aa80aab9-33e6-42b6-b5db-def2cb8d7844,0x5000,0x1a800)

I am missing a PCI() node here for the USB root Hub. In the DM tree: Is
xhci_pci the root hub and usb_hub a hub connected to this root hub? Or
do both nodes together model a root hub? Anyway USB(0xf3,0x0) seems to
be wrong. There is no USB hub with a port 243.

Please, see the examples on p. 303ff of the UEFI 2.9 specification:

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

The first example is a USB controller connected to port 0 of the root hub.
The second is a USB controller connected to port 3 of a USB hub which is
connected to port 1 of the root hub.

Best regards

Heinrich

000000013ef00270 
/VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/MAC(525252525252,1)

Signed-off-by: AKASHI Takahiro <takahiro.aka...@linaro.org>
---
  lib/efi_loader/efi_device_path.c         | 36 ++++++++++++++++++++++--
  lib/efi_loader/efi_device_path_to_text.c | 17 ++++++++---
  2 files changed, 47 insertions(+), 6 deletions(-)

diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c
index 735ed0bd0f4c..a8ec6809c422 100644
--- a/lib/efi_loader/efi_device_path.c
+++ b/lib/efi_loader/efi_device_path.c
@@ -557,10 +557,18 @@ __maybe_unused static unsigned int dp_size(struct udevice 
*dev)
                return dp_size(dev->parent) +
                        sizeof(struct efi_device_path_sd_mmc_path);
  #endif
+#if 1
+       case UCLASS_USB:
+       case UCLASS_USB_HUB:
+       case UCLASS_MASS_STORAGE:
+               return dp_size(dev->parent) +
+                       sizeof(struct efi_device_path_usb);
+#else
        case UCLASS_MASS_STORAGE:
        case UCLASS_USB_HUB:
                return dp_size(dev->parent) +
                        sizeof(struct efi_device_path_usb_class);
+#endif
        default:
                /* just skip over unknown classes: */
                return dp_size(dev->parent);
@@ -740,6 +748,24 @@ __maybe_unused static void *dp_fill(void *buf, struct 
udevice *dev)
                return &sddp[1];
        }
  #endif
+#if 1
+       case UCLASS_USB:
+       case UCLASS_USB_HUB:
+       case UCLASS_MASS_STORAGE: {
+               struct efi_device_path_usb *udp =
+                       dp_fill(buf, dev->parent);
+               struct usb_device *udev = dev_get_parent_priv(dev);
+
+               udp->dp.type     = DEVICE_PATH_TYPE_MESSAGING_DEVICE;
+               udp->dp.sub_type = DEVICE_PATH_SUB_TYPE_MSG_USB;
+               udp->dp.length   = sizeof(*udp);
+               udp->parent_port_number = udev->portnr;
+               /* TODO: see usb_new_device() */
+               udp->usb_interface = 0;
+
+               return &udp[1];
+       }
+#else
        case UCLASS_MASS_STORAGE:
        case UCLASS_USB_HUB: {
                struct efi_device_path_usb_class *udp =
@@ -750,14 +776,20 @@ __maybe_unused static void *dp_fill(void *buf, struct 
udevice *dev)
                udp->dp.type     = DEVICE_PATH_TYPE_MESSAGING_DEVICE;
                udp->dp.sub_type = DEVICE_PATH_SUB_TYPE_MSG_USB_CLASS;
                udp->dp.length   = sizeof(*udp);
-               udp->vendor_id   = desc->idVendor;
-               udp->product_id  = desc->idProduct;
+#if 1
+               if (dev->driver->id == UCLASS_MASS_STORAGE)
+                       udp->device_class    = 8;
+               else if (dev->driver->id == UCLASS_USB_HUB)
+                       udp->device_class    = 9;
+#else
                udp->device_class    = desc->bDeviceClass;
+#endif
                udp->device_subclass = desc->bDeviceSubClass;
                udp->device_protocol = desc->bDeviceProtocol;

                return &udp[1];
        }
+#endif
        default:
                debug("%s(%u) %s: unhandled device class: %s (%u)\n",
                      __FILE__, __LINE__, __func__,
diff --git a/lib/efi_loader/efi_device_path_to_text.c 
b/lib/efi_loader/efi_device_path_to_text.c
index 57fa9d97f712..a21de4157307 100644
--- a/lib/efi_loader/efi_device_path_to_text.c
+++ b/lib/efi_loader/efi_device_path_to_text.c
@@ -158,10 +158,19 @@ static char *dp_msging(char *s, struct efi_device_path 
*dp)
                struct efi_device_path_usb_class *ucdp =
                        (struct efi_device_path_usb_class *)dp;

-               s += sprintf(s, "UsbClass(0x%x,0x%x,0x%x,0x%x,0x%x)",
-                       ucdp->vendor_id, ucdp->product_id,
-                       ucdp->device_class, ucdp->device_subclass,
-                       ucdp->device_protocol);
+               if (ucdp->device_class == 8)
+                       s += sprintf(s, "UsbMassStorage(0x%x,0x%x,0x%x,0x%x)",
+                               ucdp->vendor_id, ucdp->product_id,
+                               ucdp->device_subclass, ucdp->device_protocol);
+               else if (ucdp->device_class == 9)
+                       s += sprintf(s, "UsbHub(0x%x,0x%x,0x%x,0x%x)",
+                               ucdp->vendor_id, ucdp->product_id,
+                               ucdp->device_subclass, ucdp->device_protocol);
+               else
+                       s += sprintf(s, "UsbClass(0x%x,0x%x,0x%x,0x%x,0x%x)",
+                               ucdp->vendor_id, ucdp->product_id,
+                               ucdp->device_class, ucdp->device_subclass,
+                               ucdp->device_protocol);

                break;
        }


Reply via email to