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.
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.
Best regards
Heinrich
and below too
return dp_size(dev->parent)+ sizeof(struct efi_device_path_controller);+#endif #ifdef CONFIG_VIRTIO_BLK case UCLASS_VIRTIO: /* @@ -585,7 +590,7 @@ __maybe_unused static unsigned int dp_size(struct udevice *dev) case UCLASS_MASS_STORAGE: case UCLASS_USB_HUB: return dp_size(dev->parent) +
sizeof(struct efi_device_path_usb_class);
sizeof(struct efi_device_path_usb); default: /* just skip over unknown classes: */ return dp_size(dev->parent);@@ -741,6 +746,19 @@ __maybe_unused static void *dp_fill(void *buf, struct udevice *dev) memcpy(&dp->ns_id, &ns_id, sizeof(ns_id)); return &dp[1]; } +#endif +#if defined(CONFIG_USB)
case UCLASS_MASS_STORAGE: {struct blk_desc *desc = desc = dev_get_uclass_plat(dev);struct efi_device_path_controller *dp =dp_fill(buf, dev->parent);dp->dp.type = DEVICE_PATH_TYPE_HARDWARE_DEVICE;dp->dp.sub_type = DEVICE_PATH_SUB_TYPE_CONTROLLER;dp->dp.length = sizeof(*dp);dp->controller_number = desc->lun;return &dp[1];}[..]
Regards, SImon