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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4263e586-f1ea-4797-8789-116d173058d9@igalia.com>
Date: Tue, 3 Jun 2025 15:01:38 +0900
From: Changwoo Min <changwoo@...lia.com>
To: Lukas Wunner <lukas@...ner.de>
Cc: lukasz.luba@....com, rafael@...nel.org, len.brown@...el.com,
 pavel@...nel.org, christian.loehle@....com, tj@...nel.org,
 kernel-dev@...lia.com, linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 02/11] PM: EM: Add a skeleton code for netlink
 notification.

Hi Lukas,

Thank you for the comments!

On 6/3/25 04:53, Lukas Wunner wrote:
>> 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.

Right, I followed the structure and design of thermal_netlink.{ch}.

>                                                     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.

Thank you for the suggestion! Using YNL will definitely be easier
to maintain in the long run. I will work on defining the YNL and
generating the boilerplate code using YNL. It will require some
reorg of files to keep the autogenerated files intact.

Besides the boilerplate generation, what do you think about the
current commands and events defined? Does it look reasonable? If
you have any feedback, I will incorporate it in the next version.

> 
> 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().

You are right. I will drop em_netlink_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 for the suggestion. That's simpler. I will change it as suggested.

Regards,
Changwoo Min