[BUG] efi_loader: incorrect creation of device paths

Heinrich Schuchardt xypron.glpk at gmx.de
Wed Jul 19 12:15:58 CEST 2023


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 at 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 at 1: scanning bus usb at 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 at 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 at 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 at 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;
>>       }
>>
> 
> 



More information about the U-Boot mailing list