Hi Heinrich, On Mon, 27 Mar 2023 at 18:30, Heinrich Schuchardt <heinrich.schucha...@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.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. > > 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