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 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
Best regards
Heinrich
