Re: [RFC v2 06/16] luo: luo_subsystems: add subsystem registration

[Pasha Tatashin]

On Wed, Jun 4, 2025 at 12:30 PM Pratyush Yadav <pratyush@xxxxxxxxxx> wrote:
>
> On Thu, May 15 2025, Pasha Tatashin wrote:
>
> > Introduce the framework for kernel subsystems (e.g., KVM, IOMMU, device
> > drivers) to register with LUO and participate in the live update process
> > via callbacks.
> >
> > Subsystem Registration:
> > - Defines struct liveupdate_subsystem in linux/liveupdate.h,
> >   which subsystems use to provide their name and optional callbacks
> >   (prepare, freeze, cancel, finish). The callbacks accept
> >   a u64 *data intended for passing state/handles.
> > - Exports liveupdate_register_subsystem() and
> >   liveupdate_unregister_subsystem() API functions.
> > - Adds drivers/misc/liveupdate/luo_subsystems.c to manage a list
> >   of registered subsystems.
> >   Registration/unregistration is restricted to
> >   specific LUO states (NORMAL/UPDATED).
> >
> > Callback Framework:
> > - The main luo_core.c state transition functions
> >   now delegate to new luo_do_subsystems_*_calls() functions
> >   defined in luo_subsystems.c.
> > - These new functions are intended to iterate through the registered
> >   subsystems and invoke their corresponding callbacks.
> >
> > FDT Integration:
> > - Adds a /subsystems subnode within the main LUO FDT created in
> >   luo_core.c. This node has its own compatibility string
> >   (subsystems-v1).
> > - luo_subsystems_fdt_setup() populates this node by adding a
> >   property for each registered subsystem, using the subsystem's
> >   name.
> >   Currently, these properties are initialized with a placeholder
> >   u64 value (0).
> > - luo_subsystems_startup() is called from luo_core.c on boot to
> >   find and validate the /subsystems node in the FDT received via
> >   KHO. It panics if the node is missing or incompatible.
> > - Adds a stub API function liveupdate_get_subsystem_data() intended
> >   for subsystems to retrieve their persisted u64 data from the FDT
> >       in the new kernel.
> >
> > Signed-off-by: Pasha Tatashin <pasha.tatashin@xxxxxxxxxx>
> [...]
> > +/**
> > + * liveupdate_unregister_subsystem - Unregister a kernel subsystem handler from
> > + * LUO
> > + * @h: Pointer to the same liveupdate_subsystem structure that was used during
> > + * registration.
> > + *
> > + * Unregisters a previously registered subsystem handler. Typically called
> > + * during module exit or subsystem teardown. LUO removes the structure from its
> > + * internal list; the caller is responsible for any necessary memory cleanup
> > + * of the structure itself.
> > + *
> > + * Return: 0 on success, negative error code otherwise.
> > + * -EINVAL if h is NULL.
> > + * -ENOENT if the specified handler @h is not found in the registration list.
> > + * -EBUSY if LUO is not in the NORMAL state.
> > + */
> > +int liveupdate_unregister_subsystem(struct liveupdate_subsystem *h)
> > +{
> > +     struct liveupdate_subsystem *iter;
> > +     bool found = false;
> > +     int ret = 0;
> > +
> > +     luo_state_read_enter();
> > +     if (!liveupdate_state_normal() && !liveupdate_state_updated()) {
> > +             luo_state_read_exit();
> > +             return -EBUSY;
> > +     }
> > +
> > +     mutex_lock(&luo_subsystem_list_mutex);
> > +     list_for_each_entry(iter, &luo_subsystems_list, list) {
> > +             if (iter == h) {
> > +                     found = true;
>
> Nit: you don't actually need the found variable. You can do the same
> check that list_for_each_entry() uses, which is to call
> list_entry_is_head().

True, but for readability, 'found' makes more sense here. I do not
like using iterator outside of the loop, and also if
(list_entry_is_head(iter, &luo_subsystems_list, list) {} harder to
understand, and would require a  comment, instead of simple:  if
(found) {}

>
> > +                     break;
> > +             }
> > +     }
> > +
> > +     if (found) {
> > +             list_del_init(&h->list);
> > +     } else {
> > +             pr_warn("Subsystem handler '%s' not found for unregistration.\n",
> > +                     h->name);
> > +             ret = -ENOENT;
> > +     }
> > +
> > +     mutex_unlock(&luo_subsystem_list_mutex);
> > +     luo_state_read_exit();
> > +
> > +     return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(liveupdate_unregister_subsystem);
> [...]
>
> --
> Regards,
> Pratyush Yadav