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

[Mike Rapoport <rppt@kernel.org>]

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.