Hi Heinrich,
On Mon, 27 Mar 2023 at 18:30, Heinrich Schuchardt heinrich.schuchardt@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@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@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@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@foss.st.com > Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com > --- > lib/efi_loader/efi_device_path.c | 45 +++++++++++++++++++++++--------- > 1 file changed, 33 insertions(+), 12 deletions(-)
Reviewed-by: Simon Glass sjg@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