efi_loader: disk: Fix CONFIG_BLK breakage

When using CONFIG_BLK, there were 2 issues:

1) The name we generate the device with has to match the name we set in efi_set_bootdev()

2) The device we pass into our block functions was wrong, we should not rediscover it but just use the already known pointer.

This patch fixes both issues.

Signed-off-by Alexander Graf <[email protected]>

---
 cmd/bootefi.c             | 23 ++++++++++++++++++-----
 lib/efi_loader/efi_disk.c | 18 +++++++++++-------
 2 files changed, 29 insertions(+), 12 deletions(-)

cmd/bootefi.c +18 -5

index b8ecf4c..53a6ee3 100644
@@ -8,6 +8,7 @@
 
 #include <common.h>
 #include <command.h>
+#include <dm/device.h>
 #include <efi_loader.h>
 #include <errno.h>
 #include <libfdt.h>

@@ -269,18 +270,30 @@ void efi_set_bootdev(const char *dev, const char *devnr, const char *path)
        char devname[32] = { 0 }; /* dp->str is u16[32] long */
        char *colon;
 
-       /* Assemble the condensed device name we use in efi_disk.c */
-       snprintf(devname, sizeof(devname), "%s%s", dev, devnr);
+#if defined(CONFIG_BLK) || defined(CONFIG_ISO_PARTITION)
+       desc = blk_get_dev(dev, simple_strtol(devnr, NULL, 10));
+#endif
+
+#ifdef CONFIG_BLK
+       if (desc) {
+               snprintf(devname, sizeof(devname), "%s", desc->bdev->name);
+       } else
+#endif
+
+       {
+               /* Assemble the condensed device name we use in efi_disk.c */
+               snprintf(devname, sizeof(devname), "%s%s", dev, devnr);
+       }
+
        colon = strchr(devname, ':');
 
 #ifdef CONFIG_ISO_PARTITION
        /* For ISOs we create partition block devices */
-       desc = blk_get_dev(dev, simple_strtol(devnr, NULL, 10));
        if (desc && (desc->type != DEV_TYPE_UNKNOWN) &&
            (desc->part_type == PART_TYPE_ISO)) {
                if (!colon)
-                       snprintf(devname, sizeof(devname), "%s%s:1", dev,
-                                devnr);
+                       snprintf(devname, sizeof(devname), "%s:1", devname);
+
                colon = NULL;
        }
 #endif

lib/efi_loader/efi_disk.c +0 -0

index c434c92..e00a747 100644

lib/efi_loader/efi_disk.c +9 -8

 };
 
 static efi_status_t efi_disk_open_block(void *handle, efi_guid_t *protocol,

@@ -78,8 +80,7 @@ static efi_status_t EFIAPI efi_disk_rw_blocks(struct efi_block_io *this,
        unsigned long n;
 
        diskobj = container_of(this, struct efi_disk_obj, ops);
-       if (!(desc = blk_get_dev(diskobj->ifname, diskobj->dev_index)))
-               return EFI_EXIT(EFI_DEVICE_ERROR);
+       desc = (struct blk_desc *) diskobj->desc;
        blksz = desc->blksz;
        blocks = buffer_size / blksz;
        lba += diskobj->offset;

@@ -213,6 +214,7 @@ static void efi_disk_add_dev(const char *name,
        diskobj->ifname = if_typename;
        diskobj->dev_index = dev_index;
        diskobj->offset = offset;
+       diskobj->desc = desc;
 
        /* Fill in EFI IO Media info (for read/write callbacks) */
        diskobj->media.removable_media = desc->removable;

@@ -240,7 +242,8 @@ static void efi_disk_add_dev(const char *name,
 
 static int efi_disk_create_eltorito(struct blk_desc *desc,
                                    const char *if_typename,
-                                   int diskid)
+                                   int diskid,
+                                   const char *pdevname)
 {
        int disks = 0;
 #ifdef CONFIG_ISO_PARTITION

@@ -252,8 +255,8 @@ static int efi_disk_create_eltorito(struct blk_desc *desc,
                return 0;
 
        while (!part_get_info(desc, part, &info)) {
-               snprintf(devname, sizeof(devname), "%s%d:%d", if_typename,
-                        diskid, part);
+               snprintf(devname, sizeof(devname), "%s:%d", pdevname,
+                        part);
                efi_disk_add_dev(devname, if_typename, desc, diskid,
                                 info.start);
                part++;

@@ -296,7 +299,7 @@ int efi_disk_register(void)
                * so let's create them here
                */
                disks += efi_disk_create_eltorito(desc, if_typename,
-                                                 desc->devnum);
+                                                 desc->devnum, dev->name);
        }
 #else
        int i, if_type;

@@ -331,7 +334,8 @@ int efi_disk_register(void)
                         * El Torito images show up as block devices
                         * in an EFI world, so let's create them here
                         */
-                       disks += efi_disk_create_eltorito(desc, if_typename, i);
+                       disks += efi_disk_create_eltorito(desc, if_typename,
+                                                         i, devname);
                }
        }
 #endif
--

2.6.6

Hi Alexander,

Quoted 69 lines · Alexander Graf

When using CONFIG_BLK, there were 2 issues:

1) The name we generate the device with has to match the name we set in efi_set_bootdev()

2) The device we pass into our block functions was wrong, we should not rediscover it but just use the already known pointer.

This patch fixes both issues.

Signed-off-by Alexander Graf <[email protected]>

---
 cmd/bootefi.c             | 23 ++++++++++++++++++-----
 lib/efi_loader/efi_disk.c | 18 +++++++++++-------
 2 files changed, 29 insertions(+), 12 deletions(-)

cmd/bootefi.c +18 -5

index b8ecf4c..53a6ee3 100644
@@ -8,6 +8,7 @@
 
 #include <common.h>
 #include <command.h>
+#include <dm/device.h>
 #include <efi_loader.h>
 #include <errno.h>
 #include <libfdt.h>

@@ -269,18 +270,30 @@ void efi_set_bootdev(const char *dev, const char *devnr, const char *path)
        char devname[32] = { 0 }; /* dp->str is u16[32] long */
        char *colon;
 
-       /* Assemble the condensed device name we use in efi_disk.c */
-       snprintf(devname, sizeof(devname), "%s%s", dev, devnr);
+#if defined(CONFIG_BLK) || defined(CONFIG_ISO_PARTITION)
+       desc = blk_get_dev(dev, simple_strtol(devnr, NULL, 10));
+#endif
+
+#ifdef CONFIG_BLK
+       if (desc) {
+               snprintf(devname, sizeof(devname), "%s", desc->bdev->name);
+       } else
+#endif
+
+       {
+               /* Assemble the condensed device name we use in efi_disk.c */
+               snprintf(devname, sizeof(devname), "%s%s", dev, devnr);
+       }
+
        colon = strchr(devname, ':');
 
 #ifdef CONFIG_ISO_PARTITION
        /* For ISOs we create partition block devices */
