Re: [PATCH 2/2] efi_loader: fix device-path for USB devices

[Ilias Apalodimas <ilias.apalodimas@linaro.org>]

On Sun, Mar 19, 2023 at 04:18:09PM +0100, Heinrich Schuchardt 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(-)
>
> 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:
> +			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];
> +		}
>  #endif
>  		default:
>  			debug("%s(%u) %s: unhandled parent class: %s (%u)\n",
> @@ -767,19 +785,22 @@ __maybe_unused static void *dp_fill(void *buf, struct udevice *dev)
>  #endif
>  	case UCLASS_MASS_STORAGE:
>  	case UCLASS_USB_HUB: {
> -		struct efi_device_path_usb_class *udp =
> -			dp_fill(buf, dev->parent);
> -		struct usb_device *udev = dev_get_parent_priv(dev);
> -		struct usb_device_descriptor *desc = &udev->descriptor;
> +		struct efi_device_path_usb *udp = dp_fill(buf, dev->parent);
> +
> +		switch (device_get_uclass_id(dev->parent)) {
> +		case UCLASS_USB_HUB: {
> +			struct usb_device *udev = dev_get_parent_priv(dev);
>
> +			udp->parent_port_number = udev->portnr;
> +			break;
> +		}
> +		default:
> +			udp->parent_port_number = 0;
> +		}
>  		udp->dp.type     = DEVICE_PATH_TYPE_MESSAGING_DEVICE;
> -		udp->dp.sub_type = DEVICE_PATH_SUB_TYPE_MSG_USB_CLASS;
> +		udp->dp.sub_type = DEVICE_PATH_SUB_TYPE_MSG_USB;
>  		udp->dp.length   = sizeof(*udp);
> -		udp->vendor_id   = desc->idVendor;
> -		udp->product_id  = desc->idProduct;
> -		udp->device_class    = desc->bDeviceClass;
> -		udp->device_subclass = desc->bDeviceSubClass;
> -		udp->device_protocol = desc->bDeviceProtocol;
> +		udp->usb_interface = 0;

Why is this 0?  Can't we derive something reasonable from the uclass?

Thanks
/Ilias
>
>  		return &udp[1];
>  	}
> --
> 2.39.2
>