| Message ID | 20221213133748.1283705-1-trini@konsulko.com |
|---|---|
| State | Rejected |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | Revert "cmd: pxe: use strdup to copy config" et al | expand |
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-6125d4c7b6c6@theobroma-systems.com/ for my worry https://lore.kernel.org/u-boot/f0dd213c-4a34-926d-3f3b-f2ed49bb92c3@linaro.org/ for confirmation from Neil it is breaking existing extlinux.conf Thanks, Quentin > Cc: Neil Armstrong <neil.armstrong@linaro.org> > Cc: Patrick Delaunay <patrick.delaunay@foss.st.com> > Reported-by: Quentin Schulz <quentin.schulz@theobroma-systems.com> > Signed-off-by: Tom Rini <trini@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;
On 13/12/2022 14:45, Quentin Schulz wrote: > 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. >> The situation is much complex than that: - Since the commit d5ba6188dfbf ("cmd: pxe_utils: Check fdtcontroladdr in label_boot"),\ all the extlinux.conf with only "KERNEL /fitImage" don't work anymore. Those are generated by Poky/OE WIC bootimg-partition bootloader partition generator. - With the changeset introduced by Patrick, this doesn't permit booting those images, but it solves the issue by explicitly specifying we want to get the dtb from the fitImage with "FDT /fitImage", which is smart. - If we start generating extlinux.conf with "FDT /fitImage", those won't boot on previous u-boot versions (as I reported in the second link shared by Quentin). But honestly, as of today we need to patch by reverting d5ba6188dfbf to use extlinux.conf with fitImage, so I'll vote to keep this solution and go forward and implement the change in the Poky/OE bootimg-partition bootloader partition generator with perhaps a check on the u-boot version. Neil > > For reference: > https://lore.kernel.org/u-boot/b7e891d1-d134-b489-eb2d-6125d4c7b6c6@theobroma-systems.com/ for my worry > https://lore.kernel.org/u-boot/f0dd213c-4a34-926d-3f3b-f2ed49bb92c3@linaro.org/ for confirmation from Neil it is breaking existing extlinux.conf > > Thanks, > Quentin > >> Cc: Neil Armstrong <neil.armstrong@linaro.org> >> Cc: Patrick Delaunay <patrick.delaunay@foss.st.com> >> Reported-by: Quentin Schulz <quentin.schulz@theobroma-systems.com> >> Signed-off-by: Tom Rini <trini@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;
On Tue, Dec 13, 2022 at 03:14:43PM +0100, Neil Armstrong wrote: > On 13/12/2022 14:45, Quentin Schulz wrote: > > 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. > > > > > The situation is much complex than that: > - Since the commit d5ba6188dfbf ("cmd: pxe_utils: Check fdtcontroladdr in label_boot"),\ > all the extlinux.conf with only "KERNEL /fitImage" don't work anymore. > Those are generated by Poky/OE WIC bootimg-partition bootloader partition generator. > - With the changeset introduced by Patrick, this doesn't permit booting > those images, but it solves the issue by explicitly specifying we want to > get the dtb from the fitImage with "FDT /fitImage", which is smart. > - If we start generating extlinux.conf with "FDT /fitImage", those won't > boot on previous u-boot versions (as I reported in the second link shared by Quentin). > > But honestly, as of today we need to patch by reverting d5ba6188dfbf to > use extlinux.conf with fitImage, so I'll vote to keep this solution and > go forward and implement the change in the Poky/OE bootimg-partition > bootloader partition generator with perhaps a check on the u-boot version. So, I'll post a revert for d5ba6188dfbf in a moment then, which of these should I use for Link's > > For reference: > > https://lore.kernel.org/u-boot/b7e891d1-d134-b489-eb2d-6125d4c7b6c6@theobroma-systems.com/ for my worry > > https://lore.kernel.org/u-boot/f0dd213c-4a34-926d-3f3b-f2ed49bb92c3@linaro.org/ for confirmation from Neil it is breaking existing extlinux.conf Or something else? And then to be clear, you're saying the 3 changes here don't break anything really?
On 13/12/2022 15:22, Tom Rini wrote: > On Tue, Dec 13, 2022 at 03:14:43PM +0100, Neil Armstrong wrote: >> On 13/12/2022 14:45, Quentin Schulz wrote: >>> 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. >>>> >> >> The situation is much complex than that: >> - Since the commit d5ba6188dfbf ("cmd: pxe_utils: Check fdtcontroladdr in label_boot"),\ >> all the extlinux.conf with only "KERNEL /fitImage" don't work anymore. >> Those are generated by Poky/OE WIC bootimg-partition bootloader partition generator. >> - With the changeset introduced by Patrick, this doesn't permit booting >> those images, but it solves the issue by explicitly specifying we want to >> get the dtb from the fitImage with "FDT /fitImage", which is smart. >> - If we start generating extlinux.conf with "FDT /fitImage", those won't >> boot on previous u-boot versions (as I reported in the second link shared by Quentin). >> >> But honestly, as of today we need to patch by reverting d5ba6188dfbf to >> use extlinux.conf with fitImage, so I'll vote to keep this solution and >> go forward and implement the change in the Poky/OE bootimg-partition >> bootloader partition generator with perhaps a check on the u-boot version. > > So, I'll post a revert for d5ba6188dfbf in a moment then, which of these > should I use for Link's > >>> For reference: >>> https://lore.kernel.org/u-boot/b7e891d1-d134-b489-eb2d-6125d4c7b6c6@theobroma-systems.com/ for my worry >>> https://lore.kernel.org/u-boot/f0dd213c-4a34-926d-3f3b-f2ed49bb92c3@linaro.org/ for confirmation from Neil it is breaking existing extlinux.conf > > Or something else? And then to be clear, you're saying the 3 changes > here don't break anything really? > Yea no they will only break old u-boot with future extlinux.conf files with "FDT /fitImage", but without booting with fitImage as KERNEL is already broken since d5ba6188dfbf anyway. Neil
Hi, On 12/13/22 15:14, Neil Armstrong wrote: > On 13/12/2022 14:45, Quentin Schulz wrote: >> 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. >>> > > The situation is much complex than that: > - Since the commit d5ba6188dfbf ("cmd: pxe_utils: Check fdtcontroladdr > in label_boot"),\ > all the extlinux.conf with only "KERNEL /fitImage" don't work anymore. > Those are generated by Poky/OE WIC bootimg-partition bootloader > partition generator. > - With the changeset introduced by Patrick, this doesn't permit booting > those images, but it solves the issue by explicitly specifying we want to > get the dtb from the fitImage with "FDT /fitImage", which is smart. > - If we start generating extlinux.conf with "FDT /fitImage", those won't > boot on previous u-boot versions (as I reported in the second link > shared by Quentin). > > But honestly, as of today we need to patch by reverting d5ba6188dfbf to > use extlinux.conf with fitImage, so I'll vote to keep this solution and > go forward and implement the change in the Poky/OE bootimg-partition > bootloader partition generator with perhaps a check on the u-boot version. > Yocto could do this, though I'm not entirely sure they'll like it either. But you have plenty other distributions, and I imagine it is perfectly possible it does not have knowledge of the bootloader being used to boot the distribution. I'm voting against because currently I see d5ba6188dfbf as a regression and distros are free to handle them how they wish, by e.g. reverting this patch only (which I have done for our boards for example). I'd expect Yocto to have a local patch reverting this in their next release, where U-Boot would be bumped to something containing this commit, for example. By merging the patchset, we limit other possible implementations by adding one more logic we should then maintain for backward compatibility. Cheers, Quentin
On 13/12/2022 15:31, Quentin Schulz wrote: > Hi, > > On 12/13/22 15:14, Neil Armstrong wrote: >> On 13/12/2022 14:45, Quentin Schulz wrote: >>> 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. >>>> >> >> The situation is much complex than that: >> - Since the commit d5ba6188dfbf ("cmd: pxe_utils: Check fdtcontroladdr in label_boot"),\ >> all the extlinux.conf with only "KERNEL /fitImage" don't work anymore. >> Those are generated by Poky/OE WIC bootimg-partition bootloader partition generator. >> - With the changeset introduced by Patrick, this doesn't permit booting >> those images, but it solves the issue by explicitly specifying we want to >> get the dtb from the fitImage with "FDT /fitImage", which is smart. >> - If we start generating extlinux.conf with "FDT /fitImage", those won't >> boot on previous u-boot versions (as I reported in the second link shared by Quentin). >> >> But honestly, as of today we need to patch by reverting d5ba6188dfbf to >> use extlinux.conf with fitImage, so I'll vote to keep this solution and >> go forward and implement the change in the Poky/OE bootimg-partition >> bootloader partition generator with perhaps a check on the u-boot version. >> > > Yocto could do this, though I'm not entirely sure they'll like it either. But you have plenty other distributions, and I imagine it is perfectly possible it does not have knowledge of the bootloader being used to boot the distribution. > > I'm voting against because currently I see d5ba6188dfbf as a regression and distros are free to handle them how they wish, by e.g. reverting this patch only (which I have done for our boards for example). I'd expect Yocto to have a local patch reverting this in their next release, where U-Boot would be bumped to something containing this commit, for example. > > By merging the patchset, we limit other possible implementations by adding one more logic we should then maintain for backward compatibility. Ack, you're right. Neil > > Cheers, > Quentin
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;
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. Cc: Neil Armstrong <neil.armstrong@linaro.org> Cc: Patrick Delaunay <patrick.delaunay@foss.st.com> Reported-by: Quentin Schulz <quentin.schulz@theobroma-systems.com> Signed-off-by: Tom Rini <trini@konsulko.com> --- boot/pxe_utils.c | 69 ++++++++++++++++++++------------------------- doc/README.pxe | 8 ------ include/pxe_utils.h | 2 -- 3 files changed, 30 insertions(+), 49 deletions(-)