On Sun, Oct 26, 2025 at 01:41:30PM -0400, Pasha Tatashin wrote:
On Sun, Oct 26, 2025 at 12:29 PM Mike Rapoport rppt@kernel.org wrote:
@@ -2462,12 +2463,14 @@ static int __init prepare_kho_fdt(void)
err |= fdt_begin_node(fdt, ""); err |= fdt_property_string(fdt, "compatible", MEMBLOCK_KHO_NODE_COMPATIBLE);
for (i = 0; i < reserved_mem_count; i++) {
for (i = 0; !err && i < reserved_mem_count; i++) { struct reserve_mem_table *map = &reserved_mem_table[i]; struct page *page = phys_to_page(map->start); unsigned int nr_pages = map->size >> PAGE_SHIFT;
err |= kho_preserve_pages(page, nr_pages);
err = kho_preserve_pages(page, nr_pages);if (err)break;Please
goto err_unpreserve;While we can do that, we loose some symmetry of not performing fdt_end_node() and fdt_finish() if fdt lib ever adds some debugging facility to make sure that open nodes/trees are properly clodes, this is going to flag that. I prefer my current implementation.
Why do we care about fdt that we are never going to use and that's freed a few lines below?
err |= fdt_begin_node(fdt, map->name); err |= fdt_property_string(fdt, "compatible", RESERVE_MEM_KHO_NODE_COMPATIBLE); err |= fdt_property(fdt, "start", &map->start, sizeof(map->start));if (err) goto err_unpreserve;and drop !err from the loop condition.
That is going to miss one 'nr_preserved++' . We cannot do that, we could move it to the beginning of the loop, but I prefer keeping err right in the condition.
I very much dislike the error handling after this patch. From a single
if (err) put_page()
it grew into a complex beast with special variables just for the sake of it.
What I'd like to see is something like
err_unpreserve_fdt: kho_unpreserve_folio(page_folio(fdt_page)); err_unpreserve_mems: for (unsigned int i = 0; i < nr_preserved; i++) { /* unpreserve mem[i] */ } err_free_fdt: put_page(fdt_page); return err;