On Sat, Nov 15, 2025 at 09:51:07AM -0500, Pasha Tatashin wrote: > On Sat, Nov 15, 2025 at 4:40 AM Mike Rapoport <[email protected]> wrote: > > > > On Fri, Nov 14, 2025 at 01:59:59PM -0500, Pasha Tatashin wrote: > > > - struct kho_sub_fdt *sub_fdt; > > > + phys_addr_t phys = virt_to_phys(fdt); > > > + void *root_fdt = kho_out.fdt; > > > + int err = -ENOMEM; > > > + int off, fdt_err; > > > > > > - sub_fdt = kmalloc(sizeof(*sub_fdt), GFP_KERNEL); > > > - if (!sub_fdt) > > > - return -ENOMEM; > > > + guard(mutex)(&kho_out.lock); > > > + > > > + fdt_err = fdt_open_into(root_fdt, root_fdt, PAGE_SIZE); > > > + if (fdt_err < 0) > > > + return err; > > > > > > - INIT_LIST_HEAD(&sub_fdt->l); > > > - sub_fdt->name = name; > > > - sub_fdt->fdt = fdt; > > > + off = fdt_add_subnode(root_fdt, 0, name); > > > > Why not > > fdt_err = fdt_add_subnode() > > > > as I asked in v1 review? > > > > Oh, I missed that, there is a slight difference between the two: > 'fdt_err' only contains FDT return value, i.e. error if negative. The > 'off' on the other hand in the happy path contains subnode offset, and > contains error only in the unhappy path. This is why I think it is a > little cleaner to keep different name, however, if you still prefer > re-using a single local variable for both, this is fix-up patch: > > diff --git a/kernel/liveupdate/kexec_handover.c > b/kernel/liveupdate/kexec_handover.c > index 224bdf5becb6..81f60ccb2dc7 100644 > --- a/kernel/liveupdate/kexec_handover.c > +++ b/kernel/liveupdate/kexec_handover.c > @@ -713,7 +713,7 @@ int kho_add_subtree(const char *name, void *fdt) > phys_addr_t phys = virt_to_phys(fdt); > void *root_fdt = kho_out.fdt; > int err = -ENOMEM; > - int off, fdt_err; > + int fdt_err; > > guard(mutex)(&kho_out.lock); > > @@ -721,14 +721,14 @@ int kho_add_subtree(const char *name, void *fdt) > if (fdt_err < 0) > return err; > > - off = fdt_add_subnode(root_fdt, 0, name); > - if (off < 0) { > - if (off == -FDT_ERR_EXISTS) > + fdt_err = fdt_add_subnode(root_fdt, 0, name); > + if (fdt_err < 0) { > + if (fdt_err == -FDT_ERR_EXISTS) > err = -EEXIST; > goto out_pack; > } > > - err = fdt_setprop(root_fdt, off, PROP_SUB_FDT, &phys, sizeof(phys)); > + err = fdt_setprop(root_fdt, fdt_err, PROP_SUB_FDT, &phys, > sizeof(phys));
I missed 'off' here, never mind > if (err < 0) > goto out_pack; > -- Sincerely yours, Mike.
