Re: [PATCH v2 2/3] printk: console_flush_one_record() code cleanup

[Petr Mladek <pmladek@suse.com>]

On Wed 2025-10-01 17:26:27, Andrew Murray wrote:
> On Wed, 1 Oct 2025 at 10:53, John Ogness <john.ogness@linutronix.de> wrote:
> >
> > On 2025-09-30, Andrew Murray <amurray@thegoodpenguin.co.uk> wrote:
> > >> On 2025-09-27, Andrew Murray <amurray@thegoodpenguin.co.uk> wrote:
> > >> > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> > >> > index 060d4919de320fe21fd7aca73ba497e27c4ff334..e2c1cacdb4164489c60fe38f1e2837eb838107d6 100644
> > >> > --- a/kernel/printk/printk.c
> > >> > +++ b/kernel/printk/printk.c
> > >> > @@ -3280,21 +3284,16 @@ static bool console_flush_one_record(bool do_cond_resched, u64 *next_seq, bool *
> > >> >   */
> > >> >  static bool console_flush_all(bool do_cond_resched, u64 *next_seq, bool *handover)
> > >> >  {
> > >> > -     bool any_usable = false;
> > >> > +     bool any_usable;
> > >>
> > >> Since console_flush_all() does read @any_usable, I would expect it to
> > >> initialize @any_usable. So I would not remove this definition initialization.
> > >
> > > Prior to this series, console_flush_all would set any_usable to false.
> > > It would be set to true if at any point a usable console is found,
> > > that value would be returned, or otherwise false if handover or panic.
> > > When the first patch split out this function, any_usable was kept as
> > > it was, leading to any_usable being true, even if a handover or panic
> > > had happened (hence additional checks needed, which are removed in
> > > this patch).
> > >
> > > By setting any_usable at the start of flush_one_record, it allows for
> > > any_usable to revert back to false, in the case where a once usable
> > > console is no longer usable. Thus representing the situation for the
> > > last record printed. It also makes console_flush_one_record easier to
> > > understand, as the any_usable flag will always be set, rather than
> > > only changing from false to true.
> >
> > OK. But then just have console_flush_all() set @any_usable in the loop:
> >
> > static bool console_flush_all(bool do_cond_resched, u64 *next_seq, bool *handover)
> > {
> >         bool any_progress;
> >         bool any_usable;
> >
> >         *next_seq = 0;
> >         *handover = false;
> >
> >         do {
> >                 any_usable = false;
> >                 any_progress = console_flush_one_record(do_cond_resched, next_seq,
> >                                                         handover, &any_usable);
> >         } while (any_progress);
> >
> >         return any_usable;
> > }
> 
> Yes, that seems like common sense, I have no idea why I didn't think of that :|

Looks good to me.

> 
> >
> > > Alternatively, it may be possible for console_flush_one_record to
> > > return any_usable, thus dropping it as an argument and removing the
> > > return of any_progress. Instead the caller could keep calling
> > > console_flush_one_record until it returns false or until next_seq
> > > stops increasing?

No, this won't work. @next_seq shows the highest value from all
consoles. It is no longer increased when at least one console
flushed all pending messages. But other consoles might be
behind, still having pending messages, and still making progress.

Honestly, I do not know how to make it better. We need to pass
both information: @next_seq and if some console flushed something.

Note that @next_seq is valid only when all consoles are flushed
and returning the same @next_seq. But it does not help to remove
the @any_progress parameter.

Best Regards,
Petr