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

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

[Heinrich Schuchardt]

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 and hence are unique.

A USB storage device may contain multiple logical units. These can be
modeled as Ctrl() nodes.

Given a driver model tree

    root       0 root_driver   root_driver
    usb        0 ehci_generic  |-- usb@1c1a000
    usb_hub    0 usb_hub          `-- usb_hub
    usb_hub    1 usb_hub              `-- usb_hub
    usb_mass_s 2 usb_mass_storage         `-- usb_mass_storage
    blk        4 usb_storage_blk              |-- usb_mass_storage.lun0

where the USB mass storage device is connected to port 4 of the 2nd hub,
the device path for LUN 0 will be:

    /VenHw()/USB(0x0,0x0)/USB(0x1,0x0)/USB(0x4,0x0)/Ctrl(0x0)

Heinrich Schuchardt (2):
  efi_loader: support for Ctrl() device path node
  efi_loader: fix device-path for USB devices

 include/efi_api.h                        |  6 ++++
 lib/efi_loader/efi_device_path.c         | 45 +++++++++++++++++-------
 lib/efi_loader/efi_device_path_to_text.c |  7 ++++
 3 files changed, 46 insertions(+), 12 deletions(-)

-- 
2.39.2

[PATCH 1/2] efi_loader: support for Ctrl() device path node

[Heinrich Schuchardt]

* Add the definitions for Ctrl() device path nodes.
* Implement Ctrl() nodes in the device path to text protocol.

Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
 include/efi_api.h                        | 6 ++++++
 lib/efi_loader/efi_device_path_to_text.c | 7 +++++++
 2 files changed, 13 insertions(+)

diff --git a/include/efi_api.h b/include/efi_api.h
index 2d18d25a71..c57868abbd 100644
--- a/include/efi_api.h
+++ b/include/efi_api.h
@@ -570,6 +570,7 @@ struct efi_mac_addr {
 #define DEVICE_PATH_TYPE_HARDWARE_DEVICE	0x01
 #  define DEVICE_PATH_SUB_TYPE_MEMORY		0x03
 #  define DEVICE_PATH_SUB_TYPE_VENDOR		0x04
+#  define DEVICE_PATH_SUB_TYPE_CONTROLLER	0x05
 
 struct efi_device_path_memory {
 	struct efi_device_path dp;
@@ -584,6 +585,11 @@ struct efi_device_path_vendor {
 	u8 vendor_data[];
 } __packed;
 
+struct efi_device_path_controller {
+	struct efi_device_path dp;
+	u32 controller_number;
+} __packed;
+
 #define DEVICE_PATH_TYPE_ACPI_DEVICE		0x02
 #  define DEVICE_PATH_SUB_TYPE_ACPI_DEVICE	0x01
 
diff --git a/lib/efi_loader/efi_device_path_to_text.c b/lib/efi_loader/efi_device_path_to_text.c
index 9062058ac2..9c0b39311a 100644
--- a/lib/efi_loader/efi_device_path_to_text.c
+++ b/lib/efi_loader/efi_device_path_to_text.c
@@ -77,6 +77,13 @@ static char *dp_hardware(char *s, struct efi_device_path *dp)
 		s += sprintf(s, ")");
 		break;
 	}
+	case DEVICE_PATH_SUB_TYPE_CONTROLLER: {
+		struct efi_device_path_controller *cdp =
+			(struct efi_device_path_controller *)dp;
+
+		s += sprintf(s, "Ctrl(0x%0x)", cdp->controller_number);
+		break;
+	}
 	default:
 		s = dp_unknown(s, dp);
 		break;
-- 
2.39.2

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

[Heinrich Schuchardt]

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;
 
 		return &udp[1];
 	}
-- 
2.39.2

Re: [PATCH 1/2] efi_loader: support for Ctrl() device path node

[Simon Glass]

On Mon, 20 Mar 2023 at 04:18, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
> * Add the definitions for Ctrl() device path nodes.
> * Implement Ctrl() nodes in the device path to text protocol.
>
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
>  include/efi_api.h                        | 6 ++++++
>  lib/efi_loader/efi_device_path_to_text.c | 7 +++++++
>  2 files changed, 13 insertions(+)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

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

[Simon Glass]

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)) {
                           ...
                  }

?

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];
> +               }
>  #endif

[..]

Regards,
SImon

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

[Heinrich Schuchardt]

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];
>> +               }
>>   #endif
> 
> [..]
> 
> Regards,
> SImon

Re: [PATCH 1/2] efi_loader: support for Ctrl() device path node

[Ilias Apalodimas]

On Sun, Mar 19, 2023 at 04:18:08PM +0100, Heinrich Schuchardt wrote:
> * Add the definitions for Ctrl() device path nodes.
> * Implement Ctrl() nodes in the device path to text protocol.
>
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
>  include/efi_api.h                        | 6 ++++++
>  lib/efi_loader/efi_device_path_to_text.c | 7 +++++++
>  2 files changed, 13 insertions(+)
>
> diff --git a/include/efi_api.h b/include/efi_api.h
> index 2d18d25a71..c57868abbd 100644
> --- a/include/efi_api.h
> +++ b/include/efi_api.h
> @@ -570,6 +570,7 @@ struct efi_mac_addr {
>  #define DEVICE_PATH_TYPE_HARDWARE_DEVICE	0x01
>  #  define DEVICE_PATH_SUB_TYPE_MEMORY		0x03
>  #  define DEVICE_PATH_SUB_TYPE_VENDOR		0x04
> +#  define DEVICE_PATH_SUB_TYPE_CONTROLLER	0x05
>
>  struct efi_device_path_memory {
>  	struct efi_device_path dp;
> @@ -584,6 +585,11 @@ struct efi_device_path_vendor {
>  	u8 vendor_data[];
>  } __packed;
>
> +struct efi_device_path_controller {
> +	struct efi_device_path dp;
> +	u32 controller_number;
> +} __packed;
> +
>  #define DEVICE_PATH_TYPE_ACPI_DEVICE		0x02
>  #  define DEVICE_PATH_SUB_TYPE_ACPI_DEVICE	0x01
>
> diff --git a/lib/efi_loader/efi_device_path_to_text.c b/lib/efi_loader/efi_device_path_to_text.c
> index 9062058ac2..9c0b39311a 100644
> --- a/lib/efi_loader/efi_device_path_to_text.c
> +++ b/lib/efi_loader/efi_device_path_to_text.c
> @@ -77,6 +77,13 @@ static char *dp_hardware(char *s, struct efi_device_path *dp)
>  		s += sprintf(s, ")");
>  		break;
>  	}
> +	case DEVICE_PATH_SUB_TYPE_CONTROLLER: {
> +		struct efi_device_path_controller *cdp =
> +			(struct efi_device_path_controller *)dp;
> +
> +		s += sprintf(s, "Ctrl(0x%0x)", cdp->controller_number);
> +		break;
> +	}
>  	default:
>  		s = dp_unknown(s, dp);
>  		break;
> --
> 2.39.2
>

Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

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

[Ilias Apalodimas]

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
>

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

[Simon Glass]

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.

Regards,
Simon
[..]

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

[Heinrich Schuchardt]

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 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

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

[Simon Glass]

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.

Regards,
Simon

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

[Heinrich Schuchardt]

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.

So DM would have to register a listener per uclass if we use events.

Best regards

Heinrich

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

[Simon Glass]

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