On Tue, Apr 22, 2025 at 16:31:19 +0300, Mike Rapoport <rppt@xxxxxxxxxx> wrote:
> On Thu, Apr 10, 2025 at 10:37:43PM -0700, Changyuan Lyu wrote:
> > [...]
> > +static struct notifier_block reserve_mem_kho_nb = {
> > + .notifier_call = reserve_mem_kho_notifier,
> > +};
> > +
> > +static void __init prepare_kho_fdt(void)
> > +{
> > + int err = 0, i;
> > + void *fdt;
> > +
> > + if (!reserved_mem_count)
> > + return;
>
> It's better to have this check in reserve_mem_init() before registering kho
> notifier.
Sounds good!
> > +
> > + kho_fdt = alloc_page(GFP_KERNEL);
> > + if (!kho_fdt) {
> > + kho_fdt = ERR_PTR(-ENOMEM);
>
> Do we really care about having errno in kho_fdt? I think NULL would work
> just fine.
I was originally using ERR_PTR(-ENOMEM) and NULL to differentiate the following
2 cases:
1. prepare_kho_fdt() failed,
2. reserved_mem_count == 0, so no memblock FDT was created.
Based on the suggestion above, since now we only register the notifier when
reserved_mem_count == 0, case 2 shall never happen. So NULL is enough.
> > + return;
>
> And actually, it makes sense to me to return -ENOMEM here and let
> reserve_mem_init() bail out before registering notifier if fdt preparation
> failed.
>
> That will save the checks in reserve_mem_kho_finalize() because it would be
> called only if we have reserve_mem areas and fdt is ready.
>
Sounds good!
> > + }
> > +
> > + fdt = page_to_virt(kho_fdt);
> > +
> > + err |= fdt_create(fdt, PAGE_SIZE);
> > + err |= fdt_finish_reservemap(fdt);
> > +
> > + err |= fdt_begin_node(fdt, "");
> > + err |= fdt_property_string(fdt, "compatible", MEMBLOCK_KHO_NODE_COMPATIBLE);
> > + for (i = 0; i < reserved_mem_count; i++) {
> > + struct reserve_mem_table *map = &reserved_mem_table[i];
> > +
> > + 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));
> > + err |= fdt_property(fdt, "size", &map->size, sizeof(map->size));
> > + err |= fdt_end_node(fdt);
> > + }
> > + err |= fdt_end_node(fdt);
> > +
> > + err |= fdt_finish(fdt);
> > +
> > + if (err) {
> > + pr_err("failed to prepare memblock FDT for KHO: %d\n", err);
> > + put_page(kho_fdt);
> > + kho_fdt = ERR_PTR(-EINVAL);
> > + }
> > +}
> > +
> > +static int __init reserve_mem_init(void)
> > +{
> > + if (!kho_is_enabled())
> > + return 0;
> > +
> > + prepare_kho_fdt();
> > +
> > + return register_kho_notifier(&reserve_mem_kho_nb);
> > +}
> > +late_initcall(reserve_mem_init);
> > +
> > +static void *kho_fdt_in __initdata;
> > +
> > +static void *__init reserve_mem_kho_retrieve_fdt(void)
> > +{
> > + phys_addr_t fdt_phys;
> > + struct folio *fdt_folio;
> > + void *fdt;
> > + int err;
> > +
> > + err = kho_retrieve_subtree(MEMBLOCK_KHO_FDT, &fdt_phys);
> > + if (err) {
> > + if (err != -ENOENT)
> > + pr_warn("failed to retrieve FDT '%s' from KHO: %d\n",
> > + MEMBLOCK_KHO_FDT, err);
> > + return ERR_PTR(err);
>
> Wouldn't just 'return NULL' work here?
If we have multiple `reserve_mem` in the kernel command line,
reserve_mem_kho_revive() will also be called multiple times. However
reserve_mem_kho_retrieve_fdt() should only be called once.
Here I am returning the ERR_PTR(err) such that if the first
reserve_mem_kho_retrieve_fdt() failed, subsequent reserve_mem_kho_revive()
can tell that reserve_mem_kho_retrieve_fdt() has failed so no need to try it
again. If we return NULL here, subsequent reserve_mem_kho_revive() would find
kho_fdt_in == NULL, and it could not tell whether it was due to previously
failed reserve_mem_kho_retrieve_fdt(), or it is the first
reserve_mem_kho_revive().
> > + }
> > +
> > + fdt_folio = kho_restore_folio(fdt_phys);
> > + if (!fdt_folio) {
> > + pr_warn("failed to restore memblock KHO FDT (0x%llx)\n", fdt_phys);
> > + return ERR_PTR(-EFAULT);
> > + }
> > +
> > + fdt = page_to_virt(folio_page(fdt_folio, 0));
>
> fdt = folio_address(folio);
Fixed.
> > +
> > + err = fdt_node_check_compatible(fdt, 0, MEMBLOCK_KHO_NODE_COMPATIBLE);
> > + if (err) {
> > + pr_warn("FDT '%s' is incompatible with '%s': %d\n",
> > + MEMBLOCK_KHO_FDT, MEMBLOCK_KHO_NODE_COMPATIBLE, err);
> > + return ERR_PTR(-EINVAL);
> > + }
> > +
> > + return fdt;
> > +}
> > +
> > +static bool __init reserve_mem_kho_revive(const char *name, phys_addr_t size,
> > + phys_addr_t align)
> > +{
> > + int err, len_start, len_size, offset;
> > + const phys_addr_t *p_start, *p_size;
> > + const void *fdt;
> > +
> > + if (!kho_fdt_in)
> > + kho_fdt_in = reserve_mem_kho_retrieve_fdt();
>
> I'd invert this and move to reserve_mem_kho_retrieve_fdt(), so there it
> would be
>
> if (kho_fdt_in)
> return kho_fdt_in;
>
> /* actually retrieve the fdt */
> kho_fdt_in = fdt;
>
> return fdt;
>
> and here
>
> fdt = reserve_mem_kho_retrieve_fdt();
> if (!fdt)
> return false;
Ah Ok, this is more elegant!
> > +
> > + if (IS_ERR(kho_fdt_in))
> > + return false;
> > +
> > + fdt = kho_fdt_in;
> > +
> >[...]
> > --
> > 2.49.0.604.gff1f9ca942-goog
> >
>
> --
> Sincerely yours,
> Mike.
Best,
Changyuan
---- 8< ----
