Re: [PATCH v2 2/4] printk: deprecate boot_delay in favour of printk_delay

[Petr Mladek <pmladek@suse.com>]

On Thu 2026-07-09 13:32:10, Andrew Murray wrote:
> Hi Petr,
> 
> On Wed, 8 Jul 2026 at 16:01, Petr Mladek <pmladek@suse.com> wrote:
> >
> > On Tue 2026-06-30 17:35:58, Andrew Murray wrote:
> > > The boot_delay (BOOT_PRINTK_DELAY) kernel parameter and printk_delay sysctl
> > > are two distinct mechanisms for providing similar functionality which add a
> > > delay prior to each printed printk message.
> > >
> > > boot_delay provides a kernel parameter for delaying printk output from
> > > kernel start through to boot (SYSTEM_RUNNING), whereas printk_delay is
> > > configurable only via sysctl and thus is only used post boot.
> > >
> > > Let's deprecate the boot_delay feature in favour of printk_delay. In order
> > > to preserve functionality, we'll also extend printk_delay such that it can
> > > additionally configured via an early kernel parameter.
> > >
> > > Behavior change:
> > >
> > > The delay enabled by both "boot_delay" and "printk_delay" continues
> > > working even in SYSTEM_RUNNING state. It must be explicitly stopped
> > > by setting printk_delay=0 via sysctl.
> > >
> > > The delay is skipped when the message is suppressed in all system
> > > states. It used to skipped only for the boot_delay.
> > >
> > > Signed-off-by: Andrew Murray <amurray@thegoodpenguin.co.uk>
> >
> > Looks good to me:
> >
> > Reviewed-by: Petr Mladek <pmladek@suse.com>
> >
> > Note that Sashiko AI warns about possible problems with negative
> > printk_delay values, see
> > https://sashiko.dev/#/patchset/20260630-deprecate_boot_delay-v2-0-f9883d36aa4b%40thegoodpenguin.co.uk
> >
> > But they should be handled in both the early parameter
> > and sysctl interface by the "10 * 1000" and "&ten_thousand"
> > limits.
> 
> Yes, that was my also view.
> 
> 
> >
> > The only potential problem might be a warning about possible
> > "sign" mismatch from the compiler. But I do not see any
> > even with make W=2.
> 
> I suspect this will come up everytime a change is made in this area,
> or perhaps some W=2 errors will pop up via kernelci or similar. I
> propose updating the patch as follows:
> 
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 5278d9cb19e4..fbb67f10c21e 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -1338,10 +1338,13 @@ static inline void early_boot_delay_msec(void)
> 
>  static int __init printk_delay_setup(char *str)
>  {
> -       get_option(&str, &printk_delay_msec);
> -       if (printk_delay_msec > 10 * 1000)
> -               printk_delay_msec = 0;
> +       int printk_delay_val = 0;
> 
> +       get_option(&str, &printk_delay_val);
> +       if (printk_delay_val < 0 || printk_delay_val > 10 * 1000)
> +               return 0;
> +
> +       printk_delay_msec = (unsigned int)printk_delay_val;
>         printk_delay_calculate();
> 
>         return 0;
> diff --git a/kernel/printk/sysctl.c b/kernel/printk/sysctl.c
> index f15732e93c2e..f256fc05faaf 100644
> --- a/kernel/printk/sysctl.c
> +++ b/kernel/printk/sysctl.c
> @@ -46,7 +46,7 @@ static const struct ctl_table printk_sysctls[] = {
>                 .data           = &printk_delay_msec,
>                 .maxlen         = sizeof(int),

I would change this to sizeof(unsigned int) as well.

>                 .mode           = 0644,
> -               .proc_handler   = proc_dointvec_minmax,
> +               .proc_handler   = proc_douintvec_minmax,
>                 .extra1         = SYSCTL_ZERO,
>                 .extra2         = (void *)&ten_thousand,
>         },

Otherwise, it looks good to me.

> This makes the cast explict, verifies the range before casting

Yup.

> (I guess a range of negative values could result in a postive value
> within the 10,000 range) and also uses the correct proc_handler.

IMHO, this should not happen. I have never seen int smaller than 4
bytes. If I count it correctly, positive numbers should be up to 32767.


> This has the side effect of not showing the following pr_debug for the
> lpj calculation when a delay is not set. Though loops_per_msec is only
> used during boot, so I don't think there is any loss here.

I agree that it should not harm. But please mention this in
the commit message.

>         pr_debug("printk_delay: %u, preset_lpj: %ld, lpj: %lu, "
>                 "HZ: %d, loops_per_msec: %llu\n",
>                 printk_delay_msec, preset_lpj, lpj, HZ, loops_per_msec);
> 
> Can I keep your Reviewed-By with these changes?

Feel free to keep it. That said, please put at least the change
of the ctl_table into a separate patch. I would rather put both new
changes into separate file. It is never a good idea to hide
such changes in a complex patch which mostly reshuffles an existing
code.

Best Regards,
Petr