Re: [RFC v1 1/3] luo: Live Update Orchestrator

[Pasha Tatashin <pasha.tatashin@soleen.com>]

On Thu, Mar 20, 2025 at 1:50 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Mar 20, 2025 at 12:35:20PM -0400, Pasha Tatashin wrote:
> > On Thu, Mar 20, 2025 at 9:40 AM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Thu, Mar 20, 2025 at 02:40:09AM +0000, Pasha Tatashin wrote:
>
> ...
>
> > > > +#ifndef _LINUX_LIVEUPDATE_H
> > > > +#define _LINUX_LIVEUPDATE_H
> > >
> > > > +#include <linux/compiler.h>
> > > > +#include <linux/notifier.h>
> > >
> > > This is semi-random list of inclusions. Try to follow IWYU principle.
> > > See below.
> >
> > I will remove <linux/compiler.h>
>
> But you need to add something more...

...

>
> ...
>
> > > > +bool liveupdate_state_updated(void);
> > >
> > > Where bool is defined?
> >
> > in kernel/liveupdate.c
>
> Nope, I meant where the type is defined. It is IIRC in types.h which needs to
> be included.

Ah, I see what you mean, sure I will include types.h.

>
> ...
>
> > > > +     if (kstrtol(buf, 0, &val) < 0)
> > > > +             return -EINVAL;
> > >
> > > Shadower error code.
> >
> > In this case it is appropriate, we do not case why kstrtol() could not
> > be converted into an appropriate integer, all we care is that the
> > input was invalid, and that what we return back to user.
>
> The kstrtox() may give different error codes. User may want to know more about
> what's wrong with the input. Shadowed error codes are discouraged and should be
> explained.
>

...

> > > > +     if (val != 1 && val != 0)
> > > > +             return -EINVAL;
> > >
> > > What's wrong with using kstrtobool() from the beginning?
> >
> > It makes the input less defined, here we only allow '1' or '0',
> > kstrtobool() allows almost anything.
>
> But kstrtobool() is the interface for boolean input. You may document only
> 0 and 1 and don't tell people to use anything else. ABI documentation should
> be clear, that's it.

Sure, I will use kstrtobool().

>
> ...
>
> > > > +EXPORT_SYMBOL_GPL(liveupdate_state_normal);
> > >
> > > No namespace?
> >
> > Namespace is 'liveupdate_', all public interfaces have this prefix,
> > private functions are prefixed with luo_ where it makes sense.
>
> No, I'm talking about export namespace. Why does the entire kernel need these APIs?

These functions are intended for use by drivers and other subsystems
participating in the live update. They allow these components to
determine, during boot, whether to restore their state from the
serialized state, or, during runtime, whether a live update is in the
prepared state, causing different behavior compared to normal mode
(e.g., prohibiting DMA mappings modifications, binding/unbinding,
etc.).

Pasha

> --
> With Best Regards,
> Andy Shevchenko