Re: devctl_notify system is inconsistent

On Thu, Dec 1, 2022 at 10:40 AM Baptiste Daroussin <bapt@freebsd.org> wrote:

> On Thu, Dec 01, 2022 at 08:38:37AM -0700, Warner Losh wrote:
> > On Thu, Dec 1, 2022 at 7:59 AM Baptiste Daroussin <bapt@freebsd.org>
> wrote:
> >
> > > On Thu, Dec 01, 2022 at 07:45:32AM -0700, Warner Losh wrote:
> > > > On Thu, Dec 1, 2022 at 1:36 AM Baptiste Daroussin <bapt@freebsd.org>
> > > wrote:
> > > >
> > > > > Hello,
> > > > >
> > > > > After the addition of netlink(4) by melifaro@, I started working
> on a
> > > new
> > > > > genetlink(4) module, to send kernel notification to the userland
> via
> > > > > netlink.
> > > > >
> > > > > The goal is to be able to have multiple consumers without the need
> of
> > > devd
> > > > > to be
> > > > > running.
> > > > >
> > > > > The goal is also to be able subscribe to the events the consumer is
> > > > > willing to
> > > > > receive.
> > > > >
> > > > > https://reviews.freebsd.org/D37574
> > > > >
> > > > > I also added a hook to devctl_notify to make sure all its event got
> > > sent
> > > > > via
> > > > > nlsysevent. (https://reviews.freebsd.org/D37573)
> > > > >
> > > >
> > > > You're connecting it at the wrong level: it will miss all device
> events.
> > > > devctl_notify
> > > > is used by everything except newbus's device attach, detach and
> nomatch
> > > > events, so none of those events will make it through.
> > >
> > > Where should I hook to get the device events?
> > >
> >
> > Either you need to drop down a level to where the formated events are
> > queued,
> > or you'll need to add something in devaddq() to deal with the events.
> These
> > are
> > a slightly different format than the devctl_notify() events because the
> > latter was
> > added later and I didn't want to cope with transitioning the old
> formatted
> > messages
> > to the new at that time (silly me).
> >
> >
> > > >
> > > >
> > > > > It works great and so far I am happy with it. on thing I figured
> out
> > > it is:
> > > > > the "system" argment of devctl_notify is inconsistent:
> > > > > Upper case vs lower case
> > > > > "kern" vs "kernel"
> > > > >
> > > >
> > > > They are consistent. With one exception that I deprecated in 13.x to
> be
> > > > removed in 14.x. I documented all of them at the time in devd.conf. I
> > > think
> > > > I'll go ahead and complete the deprecation.
> > > >
> > > >
> > > > > I intent to fix the following way:
> > > > > Create a new function similar to devctl_notify but with the first
> > > argument
> > > > > being
> > > > > an enum.
> > > > >
> > > >
> > > > I'm a pretty hard no on the enum. I rejected doing it that way when I
> > > wrote
> > > > devd
> > > > years ago. These have always been strings, and need to continue to
> > > always be
> > > > strings, or we're forever modifying devd(8) when we add a new one and
> > > > introducing
> > > > yet another opportunity for mismatch. I don't think this is a good
> idea
> > > at
> > > > all.
> > > >
> > > > There's been users of devd in the past that are loadable modules that
> > > pass
> > > > their
> > > > own system name in that devd.conf files would then process. Having an
> > > enum
> > > > with
> > > > enforcement would just get in the way of this.
> > > >
> > > > I have toyed with the idea of having a centralized list in bus.h
> that's a
> > > > bunch of
> > > > #defines, ala old-school X11 resources (which this is very very
> loosely
> > > > based on),
> > > > but it didn't seem worth the effort.
> > >
> > > That is fine with me and actually I do need the string name to
> register as
> > > group
> > > name, that I didn't want to trash the strings away.
> > >
> > > But I need a way to list them all.
> > >
> >
> > We have no current mechanism to do that. We could add something that
> would
> > register / deregister them with a sysinit + call to something in
> > kern_devctl.c which
> > could do the trick (and also deal with the ordering issues), though
> having
> > netlink(4)
> > as a loadable modules might be an interesting case to get right.
> >
> > If we did that, we could return a 'token' that you'd use to call a new
> > version of
> > devctl_notify(), perhaps with some glue for legacy users (or perhaps not:
> > we change
> > kernel interfaces all the time, and could just rename it and convert
> > everything in the
> > tree).
> >
> > >
> > > >
> > > > > Make the current devctl_notify convert its first argument into that
> > > enum
> > > > > and
> > > > > yell if an unkwown "system" is passed. (and probably declare
> > > devctl_notify
> > > > > deprecated)
> > > > >
> > > >
> > > > I don't think this buys us anything. devctl_notify cannot possibly
> know
> > > > about all
> > > > the possible subsystems, so this is likely doomed to high maintenance
> > > that
> > > > would
> > > > be largely ineffective.
> > > >
> > > > Then fix the inconsistencies: all upper case as it seems the most
> wildly
> > > use
> > > > > case
> > > > > s/kern/kernel/g
> > > > >
> > > > > WDYT?
> > > > >
> > > >
> > > > I wouldn't worry about the upper vs lower case. All the documented
> ones
> > > are
> > > > upper case, except kernel. There's no harm it being one exception
> since
> > > > changing
> > > > it will break user's devd.conf setups. I got enough feedback when I
> > > changed
> > > > "kern" to "kernel" last year for power events. And as far as I know,
> I've
> > > > documented
> > > > all the events that are generated today.
> > > >
> > > > Warner
> > >
> > >
> > > Note that if you think nlsysevent is a bad idea, I will just forget
> about
> > > it.
> > >
> >
> > I love the idea. I got over any resistance I had when melifaro@ moved
> > things into kern_devctl.c and we talked through the issues at that time.
> > I've been hoping that someone would replace the hacky thing I did with
> > a 'routing socket'-like interface (I never could figure out hose to do
> it so
> > many years ago w/o gross hacks). netlink(4) has finally provided a way
> > to do it that doesn't get the routing code all jumbled up for this.
> >
> > I just have some specific issues with your proposal. In hindsight, I
> should
> > have led with that in my first message :).
> >
> > Warner
>
> Here is my new proposal:
>
> What about:
>
> 1. I add a static list of systems in sys/devctl.h something like
>
> enum {
>  DEVCTL_SYSTEM_ACPI = 0,
>  DEVCTL_SYSTEM_AEON = 1,
>  ...
>  DEVCTL_SYSTEM_ZFS = 19
> };
>
> static const char *devctl_systems[] = {
>   "ACPI",
>   "AEON",
>   ...
>   "ZFS",
> };
>
> this way we have a list of official freebsd's systems.
> We don't change the devctl_notify interface
>
> and in the kernel we change the devctl_notify("ZFS" into
> devctl_notify(devctl_systems[DEVCTL_SYSTEM_ZFS],...
>
> This is not too intrusive, and breaks none of the existing code
>

So what happens if you see one not in the list?


> 2. I also hook devadq using the same interface as I already have done, but
> all the attach,detach,nomatch become an event (only in nlsysevent) in the
> "DEVICE" system,
> the "SUBSYSTEM" is the current what of devaddq
>
> The type is changed into:
> + -> ATTACH
> - -> DETACH
> anythingelse -> NOMATCH
> and the rest of the current line becomes the data
>

This is fine. I kinda think it might not be terrible to expose this to devd
and have it cope, but that's a zero sum for not a lot of gain.

so from nlsysvent point of view this is exactly the same kind of events as
> the
> one hooked in devctl_notify.
>

Sure.


> 3. in nlsysevent we have one category one can subscribe per known systems.
> All other "systems" falls into a new category named "extra" "vendor" or
> "other"
>

So all events that match elements of the array are 'system' and the others
are something else?


> from the consumer point of view the name of the system is anyway contained
> in
> the message itself, so the category it is subscribed to can differ.
>

Right. No data is lost...


> This way, I should be able to get ALL the events in a consistent way.
> I should be compatible with people who uses devctl_notify in their custom
> kernel
> modules.
>

Yea.


> Sounds easy enough without the requirement of complexifying kern_devctl.c
> with a
> registration of extra systems.
>

Yea, I kinda prefer that... Unless we add too many new systems. It's still
extra work to add one, but likely not enough extra to be worth the
automation.


> What do you think?
>

Not bad.

Warner