Hi Heinrich, On Wed, 22 Mar 2023 at 02:21, Heinrich Schuchardt <heinrich.schucha...@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.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. > > 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. Regards, Simon