[PATCH 2/2] efi_loader: fix device-path for USB devices

[Heinrich Schuchardt]

On 3/27/23 06:00, Simon Glass wrote:
> Hi Heinrich,
> 
> On Wed, 22 Mar 2023 at 02:21, Heinrich Schuchardt
> <heinrich.schuchardt at canonical.com> wrote:
>>
>> On 3/20/23 19:39, Simon Glass wrote:
>>> Hi Heinrich,
>>>
>>> On Mon, 20 Mar 2023 at 09:58, Heinrich Schuchardt
>>> <heinrich.schuchardt at canonical.com> wrote:
>>>>
>>>>
>>>>
>>>> On 3/19/23 20:29, Simon Glass wrote:
>>>>> Hi Heinrich,
>>>>>
>>>>> On Mon, 20 Mar 2023 at 04:25, Heinrich Schuchardt
>>>>> <heinrich.schuchardt at canonical.com> wrote:
>>>>>>
>>>>>> EFI device paths for block devices must be unique. If a non-unique device
>>>>>> path is discovered, probing of the block device fails.
>>>>>>
>>>>>> Currently we use UsbClass() device path nodes. As multiple devices may
>>>>>> have the same vendor and product id these are non-unique. Instead we
>>>>>> should use Usb() device path nodes. They include the USB port on the
>>>>>> parent hub. Hence they are unique.
>>>>>>
>>>>>> A USB storage device may contain multiple logical units. These can be
>>>>>> modeled as Ctrl() nodes.
>>>>>>
>>>>>> Reported-by: Patrick Delaunay <patrick.delaunay at foss.st.com>
>>>>>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt at canonical.com>
>>>>>> ---
>>>>>>     lib/efi_loader/efi_device_path.c | 45 +++++++++++++++++++++++---------
>>>>>>     1 file changed, 33 insertions(+), 12 deletions(-)
>>>>>
>>>>> Reviewed-by: Simon Glass <sjg at chromium.org>
>>>>>
>>>>> [..]
>>>>>
>>>>>>
>>>>>> diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c
>>>>>> index 3b267b713e..b6dd575b13 100644
>>>>>> --- a/lib/efi_loader/efi_device_path.c
>>>>>> +++ b/lib/efi_loader/efi_device_path.c
>>>>>> @@ -147,7 +147,7 @@ struct efi_device_path *efi_dp_shorten(struct efi_device_path *dp)
>>>>>>                     * in practice fallback.efi just uses MEDIA:HARD_DRIVE
>>>>>>                     * so not sure when we would see these other cases.
>>>>>>                     */
>>>>>> -               if (EFI_DP_TYPE(dp, MESSAGING_DEVICE, MSG_USB_CLASS) ||
>>>>>> +               if (EFI_DP_TYPE(dp, MESSAGING_DEVICE, MSG_USB) ||
>>>>>>                        EFI_DP_TYPE(dp, MEDIA_DEVICE, HARD_DRIVE_PATH) ||
>>>>>>                        EFI_DP_TYPE(dp, MEDIA_DEVICE, FILE_PATH))
>>>>>>                            return dp;
>>>>>> @@ -564,6 +564,11 @@ __maybe_unused static unsigned int dp_size(struct udevice *dev)
>>>>>>                            return dp_size(dev->parent)
>>>>>>                                    + sizeof(struct efi_device_path_vendor) + 1;
>>>>>>     #endif
>>>>>> +#ifdef CONFIG_USB
>>>>>> +               case UCLASS_MASS_STORAGE:
>>>>>
>>>>> Can we do:
>>>>>
>>>>>                   case UCLASS_MASS_STORAGE:
>>>>>                      if (IS_ENABLED(CONFIG_USB)) {
>>>>>                               ...
>>>>>                      }
>>>>>
>>>>> ?
>>>>
>>>> That should be possible too. Didn't you want to get rid of IS_ENABLED()?
>>>> CONFIG_IS_ENABLED() would work here too.
>>>
>>> I was wanting to get rid of CONFIG_IS_ENABLED() for things that don't
>>> have an SPL Kconfig, then eventually get rid of CONFIG_IS_ENABLED().
>>> I've got a bit lost in all the problems though.
>>>
>>>>
>>>> The whole way that we create device paths is not consistent. We should
>>>> have a device path node for each DM device.
>>>>
>>>> With v2023.07 I want to add
>>>>
>>>>        struct efi_device_path *(*get_dp_node)(struct udevice *dev);
>>>>
>>>> to struct uclass_driver and move the generation of device path nodes to
>>>> the individual uclass drivers.
>>>
>>> We could also send an event (EVT_EFI_GET_DP_NODE) and connect it that
>>> way...ie would add less space to driver model. But yes it would be
>>> good to line up EFI and DM a bit better.
>>
>> The type of device-path node to be created is uclass specific:
>>
>> For an NVMe device we will always create a NVMe() node.
>> For a block device we will always create a Ctrl() node with the LUN number.
>> ...
>>
>> For drivers that don't implement the method yet we can create a VenHw()
>> node per default with uclass-id and device number encoded in the node.
>>
>> You suggested yourself that the DM and the EFI implementation should be
>> tightly integrated.
> 
> I mean that EFI should make full use of DM rather than maintaining
> parallel structures that need manual updating.
> 
>>
>> I cannot see what the use of an event should be. Why should each udevice
>> register to an event when all we need is a simple function like:
>>
>> #if CONFIG_IS_ENABLED(EFI_LOADER)
>> struct efi_device_path *uclass_get_dp_node(struct udevice *dev)
>> {
>>          struct uclass *uc;
>>          struct efi_device_path_uboot *dp;
>>
>>          uc = dev->uclass;
>>          if (uc->uc_drv->get_dp_node)
>>                  return uc->uc_drv->get_dp_node(dev);
>>
>>          dp = efi_alloc(sizeof(struct efi_device_path_uboot));
>>          if (!dp)
>>                  return NULL;
>>
>>          dp->dp.type = DEVICE_PATH_TYPE_HARDWARE_DEVICE;
>>          dp->dp.sub_type = DEVICE_PATH_SUB_TYPE_VENDOR;
>>          dp->dp.length = sizeof(struct efi_device_path_uboot);
>>          dp->guid = efi_u_boot_guid;
>>          dp->uclass_id = uc->uc_drv->id;
>>          dp->seq_ = dev->seq_;
>>
>>          return &dp->dp;
>> }
>> #endif
>>
> 
> I'll take a look at your series. Basically the idea with an event is
> that it can tell EFI when action is needed, without requiring #ifdefs
> and the like to build it into DM itself.

The trigger is not on the DM side but on the EFI side. EFI is asking DM 
for a device-path node for a device of a specific uclass.

So DM would have to register a listener per uclass if we use events.

Best regards

Heinrich