Re: [PATCH v2] printk: Remove redundant CONFIG_BASE_SMALL

[Masahiro Yamada]

On Tue, Jan 30, 2024 at 1:00 AM Luis Chamberlain <mcgrof@xxxxxxxxxx> 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 => 8 KB, 17 => 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@xxxxxxxxxx>







-- 
Best Regards
Masahiro Yamada