[PATCH 2/2] efi_loader: fix device-path for USB devices
Simon Glass
sjg at chromium.org
Mon Mar 27 10:24:11 CEST 2023
Hi Heinrich,
On Mon, 27 Mar 2023 at 18:30, Heinrich Schuchardt
<heinrich.schuchardt at canonical.com> wrote:
>
>
>
> 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.
Yes, I understand that.
>
> So DM would have to register a listener per uclass if we use events.
We could have a spy in each uclass, right?
Regards,
Simon
More information about the U-Boot
mailing list