On Fri, Nov 26, 2021 at 01:51:38PM +0100, Petr Mladek wrote: > On Wed 2021-11-24 15:14:32, Luis Chamberlain wrote: > > From: Xiaoming Ni <nixiaoming@xxxxxxxxxx> > > > > 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 ;-) It has been used in mac80211 for over a decade now :) See net/mac80211/Makefile > 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 I suppose it is a matter of taste, either way works with me, but I think less kconfig logic is better here. > > 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. Ah yes that variable is now unused there. Thanks Luis
