[RFC 0/7] efi_loader: move device-path node generation to DM

[RFC 0/7] efi_loader: move device-path node generation to DM

[Heinrich Schuchardt]

UEFI device-paths are used in the EFI world to depict the parent-child
relationship between devices. No two device can have the same device-path.
Currently we fail to generate unique devices-paths.

The nodes in UEFI device paths should match the devices on the path
to the DM root node.

I have started drafting a solution. Several block device classes still need
to be handled (IDE, NVMe, EFI, VirtIO). Also handling of PCI addresses is
missing. But this series contains enough to see the direction of
development.

Heinrich Schuchardt (7):
  dm: add get_dp_node() to struct uclass_driver
  dm: implement uclass_get_dp_node()
  dm: implement get_dp_node for block devices
  dm: implement get_dp_node for USB hub devices
  dm: implement get_dp_node for USB mass storage devices
  dm: implement get_dp_node for MMC devices
  efi_loader: use uclass_get_dp_node

 common/usb_hub.c                 |  33 ++++
 common/usb_storage.c             |  33 ++++
 drivers/block/blk-uclass.c       |  56 ++++++
 drivers/core/uclass.c            |  26 +++
 drivers/mmc/mmc-uclass.c         |  25 +++
 include/dm/uclass.h              |  13 ++
 include/efi_loader.h             |   7 +
 lib/efi_loader/efi_device_path.c | 325 +++----------------------------
 8 files changed, 221 insertions(+), 297 deletions(-)

-- 
2.39.2

[RFC 1/7] dm: add get_dp_node() to struct uclass_driver

[Heinrich Schuchardt]

Currently the device paths don't match the dm tree.
We should create a device path node per dm tree node.

Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
 include/dm/uclass.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/dm/uclass.h b/include/dm/uclass.h
index ee15c92063..e11637ce4d 100644
--- a/include/dm/uclass.h
+++ b/include/dm/uclass.h
@@ -11,6 +11,7 @@
 
 #include <dm/ofnode.h>
 #include <dm/uclass-id.h>
+#include <efi.h>
 #include <linker_lists.h>
 #include <linux/list.h>
 
@@ -68,6 +69,7 @@ struct udevice;
  * @child_post_probe: Called after a child in this uclass is probed
  * @init: Called to set up the uclass
  * @destroy: Called to destroy the uclass
+ * @get_dp_node: Get EFI device path node
  * @priv_auto: If non-zero this is the size of the private data
  * to be allocated in the uclass's ->priv pointer. If zero, then the uclass
  * driver is responsible for allocating any data required.
