| Message ID | 20241208002121.31887-1-semen.protsenko@linaro.org |
|---|---|
| Headers | show |
| Series | lmb: Fix reserving the same region multiple times | expand |
Hi Sam, On Sun, 8 Dec 2024 at 02:21, Sam Protsenko <semen.protsenko@linaro.org> wrote: > > Since commit 1d9aa4a283da ("lmb: Fix the allocation of overlapping > memory areas with !LMB_NONE") the lmb_add_region_flags() returns -1 when > the caller tries to add the already existing region with !LMB_NONE > flags (it was returning 0 before that patch). That causes > boot_fdt_reserve_region() function to print erroneous error messages > when it's called consequently more than one time. > > Make lmb_add_region_flags() return -EEXIST when the already added region > with !LMB_NONE flags is being added, and then check that error code in > boot_fdt_reserve_region() to avoid printing the misleading error > messages. Thanks for cathing this. It's a print only error, but still misleading > > I didn't want to change lmb_add_region_flags() behavior back to always > returning 0 on attempts to add already existing regions with !LMB_NONE > flags, as the unit tests in test/lib/lmb.c would break, and also > commit 1d9aa4a283da ("lmb: Fix the allocation of overlapping memory > areas with !LMB_NONE") specifically states that behavior is expected and > required for efi_allocate_pages() calls with type=EFI_ALLOCATE_ADDRESS. TBH it's not only the EFI code that needs this. I don't think returning 0 for a region that's already allocated and the flags that happen to match is a good idea to begin with. The code should be as precise as possible. Thanks /Ilias > > All unit tests pass in sandbox U-Boot, with a small test modification in > [PATCH 2/6]. I also made a bit of LMB cleanups in this series, while at > it. Only [PATCH 6/6] introduces an actual functional change. > > Sam Protsenko (6): > lmb: Fix flags data type in lmb_add_region_flags() > lmb: Return -EEXIST in lmb_add_region_flags() if region already added > lmb: Make const flag_str[] in lmb_print_region_flags() more const > lmb: Improve coding style > lmb: Improve kernel-doc comments > boot: fdt: Handle already reserved memory in boot_fdt_reserve_region() > > boot/image-fdt.c | 2 +- > include/lmb.h | 125 ++++++++++++++++++++++++++++------------------- > lib/lmb.c | 105 +++++++++++++-------------------------- > test/lib/lmb.c | 2 +- > 4 files changed, 109 insertions(+), 125 deletions(-) > > -- > 2.39.5 >
Since commit 1d9aa4a283da ("lmb: Fix the allocation of overlapping memory areas with !LMB_NONE") the lmb_add_region_flags() returns -1 when the caller tries to add the already existing region with !LMB_NONE flags (it was returning 0 before that patch). That causes boot_fdt_reserve_region() function to print erroneous error messages when it's called consequently more than one time. Make lmb_add_region_flags() return -EEXIST when the already added region with !LMB_NONE flags is being added, and then check that error code in boot_fdt_reserve_region() to avoid printing the misleading error messages. I didn't want to change lmb_add_region_flags() behavior back to always returning 0 on attempts to add already existing regions with !LMB_NONE flags, as the unit tests in test/lib/lmb.c would break, and also commit 1d9aa4a283da ("lmb: Fix the allocation of overlapping memory areas with !LMB_NONE") specifically states that behavior is expected and required for efi_allocate_pages() calls with type=EFI_ALLOCATE_ADDRESS. All unit tests pass in sandbox U-Boot, with a small test modification in [PATCH 2/6]. I also made a bit of LMB cleanups in this series, while at it. Only [PATCH 6/6] introduces an actual functional change. Sam Protsenko (6): lmb: Fix flags data type in lmb_add_region_flags() lmb: Return -EEXIST in lmb_add_region_flags() if region already added lmb: Make const flag_str[] in lmb_print_region_flags() more const lmb: Improve coding style lmb: Improve kernel-doc comments boot: fdt: Handle already reserved memory in boot_fdt_reserve_region() boot/image-fdt.c | 2 +- include/lmb.h | 125 ++++++++++++++++++++++++++++------------------- lib/lmb.c | 105 +++++++++++++-------------------------- test/lib/lmb.c | 2 +- 4 files changed, 109 insertions(+), 125 deletions(-)