-       desc = blk_get_dev(dev, simple_strtol(devnr, NULL, 10));
        if (desc && (desc->type != DEV_TYPE_UNKNOWN) &&
            (desc->part_type == PART_TYPE_ISO)) {
                if (!colon)
-                       snprintf(devname, sizeof(devname), "%s%s:1", dev,
-                                devnr);
+                       snprintf(devname, sizeof(devname), "%s:1", devname);
+
                colon = NULL;
        }
 #endif

lib/efi_loader/efi_disk.c +0 -0

index c434c92..e00a747 100644

lib/efi_loader/efi_disk.c struct efi_disk_obj { · line 35

Quoted 6 lines

lib/efi_loader/efi_disk.c +2 -0

@@ -31,6 +31,8 @@ struct efi_disk_obj {
        struct efi_device_path_file_path *dp;
        /* Offset into disk for simple partitions */
        lbaint_t offset;
+       /* Internal block device */
+       const struct blk_desc *desc;

Rather than storing this, can you store the udevice?

Hi Alexander,

Quoted 69 lines · Alexander Graf

When using CONFIG_BLK, there were 2 issues:

1) The name we generate the device with has to match the name we set in efi_set_bootdev()

2) The device we pass into our block functions was wrong, we should not rediscover it but just use the already known pointer.

This patch fixes both issues.

Signed-off-by Alexander Graf <[email protected]>

---
cmd/bootefi.c | 23 ++++++++++++++++++-----
lib/efi_loader/efi_disk.c | 18 +++++++++++-------

2 files changed, 29 insertions(+), 12 deletions(-)

cmd/bootefi.c +18 -5

index b8ecf4c..53a6ee3 100644
@@ -8,6 +8,7 @@
 
 #include <common.h>
 #include <command.h>
+#include <dm/device.h>
 #include <efi_loader.h>
 #include <errno.h>
 #include <libfdt.h>

@@ -269,18 +270,30 @@ void efi_set_bootdev(const char *dev, const char *devnr, const char *path)
       char devname[32] = { 0 }; /* dp->str is u16[32] long */
       char *colon;
 
-       /* Assemble the condensed device name we use in efi_disk.c */
-       snprintf(devname, sizeof(devname), "%s%s", dev, devnr);
+#if defined(CONFIG_BLK) || defined(CONFIG_ISO_PARTITION)
+       desc = blk_get_dev(dev, simple_strtol(devnr, NULL, 10));
+#endif
+
+#ifdef CONFIG_BLK
+       if (desc) {
+               snprintf(devname, sizeof(devname), "%s", desc->bdev->name);
+       } else
+#endif
+
+       {
+               /* Assemble the condensed device name we use in efi_disk.c */
+               snprintf(devname, sizeof(devname), "%s%s", dev, devnr);
+       }
+
       colon = strchr(devname, ':');
 
 #ifdef CONFIG_ISO_PARTITION
       /* For ISOs we create partition block devices */
-       desc = blk_get_dev(dev, simple_strtol(devnr, NULL, 10));
       if (desc && (desc->type != DEV_TYPE_UNKNOWN) &&
           (desc->part_type == PART_TYPE_ISO)) {
               if (!colon)
-                       snprintf(devname, sizeof(devname), "%s%s:1", dev,
-                                devnr);
+                       snprintf(devname, sizeof(devname), "%s:1", devname);
+
               colon = NULL;
       }
 #endif

lib/efi_loader/efi_disk.c +0 -0

index c434c92..e00a747 100644

lib/efi_loader/efi_disk.c struct efi_disk_obj { · line 35

Quoted 6 lines

lib/efi_loader/efi_disk.c +2 -0

@@ -31,6 +31,8 @@ struct efi_disk_obj {
       struct efi_device_path_file_path *dp;
       /* Offset into disk for simple partitions */
       lbaint_t offset;
+       /* Internal block device */
+       const struct blk_desc *desc;

Rather than storing this, can you store the udevice?

Quoted 23 lines · Simon Glass

Hi Alexander,

Quoted 17 lines · Alexander Graf

When using CONFIG_BLK, there were 2 issues:

1) The name we generate the device with has to match the name we set in efi_set_bootdev()

2) The device we pass into our block functions was wrong, we should not rediscover it but just use the already known pointer.

This patch fixes both issues.

Signed-off-by Alexander Graf <[email protected]>

---
cmd/bootefi.c | 23 ++++++++++++++++++-----
lib/efi_loader/efi_disk.c | 18 +++++++++++-------

2 files changed, 29 insertions(+), 12 deletions(-)

Quoted 4 lines

Quoted 4 lines

lib/efi_loader/efi_disk.c +0 -0

index c434c92..e00a747 100644

Quoted 8 lines

lib/efi_loader/efi_disk.c struct efi_disk_obj { · line 35

Quoted 6 lines

lib/efi_loader/efi_disk.c +2 -0

@@ -31,6 +31,8 @@ struct efi_disk_obj {
       struct efi_device_path_file_path *dp;
       /* Offset into disk for simple partitions */
       lbaint_t offset;
+       /* Internal block device */
+       const struct blk_desc *desc;

Rather than storing this, can you store the udevice?

I could, but then I would diverge between the CONFIG_BLK and non-CONFIG_BLK path again, which would turn the code into an #ifdef mess (read: hard to maintain), because the whole device creation path relies on struct blk_desc * today and doesn?t pass the udevice anywhere.

Do you feel strongly about this? To give you an idea how messy it gets, the diff is below.

+Tom

Hi Alex,

Quoted 22 lines · Alexander Graf

Quoted 22 lines · Simon Glass

Hi Alexander,

Quoted 17 lines · Alexander Graf

When using CONFIG_BLK, there were 2 issues:

1) The name we generate the device with has to match the name we set in efi_set_bootdev()

2) The device we pass into our block functions was wrong, we should not rediscover it but just use the already known pointer.

This patch fixes both issues.

Signed-off-by Alexander Graf <[email protected]>

---
cmd/bootefi.c | 23 ++++++++++++++++++-----
lib/efi_loader/efi_disk.c | 18 +++++++++++-------

2 files changed, 29 insertions(+), 12 deletions(-)

[...]

Quoted 11 lines

Quoted 11 lines

