Hi Tom,

On 12/13/22 14:37, Tom Rini wrote:
This reverts commits
51c5c28af59c ("cmd: pxe: use strdup to copy config"),
a5dacef7380e ("cmd: pxe: support INITRD and FDT selection with FIT") and
f723c2778cf8 ("cmd: pxe: reorder kernel treatment in label_boot").

As reported by Quentin Schulz, this introduces a regression with
previously generated and working extlinux.conf files.


For reference:
https://lore.kernel.org/u-boot/b7e891d1-d134-b489-eb2d-6125d4c7b...@theobroma-systems.com/ for my worry https://lore.kernel.org/u-boot/f0dd213c-4a34-926d-3f3b-f2ed49bb9...@linaro.org/ for confirmation from Neil it is breaking existing extlinux.conf

Thanks,
Quentin

Cc: Neil Armstrong <neil.armstr...@linaro.org>
Cc: Patrick Delaunay <patrick.delau...@foss.st.com>
Reported-by: Quentin Schulz <quentin.sch...@theobroma-systems.com>
Signed-off-by: Tom Rini <tr...@konsulko.com>
---
  boot/pxe_utils.c    | 69 ++++++++++++++++++++-------------------------
  doc/README.pxe      |  8 ------
  include/pxe_utils.h |  2 --
  3 files changed, 30 insertions(+), 49 deletions(-)

diff --git a/boot/pxe_utils.c b/boot/pxe_utils.c
index 272431381763..075a0f28309e 100644
--- a/boot/pxe_utils.c
+++ b/boot/pxe_utils.c
@@ -258,7 +258,6 @@ static struct pxe_label *label_create(void)
  static void label_destroy(struct pxe_label *label)
  {
        free(label->name);
-       free(label->kernel_label);
        free(label->kernel);
        free(label->config);
        free(label->append);
@@ -522,44 +521,28 @@ static int label_boot(struct pxe_context *ctx, struct 
pxe_label *label)
                return 1;
        }
- if (get_relfile_envaddr(ctx, label->kernel, "kernel_addr_r",
-                               NULL) < 0) {
-               printf("Skipping %s for failure retrieving kernel\n",
-                      label->name);
-               return 1;
-       }
-
-       kernel_addr = env_get("kernel_addr_r");
-       /* for FIT, append the configuration identifier */
-       if (label->config) {
-               int len = strlen(kernel_addr) + strlen(label->config) + 1;
-
-               fit_addr = malloc(len);
-               if (!fit_addr) {
-                       printf("malloc fail (FIT address)\n");
-                       return 1;
-               }
-               snprintf(fit_addr, len, "%s%s", kernel_addr, label->config);
-               kernel_addr = fit_addr;
-       }
-
-       /* For FIT, the label can be identical to kernel one */
-       if (label->initrd && !strcmp(label->kernel_label, label->initrd)) {
-               initrd_addr_str =  kernel_addr;
-       } else if (label->initrd) {
+       if (label->initrd) {
                ulong size;
+
                if (get_relfile_envaddr(ctx, label->initrd, "ramdisk_addr_r",
                                        &size) < 0) {
                        printf("Skipping %s for failure retrieving initrd\n",
                               label->name);
-                       goto cleanup;
+                       return 1;
                }
initrd_addr_str = env_get("ramdisk_addr_r");
                size = snprintf(initrd_str, sizeof(initrd_str), "%s:%lx",
                                initrd_addr_str, size);
                if (size >= sizeof(initrd_str))
-                       goto cleanup;
+                       return 1;
+       }
+
+       if (get_relfile_envaddr(ctx, label->kernel, "kernel_addr_r",
+                               NULL) < 0) {
+               printf("Skipping %s for failure retrieving kernel\n",
+                      label->name);
+               return 1;
        }
if (label->ipappend & 0x1) {
@@ -589,7 +572,7 @@ static int label_boot(struct pxe_context *ctx, struct 
pxe_label *label)
                               strlen(label->append ?: ""),
                               strlen(ip_str), strlen(mac_str),
                               sizeof(bootargs));
-                       goto cleanup;
+                       return 1;
                }
