Hi Heinrich, On Mon, 20 Mar 2023 at 09:58, Heinrich Schuchardt <heinrich.schucha...@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.schucha...@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.delau...@foss.st.com> > >> Signed-off-by: Heinrich Schuchardt <heinrich.schucha...@canonical.com> > >> --- > >> lib/efi_loader/efi_device_path.c | 45 +++++++++++++++++++++++--------- > >> 1 file changed, 33 insertions(+), 12 deletions(-) > > > > Reviewed-by: Simon Glass <s...@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 [..]