Re: [PATCH v2 10/13] kho: Update FDT dynamically for subtree addition/removal

Mike Rapoport
2025-11-16T05:47:05Z

On Sat, Nov 15, 2025 at 09:51:07AM -0500, Pasha Tatashin wrote:
> On Sat, Nov 15, 2025 at 4:40 AM Mike Rapoport <rppt@kernel.org> 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.