if (label->append)
@@ -604,6 +587,21 @@ static int label_boot(struct pxe_context *ctx, struct 
pxe_label *label)
                printf("append: %s\n", finalbootargs);
        }
+ kernel_addr = env_get("kernel_addr_r");
+
+       /* for FIT, append the configuration identifier */
+       if (label->config) {
+               int len = strlen(kernel_addr) + strlen(label->config) + 1;
+
+               fit_addr = malloc(len);
+               if (!fit_addr) {
+                       printf("malloc fail (FIT address)\n");
+                       return 1;
+               }
+               snprintf(fit_addr, len, "%s%s", kernel_addr, label->config);
+               kernel_addr = fit_addr;
+       }
+
        /*
         * fdt usage is optional:
         * It handles the following scenarios.
@@ -625,11 +623,8 @@ static int label_boot(struct pxe_context *ctx, struct 
pxe_label *label)
         */
        bootm_argv[3] = env_get("fdt_addr_r");
- /* For FIT, the label can be identical to kernel one */
-       if (label->fdt && !strcmp(label->kernel_label, label->fdt)) {
-               bootm_argv[3] = kernel_addr;
        /* if fdt label is defined then get fdt from server */
-       } else if (bootm_argv[3]) {
+       if (bootm_argv[3]) {
                char *fdtfile = NULL;
                char *fdtfilefree = NULL;
@@ -1170,19 +1165,15 @@ static int parse_label_kernel(char **c, struct pxe_label *label)
        if (err < 0)
                return err;
- /* copy the kernel label to compare with FDT / INITRD when FIT is used */
-       label->kernel_label = strdup(label->kernel);
-       if (!label->kernel_label)
-               return -ENOMEM;
-
        s = strstr(label->kernel, "#");
        if (!s)
                return 1;
- label->config = strdup(s);
+       label->config = malloc(strlen(s) + 1);
        if (!label->config)
                return -ENOMEM;
+ strcpy(label->config, s);
        *s = 0;
return 1;
diff --git a/doc/README.pxe b/doc/README.pxe
index 172201093d02..d14d2bdcc9b0 100644
--- a/doc/README.pxe
+++ b/doc/README.pxe
@@ -179,19 +179,11 @@ initrd <path>           - if this label is chosen, use 
tftp to retrieve the initrd
                      at <path>. it will be stored at the address indicated in
                      the initrd_addr_r environment variable, and that address
                      will be passed to bootm.
-                     For FIT image, the initrd can be provided with the same 
value than
-                     kernel, including configuration:
-                       <path>#<conf>[#<extra-conf[#...]]
-                     In this case, kernel_addr_r is passed to bootm.
fdt <path> - if this label is chosen, use tftp to retrieve the fdt blob
                      at <path>. it will be stored at the address indicated in
                      the fdt_addr_r environment variable, and that address will
                      be passed to bootm.
-                     For FIT image, the device tree can be provided with the 
same value
-                     than kernel, including configuration:
-                       <path>#<conf>[#<extra-conf[#...]]
-                     In this case, kernel_addr_r is passed to bootm.
devicetree <path> - if this label is chosen, use tftp to retrieve the fdt blob
                      at <path>. it will be stored at the address indicated in
diff --git a/include/pxe_utils.h b/include/pxe_utils.h
index 1e5e8424f53f..4a73b2aace34 100644
--- a/include/pxe_utils.h
+++ b/include/pxe_utils.h
@@ -28,7 +28,6 @@
   * Create these with the 'label_create' function given below.
   *
   * name - the name of the menu as given on the 'menu label' line.
- * kernel_label - the kernel label, including FIT config if present.
   * kernel - the path to the kernel file to use for this label.
   * append - kernel command line to use when booting this label
   * initrd - path to the initrd to use for this label.
@@ -41,7 +40,6 @@ struct pxe_label {
        char num[4];
        char *name;
        char *menu;
-       char *kernel_label;
        char *kernel;
        char *config;
        char *append;

Reply via email to