lib/efi_loader/efi_disk.c struct efi_disk_obj { · line 35

Quoted 10 lines

lib/efi_loader/efi_disk.c +2 -0

index c434c92..e00a747 100644
@@ -31,6 +31,8 @@ struct efi_disk_obj {
        struct efi_device_path_file_path *dp;
        /* Offset into disk for simple partitions */
        lbaint_t offset;
+       /* Internal block device */
+       const struct blk_desc *desc;

Rather than storing this, can you store the udevice?

Quoted 3 lines

I could, but then I would diverge between the CONFIG_BLK and non-CONFIG_BLK path again, which would turn the code into an #ifdef mess (read: hard to maintain), because the whole device creation path relies on struct blk_desc * today and doesn?t pass the udevice anywhere.

Do you feel strongly about this? To give you an idea how messy it gets, the diff is below.

Actually I'd like to make this feature depend on CONFIG_BLK. If we add new features that don't use driver model, and then use the legacy data structures such that converting to driver model becomes harder, we'll never be done.

I did mention this at the beginning and it seems to have come to pass.

In order of preference from my side:

1. Make EFI_LOADER depend on BLK

Quoted 60 lines · Simon Glass

+Tom

Hi Alex,

Quoted 24 lines · Alexander Graf

Quoted 24 lines · Simon Glass

Hi Alexander,

Quoted 18 lines · Alexander Graf

When using CONFIG_BLK, there were 2 issues:

1) The name we generate the device with has to match the name we set in efi_set_bootdev()

2) The device we pass into our block functions was wrong, we should not rediscover it but just use the already known pointer.

This patch fixes both issues.

Signed-off-by Alexander Graf <[email protected]>

---
cmd/bootefi.c | 23 ++++++++++++++++++-----
lib/efi_loader/efi_disk.c | 18 +++++++++++-------

2 files changed, 29 insertions(+), 12 deletions(-)

[...]

Quoted 13 lines

Quoted 4 lines

Quoted 4 lines

lib/efi_loader/efi_disk.c +0 -0

index c434c92..e00a747 100644

Quoted 8 lines

lib/efi_loader/efi_disk.c struct efi_disk_obj { · line 35

Quoted 6 lines

lib/efi_loader/efi_disk.c +2 -0

@@ -31,6 +31,8 @@ struct efi_disk_obj {
        struct efi_device_path_file_path *dp;
        /* Offset into disk for simple partitions */
        lbaint_t offset;
+       /* Internal block device */
+       const struct blk_desc *desc;

Rather than storing this, can you store the udevice?

Quoted 7 lines

I could, but then I would diverge between the CONFIG_BLK and non-CONFIG_BLK path again, which would turn the code into an #ifdef mess (read: hard to maintain), because the whole device creation path relies on struct blk_desc * today and doesn?t pass the udevice anywhere.

Do you feel strongly about this? To give you an idea how messy it gets, the diff is below.

Actually I'd like to make this feature depend on CONFIG_BLK. If we add new features that don't use driver model, and then use the legacy data structures such that converting to driver model becomes harder, we'll never be done.

I did mention this at the beginning and it seems to have come to pass.

Quoted 3 lines

In order of preference from my side:

1. Make EFI_LOADER depend on BLK

If we make EFI_LOADER depend on BLK, doesn't that break all systems that need storage that isn't converted to device model today? Like the SATA breakage on Xilinx systems, just at a much bigger scale?

Am 10.08.2016 um 15:16 schrieb Simon Glass <[email protected]>:

Hi Alex,

Quoted 66 lines · Alexander Graf

Quoted 63 lines · Simon Glass

+Tom

Hi Alex,

Quoted 23 lines · Alexander Graf

Quoted 23 lines · Simon Glass

Hi Alexander,

Quoted 18 lines · Alexander Graf

When using CONFIG_BLK, there were 2 issues:

1) The name we generate the device with has to match the name we set in efi_set_bootdev()

2) The device we pass into our block functions was wrong, we should not rediscover it but just use the already known pointer.

This patch fixes both issues.

Signed-off-by Alexander Graf <[email protected]>

---
cmd/bootefi.c | 23 ++++++++++++++++++-----
lib/efi_loader/efi_disk.c | 18 +++++++++++-------

2 files changed, 29 insertions(+), 12 deletions(-)

[...]

Quoted 13 lines

Quoted 4 lines

Quoted 4 lines

lib/efi_loader/efi_disk.c +0 -0

index c434c92..e00a747 100644

Quoted 8 lines

lib/efi_loader/efi_disk.c struct efi_disk_obj { · line 35

Quoted 6 lines

lib/efi_loader/efi_disk.c +2 -0

@@ -31,6 +31,8 @@ struct efi_disk_obj {
       struct efi_device_path_file_path *dp;
       /* Offset into disk for simple partitions */
       lbaint_t offset;
+       /* Internal block device */
+       const struct blk_desc *desc;

Rather than storing this, can you store the udevice?

Quoted 7 lines

I could, but then I would diverge between the CONFIG_BLK and non-CONFIG_BLK path again, which would turn the code into an #ifdef mess (read: hard to maintain), because the whole device creation path relies on struct blk_desc * today and doesn?t pass the udevice anywhere.

Do you feel strongly about this? To give you an idea how messy it gets, the diff is below.

Actually I'd like to make this feature depend on CONFIG_BLK. If we add new features that don't use driver model, and then use the legacy data structures such that converting to driver model becomes harder, we'll never be done.

I did mention this at the beginning and it seems to have come to pass.

In order of preference from my side:

1. Make EFI_LOADER depend on BLK

Quoted 3 lines

If we make EFI_LOADER depend on BLK, doesn't that break all systems that need storage that isn't converted to device model today? Like the SATA breakage on Xilinx systems, just at a much bigger scale?

No it just means that these platforms need to move to BLK before they can use the EFI loader. Given the embryonic nature of this feature, that seems reasonable, and the impact would be small. It will also encourage conversion and keep the code cleaner.

Regards,
Simon

Quoted 76 lines

Am 10.08.2016 um 15:16 schrieb Simon Glass <[email protected]>:

Hi Alex,

Quoted 69 lines · Alexander Graf

Quoted 58 lines · Simon Glass

+Tom

Hi Alex,

Quoted 23 lines · Alexander Graf

Quoted 23 lines · Simon Glass

Hi Alexander,

Quoted 18 lines · Alexander Graf

When using CONFIG_BLK, there were 2 issues:

1) The name we generate the device with has to match the name we set in efi_set_bootdev()

2) The device we pass into our block functions was wrong, we should not rediscover it but just use the already known pointer.

This patch fixes both issues.

Signed-off-by Alexander Graf <[email protected]>

---
cmd/bootefi.c | 23 ++++++++++++++++++-----
lib/efi_loader/efi_disk.c | 18 +++++++++++-------

2 files changed, 29 insertions(+), 12 deletions(-)

[...]

Quoted 13 lines

Quoted 4 lines

Quoted 4 lines

lib/efi_loader/efi_disk.c +0 -0

index c434c92..e00a747 100644

Quoted 8 lines

lib/efi_loader/efi_disk.c struct efi_disk_obj { · line 35

Quoted 6 lines

lib/efi_loader/efi_disk.c +2 -0

@@ -31,6 +31,8 @@ struct efi_disk_obj {
       struct efi_device_path_file_path *dp;
       /* Offset into disk for simple partitions */
       lbaint_t offset;
+       /* Internal block device */
+       const struct blk_desc *desc;

Rather than storing this, can you store the udevice?

Quoted 7 lines

I could, but then I would diverge between the CONFIG_BLK and non-CONFIG_BLK path again, which would turn the code into an #ifdef mess (read: hard to maintain), because the whole device creation path relies on struct blk_desc * today and doesn?t pass the udevice anywhere.

Do you feel strongly about this? To give you an idea how messy it gets, the diff is below.

Actually I'd like to make this feature depend on CONFIG_BLK. If we add new features that don't use driver model, and then use the legacy data structures such that converting to driver model becomes harder, we'll never be done.

Quoted 5 lines

I did mention this at the beginning and it seems to have come to pass.

In order of preference from my side:

1. Make EFI_LOADER depend on BLK

If we make EFI_LOADER depend on BLK, doesn't that break all systems that need storage that isn't converted to device model today? Like the SATA breakage on Xilinx systems, just at a much bigger scale?

Quoted 4 lines

No it just means that these platforms need to move to BLK before they can use the EFI loader. Given the embryonic nature of this feature, that seems reasonable, and the impact would be small. It will also encourage conversion and keep the code cleaner.

No, it will simply make my life harder because I would have to sit down and vonvert every single board to BLK that I need EFI enabled.

Hi Alex,

Quoted 81 lines · Alexander Graf

Quoted 80 lines

Am 10.08.2016 um 15:16 schrieb Simon Glass <[email protected]>:

Hi Alex,

Quoted 61 lines · Alexander Graf

Quoted 60 lines · Simon Glass

+Tom

Hi Alex,

Quoted 23 lines · Alexander Graf

Quoted 23 lines · Simon Glass

Hi Alexander,

Quoted 18 lines · Alexander Graf

When using CONFIG_BLK, there were 2 issues:

1) The name we generate the device with has to match the name we set in efi_set_bootdev()

2) The device we pass into our block functions was wrong, we should not rediscover it but just use the already known pointer.

This patch fixes both issues.

Signed-off-by Alexander Graf <[email protected]>

---
cmd/bootefi.c | 23 ++++++++++++++++++-----
lib/efi_loader/efi_disk.c | 18 +++++++++++-------

2 files changed, 29 insertions(+), 12 deletions(-)

[...]

Quoted 13 lines

Quoted 4 lines

Quoted 4 lines

lib/efi_loader/efi_disk.c +0 -0

index c434c92..e00a747 100644

Quoted 8 lines

lib/efi_loader/efi_disk.c struct efi_disk_obj { · line 35

Quoted 6 lines

lib/efi_loader/efi_disk.c +2 -0

@@ -31,6 +31,8 @@ struct efi_disk_obj {
      struct efi_device_path_file_path *dp;
      /* Offset into disk for simple partitions */
      lbaint_t offset;
+       /* Internal block device */
+       const struct blk_desc *desc;

Rather than storing this, can you store the udevice?

Quoted 7 lines

I could, but then I would diverge between the CONFIG_BLK and non-CONFIG_BLK path again, which would turn the code into an #ifdef mess (read: hard to maintain), because the whole device creation path relies on struct blk_desc * today and doesn?t pass the udevice anywhere.

Do you feel strongly about this? To give you an idea how messy it gets, the diff is below.

Actually I'd like to make this feature depend on CONFIG_BLK. If we add new features that don't use driver model, and then use the legacy data structures such that converting to driver model becomes harder, we'll never be done.

I did mention this at the beginning and it seems to have come to pass.

Quoted 8 lines

Quoted 3 lines

In order of preference from my side:

1. Make EFI_LOADER depend on BLK

If we make EFI_LOADER depend on BLK, doesn't that break all systems that need storage that isn't converted to device model today? Like the SATA breakage on Xilinx systems, just at a much bigger scale?

No it just means that these platforms need to move to BLK before they can use the EFI loader. Given the embryonic nature of this feature, that seems reasonable, and the impact would be small. It will also encourage conversion and keep the code cleaner.

Quoted 1 line

No, it will simply make my life harder because I would have to sit down and vonvert every single board to BLK that I need EFI enabled.

Yes that's right. But it is mostly just a simple case of enabling the option. For a few boards there might be an MMC driver that needs converting, but we can approach those one by one.

Quoted 87 lines · Simon Glass

Hi Alex,

Quoted 81 lines · Alexander Graf

Quoted 80 lines

Am 10.08.2016 um 15:16 schrieb Simon Glass <[email protected]>:

Hi Alex,

Quoted 61 lines · Alexander Graf

Quoted 60 lines · Simon Glass

+Tom

Hi Alex,

Quoted 23 lines · Alexander Graf

Quoted 23 lines · Simon Glass

Hi Alexander,

Quoted 18 lines · Alexander Graf

When using CONFIG_BLK, there were 2 issues:

1) The name we generate the device with has to match the name we set in efi_set_bootdev()

2) The device we pass into our block functions was wrong, we should not rediscover it but just use the already known pointer.

This patch fixes both issues.

Signed-off-by Alexander Graf <[email protected]>

---
cmd/bootefi.c | 23 ++++++++++++++++++-----
lib/efi_loader/efi_disk.c | 18 +++++++++++-------

2 files changed, 29 insertions(+), 12 deletions(-)

[...]

Quoted 13 lines

Quoted 4 lines

Quoted 4 lines

lib/efi_loader/efi_disk.c +0 -0

index c434c92..e00a747 100644

Quoted 8 lines

lib/efi_loader/efi_disk.c struct efi_disk_obj { · line 35

Quoted 6 lines

lib/efi_loader/efi_disk.c +2 -0

@@ -31,6 +31,8 @@ struct efi_disk_obj {
      struct efi_device_path_file_path *dp;
      /* Offset into disk for simple partitions */
      lbaint_t offset;
+       /* Internal block device */
+       const struct blk_desc *desc;

Rather than storing this, can you store the udevice?

Quoted 7 lines

I could, but then I would diverge between the CONFIG_BLK and non-CONFIG_BLK path again, which would turn the code into an #ifdef mess (read: hard to maintain), because the whole device creation path relies on struct blk_desc * today and doesn?t pass the udevice anywhere.

Do you feel strongly about this? To give you an idea how messy it gets, the diff is below.

Actually I'd like to make this feature depend on CONFIG_BLK. If we add new features that don't use driver model, and then use the legacy data structures such that converting to driver model becomes harder, we'll never be done.

I did mention this at the beginning and it seems to have come to pass.

Quoted 8 lines

Quoted 3 lines

In order of preference from my side:

1. Make EFI_LOADER depend on BLK

If we make EFI_LOADER depend on BLK, doesn't that break all systems that need storage that isn't converted to device model today? Like the SATA breakage on Xilinx systems, just at a much bigger scale?

No it just means that these platforms need to move to BLK before they can use the EFI loader. Given the embryonic nature of this feature, that seems reasonable, and the impact would be small. It will also encourage conversion and keep the code cleaner.

Quoted 5 lines

Quoted 1 line

No, it will simply make my life harder because I would have to sit down and vonvert every single board to BLK that I need EFI enabled.

Yes that's right. But it is mostly just a simple case of enabling the option. For a few boards there might be an MMC driver that needs converting, but we can approach those one by one.

That approach sounds terribly wrong tbh. If it?s so simple, why not just mark a flag day where CONFIG_BLK becomes mandatory and fix all fallout? Then you?ll have much more help at your disposal than a few percent of my overloaded days ;).

Quoted 76 lines

Am 10.08.2016 um 15:16 schrieb Simon Glass <[email protected]>:

Hi Alex,

Quoted 69 lines · Alexander Graf

Quoted 58 lines · Simon Glass

+Tom

Hi Alex,

Quoted 23 lines · Alexander Graf

Quoted 23 lines · Simon Glass

Hi Alexander,

Quoted 18 lines · Alexander Graf

When using CONFIG_BLK, there were 2 issues:

1) The name we generate the device with has to match the name we set in efi_set_bootdev()

2) The device we pass into our block functions was wrong, we should not rediscover it but just use the already known pointer.

This patch fixes both issues.

Signed-off-by Alexander Graf <[email protected]>

---
cmd/bootefi.c | 23 ++++++++++++++++++-----
lib/efi_loader/efi_disk.c | 18 +++++++++++-------

2 files changed, 29 insertions(+), 12 deletions(-)

[...]

Quoted 13 lines

Quoted 4 lines

Quoted 4 lines

lib/efi_loader/efi_disk.c +0 -0

index c434c92..e00a747 100644

Quoted 8 lines

lib/efi_loader/efi_disk.c struct efi_disk_obj { · line 35

Quoted 6 lines

lib/efi_loader/efi_disk.c +2 -0

@@ -31,6 +31,8 @@ struct efi_disk_obj {
       struct efi_device_path_file_path *dp;
       /* Offset into disk for simple partitions */
       lbaint_t offset;
+       /* Internal block device */
+       const struct blk_desc *desc;

Rather than storing this, can you store the udevice?

Quoted 7 lines

I could, but then I would diverge between the CONFIG_BLK and non-CONFIG_BLK path again, which would turn the code into an #ifdef mess (read: hard to maintain), because the whole device creation path relies on struct blk_desc * today and doesn?t pass the udevice anywhere.

Do you feel strongly about this? To give you an idea how messy it gets, the diff is below.

Actually I'd like to make this feature depend on CONFIG_BLK. If we add new features that don't use driver model, and then use the legacy data structures such that converting to driver model becomes harder, we'll never be done.

Quoted 5 lines

I did mention this at the beginning and it seems to have come to pass.

In order of preference from my side:

1. Make EFI_LOADER depend on BLK

If we make EFI_LOADER depend on BLK, doesn't that break all systems that need storage that isn't converted to device model today? Like the SATA breakage on Xilinx systems, just at a much bigger scale?

Quoted 4 lines

No it just means that these platforms need to move to BLK before they can use the EFI loader. Given the embryonic nature of this feature, that seems reasonable, and the impact would be small. It will also encourage conversion and keep the code cleaner.

No, it will simply make my life harder because I would have to sit down and vonvert every single board to BLK that I need EFI enabled.

Quoted 81 lines · Alexander Graf

Quoted 80 lines

Am 10.08.2016 um 15:16 schrieb Simon Glass <[email protected]>:

Hi Alex,

Quoted 61 lines · Alexander Graf

Quoted 60 lines · Simon Glass

+Tom

Hi Alex,

Quoted 23 lines · Alexander Graf

Quoted 23 lines · Simon Glass

Hi Alexander,

Quoted 18 lines · Alexander Graf

When using CONFIG_BLK, there were 2 issues:

1) The name we generate the device with has to match the name we set in efi_set_bootdev()

2) The device we pass into our block functions was wrong, we should not rediscover it but just use the already known pointer.

This patch fixes both issues.

Signed-off-by Alexander Graf <[email protected]>

---
cmd/bootefi.c | 23 ++++++++++++++++++-----
lib/efi_loader/efi_disk.c | 18 +++++++++++-------

2 files changed, 29 insertions(+), 12 deletions(-)

[...]

Quoted 13 lines

Quoted 4 lines

Quoted 4 lines

lib/efi_loader/efi_disk.c +0 -0

index c434c92..e00a747 100644

Quoted 8 lines

lib/efi_loader/efi_disk.c struct efi_disk_obj { · line 35

Quoted 6 lines

lib/efi_loader/efi_disk.c +2 -0

@@ -31,6 +31,8 @@ struct efi_disk_obj {
      struct efi_device_path_file_path *dp;
      /* Offset into disk for simple partitions */
      lbaint_t offset;
+       /* Internal block device */
+       const struct blk_desc *desc;

Rather than storing this, can you store the udevice?

Quoted 7 lines

I could, but then I would diverge between the CONFIG_BLK and non-CONFIG_BLK path again, which would turn the code into an #ifdef mess (read: hard to maintain), because the whole device creation path relies on struct blk_desc * today and doesn?t pass the udevice anywhere.

Do you feel strongly about this? To give you an idea how messy it gets, the diff is below.

Actually I'd like to make this feature depend on CONFIG_BLK. If we add new features that don't use driver model, and then use the legacy data structures such that converting to driver model becomes harder, we'll never be done.

I did mention this at the beginning and it seems to have come to pass.

Quoted 8 lines

Quoted 3 lines

In order of preference from my side:

1. Make EFI_LOADER depend on BLK

If we make EFI_LOADER depend on BLK, doesn't that break all systems that need storage that isn't converted to device model today? Like the SATA breakage on Xilinx systems, just at a much bigger scale?

No it just means that these platforms need to move to BLK before they can use the EFI loader. Given the embryonic nature of this feature, that seems reasonable, and the impact would be small. It will also encourage conversion and keep the code cleaner.

Quoted 2 lines

No, it will simply make my life harder because I would have to sit down and vonvert every single board to BLK that I need EFI enabled.

Seems like as good a place as any to jump in, of the boards that you need EFI enabled on, what isn't converted today?

Quoted 85 lines · Tom Rini

Quoted 81 lines · Alexander Graf

Quoted 80 lines

Am 10.08.2016 um 15:16 schrieb Simon Glass <[email protected]>:

Hi Alex,

Quoted 61 lines · Alexander Graf

Quoted 60 lines · Simon Glass

+Tom

Hi Alex,

Quoted 23 lines · Alexander Graf

Quoted 23 lines · Simon Glass

Hi Alexander,

Quoted 18 lines · Alexander Graf

When using CONFIG_BLK, there were 2 issues:

1) The name we generate the device with has to match the name we set in efi_set_bootdev()

2) The device we pass into our block functions was wrong, we should not rediscover it but just use the already known pointer.

This patch fixes both issues.

Signed-off-by Alexander Graf <[email protected]>

---
cmd/bootefi.c | 23 ++++++++++++++++++-----
lib/efi_loader/efi_disk.c | 18 +++++++++++-------

2 files changed, 29 insertions(+), 12 deletions(-)

[...]

Quoted 13 lines

Quoted 4 lines

Quoted 4 lines

lib/efi_loader/efi_disk.c +0 -0

index c434c92..e00a747 100644

Quoted 8 lines

lib/efi_loader/efi_disk.c struct efi_disk_obj { · line 35

Quoted 6 lines

lib/efi_loader/efi_disk.c +2 -0

@@ -31,6 +31,8 @@ struct efi_disk_obj {
      struct efi_device_path_file_path *dp;
      /* Offset into disk for simple partitions */
      lbaint_t offset;
+       /* Internal block device */
+       const struct blk_desc *desc;

Rather than storing this, can you store the udevice?

Quoted 7 lines

I could, but then I would diverge between the CONFIG_BLK and non-CONFIG_BLK path again, which would turn the code into an #ifdef mess (read: hard to maintain), because the whole device creation path relies on struct blk_desc * today and doesn?t pass the udevice anywhere.

Do you feel strongly about this? To give you an idea how messy it gets, the diff is below.

Actually I'd like to make this feature depend on CONFIG_BLK. If we add new features that don't use driver model, and then use the legacy data structures such that converting to driver model becomes harder, we'll never be done.

I did mention this at the beginning and it seems to have come to pass.

Quoted 8 lines

Quoted 3 lines

In order of preference from my side:

1. Make EFI_LOADER depend on BLK

If we make EFI_LOADER depend on BLK, doesn't that break all systems that need storage that isn't converted to device model today? Like the SATA breakage on Xilinx systems, just at a much bigger scale?

No it just means that these platforms need to move to BLK before they can use the EFI loader. Given the embryonic nature of this feature, that seems reasonable, and the impact would be small. It will also encourage conversion and keep the code cleaner.

Quoted 5 lines

Quoted 2 lines

No, it will simply make my life harder because I would have to sit down and vonvert every single board to BLK that I need EFI enabled.

Seems like as good a place as any to jump in, of the boards that you need EFI enabled on, what isn't converted today?

I want to make EFI the default boot path in openSUSE, which means any board that anyone out there wants to run openSUSE on would be on the list. Anything that keeps them from running EFI on a random board is a road block that I?d rather not have if I can avoid it.

Again, I strongly object any dependency on BLK for EFI. If you want to push people to using CONFIG_BLK, make CONFIG_ARM depend on CONFIG_BLK.

Hi Alex,

Quoted 88 lines · Alexander Graf

Quoted 88 lines · Tom Rini

Quoted 83 lines · Alexander Graf

Quoted 72 lines

Am 10.08.2016 um 15:16 schrieb Simon Glass <[email protected]>:

Hi Alex,

Quoted 66 lines · Alexander Graf

Quoted 63 lines · Simon Glass

+Tom

Hi Alex,

Quoted 23 lines · Alexander Graf

Quoted 23 lines · Simon Glass

Hi Alexander,

Quoted 18 lines · Alexander Graf

When using CONFIG_BLK, there were 2 issues:

1) The name we generate the device with has to match the name we set in efi_set_bootdev()

2) The device we pass into our block functions was wrong, we should not rediscover it but just use the already known pointer.

This patch fixes both issues.

Signed-off-by Alexander Graf <[email protected]>

---
cmd/bootefi.c | 23 ++++++++++++++++++-----
lib/efi_loader/efi_disk.c | 18 +++++++++++-------

2 files changed, 29 insertions(+), 12 deletions(-)

[...]

Quoted 13 lines

Quoted 4 lines

Quoted 4 lines

lib/efi_loader/efi_disk.c +0 -0

index c434c92..e00a747 100644

Quoted 8 lines

lib/efi_loader/efi_disk.c struct efi_disk_obj { · line 35

Quoted 6 lines

lib/efi_loader/efi_disk.c +2 -0

@@ -31,6 +31,8 @@ struct efi_disk_obj {
      struct efi_device_path_file_path *dp;
      /* Offset into disk for simple partitions */
      lbaint_t offset;
+       /* Internal block device */
+       const struct blk_desc *desc;

Rather than storing this, can you store the udevice?

Quoted 7 lines

I could, but then I would diverge between the CONFIG_BLK and non-CONFIG_BLK path again, which would turn the code into an #ifdef mess (read: hard to maintain), because the whole device creation path relies on struct blk_desc * today and doesn?t pass the udevice anywhere.

Do you feel strongly about this? To give you an idea how messy it gets, the diff is below.

Actually I'd like to make this feature depend on CONFIG_BLK. If we add new features that don't use driver model, and then use the legacy data structures such that converting to driver model becomes harder, we'll never be done.

I did mention this at the beginning and it seems to have come to pass.

In order of preference from my side:

1. Make EFI_LOADER depend on BLK

Quoted 8 lines

Quoted 3 lines

If we make EFI_LOADER depend on BLK, doesn't that break all systems that need storage that isn't converted to device model today? Like the SATA breakage on Xilinx systems, just at a much bigger scale?

No it just means that these platforms need to move to BLK before they can use the EFI loader. Given the embryonic nature of this feature, that seems reasonable, and the impact would be small. It will also encourage conversion and keep the code cleaner.

No, it will simply make my life harder because I would have to sit down and vonvert every single board to BLK that I need EFI enabled.

Quoted 4 lines

Quoted 2 lines

Seems like as good a place as any to jump in, of the boards that you need EFI enabled on, what isn't converted today?

I want to make EFI the default boot path in openSUSE, which means any board that anyone out there wants to run openSUSE on would be on the list. Anything that keeps them from running EFI on a random board is a road block that I?d rather not have if I can avoid it.

Of course, I fully understand that. However as mentioned in this patch, you are creating future problems.

Can you address Tom's question? I take it that it boots on Raspberry Pi (which I'd like to try actually - are there instructions somewhere?). We can easily convert that over. Anything else?

Quoted 3 lines

Again, I strongly object any dependency on BLK for EFI. If you want to push people to using CONFIG_BLK, make CONFIG_ARM depend on CONFIG_BLK.

:-) That won't work. If we could wave a magic wand and convert everything in one day, we would.

Quoted 91 lines · Simon Glass

Hi Alex,

Quoted 88 lines · Alexander Graf

Quoted 88 lines · Tom Rini

Quoted 83 lines · Alexander Graf

Quoted 72 lines

Am 10.08.2016 um 15:16 schrieb Simon Glass <[email protected]>:

Hi Alex,

Quoted 66 lines · Alexander Graf

Quoted 63 lines · Simon Glass

+Tom

Hi Alex,

Quoted 23 lines · Alexander Graf

Quoted 23 lines · Simon Glass

Hi Alexander,

Quoted 18 lines · Alexander Graf

When using CONFIG_BLK, there were 2 issues:

1) The name we generate the device with has to match the name we set in efi_set_bootdev()

2) The device we pass into our block functions was wrong, we should not rediscover it but just use the already known pointer.

This patch fixes both issues.

Signed-off-by Alexander Graf <[email protected]>

---
cmd/bootefi.c | 23 ++++++++++++++++++-----
lib/efi_loader/efi_disk.c | 18 +++++++++++-------

2 files changed, 29 insertions(+), 12 deletions(-)

[...]

Quoted 13 lines

Quoted 4 lines

Quoted 4 lines

lib/efi_loader/efi_disk.c +0 -0

index c434c92..e00a747 100644

Quoted 8 lines

lib/efi_loader/efi_disk.c struct efi_disk_obj { · line 35

Quoted 6 lines

lib/efi_loader/efi_disk.c +2 -0

@@ -31,6 +31,8 @@ struct efi_disk_obj {
      struct efi_device_path_file_path *dp;
      /* Offset into disk for simple partitions */
      lbaint_t offset;
+       /* Internal block device */
+       const struct blk_desc *desc;

Rather than storing this, can you store the udevice?

Quoted 7 lines

I could, but then I would diverge between the CONFIG_BLK and non-CONFIG_BLK path again, which would turn the code into an #ifdef mess (read: hard to maintain), because the whole device creation path relies on struct blk_desc * today and doesn?t pass the udevice anywhere.

Do you feel strongly about this? To give you an idea how messy it gets, the diff is below.

Actually I'd like to make this feature depend on CONFIG_BLK. If we add new features that don't use driver model, and then use the legacy data structures such that converting to driver model becomes harder, we'll never be done.

I did mention this at the beginning and it seems to have come to pass.

In order of preference from my side:

1. Make EFI_LOADER depend on BLK

Quoted 8 lines

Quoted 3 lines

If we make EFI_LOADER depend on BLK, doesn't that break all systems that need storage that isn't converted to device model today? Like the SATA breakage on Xilinx systems, just at a much bigger scale?

No it just means that these platforms need to move to BLK before they can use the EFI loader. Given the embryonic nature of this feature, that seems reasonable, and the impact would be small. It will also encourage conversion and keep the code cleaner.

No, it will simply make my life harder because I would have to sit down and vonvert every single board to BLK that I need EFI enabled.

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

Seems like as good a place as any to jump in, of the boards that you need EFI enabled on, what isn't converted today?

I want to make EFI the default boot path in openSUSE, which means any board that anyone out there wants to run openSUSE on would be on the list. Anything that keeps them from running EFI on a random board is a road block that I?d rather not have if I can avoid it.

Of course, I fully understand that. However as mentioned in this patch, you are creating future problems.

I don't see how I am creating future problems, really. Passing a udevice* instead of the struct blk_desc* internally doesn't improve the code really at the end of the day.

Quoted 3 lines

Can you address Tom's question? I take it that it boots on Raspberry Pi (which I'd like to try actually - are there instructions somewhere?). We can easily convert that over. Anything else?

For a list of currently available "upstream" openSUSE images, see
https://build.opensuse.org/package/view_file/openSUSE:Factory:ARM/JeOS/pre_checkin.sh?expand=1

Every one of those is on the short-term list. Any other board that people want to run on is potentially on the mid-term to long-term list.

Am 19.08.2016 um 14:14 schrieb Tom Rini <[email protected]>:

Quoted 114 lines · Alexander Graf

Quoted 94 lines · Simon Glass

Hi Alex,

Quoted 89 lines · Alexander Graf

Quoted 88 lines · Tom Rini

Quoted 83 lines · Alexander Graf

Quoted 72 lines

Am 10.08.2016 um 15:16 schrieb Simon Glass <[email protected]>:

Hi Alex,

Quoted 66 lines

Quoted 64 lines · Alexander Graf

On 08/10/2016 02:56 PM, Simon Glass wrote:

+Tom

Hi Alex,

Quoted 23 lines · Alexander Graf

Quoted 23 lines · Simon Glass

Hi Alexander,

Quoted 18 lines · Alexander Graf

When using CONFIG_BLK, there were 2 issues:

1) The name we generate the device with has to match the name we set in efi_set_bootdev()

2) The device we pass into our block functions was wrong, we should not rediscover it but just use the already known pointer.

This patch fixes both issues.

Signed-off-by Alexander Graf <[email protected]>

---
cmd/bootefi.c | 23 ++++++++++++++++++-----
lib/efi_loader/efi_disk.c | 18 +++++++++++-------

2 files changed, 29 insertions(+), 12 deletions(-)

[...]

Quoted 13 lines

Quoted 4 lines

Quoted 4 lines

lib/efi_loader/efi_disk.c +0 -0

index c434c92..e00a747 100644

Quoted 8 lines

lib/efi_loader/efi_disk.c struct efi_disk_obj { · line 35

Quoted 6 lines

lib/efi_loader/efi_disk.c +2 -0

@@ -31,6 +31,8 @@ struct efi_disk_obj {
     struct efi_device_path_file_path *dp;
     /* Offset into disk for simple partitions */
     lbaint_t offset;
+       /* Internal block device */
+       const struct blk_desc *desc;

Rather than storing this, can you store the udevice?

Quoted 7 lines

I could, but then I would diverge between the CONFIG_BLK and non-CONFIG_BLK path again, which would turn the code into an #ifdef mess (read: hard to maintain), because the whole device creation path relies on struct blk_desc * today and doesn?t pass the udevice anywhere.

Do you feel strongly about this? To give you an idea how messy it gets, the diff is below.

Actually I'd like to make this feature depend on CONFIG_BLK. If we add new features that don't use driver model, and then use the legacy data structures such that converting to driver model becomes harder, we'll never be done.

I did mention this at the beginning and it seems to have come to pass.

In order of preference from my side:

1. Make EFI_LOADER depend on BLK

Quoted 8 lines

Quoted 3 lines

If we make EFI_LOADER depend on BLK, doesn't that break all systems that need storage that isn't converted to device model today? Like the SATA breakage on Xilinx systems, just at a much bigger scale?

No it just means that these platforms need to move to BLK before they can use the EFI loader. Given the embryonic nature of this feature, that seems reasonable, and the impact would be small. It will also encourage conversion and keep the code cleaner.

No, it will simply make my life harder because I would have to sit down and vonvert every single board to BLK that I need EFI enabled.

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

Seems like as good a place as any to jump in, of the boards that you need EFI enabled on, what isn't converted today?

I want to make EFI the default boot path in openSUSE, which means any board that anyone out there wants to run openSUSE on would be on the list. Anything that keeps them from running EFI on a random board is a road block that I?d rather not have if I can avoid it.

Of course, I fully understand that. However as mentioned in this patch, you are creating future problems.

I don't see how I am creating future problems, really. Passing a udevice* instead of the struct blk_desc* internally doesn't improve the code really at the end of the day.

Quoted 3 lines

Can you address Tom's question? I take it that it boots on Raspberry Pi (which I'd like to try actually - are there instructions somewhere?). We can easily convert that over. Anything else?

For a list of currently available "upstream" openSUSE images, see
https://build.opensuse.org/package/view_file/openSUSE:Factory:ARM/JeOS/pre_checkin.sh?expand=1

Quoted 2 lines

Every one of those is on the short-term list. Any other board that people want to run on is potentially on the mid-term to long-term list.

OK, that is a lot of boards and such. And yes, I see both of these features / projects as important to the long-term health of U-Boot.

So, Alex, when we've got storage converted fully to DM, you're willing to do further clean-ups to make it DM-better, yes? Thanks!

Quoted 118 lines

Am 19.08.2016 um 14:14 schrieb Tom Rini <[email protected]>:

Quoted 114 lines · Alexander Graf

Quoted 94 lines · Simon Glass

Hi Alex,

Quoted 89 lines · Alexander Graf

Quoted 88 lines · Tom Rini

Quoted 83 lines · Alexander Graf

Quoted 72 lines

Am 10.08.2016 um 15:16 schrieb Simon Glass <[email protected]>:

Hi Alex,

Quoted 66 lines

Quoted 64 lines · Alexander Graf

On 08/10/2016 02:56 PM, Simon Glass wrote:

+Tom

Hi Alex,

Quoted 23 lines · Alexander Graf

Quoted 23 lines · Simon Glass

Hi Alexander,

Quoted 18 lines · Alexander Graf

When using CONFIG_BLK, there were 2 issues:

1) The name we generate the device with has to match the
name we set in efi_set_bootdev()

2) The device we pass into our block functions was wrong,
we should not rediscover it but just use the already known
pointer.

This patch fixes both issues.

Signed-off-by: Alexander Graf <[email protected]>
---
cmd/bootefi.c | 23 ++++++++++++++++++-----
lib/efi_loader/efi_disk.c | 18 +++++++++++-------
2 files changed, 29 insertions(+), 12 deletions(-)

[...]

Quoted 13 lines

Quoted 4 lines

Quoted 4 lines

diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
index c434c92..e00a747 100644
--- a/lib/efi_loader/efi_disk.c
+++ b/lib/efi_loader/efi_disk.c

Quoted 8 lines

Quoted 6 lines

@@ -31,6 +31,8 @@ struct efi_disk_obj {
struct efi_device_path_file_path *dp;
/* Offset into disk for simple partitions */
lbaint_t offset;
+ /* Internal block device */
+ const struct blk_desc *desc;

Rather than storing this, can you store the udevice?

Quoted 7 lines

I could, but then I would diverge between the CONFIG_BLK and non-CONFIG_BLK path again, which would turn the code into an #ifdef mess (read: hard to maintain), because the whole device creation path relies on struct blk_desc * today and doesn?t pass the udevice anywhere.

Do you feel strongly about this? To give you an idea how messy it gets, the diff is below.

Actually I'd like to make this feature depend on CONFIG_BLK. If we add new features that don't use driver model, and then use the legacy data structures such that converting to driver model becomes harder, we'll never be done.

I did mention this at the beginning and it seems to have come to pass.

In order of preference from my side:

1. Make EFI_LOADER depend on BLK

Quoted 8 lines

Quoted 3 lines

If we make EFI_LOADER depend on BLK, doesn't that break all systems that need storage that isn't converted to device model today? Like the SATA breakage on Xilinx systems, just at a much bigger scale?

No it just means that these platforms need to move to BLK before they can use the EFI loader. Given the embryonic nature of this feature, that seems reasonable, and the impact would be small. It will also encourage conversion and keep the code cleaner.

No, it will simply make my life harder because I would have to sit down and vonvert every single board to BLK that I need EFI enabled.

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

Seems like as good a place as any to jump in, of the boards that you need EFI enabled on, what isn't converted today?

I want to make EFI the default boot path in openSUSE, which means any board that anyone out there wants to run openSUSE on would be on the list. Anything that keeps them from running EFI on a random board is a road block that I?d rather not have if I can avoid it.

Of course, I fully understand that. However as mentioned in this patch, you are creating future problems.

I don't see how I am creating future problems, really. Passing a udevice* instead of the struct blk_desc* internally doesn't improve the code really at the end of the day.

Quoted 3 lines

Can you address Tom's question? I take it that it boots on Raspberry Pi (which I'd like to try actually - are there instructions somewhere?). We can easily convert that over. Anything else?

For a list of currently available "upstream" openSUSE images, see
https://build.opensuse.org/package/view_file/openSUSE:Factory:ARM/JeOS/pre_checkin.sh?expand=1

Quoted 8 lines

Quoted 2 lines

Every one of those is on the short-term list. Any other board that people want to run on is potentially on the mid-term to long-term list.

OK, that is a lot of boards and such. And yes, I see both of these features / projects as important to the long-term health of U-Boot.

So, Alex, when we've got storage converted fully to DM, you're willing to do further clean-ups to make it DM-better, yes? Thanks!

I don't understand the question. Eventually I agree with Simon that we should have only a single object list and type, but with the state dm is in, I definitely will not require any of the efi code on dm.

Having said that, I am definitely making sure things work with dm and as soon as a non-dm subsystem drops, I will happily integrate the efi onject hierarchy into the dm one for that class of objects.

When using CONFIG_BLK, there were 2 issues:

1) The name we generate the device with has to match the name we set in efi_set_bootdev()

2) The device we pass into our block functions was wrong, we should not rediscover it but just use the already known pointer.

This patch fixes both issues.

Signed-off-by Alexander Graf <[email protected]>