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