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

[Simon Glass]

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.

Regards,
Simon
[..]