On Tue, Apr 22, 2025 at 16:31:19 +0300, Mike Rapoport <r...@kernel.org> 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< ----
>From 7ad4379062aa9709d3702bfc53d237d0c1a4e326 Mon Sep 17 00:00:00 2001
From: Changyuan Lyu <changyu...@google.com>
Date: Thu, 24 Apr 2025 01:10:24 -0700
Subject: [PATCH] fixup! memblock: add KHO support for reserve_mem
This patch includes the suggested changes from
https://lore.kernel.org/lkml/aaeaj2iqkrv_f...@kernel.org/ and can be
squashed with "memblock: add KHO support for reserve_mem".
Fixes: 2e257a656639 ("memblock: add KHO support for reserve_mem")
Suggested-by: Mike Rapoport (Microsoft) <r...@kernel.org>
Signed-off-by: Changyuan Lyu <changyu...@google.com>
---
mm/memblock.c | 69 +++++++++++++++++++++++++--------------------------
1 file changed, 34 insertions(+), 35 deletions(-)
diff --git a/mm/memblock.c b/mm/memblock.c
index 3571a859f2fe1..d38a72f07ea0c 100644
--- a/mm/memblock.c
+++ b/mm/memblock.c
@@ -2490,15 +2490,6 @@ static int reserve_mem_kho_finalize(struct
kho_serialization *ser)
{
int err = 0, i;
- if (!reserved_mem_count)
- return NOTIFY_DONE;
-
- if (IS_ERR(kho_fdt)) {
- err = PTR_ERR(kho_fdt);
- pr_err("memblock FDT was not prepared successfully: %d\n", err);
- return notifier_from_errno(err);
- }
-
for (i = 0; i < reserved_mem_count; i++) {
struct reserve_mem_table *map = &reserved_mem_table[i];
@@ -2528,19 +2519,14 @@ static struct notifier_block reserve_mem_kho_nb = {
.notifier_call = reserve_mem_kho_notifier,
};
-static void __init prepare_kho_fdt(void)
+static int __init prepare_kho_fdt(void)
{
int err = 0, i;
void *fdt;
- if (!reserved_mem_count)
- return;
-
kho_fdt = alloc_page(GFP_KERNEL);
- if (!kho_fdt) {
- kho_fdt = ERR_PTR(-ENOMEM);
- return;
- }
+ if (!kho_fdt)
+ return -ENOMEM;
fdt = page_to_virt(kho_fdt);
@@ -2565,18 +2551,30 @@ static void __init prepare_kho_fdt(void)
if (err) {
pr_err("failed to prepare memblock FDT for KHO: %d\n", err);
put_page(kho_fdt);
- kho_fdt = ERR_PTR(-EINVAL);
+ kho_fdt = NULL;
}
+
+ return err;
}
static int __init reserve_mem_init(void)
{
- if (!kho_is_enabled())
+ int err;
+
+ if (!kho_is_enabled() || !reserved_mem_count)
return 0;
- prepare_kho_fdt();
+ err = prepare_kho_fdt();
+ if (err)
+ return err;
+
+ err = register_kho_notifier(&reserve_mem_kho_nb);
+ if (err) {
+ put_page(kho_fdt);
+ kho_fdt = NULL;
+ }
- return register_kho_notifier(&reserve_mem_kho_nb);
+ return err;
}
late_initcall(reserve_mem_init);
@@ -2586,33 +2584,38 @@ static void *__init reserve_mem_kho_retrieve_fdt(void)
{
phys_addr_t fdt_phys;
struct folio *fdt_folio;
- void *fdt;
int err;
+ if (kho_fdt_in)
+ return kho_fdt_in;
+
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);
+ goto out;
}
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);
+ err = -EFAULT;
+ goto out;
}
- fdt = page_to_virt(folio_page(fdt_folio, 0));
+ kho_fdt_in = folio_address(fdt_folio);
- err = fdt_node_check_compatible(fdt, 0, MEMBLOCK_KHO_NODE_COMPATIBLE);
+ err = fdt_node_check_compatible(kho_fdt_in, 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);
+ err = -EFAULT;
}
-
- return fdt;
+out:
+ if (err)
+ kho_fdt_in = ERR_PTR(err);
+ return kho_fdt_in;
}
static bool __init reserve_mem_kho_revive(const char *name, phys_addr_t size,
@@ -2622,14 +2625,10 @@ static bool __init reserve_mem_kho_revive(const char
*name, phys_addr_t size,
const phys_addr_t *p_start, *p_size;
const void *fdt;
- if (!kho_fdt_in)
- kho_fdt_in = reserve_mem_kho_retrieve_fdt();
-
- if (IS_ERR(kho_fdt_in))
+ fdt = reserve_mem_kho_retrieve_fdt();
+ if (IS_ERR(fdt))
return false;
- fdt = kho_fdt_in;
-
offset = fdt_subnode_offset(fdt, 0, name);
if (offset < 0) {
pr_warn("FDT '%s' has no child '%s': %d\n",
--
2.49.0.805.g082f7c87e0-goog