@@ -99,6 +101,9 @@ struct uclass_driver {
 	int (*child_post_probe)(struct udevice *dev);
 	int (*init)(struct uclass *class);
 	int (*destroy)(struct uclass *class);
+#if CONFIG_IS_ENABLED(EFI_LOADER)
+	struct efi_device_path *(*get_dp_node)(struct udevice *dev);
+#endif
 	int priv_auto;
 	int per_device_auto;
 	int per_device_plat_auto;
-- 
2.39.2

Re: [RFC 1/7] dm: add get_dp_node() to struct uclass_driver

[Simon Glass]

Hi Heinrich,

On Mon, 27 Mar 2023 at 06:27, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
> Currently the device paths don't match the dm tree.
> We should create a device path node per dm tree node.
>
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
>  include/dm/uclass.h | 5 +++++
>  1 file changed, 5 insertions(+)

This affects very few uclasses but adds a field to all of them. I
think an event might be best for this. We can add an event spy in each
of the affected uclasses.


>
> diff --git a/include/dm/uclass.h b/include/dm/uclass.h
> index ee15c92063..e11637ce4d 100644
> --- a/include/dm/uclass.h
> +++ b/include/dm/uclass.h
> @@ -11,6 +11,7 @@
>
>  #include <dm/ofnode.h>
>  #include <dm/uclass-id.h>
> +#include <efi.h>
>  #include <linker_lists.h>
>  #include <linux/list.h>
>
> @@ -68,6 +69,7 @@ struct udevice;
>   * @child_post_probe: Called after a child in this uclass is probed
>   * @init: Called to set up the uclass
>   * @destroy: Called to destroy the uclass
> + * @get_dp_node: Get EFI device path node
>   * @priv_auto: If non-zero this is the size of the private data
>   * to be allocated in the uclass's ->priv pointer. If zero, then the uclass
>   * driver is responsible for allocating any data required.
> @@ -99,6 +101,9 @@ struct uclass_driver {
>         int (*child_post_probe)(struct udevice *dev);
>         int (*init)(struct uclass *class);
>         int (*destroy)(struct uclass *class);
> +#if CONFIG_IS_ENABLED(EFI_LOADER)
> +       struct efi_device_path *(*get_dp_node)(struct udevice *dev);
> +#endif
>         int priv_auto;
>         int per_device_auto;
>         int per_device_plat_auto;
> --
> 2.39.2
>

Re: [RFC 1/7] dm: add get_dp_node() to struct uclass_driver

[Ilias Apalodimas]

Hi Simon,


On Mon, 27 Mar 2023 at 07:01, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Heinrich,
>
> On Mon, 27 Mar 2023 at 06:27, Heinrich Schuchardt
> <heinrich.schuchardt@canonical.com> wrote:
> >
> > Currently the device paths don't match the dm tree.
> > We should create a device path node per dm tree node.
> >
> > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> > ---
> >  include/dm/uclass.h | 5 +++++
> >  1 file changed, 5 insertions(+)
>
> This affects very few uclasses but adds a field to all of them. I
> think an event might be best for this. We can add an event spy in each
> of the affected uclasses.

This is supposed to be a call from the efi subsystem do get a DP for a
given device.  I am not sure how we could replace this with an event.

Regards
/Ilias
>
>
> >
> > diff --git a/include/dm/uclass.h b/include/dm/uclass.h
> > index ee15c92063..e11637ce4d 100644
> > --- a/include/dm/uclass.h
> > +++ b/include/dm/uclass.h
> > @@ -11,6 +11,7 @@
> >
> >  #include <dm/ofnode.h>
> >  #include <dm/uclass-id.h>
> > +#include <efi.h>
> >  #include <linker_lists.h>
> >  #include <linux/list.h>
> >
> > @@ -68,6 +69,7 @@ struct udevice;
> >   * @child_post_probe: Called after a child in this uclass is probed
> >   * @init: Called to set up the uclass
> >   * @destroy: Called to destroy the uclass
> > + * @get_dp_node: Get EFI device path node
> >   * @priv_auto: If non-zero this is the size of the private data
> >   * to be allocated in the uclass's ->priv pointer. If zero, then the uclass
> >   * driver is responsible for allocating any data required.
> > @@ -99,6 +101,9 @@ struct uclass_driver {
> >         int (*child_post_probe)(struct udevice *dev);
> >         int (*init)(struct uclass *class);
> >         int (*destroy)(struct uclass *class);
> > +#if CONFIG_IS_ENABLED(EFI_LOADER)
> > +       struct efi_device_path *(*get_dp_node)(struct udevice *dev);
> > +#endif
> >         int priv_auto;
> >         int per_device_auto;
> >         int per_device_plat_auto;
> > --
> > 2.39.2
> >

Re: [RFC 1/7] dm: add get_dp_node() to struct uclass_driver

[Heinrich Schuchardt]

On 3/27/23 06:00, Simon Glass wrote:
> Hi Heinrich,
> 
> On Mon, 27 Mar 2023 at 06:27, Heinrich Schuchardt
> <heinrich.schuchardt@canonical.com> wrote:
>>
>> Currently the device paths don't match the dm tree.
>> We should create a device path node per dm tree node.
>>
>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>> ---
>>   include/dm/uclass.h | 5 +++++
>>   1 file changed, 5 insertions(+)
> 
> This affects very few uclasses but adds a field to all of them. I
> think an event might be best for this. We can add an event spy in each
> of the affected uclasses.

EVENT_SPY does not allow to return information to the emitter of the event.

event_notfify() does not allow to select a single recipient.

In struct uclass_driver there are many other fields that are rarely used:

         int (*post_bind)(struct udevice *dev);
         int (*pre_unbind)(struct udevice *dev);
         int (*pre_probe)(struct udevice *dev);
         int (*post_probe)(struct udevice *dev);
         int (*pre_remove)(struct udevice *dev);
         int (*child_post_bind)(struct udevice *dev);
         int (*child_pre_probe)(struct udevice *dev);
         int (*child_post_probe)(struct udevice *dev);

If we wanted to save memory in linker generated lists we would convert 
structures with pointers to variable size arrays e.g.

struct ops * =
{
{ POST_BIND, post_bind },
{ CHILD_POST_BIND, child_post_bind},
{ END, NULL },
}

Best regards

Heinrich

> 
> 
>>
>> diff --git a/include/dm/uclass.h b/include/dm/uclass.h
>> index ee15c92063..e11637ce4d 100644
>> --- a/include/dm/uclass.h
>> +++ b/include/dm/uclass.h
>> @@ -11,6 +11,7 @@
>>
>>   #include <dm/ofnode.h>
>>   #include <dm/uclass-id.h>
>> +#include <efi.h>
>>   #include <linker_lists.h>
>>   #include <linux/list.h>
>>
>> @@ -68,6 +69,7 @@ struct udevice;
>>    * @child_post_probe: Called after a child in this uclass is probed
>>    * @init: Called to set up the uclass
>>    * @destroy: Called to destroy the uclass
>> + * @get_dp_node: Get EFI device path node
>>    * @priv_auto: If non-zero this is the size of the private data
>>    * to be allocated in the uclass's ->priv pointer. If zero, then the uclass
>>    * driver is responsible for allocating any data required.
>> @@ -99,6 +101,9 @@ struct uclass_driver {
>>          int (*child_post_probe)(struct udevice *dev);
>>          int (*init)(struct uclass *class);
>>          int (*destroy)(struct uclass *class);
>> +#if CONFIG_IS_ENABLED(EFI_LOADER)
>> +       struct efi_device_path *(*get_dp_node)(struct udevice *dev);
>> +#endif
>>          int priv_auto;
>>          int per_device_auto;
>>          int per_device_plat_auto;
>> --
>> 2.39.2
>>

Re: [RFC 1/7] dm: add get_dp_node() to struct uclass_driver

[Simon Glass]

Hi Heinrich,

On Mon, 27 Mar 2023 at 19:48, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
>
>
> On 3/27/23 06:00, Simon Glass wrote:
> > Hi Heinrich,
> >
> > On Mon, 27 Mar 2023 at 06:27, Heinrich Schuchardt
> > <heinrich.schuchardt@canonical.com> wrote:
> >>
> >> Currently the device paths don't match the dm tree.
> >> We should create a device path node per dm tree node.
> >>
> >> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> >> ---
> >>   include/dm/uclass.h | 5 +++++
> >>   1 file changed, 5 insertions(+)
> >
> > This affects very few uclasses but adds a field to all of them. I
> > think an event might be best for this. We can add an event spy in each
> > of the affected uclasses.
>
> EVENT_SPY does not allow to return information to the emitter of the event.

You just add it into unio event_data

>
> event_notfify() does not allow to select a single recipient.

Do you need that? You can check whether the uclass matches, for example.

>
> In struct uclass_driver there are many other fields that are rarely used:
>
>          int (*post_bind)(struct udevice *dev);
>          int (*pre_unbind)(struct udevice *dev);
>          int (*pre_probe)(struct udevice *dev);
>          int (*post_probe)(struct udevice *dev);
>          int (*pre_remove)(struct udevice *dev);
>          int (*child_post_bind)(struct udevice *dev);
>          int (*child_pre_probe)(struct udevice *dev);
>          int (*child_post_probe)(struct udevice *dev);
>
> If we wanted to save memory in linker generated lists we would convert
> structures with pointers to variable size arrays e.g.
>
> struct ops * =
> {
> { POST_BIND, post_bind },
> { CHILD_POST_BIND, child_post_bind},
> { END, NULL },
> }
>

Yes. We have the same problem with devices. We have a way to resolve
this today, using tags. We use DM_TAG_EFI and there are tags for the
devices but support for using them to reduce the struct sizes is not
yet added.

We might have a problem with efficiency, since direct pointers are
faster, but of course we can address that using a suitable data
structure.

Regards,
Simon

Re: [RFC 1/7] dm: add get_dp_node() to struct uclass_driver

[Heinrich Schuchardt]

On 3/27/23 10:24, Simon Glass wrote:
> Hi Heinrich,
> 
> On Mon, 27 Mar 2023 at 19:48, Heinrich Schuchardt
> <heinrich.schuchardt@canonical.com> wrote:
>>
>>
>>
>> On 3/27/23 06:00, Simon Glass wrote:
>>> Hi Heinrich,
>>>
>>> On Mon, 27 Mar 2023 at 06:27, Heinrich Schuchardt
>>> <heinrich.schuchardt@canonical.com> wrote:
>>>>
>>>> Currently the device paths don't match the dm tree.
>>>> We should create a device path node per dm tree node.
>>>>
>>>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>>>> ---
>>>>    include/dm/uclass.h | 5 +++++
>>>>    1 file changed, 5 insertions(+)
>>>
>>> This affects very few uclasses but adds a field to all of them. I
>>> think an event might be best for this. We can add an event spy in each
>>> of the affected uclasses.
>>
>> EVENT_SPY does not allow to return information to the emitter of the event.
> 
> You just add it into unio event_data

If you have multiple listeners for a single event, you can't be sure who 
added what.

> 
>>
>> event_notfify() does not allow to select a single recipient.
> 
> Do you need that? You can check whether the uclass matches, for example.

Checking the uclass multiple times is not efficient.

We are talking about 1 KiB for the pointers vs repetitive coding. We 
should go for the simplest code and most efficient code.

If you want to save memory, (temporarily) remove unused methods in 
struct uclass_driver: pre_unbind, post_bind, post_remove.

Best regards

Heinrich

> 
>>
>> In struct uclass_driver there are many other fields that are rarely used:
>>
>>           int (*post_bind)(struct udevice *dev);
>>           int (*pre_unbind)(struct udevice *dev);
>>           int (*pre_probe)(struct udevice *dev);
>>           int (*post_probe)(struct udevice *dev);
>>           int (*pre_remove)(struct udevice *dev);
>>           int (*child_post_bind)(struct udevice *dev);
>>           int (*child_pre_probe)(struct udevice *dev);
>>           int (*child_post_probe)(struct udevice *dev);
>>
>> If we wanted to save memory in linker generated lists we would convert
>> structures with pointers to variable size arrays e.g.
>>
>> struct ops * =
>> {
>> { POST_BIND, post_bind },
>> { CHILD_POST_BIND, child_post_bind},
>> { END, NULL },
>> }
>>
> 
> Yes. We have the same problem with devices. We have a way to resolve
> this today, using tags. We use DM_TAG_EFI and there are tags for the
> devices but support for using them to reduce the struct sizes is not
> yet added.
> 
> We might have a problem with efficiency, since direct pointers are
> faster, but of course we can address that using a suitable data
> structure.
> 
> Regards,
> Simon

Re: [RFC 1/7] dm: add get_dp_node() to struct uclass_driver

[Simon Glass]

Hi Heinrich,

On Mon, 27 Mar 2023 at 22:41, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
> On 3/27/23 10:24, Simon Glass wrote:
> > Hi Heinrich,
> >
> > On Mon, 27 Mar 2023 at 19:48, Heinrich Schuchardt
> > <heinrich.schuchardt@canonical.com> wrote:
> >>
> >>
> >>
> >> On 3/27/23 06:00, Simon Glass wrote:
> >>> Hi Heinrich,
> >>>
> >>> On Mon, 27 Mar 2023 at 06:27, Heinrich Schuchardt
> >>> <heinrich.schuchardt@canonical.com> wrote:
> >>>>
> >>>> Currently the device paths don't match the dm tree.
> >>>> We should create a device path node per dm tree node.
> >>>>
> >>>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> >>>> ---
> >>>>    include/dm/uclass.h | 5 +++++
> >>>>    1 file changed, 5 insertions(+)
> >>>
> >>> This affects very few uclasses but adds a field to all of them. I
> >>> think an event might be best for this. We can add an event spy in each
> >>> of the affected uclasses.
> >>
> >> EVENT_SPY does not allow to return information to the emitter of the event.
> >
> > You just add it into unio event_data
>
> If you have multiple listeners for a single event, you can't be sure who
> added what.

It is possible for a spy to claim an event and put its info in it.

>
> >
> >>
> >> event_notfify() does not allow to select a single recipient.
> >
> > Do you need that? You can check whether the uclass matches, for example.
>
> Checking the uclass multiple times is not efficient.
>
> We are talking about 1 KiB for the pointers vs repetitive coding. We
> should go for the simplest code and most efficient code.

Firstly, this should be done in an API so that the actual
implementation can be adjusted in future. Let's continue the
discussion on the other patch.

>
> If you want to save memory, (temporarily) remove unused methods in
> struct uclass_driver: pre_unbind, post_bind, post_remove.

That is a separate issue from adding more. I would like to do that,
but it would be easy enough for someone else to take this on, given
that the tag support is in there now.

[..]

Regards,
Simon

[RFC 2/7] dm: implement uclass_get_dp_node()

[Heinrich Schuchardt]

Provide a function to get the EFI device path node representing a
device.

If implemented, it invokes the uclass driver's get_dp_node() function.
Otherwise a vendor hardware node is created.

Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
 drivers/core/uclass.c | 26 ++++++++++++++++++++++++++
 include/dm/uclass.h   |  8 ++++++++
 include/efi_loader.h  |  7 +++++++
 3 files changed, 41 insertions(+)

diff --git a/drivers/core/uclass.c b/drivers/core/uclass.c
index 1762a0796d..153c954ee3 100644
--- a/drivers/core/uclass.c
+++ b/drivers/core/uclass.c
@@ -10,6 +10,7 @@
 
 #include <common.h>
 #include <dm.h>
+#include <efi_loader.h>
 #include <errno.h>
 #include <log.h>
 #include <malloc.h>
@@ -802,6 +803,31 @@ int uclass_pre_remove_device(struct udevice *dev)
 }
 #endif
 
+#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
+
 int uclass_probe_all(enum uclass_id id)
 {
 	struct udevice *dev;
diff --git a/include/dm/uclass.h b/include/dm/uclass.h
index e11637ce4d..f39dbac21d 100644
--- a/include/dm/uclass.h
+++ b/include/dm/uclass.h
@@ -503,3 +503,11 @@ int uclass_id_count(enum uclass_id id);
 	     uclass_next_device(&dev))
 
 #endif
+
+/**
+ * uclass_get_dp_node() - get EFI device path node for device
+ *
+ * @dev:	device
+ * Return:	device path node or NULL if out of memory
+ */
+struct efi_device_path *uclass_get_dp_node(struct udevice *dev);
diff --git a/include/efi_loader.h b/include/efi_loader.h
index cee04cbb9d..f111bc616d 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -22,6 +22,13 @@
 struct blk_desc;
 struct jmp_buf_data;
 
+struct efi_device_path_uboot {
+	struct efi_device_path dp;
+	efi_guid_t guid;
+	enum uclass_id uclass_id;
+	int seq_;
+} __packed;
+
 static inline int guidcmp(const void *g1, const void *g2)
 {
 	return memcmp(g1, g2, sizeof(efi_guid_t));
-- 
2.39.2

Re: [RFC 2/7] dm: implement uclass_get_dp_node()

[Simon Glass]

Hi Heinrich,

On Mon, 27 Mar 2023 at 06:27, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
> Provide a function to get the EFI device path node representing a
> device.
>
> If implemented, it invokes the uclass driver's get_dp_node() function.
> Otherwise a vendor hardware node is created.
>
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
>  drivers/core/uclass.c | 26 ++++++++++++++++++++++++++
>  include/dm/uclass.h   |  8 ++++++++
>  include/efi_loader.h  |  7 +++++++
>  3 files changed, 41 insertions(+)

Can you add a new file, like drivers/core/efi.c for this stuff?

You should also pull out the header stuff you need and put it in
include/dm/efi.h since we don't want to include efi_loader.h from
driver/core.

>
> diff --git a/drivers/core/uclass.c b/drivers/core/uclass.c
> index 1762a0796d..153c954ee3 100644
> --- a/drivers/core/uclass.c
> +++ b/drivers/core/uclass.c
> @@ -10,6 +10,7 @@
>
>  #include <common.h>
>  #include <dm.h>
> +#include <efi_loader.h>
>  #include <errno.h>
>  #include <log.h>
>  #include <malloc.h>
> @@ -802,6 +803,31 @@ int uclass_pre_remove_device(struct udevice *dev)
>  }
>  #endif
>
> +#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));

This should call malloc(), right?

How does this memory get freed? How do we avoid allocating it twice if
it is called twice?

> +       if (!dp)
> +               return NULL;

-ENOMEM ? I suggest changing the function return value to an int.

> +
> +       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
> +
>  int uclass_probe_all(enum uclass_id id)
>  {
>         struct udevice *dev;
> diff --git a/include/dm/uclass.h b/include/dm/uclass.h
> index e11637ce4d..f39dbac21d 100644
> --- a/include/dm/uclass.h
> +++ b/include/dm/uclass.h
> @@ -503,3 +503,11 @@ int uclass_id_count(enum uclass_id id);
>              uclass_next_device(&dev))
>
>  #endif
> +
> +/**
> + * uclass_get_dp_node() - get EFI device path node for device
> + *
> + * @dev:       device
> + * Return:     device path node or NULL if out of memory
> + */
> +struct efi_device_path *uclass_get_dp_node(struct udevice *dev);
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index cee04cbb9d..f111bc616d 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -22,6 +22,13 @@
>  struct blk_desc;
>  struct jmp_buf_data;
>
> +struct efi_device_path_uboot {
> +       struct efi_device_path dp;
> +       efi_guid_t guid;
> +       enum uclass_id uclass_id;
> +       int seq_;

What is this member for? Can you please comment this struct?

> +} __packed;
> +
>  static inline int guidcmp(const void *g1, const void *g2)
>  {
>         return memcmp(g1, g2, sizeof(efi_guid_t));
> --
> 2.39.2
>

Regards,
Simon

[RFC 3/7] dm: implement get_dp_node for block devices

[Heinrich Schuchardt]

Generate a Ctrl() node for block devices.

Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
 drivers/block/blk-uclass.c | 56 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 56 insertions(+)

diff --git a/drivers/block/blk-uclass.c b/drivers/block/blk-uclass.c
index c69fc4d518..08202aaaba 100644
--- a/drivers/block/blk-uclass.c
+++ b/drivers/block/blk-uclass.c
@@ -9,6 +9,7 @@
 #include <common.h>
 #include <blk.h>
 #include <dm.h>
+#include <efi_loader.h>
 #include <log.h>
 #include <malloc.h>
 #include <part.h>
@@ -779,9 +780,64 @@ static int blk_post_probe(struct udevice *dev)
 	return 0;
 }
 
+#if CONFIG_IS_ENABLED(EFI_LOADER)
+__maybe_unused
+static struct efi_device_path *blk_scsi_get_dp_node(struct udevice *dev)
+{
+	struct efi_device_path_scsi *dp;
+	struct blk_desc *desc = dev_get_uclass_plat(dev);
+
+	dp = efi_alloc(sizeof(struct efi_device_path_scsi));
+	if (!dp)
+		return NULL;
+
+	dp->dp.type = DEVICE_PATH_TYPE_MESSAGING_DEVICE;
+	dp->dp.sub_type = DEVICE_PATH_SUB_TYPE_MSG_SCSI;
+	dp->dp.length = sizeof(*dp);
+	dp->logical_unit_number = desc->lun;
+	dp->target_id = desc->target;
+
+	return &dp->dp;
+}
+
+static struct efi_device_path *blk_get_dp_node(struct udevice *dev)
+{
+	struct efi_device_path_controller *dp;
+	struct blk_desc *desc = dev_get_uclass_plat(dev);
+	u32 controller_number;
+
+	switch (device_get_uclass_id(dev->parent)) {
+#if CONFIG_IS_ENABLED(SCSI)
+	case UCLASS_SCSI:
+		return blk_scsi_get_dp_node(dev);
+#endif
+	case UCLASS_MMC:
+		dp->controller_number = desc->hwpart;
+		break;
+	default:
+		dp->controller_number = desc->lun;
+		break;
+	}
+
+	dp = efi_alloc(sizeof(struct efi_device_path_controller));
+	if (!dp)
+		return NULL;
+
+	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 = controller_number;
+
+	return &dp->dp;
+}
+#endif
+
 UCLASS_DRIVER(blk) = {
 	.id		= UCLASS_BLK,
 	.name		= "blk",
 	.post_probe	= blk_post_probe,
 	.per_device_plat_auto	= sizeof(struct blk_desc),
+#if CONFIG_IS_ENABLED(EFI_LOADER)
+	.get_dp_node	= blk_get_dp_node,
+#endif
 };
-- 
2.39.2

Re: [RFC 3/7] dm: implement get_dp_node for block devices

[Simon Glass]

Hi Heinrich,

On Mon, 27 Mar 2023 at 06:27, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
> Generate a Ctrl() node for block devices.
>
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
>  drivers/block/blk-uclass.c | 56 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 56 insertions(+)

Can this go in drivers/block/blk_efi.c ?

Can you create a function which returns the path given a device? Then
we are discuss the get_dp_node() member or moving to an event. I
favour the event for now.

>
> diff --git a/drivers/block/blk-uclass.c b/drivers/block/blk-uclass.c
> index c69fc4d518..08202aaaba 100644
> --- a/drivers/block/blk-uclass.c
> +++ b/drivers/block/blk-uclass.c
> @@ -9,6 +9,7 @@
>  #include <common.h>
>  #include <blk.h>
>  #include <dm.h>
> +#include <efi_loader.h>
>  #include <log.h>
>  #include <malloc.h>
>  #include <part.h>
> @@ -779,9 +780,64 @@ static int blk_post_probe(struct udevice *dev)
>         return 0;
>  }
>
> +#if CONFIG_IS_ENABLED(EFI_LOADER)
> +__maybe_unused
> +static struct efi_device_path *blk_scsi_get_dp_node(struct udevice *dev)
> +{
> +       struct efi_device_path_scsi *dp;
> +       struct blk_desc *desc = dev_get_uclass_plat(dev);
> +
> +       dp = efi_alloc(sizeof(struct efi_device_path_scsi));
> +       if (!dp)
> +               return NULL;
> +
> +       dp->dp.type = DEVICE_PATH_TYPE_MESSAGING_DEVICE;
> +       dp->dp.sub_type = DEVICE_PATH_SUB_TYPE_MSG_SCSI;
> +       dp->dp.length = sizeof(*dp);
> +       dp->logical_unit_number = desc->lun;
> +       dp->target_id = desc->target;
> +
> +       return &dp->dp;
> +}
> +
> +static struct efi_device_path *blk_get_dp_node(struct udevice *dev)
> +{
> +       struct efi_device_path_controller *dp;
> +       struct blk_desc *desc = dev_get_uclass_plat(dev);
> +       u32 controller_number;
> +
> +       switch (device_get_uclass_id(dev->parent)) {
> +#if CONFIG_IS_ENABLED(SCSI)
> +       case UCLASS_SCSI:
> +               return blk_scsi_get_dp_node(dev);
> +#endif
> +       case UCLASS_MMC:
> +               dp->controller_number = desc->hwpart;
> +               break;
> +       default:

Since this is checking the parent class, it seems to me that this info
should be attacked there (the 'media' device) instead of the block
device.

> +               dp->controller_number = desc->lun;
> +               break;
> +       }
> +
> +       dp = efi_alloc(sizeof(struct efi_device_path_controller));
> +       if (!dp)
> +               return NULL;
> +
> +       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 = controller_number;
> +
> +       return &dp->dp;
> +}
> +#endif
> +
>  UCLASS_DRIVER(blk) = {
>         .id             = UCLASS_BLK,
>         .name           = "blk",
>         .post_probe     = blk_post_probe,
>         .per_device_plat_auto   = sizeof(struct blk_desc),
> +#if CONFIG_IS_ENABLED(EFI_LOADER)
> +       .get_dp_node    = blk_get_dp_node,
> +#endif
>  };
> --
> 2.39.2
>

Regards,
Simon

Re: [RFC 3/7] dm: implement get_dp_node for block devices

[Heinrich Schuchardt]

On 3/27/23 06:00, Simon Glass wrote:
> Hi Heinrich,
> 
> On Mon, 27 Mar 2023 at 06:27, Heinrich Schuchardt
> <heinrich.schuchardt@canonical.com> wrote:
>>
>> Generate a Ctrl() node for block devices.
>>
>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>> ---
>>   drivers/block/blk-uclass.c | 56 ++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 56 insertions(+)
> 
> Can this go in drivers/block/blk_efi.c ?
> 
> Can you create a function which returns the path given a device? Then

That function already exists in lib/efi_loader/device_path.

> we are discuss the get_dp_node() member or moving to an event. I
> favour the event for now.

If for a device we already have an EFI handle with a device-path 
installed, the device-path of its children has to be constructed by 
adding a uclass specific device path node.

Otherwise we have to recurse to the root node to collect all device path 
nodes.

> 
>>
>> diff --git a/drivers/block/blk-uclass.c b/drivers/block/blk-uclass.c
>> index c69fc4d518..08202aaaba 100644
>> --- a/drivers/block/blk-uclass.c
>> +++ b/drivers/block/blk-uclass.c
>> @@ -9,6 +9,7 @@
>>   #include <common.h>
>>   #include <blk.h>
>>   #include <dm.h>
>> +#include <efi_loader.h>
>>   #include <log.h>
>>   #include <malloc.h>
>>   #include <part.h>
>> @@ -779,9 +780,64 @@ static int blk_post_probe(struct udevice *dev)
>>          return 0;
>>   }
>>
>> +#if CONFIG_IS_ENABLED(EFI_LOADER)
>> +__maybe_unused
>> +static struct efi_device_path *blk_scsi_get_dp_node(struct udevice *dev)
>> +{
>> +       struct efi_device_path_scsi *dp;
>> +       struct blk_desc *desc = dev_get_uclass_plat(dev);
>> +
>> +       dp = efi_alloc(sizeof(struct efi_device_path_scsi));
>> +       if (!dp)
>> +               return NULL;
>> +
>> +       dp->dp.type = DEVICE_PATH_TYPE_MESSAGING_DEVICE;
>> +       dp->dp.sub_type = DEVICE_PATH_SUB_TYPE_MSG_SCSI;
>> +       dp->dp.length = sizeof(*dp);
>> +       dp->logical_unit_number = desc->lun;
>> +       dp->target_id = desc->target;
>> +
>> +       return &dp->dp;
>> +}
>> +
>> +static struct efi_device_path *blk_get_dp_node(struct udevice *dev)
>> +{
>> +       struct efi_device_path_controller *dp;
>> +       struct blk_desc *desc = dev_get_uclass_plat(dev);
>> +       u32 controller_number;
>> +
>> +       switch (device_get_uclass_id(dev->parent)) {
>> +#if CONFIG_IS_ENABLED(SCSI)
>> +       case UCLASS_SCSI:
>> +               return blk_scsi_get_dp_node(dev);
>> +#endif
>> +       case UCLASS_MMC:
>> +               dp->controller_number = desc->hwpart;
>> +               break;
>> +       default:
> 
> Since this is checking the parent class, it seems to me that this info
> should be attacked there (the 'media' device) instead of the block
> device.

For MMC we have these layers:

* MMC controller - UCLASS_MMC
* MMC slot (or channel) - missing uclass
* hardware partition - UCLASS_BLK

See https://www.sage-micro.com/us/portfolio-items/s881/

Currently in the DM tree we don't model the MMC controller and the MMC 
slot separately. I think this is a gap that should be closed.

A single UCLASS_MMC (= MMC slot) device can have multiple UCLASS_BLK 
children. One for each hardware partition.

An MMC controller in EFI is modelled as VenHW() node,
a slot as SD() or eMMC() node.

An eMMC() or SD() node has a field for the slot number but none for the 
hardware partition. Hence, we cannot attach the hardware partition 
information to the UCLASS_MMC device. For the UCLASS_BLK device I use a 
Ctrl() node. We could also consider using a Unit() node but Unit() nodes 
are meant to be used for LUNs.

We have a similar situation for USB and SCSI LUNs.

Best regards

Heinrich

> 
>> +               dp->controller_number = desc->lun;
>> +               break;
>> +       }
>> +
>> +       dp = efi_alloc(sizeof(struct efi_device_path_controller));
>> +       if (!dp)
>> +               return NULL;
>> +
>> +       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 = controller_number;
>> +
>> +       return &dp->dp;
>> +}
>> +#endif
>> +
>>   UCLASS_DRIVER(blk) = {
>>          .id             = UCLASS_BLK,
>>          .name           = "blk",
>>          .post_probe     = blk_post_probe,
>>          .per_device_plat_auto   = sizeof(struct blk_desc),
>> +#if CONFIG_IS_ENABLED(EFI_LOADER)
>> +       .get_dp_node    = blk_get_dp_node,
>> +#endif
>>   };
>> --
>> 2.39.2
>>
> 
> Regards,
> Simon

Re: [RFC 3/7] dm: implement get_dp_node for block devices

[Simon Glass]

Hi Heinrich,

On Mon, 27 Mar 2023 at 19:13, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
>
>
> On 3/27/23 06:00, Simon Glass wrote:
> > Hi Heinrich,
> >
> > On Mon, 27 Mar 2023 at 06:27, Heinrich Schuchardt
> > <heinrich.schuchardt@canonical.com> wrote:
> >>
> >> Generate a Ctrl() node for block devices.
> >>
> >> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> >> ---
> >>   drivers/block/blk-uclass.c | 56 ++++++++++++++++++++++++++++++++++++++
> >>   1 file changed, 56 insertions(+)
> >
> > Can this go in drivers/block/blk_efi.c ?
> >
> > Can you create a function which returns the path given a device? Then
>
> That function already exists in lib/efi_loader/device_path.

Yes, but I mean one for each media type.

>
> > we are discuss the get_dp_node() member or moving to an event. I
> > favour the event for now.
>
> If for a device we already have an EFI handle with a device-path
> installed, the device-path of its children has to be constructed by
> adding a uclass specific device path node.
>
> Otherwise we have to recurse to the root node to collect all device path
> nodes.

OK

>
> >
> >>
> >> diff --git a/drivers/block/blk-uclass.c b/drivers/block/blk-uclass.c
> >> index c69fc4d518..08202aaaba 100644
> >> --- a/drivers/block/blk-uclass.c
> >> +++ b/drivers/block/blk-uclass.c
> >> @@ -9,6 +9,7 @@
> >>   #include <common.h>
> >>   #include <blk.h>
> >>   #include <dm.h>
> >> +#include <efi_loader.h>
> >>   #include <log.h>
> >>   #include <malloc.h>
> >>   #include <part.h>
> >> @@ -779,9 +780,64 @@ static int blk_post_probe(struct udevice *dev)
> >>          return 0;
> >>   }
> >>
> >> +#if CONFIG_IS_ENABLED(EFI_LOADER)
> >> +__maybe_unused
> >> +static struct efi_device_path *blk_scsi_get_dp_node(struct udevice *dev)
> >> +{
> >> +       struct efi_device_path_scsi *dp;
> >> +       struct blk_desc *desc = dev_get_uclass_plat(dev);
> >> +
> >> +       dp = efi_alloc(sizeof(struct efi_device_path_scsi));
> >> +       if (!dp)
> >> +               return NULL;
> >> +
> >> +       dp->dp.type = DEVICE_PATH_TYPE_MESSAGING_DEVICE;
> >> +       dp->dp.sub_type = DEVICE_PATH_SUB_TYPE_MSG_SCSI;
> >> +       dp->dp.length = sizeof(*dp);
> >> +       dp->logical_unit_number = desc->lun;
> >> +       dp->target_id = desc->target;
> >> +
> >> +       return &dp->dp;
> >> +}
> >> +
> >> +static struct efi_device_path *blk_get_dp_node(struct udevice *dev)
> >> +{
> >> +       struct efi_device_path_controller *dp;
> >> +       struct blk_desc *desc = dev_get_uclass_plat(dev);
> >> +       u32 controller_number;
> >> +
> >> +       switch (device_get_uclass_id(dev->parent)) {
> >> +#if CONFIG_IS_ENABLED(SCSI)
> >> +       case UCLASS_SCSI:
> >> +               return blk_scsi_get_dp_node(dev);
> >> +#endif
> >> +       case UCLASS_MMC:
> >> +               dp->controller_number = desc->hwpart;
> >> +               break;
> >> +       default:
> >
> > Since this is checking the parent class, it seems to me that this info
> > should be attacked there (the 'media' device) instead of the block
> > device.
>
> For MMC we have these layers:
>
> * MMC controller - UCLASS_MMC
> * MMC slot (or channel) - missing uclass
> * hardware partition - UCLASS_BLK
>
> See https://www.sage-micro.com/us/portfolio-items/s881/
>
> Currently in the DM tree we don't model the MMC controller and the MMC
> slot separately. I think this is a gap that should be closed.
>
> A single UCLASS_MMC (= MMC slot) device can have multiple UCLASS_BLK
> children. One for each hardware partition.
>
> An MMC controller in EFI is modelled as VenHW() node,
> a slot as SD() or eMMC() node.
>
> An eMMC() or SD() node has a field for the slot number but none for the
> hardware partition. Hence, we cannot attach the hardware partition
> information to the UCLASS_MMC device. For the UCLASS_BLK device I use a
> Ctrl() node. We could also consider using a Unit() node but Unit() nodes
> are meant to be used for LUNs.
>
> We have a similar situation for USB and SCSI LUNs.

The current model is to add multiple BLK children for each media
device, if needed. That seems to work OK for now. What do you think?

Regards,
Simon

Re: [RFC 3/7] dm: implement get_dp_node for block devices

[Heinrich Schuchardt]

On 3/27/23 10:24, Simon Glass wrote:
> Hi Heinrich,
> 
> On Mon, 27 Mar 2023 at 19:13, Heinrich Schuchardt
> <heinrich.schuchardt@canonical.com> wrote:
>>
>>
>>
>> On 3/27/23 06:00, Simon Glass wrote:
>>> Hi Heinrich,
>>>
>>> On Mon, 27 Mar 2023 at 06:27, Heinrich Schuchardt
>>> <heinrich.schuchardt@canonical.com> wrote:
>>>>
>>>> Generate a Ctrl() node for block devices.
>>>>
>>>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>>>> ---
>>>>    drivers/block/blk-uclass.c | 56 ++++++++++++++++++++++++++++++++++++++
>>>>    1 file changed, 56 insertions(+)
>>>
>>> Can this go in drivers/block/blk_efi.c ?
>>>
>>> Can you create a function which returns the path given a device? Then
>>
>> That function already exists in lib/efi_loader/device_path.
> 
> Yes, but I mean one for each media type.
> 
>>
>>> we are discuss the get_dp_node() member or moving to an event. I
>>> favour the event for now.
>>
>> If for a device we already have an EFI handle with a device-path
>> installed, the device-path of its children has to be constructed by
>> adding a uclass specific device path node.
>>
>> Otherwise we have to recurse to the root node to collect all device path
>> nodes.
> 
> OK
> 
>>
>>>
>>>>
>>>> diff --git a/drivers/block/blk-uclass.c b/drivers/block/blk-uclass.c
>>>> index c69fc4d518..08202aaaba 100644
>>>> --- a/drivers/block/blk-uclass.c
>>>> +++ b/drivers/block/blk-uclass.c
>>>> @@ -9,6 +9,7 @@
>>>>    #include <common.h>
>>>>    #include <blk.h>
>>>>    #include <dm.h>
>>>> +#include <efi_loader.h>
>>>>    #include <log.h>
>>>>    #include <malloc.h>
>>>>    #include <part.h>
>>>> @@ -779,9 +780,64 @@ static int blk_post_probe(struct udevice *dev)
>>>>           return 0;
>>>>    }
>>>>
>>>> +#if CONFIG_IS_ENABLED(EFI_LOADER)
>>>> +__maybe_unused
>>>> +static struct efi_device_path *blk_scsi_get_dp_node(struct udevice *dev)
>>>> +{
>>>> +       struct efi_device_path_scsi *dp;
>>>> +       struct blk_desc *desc = dev_get_uclass_plat(dev);
>>>> +
>>>> +       dp = efi_alloc(sizeof(struct efi_device_path_scsi));
>>>> +       if (!dp)
>>>> +               return NULL;
>>>> +
>>>> +       dp->dp.type = DEVICE_PATH_TYPE_MESSAGING_DEVICE;
>>>> +       dp->dp.sub_type = DEVICE_PATH_SUB_TYPE_MSG_SCSI;
>>>> +       dp->dp.length = sizeof(*dp);
>>>> +       dp->logical_unit_number = desc->lun;
>>>> +       dp->target_id = desc->target;
>>>> +
>>>> +       return &dp->dp;
>>>> +}
>>>> +
>>>> +static struct efi_device_path *blk_get_dp_node(struct udevice *dev)
>>>> +{
>>>> +       struct efi_device_path_controller *dp;
>>>> +       struct blk_desc *desc = dev_get_uclass_plat(dev);
>>>> +       u32 controller_number;
>>>> +
>>>> +       switch (device_get_uclass_id(dev->parent)) {
>>>> +#if CONFIG_IS_ENABLED(SCSI)
>>>> +       case UCLASS_SCSI:
>>>> +               return blk_scsi_get_dp_node(dev);
>>>> +#endif
>>>> +       case UCLASS_MMC:
>>>> +               dp->controller_number = desc->hwpart;
>>>> +               break;
>>>> +       default:
>>>
>>> Since this is checking the parent class, it seems to me that this info
>>> should be attacked there (the 'media' device) instead of the block
>>> device.
>>
>> For MMC we have these layers:
>>
>> * MMC controller - UCLASS_MMC
>> * MMC slot (or channel) - missing uclass
>> * hardware partition - UCLASS_BLK
>>
>> See https://www.sage-micro.com/us/portfolio-items/s881/
>>
>> Currently in the DM tree we don't model the MMC controller and the MMC
>> slot separately. I think this is a gap that should be closed.
>>
>> A single UCLASS_MMC (= MMC slot) device can have multiple UCLASS_BLK
>> children. One for each hardware partition.
>>
>> An MMC controller in EFI is modelled as VenHW() node,
>> a slot as SD() or eMMC() node.
>>
>> An eMMC() or SD() node has a field for the slot number but none for the
>> hardware partition. Hence, we cannot attach the hardware partition
>> information to the UCLASS_MMC device. For the UCLASS_BLK device I use a
>> Ctrl() node. We could also consider using a Unit() node but Unit() nodes
>> are meant to be used for LUNs.
>>
>> We have a similar situation for USB and SCSI LUNs.
> 
> The current model is to add multiple BLK children for each media
> device, if needed. That seems to work OK for now. What do you think?

There is nothing in DM tree we need to change now. As I understood your 
first reply it was not clear to you which EFI device-path node type with 
which information matches which DM tree node:

MMC controller/MMC slot (UCLASS_MMC) -> SD(slot) or MMC(slot)
Hardware partition (UCLASS_BLK) -> Ctrl()

SCSI drive (UCLASS_SCSI) -> VenHw()
SCSI LUN (UClASS_BLK) -> Scsi(PUN, LUN)

Best regards

Heinrich

Re: [RFC 3/7] dm: implement get_dp_node for block devices

[Simon Glass]

Hi Heinrich,

On Mon, 27 Mar 2023 at 22:31, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
> On 3/27/23 10:24, Simon Glass wrote:
> > Hi Heinrich,
> >
> > On Mon, 27 Mar 2023 at 19:13, Heinrich Schuchardt
> > <heinrich.schuchardt@canonical.com> wrote:
> >>
> >>
> >>
> >> On 3/27/23 06:00, Simon Glass wrote:
> >>> Hi Heinrich,
> >>>
> >>> On Mon, 27 Mar 2023 at 06:27, Heinrich Schuchardt
> >>> <heinrich.schuchardt@canonical.com> wrote:
> >>>>
> >>>> Generate a Ctrl() node for block devices.
> >>>>
> >>>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> >>>> ---
> >>>>    drivers/block/blk-uclass.c | 56 ++++++++++++++++++++++++++++++++++++++
> >>>>    1 file changed, 56 insertions(+)
> >>>
> >>> Can this go in drivers/block/blk_efi.c ?
> >>>
> >>> Can you create a function which returns the path given a device? Then
> >>
> >> That function already exists in lib/efi_loader/device_path.
> >
> > Yes, but I mean one for each media type.
> >
> >>
> >>> we are discuss the get_dp_node() member or moving to an event. I
> >>> favour the event for now.
> >>
> >> If for a device we already have an EFI handle with a device-path
> >> installed, the device-path of its children has to be constructed by
> >> adding a uclass specific device path node.
> >>
> >> Otherwise we have to recurse to the root node to collect all device path
> >> nodes.
> >
> > OK
> >
> >>
> >>>
> >>>>
> >>>> diff --git a/drivers/block/blk-uclass.c b/drivers/block/blk-uclass.c
> >>>> index c69fc4d518..08202aaaba 100644
> >>>> --- a/drivers/block/blk-uclass.c
> >>>> +++ b/drivers/block/blk-uclass.c
> >>>> @@ -9,6 +9,7 @@
> >>>>    #include <common.h>
> >>>>    #include <blk.h>
> >>>>    #include <dm.h>
> >>>> +#include <efi_loader.h>
> >>>>    #include <log.h>
> >>>>    #include <malloc.h>
> >>>>    #include <part.h>
> >>>> @@ -779,9 +780,64 @@ static int blk_post_probe(struct udevice *dev)
> >>>>           return 0;
> >>>>    }
> >>>>
> >>>> +#if CONFIG_IS_ENABLED(EFI_LOADER)
> >>>> +__maybe_unused
> >>>> +static struct efi_device_path *blk_scsi_get_dp_node(struct udevice *dev)
> >>>> +{
> >>>> +       struct efi_device_path_scsi *dp;
> >>>> +       struct blk_desc *desc = dev_get_uclass_plat(dev);
> >>>> +
> >>>> +       dp = efi_alloc(sizeof(struct efi_device_path_scsi));
> >>>> +       if (!dp)
> >>>> +               return NULL;
> >>>> +
> >>>> +       dp->dp.type = DEVICE_PATH_TYPE_MESSAGING_DEVICE;
> >>>> +       dp->dp.sub_type = DEVICE_PATH_SUB_TYPE_MSG_SCSI;
> >>>> +       dp->dp.length = sizeof(*dp);
> >>>> +       dp->logical_unit_number = desc->lun;
> >>>> +       dp->target_id = desc->target;
> >>>> +
> >>>> +       return &dp->dp;
> >>>> +}
> >>>> +
> >>>> +static struct efi_device_path *blk_get_dp_node(struct udevice *dev)
> >>>> +{
> >>>> +       struct efi_device_path_controller *dp;
> >>>> +       struct blk_desc *desc = dev_get_uclass_plat(dev);
> >>>> +       u32 controller_number;
> >>>> +
> >>>> +       switch (device_get_uclass_id(dev->parent)) {
> >>>> +#if CONFIG_IS_ENABLED(SCSI)
> >>>> +       case UCLASS_SCSI:
> >>>> +               return blk_scsi_get_dp_node(dev);
> >>>> +#endif
> >>>> +       case UCLASS_MMC:
> >>>> +               dp->controller_number = desc->hwpart;
> >>>> +               break;
> >>>> +       default:
> >>>
> >>> Since this is checking the parent class, it seems to me that this info
> >>> should be attacked there (the 'media' device) instead of the block
> >>> device.
> >>
> >> For MMC we have these layers:
> >>
> >> * MMC controller - UCLASS_MMC
> >> * MMC slot (or channel) - missing uclass
> >> * hardware partition - UCLASS_BLK
> >>
> >> See https://www.sage-micro.com/us/portfolio-items/s881/
> >>
> >> Currently in the DM tree we don't model the MMC controller and the MMC
> >> slot separately. I think this is a gap that should be closed.
> >>
> >> A single UCLASS_MMC (= MMC slot) device can have multiple UCLASS_BLK
> >> children. One for each hardware partition.
> >>
> >> An MMC controller in EFI is modelled as VenHW() node,
> >> a slot as SD() or eMMC() node.
> >>
> >> An eMMC() or SD() node has a field for the slot number but none for the
> >> hardware partition. Hence, we cannot attach the hardware partition
> >> information to the UCLASS_MMC device. For the UCLASS_BLK device I use a
> >> Ctrl() node. We could also consider using a Unit() node but Unit() nodes
> >> are meant to be used for LUNs.
> >>
> >> We have a similar situation for USB and SCSI LUNs.
> >
> > The current model is to add multiple BLK children for each media
> > device, if needed. That seems to work OK for now. What do you think?
>
> There is nothing in DM tree we need to change now. As I understood your
> first reply it was not clear to you which EFI device-path node type with
> which information matches which DM tree node:
>
> MMC controller/MMC slot (UCLASS_MMC) -> SD(slot) or MMC(slot)
> Hardware partition (UCLASS_BLK) -> Ctrl()
>
> SCSI drive (UCLASS_SCSI) -> VenHw()
> SCSI LUN (UClASS_BLK) -> Scsi(PUN, LUN)

OK, so this is explaining is why you must attach the functions to the
BLK device rather than the storage device? If so, I understand.

What are VenHw and Ctrl ?

Regrads,
Simon

[RFC 4/7] dm: implement get_dp_node for USB hub devices

[Heinrich Schuchardt]

Generate a Usb() node for USB hub devices.

Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
 common/usb_hub.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/common/usb_hub.c b/common/usb_hub.c
index 85c0822d8b..ccf9e16023 100644
--- a/common/usb_hub.c
+++ b/common/usb_hub.c
@@ -24,6 +24,7 @@
 #include <common.h>
 #include <command.h>
 #include <dm.h>
+#include <efi_loader.h>
 #include <env.h>
 #include <errno.h>
 #include <log.h>
@@ -939,6 +940,35 @@ static int usb_hub_post_probe(struct udevice *dev)
 	return usb_hub_scan(dev);
 }
 
+#if CONFIG_IS_ENABLED(EFI_LOADER)
+struct efi_device_path *usb_hub_get_dp_node(struct udevice *dev)
+{
+	struct efi_device_path_usb *dp;
+
+	dp = efi_alloc(sizeof(struct efi_device_path_usb));
+	if (!dp)
+		return NULL;
+
+	switch (device_get_uclass_id(dev->parent)) {
+	case UCLASS_USB_HUB: {
+		struct usb_device *udev = dev_get_parent_priv(dev);
+
+		dp->parent_port_number = udev->portnr;
+		break;
+	}
+	default:
+		dp->parent_port_number = 0;
+	}
+
+	dp->dp.type	  = DEVICE_PATH_TYPE_MESSAGING_DEVICE;
+	dp->dp.sub_type	  = DEVICE_PATH_SUB_TYPE_MSG_USB;
+	dp->dp.length	  = sizeof(*dp);
+	dp->usb_interface = 0;
+
+	return &dp->dp;
+}
+#endif
+
 static const struct udevice_id usb_hub_ids[] = {
 	{ .compatible = "usb-hub" },
 	{ }
@@ -960,6 +990,9 @@ UCLASS_DRIVER(usb_hub) = {
 	.per_child_auto	= sizeof(struct usb_device),
 	.per_child_plat_auto	= sizeof(struct usb_dev_plat),
 	.per_device_auto	= sizeof(struct usb_hub_device),
+#if CONFIG_IS_ENABLED(EFI_LOADER)
+	.get_dp_node	= usb_hub_get_dp_node,
+#endif
 };
 
 static const struct usb_device_id hub_id_table[] = {
-- 
2.39.2

[RFC 5/7] dm: implement get_dp_node for USB mass storage devices

[Heinrich Schuchardt]

Generate a Usb() node for USB mass storage devices.

Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
 common/usb_storage.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/common/usb_storage.c b/common/usb_storage.c
index ac64275773..03bc136156 100644
--- a/common/usb_storage.c
+++ b/common/usb_storage.c
@@ -37,6 +37,7 @@
 #include <bootdev.h>
 #include <command.h>
 #include <dm.h>
+#include <efi_loader.h>
 #include <errno.h>
 #include <log.h>
 #include <mapmem.h>
@@ -1534,6 +1535,35 @@ static int usb_mass_storage_probe(struct udevice *dev)
 	return ret;
 }
 
+#if CONFIG_IS_ENABLED(EFI_LOADER)
+struct efi_device_path *usb_storage_get_dp_node(struct udevice *dev)
+{
+	struct efi_device_path_usb *dp;
+
+	dp = efi_alloc(sizeof(struct efi_device_path_usb));
+	if (!dp)
+		return NULL;
+
+	switch (device_get_uclass_id(dev->parent)) {
+	case UCLASS_USB_HUB: {
+		struct usb_device *udev = dev_get_parent_priv(dev);
+
+		dp->parent_port_number = udev->portnr;
+		break;
+	}
+	default:
+		dp->parent_port_number = 0;
+	}
+
+	dp->dp.type	  = DEVICE_PATH_TYPE_MESSAGING_DEVICE;
+	dp->dp.sub_type	  = DEVICE_PATH_SUB_TYPE_MSG_USB;
+	dp->dp.length	  = sizeof(*dp);
+	dp->usb_interface = 0;
+
+	return &dp->dp;
+}
+#endif
+
 static const struct udevice_id usb_mass_storage_ids[] = {
 	{ .compatible = "usb-mass-storage" },
 	{ }
@@ -1552,6 +1582,9 @@ U_BOOT_DRIVER(usb_mass_storage) = {
 UCLASS_DRIVER(usb_mass_storage) = {
 	.id		= UCLASS_MASS_STORAGE,
 	.name		= "usb_mass_storage",
+#if CONFIG_IS_ENABLED(EFI_LOADER)
+	.get_dp_node	= usb_storage_get_dp_node,
+#endif
 };
 
 static const struct usb_device_id mass_storage_id_table[] = {
-- 
2.39.2

[RFC 6/7] dm: implement get_dp_node for MMC devices

[Heinrich Schuchardt]

Generate an SD() or eMMC() node for MMC devices.

Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
 drivers/mmc/mmc-uclass.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/mmc/mmc-uclass.c b/drivers/mmc/mmc-uclass.c
index 01d9b0201f..4f85d80273 100644
--- a/drivers/mmc/mmc-uclass.c
+++ b/drivers/mmc/mmc-uclass.c
@@ -9,6 +9,7 @@
 
 #include <common.h>
 #include <bootdev.h>
+#include <efi_loader.h>
 #include <log.h>
 #include <mmc.h>
 #include <dm.h>
@@ -505,6 +506,27 @@ static int mmc_blk_probe(struct udevice *dev)
 	return 0;
 }
 
+#if CONFIG_IS_ENABLED(EFI_LOADER)
+struct efi_device_path *mmc_get_dp_node(struct udevice *dev)
+{
+	struct efi_device_path_sd_mmc_path *dp;
+	struct mmc *mmc = mmc_get_mmc_dev(dev);
+
+	dp = efi_alloc(sizeof(struct efi_device_path_sd_mmc_path));
+	if (!dp)
+		return NULL;
+
+	dp->dp.type	= DEVICE_PATH_TYPE_MESSAGING_DEVICE;
+	dp->dp.sub_type = IS_SD(mmc) ?
+			  DEVICE_PATH_SUB_TYPE_MSG_SD :
+			  DEVICE_PATH_SUB_TYPE_MSG_MMC;
+	dp->dp.length	= sizeof(*dp);
+	dp->slot_number	= dev_seq(dev);
+
+	return &dp->dp;
+}
+#endif
+
 #if CONFIG_IS_ENABLED(MMC_UHS_SUPPORT) || \
     CONFIG_IS_ENABLED(MMC_HS200_SUPPORT) || \
     CONFIG_IS_ENABLED(MMC_HS400_SUPPORT)
@@ -547,4 +569,7 @@ UCLASS_DRIVER(mmc) = {
 	.name		= "mmc",
 	.flags		= DM_UC_FLAG_SEQ_ALIAS,
 	.per_device_auto	= sizeof(struct mmc_uclass_priv),
+#if CONFIG_IS_ENABLED(EFI_LOADER)
+	.get_dp_node	= mmc_get_dp_node,
+#endif
 };
-- 
2.39.2

[RFC 7/7] efi_loader: use uclass_get_dp_node

[Heinrich Schuchardt]

Use function uclass_get_dp_node() to construct device paths.

Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
 lib/efi_loader/efi_device_path.c | 325 +++----------------------------
 1 file changed, 28 insertions(+), 297 deletions(-)

diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c
index d5cc495830..288baa1ca7 100644
--- a/lib/efi_loader/efi_device_path.c
+++ b/lib/efi_loader/efi_device_path.c
@@ -45,24 +45,6 @@ static const struct efi_device_path_vendor ROOT = {
 	.guid = U_BOOT_GUID,
 };
 
-#if defined(CONFIG_MMC)
-/*
- * Determine if an MMC device is an SD card.
- *
- * @desc	block device descriptor
- * Return:	true if the device is an SD card
- */
-static bool is_sd(struct blk_desc *desc)
-{
-	struct mmc *mmc = find_mmc_device(desc->devnum);
-
-	if (!mmc)
-		return false;
-
-	return IS_SD(mmc) != 0U;
-}
-#endif
-
 /*
  * Iterate to next block in device-path, terminating (returning NULL)
  * at /End* node.
@@ -500,87 +482,24 @@ bool efi_dp_is_multi_instance(const struct efi_device_path *dp)
 /* size of device-path not including END node for device and all parents
  * up to the root device.
  */
-__maybe_unused static unsigned int dp_size(struct udevice *dev)
+__maybe_unused static size_t dp_size(struct udevice *dev)
 {
-	if (!dev || !dev->driver)
-		return sizeof(ROOT);
-
-	switch (device_get_uclass_id(dev)) {
-	case UCLASS_ROOT:
-	case UCLASS_SIMPLE_BUS:
-		/* stop traversing parents at this point: */
-		return sizeof(ROOT);
-	case UCLASS_ETH:
-		return dp_size(dev->parent) +
-			sizeof(struct efi_device_path_mac_addr);
-	case UCLASS_BLK:
-		switch (dev->parent->uclass->uc_drv->id) {
-#ifdef CONFIG_IDE
-		case UCLASS_IDE:
-			return dp_size(dev->parent) +
-				sizeof(struct efi_device_path_atapi);
-#endif
-#if defined(CONFIG_SCSI)
-		case UCLASS_SCSI:
-			return dp_size(dev->parent) +
-				sizeof(struct efi_device_path_scsi);
-#endif
-#if defined(CONFIG_MMC)
-		case UCLASS_MMC:
-			return dp_size(dev->parent) +
-				sizeof(struct efi_device_path_sd_mmc_path);
-#endif
-#if defined(CONFIG_AHCI) || defined(CONFIG_SATA)
-		case UCLASS_AHCI:
-			return dp_size(dev->parent) +
-				sizeof(struct efi_device_path_sata);
-#endif
-#if defined(CONFIG_NVME)
-		case UCLASS_NVME:
-			return dp_size(dev->parent) +
-				sizeof(struct efi_device_path_nvme);
-#endif
-#ifdef CONFIG_SANDBOX
-		case UCLASS_HOST:
-			 /*
-			  * Sandbox's host device will be represented
-			  * as vendor device with extra one byte for
-			  * device number
-			  */
-			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:
-			 /*
-			  * Virtio devices will be represented as a vendor
-			  * device node with an extra byte for the device
-			  * number.
-			  */
-			return dp_size(dev->parent)
-				+ sizeof(struct efi_device_path_vendor) + 1;
-#endif
-		default:
-			return dp_size(dev->parent);
-		}
-#if defined(CONFIG_MMC)
-	case UCLASS_MMC:
-		return dp_size(dev->parent) +
-			sizeof(struct efi_device_path_sd_mmc_path);
-#endif
-	case UCLASS_MASS_STORAGE:
-	case UCLASS_USB_HUB:
-		return dp_size(dev->parent) +
-			sizeof(struct efi_device_path_usb);
-	default:
-		/* just skip over unknown classes: */
-		return dp_size(dev->parent);
+	unsigned int size = 0;
+	struct efi_device_path *dp;
+
+	if (!dev)
+		return 0;
+
+	if (dev->parent)
+		size = dp_size(dev->parent);
+
+	dp = uclass_get_dp_node(dev);
+	if (dp) {
+		size += dp->length;
+		efi_free_pool(dp);
 	}
+
+	return size;
 }
 
 /*
@@ -592,210 +511,22 @@ __maybe_unused static unsigned int dp_size(struct udevice *dev)
  */
 __maybe_unused static void *dp_fill(void *buf, struct udevice *dev)
 {
-	if (!dev || !dev->driver)
-		return buf;
+	struct efi_device_path *dp;
 
-	switch (device_get_uclass_id(dev)) {
-	case UCLASS_ROOT:
-	case UCLASS_SIMPLE_BUS: {
-		/* stop traversing parents at this point: */
-		struct efi_device_path_vendor *vdp = buf;
-		*vdp = ROOT;
-		return &vdp[1];
-	}
-#ifdef CONFIG_NETDEVICES
-	case UCLASS_ETH: {
-		struct efi_device_path_mac_addr *dp =
-			dp_fill(buf, dev->parent);
-		struct eth_pdata *pdata = dev_get_plat(dev);
-
-		dp->dp.type = DEVICE_PATH_TYPE_MESSAGING_DEVICE;
-		dp->dp.sub_type = DEVICE_PATH_SUB_TYPE_MSG_MAC_ADDR;
-		dp->dp.length = sizeof(*dp);
-		memset(&dp->mac, 0, sizeof(dp->mac));
-		/* We only support IPv4 */
-		memcpy(&dp->mac, &pdata->enetaddr, ARP_HLEN);
-		/* Ethernet */
-		dp->if_type = 1;
-		return &dp[1];
-	}
-#endif
-	case UCLASS_BLK:
-		switch (dev->parent->uclass->uc_drv->id) {
-#ifdef CONFIG_SANDBOX
-		case UCLASS_HOST: {
-			/* stop traversing parents at this point: */
-			struct efi_device_path_vendor *dp;
-			struct blk_desc *desc = dev_get_uclass_plat(dev);
-
-			dp_fill(buf, dev->parent);
-			dp = buf;
-			++dp;
-			dp->dp.type = DEVICE_PATH_TYPE_HARDWARE_DEVICE;
-			dp->dp.sub_type = DEVICE_PATH_SUB_TYPE_VENDOR;
-			dp->dp.length = sizeof(*dp) + 1;
-			memcpy(&dp->guid, &efi_guid_host_dev,
-			       sizeof(efi_guid_t));
-			dp->vendor_data[0] = desc->devnum;
-			return &dp->vendor_data[1];
-			}
-#endif
-#ifdef CONFIG_VIRTIO_BLK
-		case UCLASS_VIRTIO: {
-			struct efi_device_path_vendor *dp;
-			struct blk_desc *desc = dev_get_uclass_plat(dev);
-
-			dp_fill(buf, dev->parent);
-			dp = buf;
-			++dp;
-			dp->dp.type = DEVICE_PATH_TYPE_HARDWARE_DEVICE;
-			dp->dp.sub_type = DEVICE_PATH_SUB_TYPE_VENDOR;
-			dp->dp.length = sizeof(*dp) + 1;
-			memcpy(&dp->guid, &efi_guid_virtio_dev,
-			       sizeof(efi_guid_t));
-			dp->vendor_data[0] = desc->devnum;
-			return &dp->vendor_data[1];
-			}
-#endif
-#ifdef CONFIG_IDE
-		case UCLASS_IDE: {
-			struct efi_device_path_atapi *dp =
-			dp_fill(buf, dev->parent);
-			struct blk_desc *desc = dev_get_uclass_plat(dev);
-
-			dp->dp.type = DEVICE_PATH_TYPE_MESSAGING_DEVICE;
-			dp->dp.sub_type = DEVICE_PATH_SUB_TYPE_MSG_ATAPI;
-			dp->dp.length = sizeof(*dp);
-			dp->logical_unit_number = desc->devnum;
-			dp->primary_secondary = IDE_BUS(desc->devnum);
-			dp->slave_master = desc->devnum %
-				(CONFIG_SYS_IDE_MAXDEVICE /
-				 CONFIG_SYS_IDE_MAXBUS);
-			return &dp[1];
-			}
-#endif
-#if defined(CONFIG_SCSI)
-		case UCLASS_SCSI: {
-			struct efi_device_path_scsi *dp =
-				dp_fill(buf, dev->parent);
-			struct blk_desc *desc = dev_get_uclass_plat(dev);
-
-			dp->dp.type = DEVICE_PATH_TYPE_MESSAGING_DEVICE;
-			dp->dp.sub_type = DEVICE_PATH_SUB_TYPE_MSG_SCSI;
-			dp->dp.length = sizeof(*dp);
-			dp->logical_unit_number = desc->lun;
-			dp->target_id = desc->target;
-			return &dp[1];
-			}
-#endif
-#if defined(CONFIG_MMC)
-		case UCLASS_MMC: {
-			struct efi_device_path_sd_mmc_path *sddp =
-				dp_fill(buf, dev->parent);
-			struct blk_desc *desc = dev_get_uclass_plat(dev);
-
-			sddp->dp.type     = DEVICE_PATH_TYPE_MESSAGING_DEVICE;
-			sddp->dp.sub_type = is_sd(desc) ?
-				DEVICE_PATH_SUB_TYPE_MSG_SD :
-				DEVICE_PATH_SUB_TYPE_MSG_MMC;
-			sddp->dp.length   = sizeof(*sddp);
-			sddp->slot_number = dev_seq(dev);
-			return &sddp[1];
-			}
-#endif
-#if defined(CONFIG_AHCI) || defined(CONFIG_SATA)
-		case UCLASS_AHCI: {
-			struct efi_device_path_sata *dp =
-				dp_fill(buf, dev->parent);
-			struct blk_desc *desc = dev_get_uclass_plat(dev);
-
-			dp->dp.type     = DEVICE_PATH_TYPE_MESSAGING_DEVICE;
-			dp->dp.sub_type = DEVICE_PATH_SUB_TYPE_MSG_SATA;
-			dp->dp.length   = sizeof(*dp);
-			dp->hba_port = desc->devnum;
-			/* default 0xffff implies no port multiplier */
-			dp->port_multiplier_port = 0xffff;
-			dp->logical_unit_number = desc->lun;
-			return &dp[1];
-			}
-#endif
-#if defined(CONFIG_NVME)
-		case UCLASS_NVME: {
-			struct efi_device_path_nvme *dp =
-				dp_fill(buf, dev->parent);
-			u32 ns_id;
-
-			dp->dp.type     = DEVICE_PATH_TYPE_MESSAGING_DEVICE;
-			dp->dp.sub_type = DEVICE_PATH_SUB_TYPE_MSG_NVME;
-			dp->dp.length   = sizeof(*dp);
-			nvme_get_namespace_id(dev, &ns_id, dp->eui64);
-			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",
-			      __FILE__, __LINE__, __func__,
-			      dev->name, dev->parent->uclass->uc_drv->id);
-			return dp_fill(buf, dev->parent);
-		}
-#if defined(CONFIG_MMC)
-	case UCLASS_MMC: {
-		struct efi_device_path_sd_mmc_path *sddp =
-			dp_fill(buf, dev->parent);
-		struct mmc *mmc = mmc_get_mmc_dev(dev);
-		struct blk_desc *desc = mmc_get_blk_desc(mmc);
-
-		sddp->dp.type     = DEVICE_PATH_TYPE_MESSAGING_DEVICE;
-		sddp->dp.sub_type = is_sd(desc) ?
-			DEVICE_PATH_SUB_TYPE_MSG_SD :
-			DEVICE_PATH_SUB_TYPE_MSG_MMC;
-		sddp->dp.length   = sizeof(*sddp);
-		sddp->slot_number = dev_seq(dev);
-
-		return &sddp[1];
-	}
-#endif
-	case UCLASS_MASS_STORAGE:
-	case UCLASS_USB_HUB: {
-		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);
+	if (!dev)
+		return buf;
 
-			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;
-		udp->dp.length   = sizeof(*udp);
-		udp->usb_interface = 0;
+	if (dev->parent)
+		buf = dp_fill(buf, dev->parent);
 
-		return &udp[1];
-	}
-	default:
-		/* If the uclass driver is missing, this will show NULL */
-		log_debug("unhandled device class: %s (%s)\n", dev->name,
-			  dev_get_uclass_name(dev));
-		return dp_fill(buf, dev->parent);
+	dp = uclass_get_dp_node(dev);
+	if (dp) {
+		memcpy(buf, dp, dp->length);
+		buf += dp->length;
+		efi_free_pool(dp);
 	}
+
+	return buf;
 }
 
 static unsigned dp_part_size(struct blk_desc *desc, int part)
-- 
2.39.2

Re: [RFC 7/7] efi_loader: use uclass_get_dp_node

[Simon Glass]

Hi Heinrich,

On Mon, 27 Mar 2023 at 06:28, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
> Use function uclass_get_dp_node() to construct device paths.
>
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
>  lib/efi_loader/efi_device_path.c | 325 +++----------------------------
>  1 file changed, 28 insertions(+), 297 deletions(-)

This is definitely a step in the right direction. Thank you for working on it.

[..]

> +       dp = uclass_get_dp_node(dev);
> +       if (dp) {
> +               memcpy(buf, dp, dp->length);
> +               buf += dp->length;
> +               efi_free_pool(dp);

This suggests that the API should be:

int uclass_get_dp_node(struct xxx *dp)

with it writing the data to the provided pointer address and returning
the number of bytes used. That would avoid the malloc() / free().

>         }
> +
> +       return buf;
>  }
>
>  static unsigned dp_part_size(struct blk_desc *desc, int part)
> --
> 2.39.2
>

Regards,
Simon