Re: [PATCH v2 5/8] printk: move printk sysctl to printk/sysctl.c

Petr Mladek
2021-11-26T12:53:48Z

On Wed 2021-11-24 15:14:32, Luis Chamberlain wrote:
> From: Xiaoming Ni <nixiaoming@huawei.com>
>
> The kernel/sysctl.c is a kitchen sink where everyone leaves
> their dirty dishes, this makes it very difficult to maintain.
>
> To help with this maintenance let's start by moving sysctls to
> places where they actually belong. The proc sysctl maintainers
> do not want to know what sysctl knobs you wish to add for your own
> piece of code, we just care about the core logic.
>
> So move printk sysctl from kernel/sysctl.c to kernel/printk/sysctl.c.
> Use register_sysctl() to register the sysctl interface.
>
> diff --git a/kernel/printk/Makefile b/kernel/printk/Makefile
> index d118739874c0..f5b388e810b9 100644
> --- a/kernel/printk/Makefile
> +++ b/kernel/printk/Makefile
> @@ -2,5 +2,8 @@
> obj-y = printk.o
> obj-$(CONFIG_PRINTK) += printk_safe.o
> obj-$(CONFIG_A11Y_BRAILLE_CONSOLE) += braille.o
> -obj-$(CONFIG_PRINTK) += printk_ringbuffer.o
> obj-$(CONFIG_PRINTK_INDEX) += index.o
> +
> +obj-$(CONFIG_PRINTK) += printk_support.o
> +printk_support-y := printk_ringbuffer.o
> +printk_support-$(CONFIG_SYSCTL) += sysctl.o

I have never seen this trick. It looks like a dirty hack ;-)
Anyway, I do not see it described in the documentation. I wonder
if it works only by chance.

IMHO, a cleaner solution would be to add the following
into init/Kconfig:

config BUILD_PRINTK_SYSCTL
bool
default (PRINTK && SYSCTL)

and then use:

obj-$(CONFIG_BUILD_PRINTK_SYSCTL) += sysctl.o

> diff --git a/kernel/printk/sysctl.c b/kernel/printk/sysctl.c
> new file mode 100644
> index 000000000000..653ae04aab7f
> --- /dev/null
> +++ b/kernel/printk/sysctl.c
> @@ -0,0 +1,85 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * sysctl.c: General linux system control interface
> + */
> +
> +#include <linux/sysctl.h>
> +#include <linux/printk.h>
> +#include <linux/capability.h>
> +#include <linux/ratelimit.h>
> +#include "internal.h"
> +
> +static const int ten_thousand = 10000;

The patch should also remove the variable in kernel/sysctl.c.

Otherwise, it looks like a really nice clean up.

Best Regards,
Petr