Re: [PATCH v2 3/4] printk: nbcon: move printk_delay to console emiting code

[Andrew Murray]

On Wed, 8 Jul 2026 at 15:00, Petr Mladek <pmladek@xxxxxxxx> wrote:
>
> On Wed 2026-07-08 12:42:22, John Ogness wrote:
> > On 2026-07-07, Petr Mladek <pmladek@xxxxxxxx> wrote:
> > >> This is too deep (also pointed out by Sashiko) because it multiplies the
> > >> delay times the number of consoles. For the legacy printing, it would be
> > >> more appropriate to put the delay inside console_flush_all() and
> > >> legacy_kthread_func().
> > >
> > > True. The question is if the proper solution is worth the complexity.
> > > We would need to pass the information down two level of the API.
> > > It would require adding a new (output) parameter to console_flush_one_record(),
> > > nbcon_legacy_emit_next_record(), and console_emit_next_record().
> > >
> > > It is not that complicated but these functions are already hairy
> > > enough so we should be careful.

Indeed - It would have been nice to put the delay in
console_flush_one_record, but nbcon_legacy_emit_next_record and
console_emit_next_record don't provide an easy way of knowing if the
record was actually printed.


> >
> > Fair enough. But then it should be consistent and
> > console_emit_next_record() should perform the delay before allowing the
> > handover. Something like this:
>
> Great catch!

I'll update the series to reflect this.


>
> > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> > index 2fe9a963c823a..62fd6a5ebef66 100644
> > --- a/kernel/printk/printk.c
> > +++ b/kernel/printk/printk.c
> > @@ -3161,6 +3161,8 @@ static bool console_emit_next_record(struct console *con, bool *handover, int co
> >                */
> >
> >               con->write(con, outbuf, pmsg.outbuf_len);
> > +             printk_delay(false);
> > +
> >               con->seq = pmsg.seq + 1;
> >       } else {
> >               /*
> > @@ -3182,6 +3184,7 @@ static bool console_emit_next_record(struct console *con, bool *handover, int co
> >               printk_legacy_allow_spinlock_enter();
> >               con->write(con, outbuf, pmsg.outbuf_len);
> >               printk_legacy_allow_spinlock_exit();
> > +             printk_delay(true);
>
> It is pity that this is inside printk_safe context with IRQs
> disabled. A solution might be to call it before the write.

I tried to be consistent about always performing the delay before or
after the emit. As part of this series I moved the delay so it now
occurs after the emit. This made it easier to know if the message was
actually emitted (rather than delaying, and then a handover occurring
before actually emitting).


>
> But maybe, this is good enough. This code path is called from
> vprintk_emit() and it might be in an atomic context anyway.
> Also it increases the chance of successful handover which
> might help as well.
>
> Anyway, it would be nice to mention these pitfalls into commit message.

Sure.

Thanks for the feedback Petr, John, Benedikt!

Andrew Murray

>
> >
> >               start_critical_timings();
> >
> >
> > John