Message-ID: <CA+CK2bDrvd1dgZSbN08aDuNYD9hOxTawR-RL2jTFLmT7B608ow@mail.gmail.com>
Date: Sat, 7 Jun 2025 20:04:00 -0400
From: Pasha Tatashin <pasha.tatashin@...een.com>
To: Pratyush Yadav <pratyush@...nel.org>
Cc: jasonmiu@...gle.com, graf@...zon.com, changyuanl@...gle.com, 
	rppt@...nel.org, dmatlack@...gle.com, rientjes@...gle.com, corbet@....net, 
	rdunlap@...radead.org, ilpo.jarvinen@...ux.intel.com, kanie@...ux.alibaba.com, 
	ojeda@...nel.org, aliceryhl@...gle.com, masahiroy@...nel.org, 
	akpm@...ux-foundation.org, tj@...nel.org, yoann.congal@...le.fr, 
	mmaurer@...gle.com, roman.gushchin@...ux.dev, chenridong@...wei.com, 
	axboe@...nel.dk, mark.rutland@....com, jannh@...gle.com, 
	vincent.guittot@...aro.org, hannes@...xchg.org, dan.j.williams@...el.com, 
	david@...hat.com, joel.granados@...nel.org, rostedt@...dmis.org, 
	anna.schumaker@...cle.com, song@...nel.org, zhangguopeng@...inos.cn, 
	linux@...ssschuh.net, linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org, 
	linux-mm@...ck.org, gregkh@...uxfoundation.org, tglx@...utronix.de, 
	mingo@...hat.com, bp@...en8.de, dave.hansen@...ux.intel.com, x86@...nel.org, 
	hpa@...or.com, rafael@...nel.org, dakr@...nel.org, 
	bartosz.golaszewski@...aro.org, cw00.choi@...sung.com, 
	myungjoo.ham@...sung.com, yesanishhere@...il.com, Jonathan.Cameron@...wei.com, 
	quic_zijuhu@...cinc.com, aleksander.lobakin@...el.com, ira.weiny@...el.com, 
	andriy.shevchenko@...ux.intel.com, leon@...nel.org, lukas@...ner.de, 
	bhelgaas@...gle.com, wagi@...nel.org, djeffery@...hat.com, 
	stuart.w.hayes@...il.com
Subject: Re: [RFC v2 06/16] luo: luo_subsystems: add subsystem registration

On Wed, Jun 4, 2025 at 12:30 PM Pratyush Yadav <pratyush@...nel.org> 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@...een.com>
> [...]
> > +/**
> > + * 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

Powered by blists - more mailing lists