Re: [BUG] efi_loader: incorrect creation of device paths

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

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.akashi@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)
000000013ef00270 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/MAC(525252525252,1)

Signed-off-by: AKASHI Takahiro <takahiro.akashi@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;
 	}
-- 
2.33.0