Received-SPF: pass (google.com: domain of linux-kernel+bounces-46610-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Precedence: bulk
MIME-Version: 1.0
References: <20240127220026.1722399-1-yoann.congal@smile.fr> <ZbfLl6PR_qxxreeX@bombadil.infradead.org>
In-Reply-To: <ZbfLl6PR_qxxreeX@bombadil.infradead.org>
From: Masahiro Yamada <masahiroy@kernel.org>
Date: Wed, 31 Jan 2024 23:34:26 +0900
Message-ID: <CAK7LNAR=YgBR=FYeZ+HKuFYOP3ad0K0tmqtsuHD6N-cHW5QoNQ@mail.gmail.com>
Subject: Re: [PATCH v2] printk: Remove redundant CONFIG_BASE_SMALL
To: Luis Chamberlain <mcgrof@kernel.org>
Cc: Yoann Congal <yoann.congal@smile.fr>, Josh Triplett <josh@joshtriplett.org>, 
	Petr Mladek <pmladek@suse.com>, Steven Rostedt <rostedt@goodmis.org>, 
	John Ogness <john.ogness@linutronix.de>, Sergey Senozhatsky <senozhatsky@chromium.org>, x86@kernel.org, 
	linux-kernel@vger.kernel.org, linux-serial@vger.kernel.org, 
	linux-fsdevel@vger.kernel.org, linux-kbuild@vger.kernel.org, 
	Geert Uytterhoeven <geert@linux-m68k.org>, Thomas Gleixner <tglx@linutronix.de>, 
	Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>, 
	Dave Hansen <dave.hansen@linux.intel.com>, "H. Peter Anvin" <hpa@zytor.com>, 
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Jiri Slaby <jirislaby@kernel.org>, 
	Willem de Bruijn <willemdebruijn.kernel@gmail.com>, Matthew Wilcox <willy@infradead.org>, 
	Peter Zijlstra <peterz@infradead.org>, Darren Hart <dvhart@infradead.org>, 
	Davidlohr Bueso <dave@stgolabs.net>, =?UTF-8?Q?Andr=C3=A9_Almeida?= <andrealmeid@igalia.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Tue, Jan 30, 2024 at 1:00=E2=80=AFAM Luis Chamberlain <mcgrof@kernel.org=
> wrote:
>
> You wanna address the printk maintainers, which I've added now.
> And Josh as he's interested in tiny linux.
>
> On Sat, Jan 27, 2024 at 11:00:26PM +0100, Yoann Congal wrote:
> > CONFIG_BASE_SMALL is currently a type int but is only used as a boolean
> > equivalent to !CONFIG_BASE_FULL.
> >
> > So, remove it entirely and move every usage to !CONFIG_BASE_FULL.
>
> Thanks for doing this.
>
> > In addition, recent kconfig changes (see the discussion in Closes: tag)
> > revealed that using:
> >   config SOMETHING
> >      default "some value" if X
> > does not work as expected if X is not of type bool.
>
> We should see if we can get kconfig to warn on this type of use.
> Also note that this was reported long ago by Vegard Nossum but he
> never really sent a fix [0] as I suggested, so thanks for doing this
> work.
>
> [0] https://lkml.iu.edu/hypermail/linux/kernel/2110.2/02402.html


It is good to know that this issue was already pointed out
in the past.


> You should mention the one case which this patch fixes is:
>
> > CONFIG_BASE_SMALL was used that way in init/Kconfig:
> >   config LOG_CPU_MAX_BUF_SHIFT
> >       default 12 if !BASE_SMALL
> >       default 0 if BASE_SMALL
>
> You should then mention this has been using 12 for a long time now
> for BASE_SMALL, and so this patch is a functional fix for those
> who used BASE_SMALL and wanted a smaller printk buffer contribtion per
> cpu. The contribution was only per CPU, and since BASE_SMALL systems
> likely don't have many CPUs the impact of this was relatively small,
> 4 KiB per CPU.  This patch fixes that back down to 0 KiB per CPU.
>
> So in practice I'd imagine this fix is not critical to stable. However
> if folks do want it backported I'll note BAS_FULL has been around since
> we started with git on Linux so it should backport just fine.
>
> > diff --git a/init/Kconfig b/init/Kconfig
> > index 8d4e836e1b6b1..877b3f6f0e605 100644
> > --- a/init/Kconfig
> > +++ b/init/Kconfig
> > @@ -734,8 +734,8 @@ config LOG_CPU_MAX_BUF_SHIFT
> >       int "CPU kernel log buffer size contribution (13 =3D> 8 KB, 17 =
=3D> 128KB)"
> >       depends on SMP
> >       range 0 21
> > -     default 12 if !BASE_SMALL
> > -     default 0 if BASE_SMALL
> > +     default 12 if BASE_FULL
> > +     default 0
> >       depends on PRINTK
> >       help
> >         This option allows to increase the default ring buffer size
>
> This is the only functional change, it is a fix, so please address
> this in a separate small patch where you can go into all the above
> details about its issue and implications of fixing this as per my
> note above.
>
> Then you can address a separate patch which addresses the move of
> BASE_SMALL users to BASE_FULL so to remove BASE_SMALL, that is
> because that commit would have no functional changes and it makes
> it easier to review.
>
>   Luis


Splitting this into two patches sounds fine to me.
Either is fine. Up to the printk maintainer.

Anyway, this patch looks good:

Reviewed-by: Masahiro Yamada <masahiroy@kernel.org>


--=20
Best Regards
Masahiro Yamada