* [BUG] efi_loader: incorrect creation of device paths
@ 2021-11-20 12:54 Heinrich Schuchardt
2021-11-24 3:10 ` AKASHI Takahiro
0 siblings, 1 reply; 9+ messages in thread
From: Heinrich Schuchardt @ 2021-11-20 12:54 UTC (permalink / raw)
To: AKASHI Takahiro
Cc: Simon Glass, U-Boot Mailing List, Ilias Apalodimas,
Alexander Graf
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.
To make migration easier: If the function pointer or the return value is
NULL we can create a CTRL() node as dummy using the uclass_id and the
device number.
Best regards
Heinrich
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [BUG] efi_loader: incorrect creation of device paths
2021-11-20 12:54 [BUG] efi_loader: incorrect creation of device paths Heinrich Schuchardt
@ 2021-11-24 3:10 ` AKASHI Takahiro
2021-11-25 5:44 ` AKASHI Takahiro
0 siblings, 1 reply; 9+ messages in thread
From: AKASHI Takahiro @ 2021-11-24 3:10 UTC (permalink / raw)
To: Heinrich Schuchardt
Cc: Simon Glass, U-Boot Mailing List, Ilias Apalodimas,
Alexander Graf
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?
-Takahiro Akashi
> To make migration easier: If the function pointer or the return value is
> NULL we can create a CTRL() node as dummy using the uclass_id and the
> device number.
>
> Best regards
>
> Heinrich
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [BUG] efi_loader: incorrect creation of device paths
2021-11-24 3:10 ` AKASHI Takahiro
@ 2021-11-25 5:44 ` AKASHI Takahiro
2021-12-03 7:12 ` AKASHI Takahiro
2021-12-03 16:32 ` Heinrich Schuchardt
0 siblings, 2 replies; 9+ messages in thread
From: AKASHI Takahiro @ 2021-11-25 5:44 UTC (permalink / raw)
To: Heinrich Schuchardt, Simon Glass, U-Boot Mailing List,
Ilias Apalodimas, Alexander Graf
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [BUG] efi_loader: incorrect creation of device paths
2021-11-25 5:44 ` AKASHI Takahiro
@ 2021-12-03 7:12 ` AKASHI Takahiro
2021-12-03 16:32 ` Heinrich Schuchardt
1 sibling, 0 replies; 9+ messages in thread
From: AKASHI Takahiro @ 2021-12-03 7:12 UTC (permalink / raw)
To: Heinrich Schuchardt, Simon Glass, U-Boot Mailing List,
Ilias Apalodimas, Alexander Graf
Heinrich,
On Thu, Nov 25, 2021 at 02:44:28PM +0900, 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".)
Any comment on my proposal?
> -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
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [BUG] efi_loader: incorrect creation of device paths
2021-11-25 5:44 ` AKASHI Takahiro
2021-12-03 7:12 ` AKASHI Takahiro
@ 2021-12-03 16:32 ` Heinrich Schuchardt
2021-12-06 4:16 ` AKASHI Takahiro
2023-07-19 10:15 ` Heinrich Schuchardt
1 sibling, 2 replies; 9+ messages in thread
From: Heinrich Schuchardt @ 2021-12-03 16:32 UTC (permalink / raw)
To: AKASHI Takahiro
Cc: Simon Glass, U-Boot Mailing List, Ilias Apalodimas,
Alexander Graf
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.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)
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.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;
> }
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [BUG] efi_loader: incorrect creation of device paths
2021-12-03 16:32 ` Heinrich Schuchardt
@ 2021-12-06 4:16 ` AKASHI Takahiro
2021-12-06 11:41 ` Mark Kettenis
2023-07-19 10:15 ` Heinrich Schuchardt
1 sibling, 1 reply; 9+ messages in thread
From: AKASHI Takahiro @ 2021-12-06 4:16 UTC (permalink / raw)
To: Heinrich Schuchardt
Cc: Simon Glass, U-Boot Mailing List, Ilias Apalodimas,
Alexander Graf
On Fri, Dec 03, 2021 at 05:32:49PM +0100, Heinrich Schuchardt wrote:
> 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.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)
>
> 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.
As the code suggests, the path was generated this way:
/VenHw(...)/
USB(0xf3,0x0)/ <= usb_controller (xhci_pci)
USB(0x0,0x0)/ <= usb_hub
USB(0x3,0x0)/ <= usb_mass_storage
HD(1,GPT,...)
USB(0xf3,0x0) was incorrectly encoded here, instead we will be able
to generate PCI(xx,yy) by examining
struct pci_child_plat *pplat = dev_get_parent_plat(dev);
xx = PCI_DEV(pplat->devfn), yy = PCI_FUNC(pplat->devfn)
if the dev's parent is a PCI (controller).
But we will have to confirm that this is the only case that
we need to take care of the parent's bus.
Please note that any kind of pci component is not encoded
in a device path under the current UEFI implementation on U-Boot.
>
> 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.
So do you agree that this is a right approach even if it will break
some sort of compatibility (of device path representation) with the current
U-Boot UEFI implementation?
-Takahiro Akashi
> Best regards
>
> Heinrich
>
> > 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;
> > }
> >
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [BUG] efi_loader: incorrect creation of device paths
2021-12-06 4:16 ` AKASHI Takahiro
@ 2021-12-06 11:41 ` Mark Kettenis
2021-12-07 1:33 ` AKASHI Takahiro
0 siblings, 1 reply; 9+ messages in thread
From: Mark Kettenis @ 2021-12-06 11:41 UTC (permalink / raw)
To: AKASHI Takahiro
Cc: takahiro.akashi, xypron.glpk, sjg, u-boot, ilias.apalodimas,
agraf
> Date: Mon, 6 Dec 2021 13:16:23 +0900
> From: AKASHI Takahiro <takahiro.akashi@linaro.org>
>
> On Fri, Dec 03, 2021 at 05:32:49PM +0100, Heinrich Schuchardt wrote:
> > 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.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)
> >
> > 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.
>
> As the code suggests, the path was generated this way:
>
> /VenHw(...)/
> USB(0xf3,0x0)/ <= usb_controller (xhci_pci)
> USB(0x0,0x0)/ <= usb_hub
> USB(0x3,0x0)/ <= usb_mass_storage
> HD(1,GPT,...)
>
> USB(0xf3,0x0) was incorrectly encoded here, instead we will be able
> to generate PCI(xx,yy) by examining
> struct pci_child_plat *pplat = dev_get_parent_plat(dev);
> xx = PCI_DEV(pplat->devfn), yy = PCI_FUNC(pplat->devfn)
> if the dev's parent is a PCI (controller).
>
> But we will have to confirm that this is the only case that
> we need to take care of the parent's bus.
> Please note that any kind of pci component is not encoded
> in a device path under the current UEFI implementation on U-Boot.
You'll need to take into account any PCI-PCI bridges, so you need to
do this in a loop, walking up the tree until you encounter the PCI
host bridge / root complex ("PciRoot").
The stadard also is of the opinion that PciRoot needs to be an ACPI
device path entry, which obviously doesn't make sense in a DT context.
Using VenHW makes more sense, but maybe UEFI needs to grow support for
proper DT device path entries...
> > 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.
>
> So do you agree that this is a right approach even if it will break
> some sort of compatibility (of device path representation) with the current
> U-Boot UEFI implementation?
This will break any Boot#### variables that have been set by users
isn't it? I guess there is no way to avoid that, but it probably is a
good idea to only break it once.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [BUG] efi_loader: incorrect creation of device paths
2021-12-06 11:41 ` Mark Kettenis
@ 2021-12-07 1:33 ` AKASHI Takahiro
0 siblings, 0 replies; 9+ messages in thread
From: AKASHI Takahiro @ 2021-12-07 1:33 UTC (permalink / raw)
To: Mark Kettenis; +Cc: xypron.glpk, sjg, u-boot, ilias.apalodimas, agraf
Hi Mark,
Thank you for the comment.
On Mon, Dec 06, 2021 at 12:41:28PM +0100, Mark Kettenis wrote:
> > Date: Mon, 6 Dec 2021 13:16:23 +0900
> > From: AKASHI Takahiro <takahiro.akashi@linaro.org>
> >
> > On Fri, Dec 03, 2021 at 05:32:49PM +0100, Heinrich Schuchardt wrote:
> > > 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.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)
> > >
> > > 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.
> >
> > As the code suggests, the path was generated this way:
> >
> > /VenHw(...)/
> > USB(0xf3,0x0)/ <= usb_controller (xhci_pci)
> > USB(0x0,0x0)/ <= usb_hub
> > USB(0x3,0x0)/ <= usb_mass_storage
> > HD(1,GPT,...)
> >
> > USB(0xf3,0x0) was incorrectly encoded here, instead we will be able
> > to generate PCI(xx,yy) by examining
> > struct pci_child_plat *pplat = dev_get_parent_plat(dev);
> > xx = PCI_DEV(pplat->devfn), yy = PCI_FUNC(pplat->devfn)
> > if the dev's parent is a PCI (controller).
> >
> > But we will have to confirm that this is the only case that
> > we need to take care of the parent's bus.
> > Please note that any kind of pci component is not encoded
> > in a device path under the current UEFI implementation on U-Boot.
>
> You'll need to take into account any PCI-PCI bridges, so you need to
> do this in a loop, walking up the tree until you encounter the PCI
> host bridge / root complex ("PciRoot").
Yes, you're right, but the current U-Boot's DM framework doesn't
model the root complex. Instead, calling pci_get_controller() would
be enough. (Need confirm this.)
-> Simon?
> The stadard also is of the opinion that PciRoot needs to be an ACPI
> device path entry, which obviously doesn't make sense in a DT context.
> Using VenHW makes more sense, but maybe UEFI needs to grow support for
> proper DT device path entries...
Yeah, the current VenHW(e61d73b9-a384-4acc-aeab-82e828f3628b) represents
"efi_root" in our UEFI implementation and it is another incompatibility
with other UEFI firmware.
After all, we might want to insert a dummy PciRoot(0) (as EDK2 does?).
> > > 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.
> >
> > So do you agree that this is a right approach even if it will break
> > some sort of compatibility (of device path representation) with the current
> > U-Boot UEFI implementation?
>
> This will break any Boot#### variables that have been set by users
> isn't it? I guess there is no way to avoid that, but it probably is a
> good idea to only break it once.
Indeed.
-Takahiro Akashi
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [BUG] efi_loader: incorrect creation of device paths
2021-12-03 16:32 ` Heinrich Schuchardt
2021-12-06 4:16 ` AKASHI Takahiro
@ 2023-07-19 10:15 ` Heinrich Schuchardt
1 sibling, 0 replies; 9+ messages in thread
From: Heinrich Schuchardt @ 2023-07-19 10:15 UTC (permalink / raw)
To: AKASHI Takahiro; +Cc: U-Boot Mailing List, Ilias Apalodimas
On 03.12.21 17:32, Heinrich Schuchardt wrote:
> 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.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)
>
> 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.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:
The USB() node is for devices attached to a USB port of the parent device.
>> + 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;
The UEFI specification requires:
"Devices that do not have a serial number string must use the USB Device
Path (type 5) as described in USB Device Path Example"
but you don't set a serial number here.
Subtype 15 (= DEVICE_PATH_SUB_TYPE_MSG_USB_CLASS) translates to
UsbMassStorage(). This does not look correct for a USB hub.
Best regards
Heinrich
>> 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;
>> }
>>
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2023-07-19 10:16 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-11-20 12:54 [BUG] efi_loader: incorrect creation of device paths Heinrich Schuchardt
2021-11-24 3:10 ` AKASHI Takahiro
2021-11-25 5:44 ` AKASHI Takahiro
2021-12-03 7:12 ` AKASHI Takahiro
2021-12-03 16:32 ` Heinrich Schuchardt
2021-12-06 4:16 ` AKASHI Takahiro
2021-12-06 11:41 ` Mark Kettenis
2021-12-07 1:33 ` AKASHI Takahiro
2023-07-19 10:15 ` Heinrich Schuchardt
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox