[PATCH 1/3] efi: Add debugging to efi_set_bootdev()
The operation of this function can be confusing. Add some debugging so we can see what it is doing and when it is called.
Also drop the preprocessor usage.
Signed-off-by: Simon Glass sjg@chromium.org ---
cmd/bootefi.c | 32 +++++++++++++++++++++----------- 1 file changed, 21 insertions(+), 11 deletions(-)
diff --git a/cmd/bootefi.c b/cmd/bootefi.c index 3a8b2b60618..d8685f0e878 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -65,6 +65,9 @@ void efi_set_bootdev(const char *dev, const char *devnr, const char *path, struct efi_device_path *device, *image; efi_status_t ret;
+ log_debug("dev=%s, devnr=%s, path=%s, buffer=%p, size=%zx\n", dev, + devnr, path, buffer, buffer_size); + /* Forget overwritten image */ if (buffer + buffer_size >= image_addr && image_addr + image_size >= buffer) @@ -72,18 +75,19 @@ void efi_set_bootdev(const char *dev, const char *devnr, const char *path,
/* Remember only PE-COFF and FIT images */ if (efi_check_pe(buffer, buffer_size, NULL) != EFI_SUCCESS) { -#ifdef CONFIG_FIT - if (fit_check_format(buffer, IMAGE_SIZE_INVAL)) + if (IS_ENABLED(CONFIG_FIT) && + !fit_check_format(buffer, IMAGE_SIZE_INVAL)) { + /* + * FIT images of type EFI_OS are started via command + * bootm. We should not use their boot device with the + * bootefi command. + */ + buffer = 0; + buffer_size = 0; + } else { + log_debug("- invalid image\n"); return; - /* - * FIT images of type EFI_OS are started via command bootm. - * We should not use their boot device with the bootefi command. - */ - buffer = 0; - buffer_size = 0; -#else - return; -#endif + } }
/* efi_set_bootdev() is typically called repeatedly, recover memory */ @@ -103,7 +107,11 @@ void efi_set_bootdev(const char *dev, const char *devnr, const char *path, efi_free_pool(image_tmp); } bootefi_image_path = image; + log_debug("- recorded device %ls\n", efi_dp_str(device)); + if (image) + log_debug("- and image %ls\n", efi_dp_str(image)); } else { + log_debug("- efi_dp_from_name() failed, err=%lx\n", ret); efi_clear_bootdev(); } } @@ -451,6 +459,7 @@ efi_status_t efi_run_image(void *source_buffer, efi_uintn_t source_size) u16 *load_options;
if (!bootefi_device_path || !bootefi_image_path) { + log_debug("Not loaded from disk\n"); /* * Special case for efi payload not loaded from disk, * such as 'bootefi hello' or for example payload @@ -476,6 +485,7 @@ efi_status_t efi_run_image(void *source_buffer, efi_uintn_t source_size) file_path = efi_dp_append(bootefi_device_path, bootefi_image_path); msg_path = bootefi_image_path; + log_debug("Loaded from disk\n"); }
log_info("Booting %pD\n", msg_path);
Use this function rather than following the pointers, since it is there for this purpose.
Add the uclass name to the debug call at the end of dp_fill() since it is quite useful.
Signed-off-by: Simon Glass sjg@chromium.org ---
lib/efi_loader/efi_device_path.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c index c61f4859330..a838a32b810 100644 --- a/lib/efi_loader/efi_device_path.c +++ b/lib/efi_loader/efi_device_path.c @@ -494,7 +494,7 @@ __maybe_unused static unsigned int dp_size(struct udevice *dev) if (!dev || !dev->driver) return sizeof(ROOT);
- switch (dev->driver->id) { + switch (device_get_uclass_id(dev)) { case UCLASS_ROOT: case UCLASS_SIMPLE_BUS: /* stop traversing parents at this point: */ @@ -579,7 +579,7 @@ __maybe_unused static void *dp_fill(void *buf, struct udevice *dev) if (!dev || !dev->driver) return buf;
- switch (dev->driver->id) { + switch (device_get_uclass_id(dev)) { case UCLASS_ROOT: case UCLASS_SIMPLE_BUS: { /* stop traversing parents at this point: */ @@ -759,9 +759,8 @@ __maybe_unused static void *dp_fill(void *buf, struct udevice *dev) return &udp[1]; } default: - debug("%s(%u) %s: unhandled device class: %s (%u)\n", - __FILE__, __LINE__, __func__, - dev->name, dev->driver->id); + log_debug("unhandled device class: %s (%u:%s)\n", dev->name, + device_get_uclass_id(dev), dev_get_uclass_name(dev)); return dp_fill(buf, dev->parent); } }
On 1/22/22 02:16, Simon Glass wrote:
Use this function rather than following the pointers, since it is there for this purpose.
Add the uclass name to the debug call at the end of dp_fill() since it is quite useful.
Signed-off-by: Simon Glass sjg@chromium.org
lib/efi_loader/efi_device_path.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c index c61f4859330..a838a32b810 100644 --- a/lib/efi_loader/efi_device_path.c +++ b/lib/efi_loader/efi_device_path.c @@ -494,7 +494,7 @@ __maybe_unused static unsigned int dp_size(struct udevice *dev) if (!dev || !dev->driver) return sizeof(ROOT);
- switch (dev->driver->id) {
- switch (device_get_uclass_id(dev)) { case UCLASS_ROOT: case UCLASS_SIMPLE_BUS: /* stop traversing parents at this point: */
@@ -579,7 +579,7 @@ __maybe_unused static void *dp_fill(void *buf, struct udevice *dev) if (!dev || !dev->driver) return buf;
- switch (dev->driver->id) {
- switch (device_get_uclass_id(dev)) { case UCLASS_ROOT: case UCLASS_SIMPLE_BUS: { /* stop traversing parents at this point: */
@@ -759,9 +759,8 @@ __maybe_unused static void *dp_fill(void *buf, struct udevice *dev) return &udp[1]; } default:
debug("%s(%u) %s: unhandled device class: %s (%u)\n",__FILE__, __LINE__, __func__,dev->name, dev->driver->id);
log_debug("unhandled device class: %s (%u:%s)\n", dev->name,device_get_uclass_id(dev), dev_get_uclass_name(dev));
The uclass id is hidden in enum uclass_id. I can't grep for it. The uclass name seems enough.
Best regards
Heinrich
return dp_fill(buf, dev->parent);} }
When we have the block descriptor we can simply access the device. Drop the unnecessary function call.
Signed-off-by: Simon Glass sjg@chromium.org ---
lib/efi_loader/efi_device_path.c | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-)
diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c index a838a32b810..f415741d528 100644 --- a/lib/efi_loader/efi_device_path.c +++ b/lib/efi_loader/efi_device_path.c @@ -768,13 +768,8 @@ __maybe_unused static void *dp_fill(void *buf, struct udevice *dev) static unsigned dp_part_size(struct blk_desc *desc, int part) { unsigned dpsize; - struct udevice *dev; - int ret; + struct udevice *dev = desc->bdev;
- ret = blk_find_device(desc->if_type, desc->devnum, &dev); - - if (ret) - dev = desc->bdev->parent; dpsize = dp_size(dev);
if (part == 0) /* the actual disk, not a partition */ @@ -865,13 +860,8 @@ static void *dp_part_node(void *buf, struct blk_desc *desc, int part) */ static void *dp_part_fill(void *buf, struct blk_desc *desc, int part) { - struct udevice *dev; - int ret; - - ret = blk_find_device(desc->if_type, desc->devnum, &dev); + struct udevice *dev = desc->bdev;
- if (ret) - dev = desc->bdev->parent; buf = dp_fill(buf, dev);
if (part == 0) /* the actual disk, not a partition */
On 1/22/22 02:16, Simon Glass wrote:
When we have the block descriptor we can simply access the device. Drop the unnecessary function call.
Signed-off-by: Simon Glass sjg@chromium.org
lib/efi_loader/efi_device_path.c | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-)
diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c index a838a32b810..f415741d528 100644 --- a/lib/efi_loader/efi_device_path.c +++ b/lib/efi_loader/efi_device_path.c @@ -768,13 +768,8 @@ __maybe_unused static void *dp_fill(void *buf, struct udevice *dev) static unsigned dp_part_size(struct blk_desc *desc, int part) { unsigned dpsize;
- struct udevice *dev;
- int ret;
- struct udevice *dev = desc->bdev;
drivers/block/blk_legacy.c still exists but it does not set bdev. CONFIG_EFI_LOADER does not require CONFIG_BLK.
Are all non-DM block devices eliminated by now?
Best regards
Heinrich
ret = blk_find_device(desc->if_type, desc->devnum, &dev);
if (ret)
dev = desc->bdev->parent;dpsize = dp_size(dev);
if (part == 0) /* the actual disk, not a partition */
@@ -865,13 +860,8 @@ static void *dp_part_node(void *buf, struct blk_desc *desc, int part) */ static void *dp_part_fill(void *buf, struct blk_desc *desc, int part) {
- struct udevice *dev;
- int ret;
- ret = blk_find_device(desc->if_type, desc->devnum, &dev);
- struct udevice *dev = desc->bdev;
if (ret)
dev = desc->bdev->parent;buf = dp_fill(buf, dev);
if (part == 0) /* the actual disk, not a partition */
Hi Heinrich,
On Mon, 24 Jan 2022 at 21:21, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 1/22/22 02:16, Simon Glass wrote:
When we have the block descriptor we can simply access the device. Drop the unnecessary function call.
Signed-off-by: Simon Glass sjg@chromium.org
lib/efi_loader/efi_device_path.c | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-)
diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c index a838a32b810..f415741d528 100644 --- a/lib/efi_loader/efi_device_path.c +++ b/lib/efi_loader/efi_device_path.c @@ -768,13 +768,8 @@ __maybe_unused static void *dp_fill(void *buf, struct udevice *dev) static unsigned dp_part_size(struct blk_desc *desc, int part) { unsigned dpsize;
struct udevice *dev;int ret;
struct udevice *dev = desc->bdev;drivers/block/blk_legacy.c still exists but it does not set bdev. CONFIG_EFI_LOADER does not require CONFIG_BLK.
It depends on BLK. Where are you looking?
Are all non-DM block devices eliminated by now?
No, but they are all way out of date and we should probably remove them.
Regards, Simon
On 1/22/22 02:16, Simon Glass wrote:
The operation of this function can be confusing. Add some debugging so we can see what it is doing and when it is called.
Also drop the preprocessor usage.
Signed-off-by: Simon Glass sjg@chromium.org
cmd/bootefi.c | 32 +++++++++++++++++++++----------- 1 file changed, 21 insertions(+), 11 deletions(-)
diff --git a/cmd/bootefi.c b/cmd/bootefi.c index 3a8b2b60618..d8685f0e878 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -65,6 +65,9 @@ void efi_set_bootdev(const char *dev, const char *devnr, const char *path, struct efi_device_path *device, *image; efi_status_t ret;
- log_debug("dev=%s, devnr=%s, path=%s, buffer=%p, size=%zx\n", dev,
devnr, path, buffer, buffer_size);- /* Forget overwritten image */ if (buffer + buffer_size >= image_addr && image_addr + image_size >= buffer)
@@ -72,18 +75,19 @@ void efi_set_bootdev(const char *dev, const char *devnr, const char *path,
/* Remember only PE-COFF and FIT images */ if (efi_check_pe(buffer, buffer_size, NULL) != EFI_SUCCESS) { -#ifdef CONFIG_FIT
if (fit_check_format(buffer, IMAGE_SIZE_INVAL))
if (IS_ENABLED(CONFIG_FIT) &&!fit_check_format(buffer, IMAGE_SIZE_INVAL)) {
This looks ok.
/** FIT images of type EFI_OS are started via command* bootm. We should not use their boot device with the* bootefi command.*/buffer = 0;buffer_size = 0;} else {log_debug("- invalid image\n");
This function is called by the 'load' command. When loading a perfectly valid device-tree or initial RAM disk the message would confuse me.
What is that '- ' good for?
Best regards
Heinrich
return;
/** FIT images of type EFI_OS are started via command bootm.* We should not use their boot device with the bootefi command.*/buffer = 0;buffer_size = 0;-#else
return;-#endif
}}
/* efi_set_bootdev() is typically called repeatedly, recover memory */
@@ -103,7 +107,11 @@ void efi_set_bootdev(const char *dev, const char *devnr, const char *path, efi_free_pool(image_tmp); } bootefi_image_path = image;
log_debug("- recorded device %ls\n", efi_dp_str(device));if (image)log_debug("- and image %ls\n", efi_dp_str(image));log_debug("- efi_dp_from_name() failed, err=%lx\n", ret);@@ -451,6 +459,7 @@ efi_status_t efi_run_image(void *source_buffer, efi_uintn_t source_size) u16 *load_options;
if (!bootefi_device_path || !bootefi_image_path) {
log_debug("Not loaded from disk\n");
- Special case for efi payload not loaded from disk,
- such as 'bootefi hello' or for example payload
@@ -476,6 +485,7 @@ efi_status_t efi_run_image(void *source_buffer, efi_uintn_t source_size) file_path = efi_dp_append(bootefi_device_path, bootefi_image_path); msg_path = bootefi_image_path;
log_debug("Loaded from disk\n");}
log_info("Booting %pD\n", msg_path);
Hi Heinrich,
On Mon, 24 Jan 2022 at 20:54, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 1/22/22 02:16, Simon Glass wrote:
The operation of this function can be confusing. Add some debugging so we can see what it is doing and when it is called.
Also drop the preprocessor usage.
Signed-off-by: Simon Glass sjg@chromium.org
cmd/bootefi.c | 32 +++++++++++++++++++++----------- 1 file changed, 21 insertions(+), 11 deletions(-)
diff --git a/cmd/bootefi.c b/cmd/bootefi.c index 3a8b2b60618..d8685f0e878 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -65,6 +65,9 @@ void efi_set_bootdev(const char *dev, const char *devnr, const char *path, struct efi_device_path *device, *image; efi_status_t ret;
log_debug("dev=%s, devnr=%s, path=%s, buffer=%p, size=%zx\n", dev,devnr, path, buffer, buffer_size);/* Forget overwritten image */ if (buffer + buffer_size >= image_addr && image_addr + image_size >= buffer)@@ -72,18 +75,19 @@ void efi_set_bootdev(const char *dev, const char *devnr, const char *path,
/* Remember only PE-COFF and FIT images */ if (efi_check_pe(buffer, buffer_size, NULL) != EFI_SUCCESS) {-#ifdef CONFIG_FIT
if (fit_check_format(buffer, IMAGE_SIZE_INVAL))
if (IS_ENABLED(CONFIG_FIT) &&!fit_check_format(buffer, IMAGE_SIZE_INVAL)) {This looks ok.
/** FIT images of type EFI_OS are started via command* bootm. We should not use their boot device with the* bootefi command.*/buffer = 0;buffer_size = 0;} else {log_debug("- invalid image\n");This function is called by the 'load' command. When loading a perfectly valid device-tree or initial RAM disk the message would confuse me.
OK I will reword it.
What is that '- ' good for?
It shows that this message is related to the one at the top of the function.
[..]
Regards, Simon
participants (2)
-
Heinrich Schuchardt -
Simon Glass