Re: [PATCH 02/11] PM: EM: Add a skeleton code for netlink notification.

[Lukas Wunner]

> diff --git a/include/uapi/linux/energy_model.h b/include/uapi/linux/energy_model.h
> new file mode 100644
> index 000000000000..42a19e614c7d
> --- /dev/null
> +++ b/include/uapi/linux/energy_model.h
> @@ -0,0 +1,40 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +#ifndef _UAPI_LINUX_ENERGY_MODEL_H
> +#define _UAPI_LINUX_ENERGY_MODEL_H
> +

It looks like you created the header file manually.  There is tooling
to auto-generate all the boilerplate code from a YAML description in
Documentation/netlink/specs/ and my (limited) understanding is that
using it is mandatory for all newly introduced Netlink protocols.

I just had to wrap my head around all that for SPDM (a device
authentication protocol), see the top-most commit on this branch,
which is in a WIP state though:

https://github.com/l1k/linux/commits/doe

Basically you create the uapi and kernel header files plus kernel source
like this:

tools/net/ynl/pyynl/ynl_gen_c.py --spec Documentation/netlink/specs/em.yaml \
  --mode uapi --header
tools/net/ynl/pyynl/ynl_gen_c.py --spec Documentation/netlink/specs/em.yaml \
  --mode kernel --header
tools/net/ynl/pyynl/ynl_gen_c.py --spec Documentation/netlink/specs/em.yaml \
  --mode kernel --source

And then you add both the YAML file as well as the generated files to
the commit.  The reason you have to do that is because Python is
optional for building the kernel per Documentation/process/changes.rst,
so the files cannot be generated at compile time.  It is possible though
to regenerate them with tools/net/ynl/ynl-regen.sh whenever the YAML file
is changed.

The tooling is somewhat brittle, see 396786af1cea.  In theory ynl_gen_c.py
is capable of auto-generating code for user space applications as well
but it crashed when parsing my YAML file.  So there are more bugs,
just haven't had the time yet to fix them.


> +int __init em_netlink_init(void)
> +{
> +	return genl_register_family(&em_genl_family);
> +}
> +
> +void __init em_netlink_exit(void)
> +{
> +	genl_unregister_family(&em_genl_family);
> +}
> +

It looks like em_netlink_exit() isn't invoked anywhere, so why define
it in the first place?  You only need this if the feature can be modular
(which it cannot - it's gated by a bool Kconfig option).  Then you'd
call em_netlink_exit() in module_exit().

Also, you may want to consider moving this to patch [03/11], where
em_netlink_init() is actually invoked.  And you may want to move the
postcore_initcall() to this file so that you can declare em_netlink_init()
static, don't need em_init() and don't need the empty inline stubs.

Thanks,

Lukas