From: Petr Mladek <pmladek@suse.com>
console_flush_one_record() and console_flush_all() duplicate several
checks. They both want to tell the caller that consoles are not
longer usable in this context because it has lost the lock or
the lock has to be reserved for the panic CPU.
Pass this information by clearing the @any_usable parameter value
which has the same effect.
Reviewed-by: Andrew Murray <amurray@thegoodpenguin.co.uk>
Signed-off-by: Petr Mladek <pmladek@suse.com>
---
kernel/printk/printk.c | 23 +++++++++++------------
1 file changed, 11 insertions(+), 12 deletions(-)
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
@@ -3175,7 +3175,8 @@ static inline void printk_kthreads_check_locked(void) { }
* console_lock, in which case the caller is no longer holding the
* console_lock. Otherwise it is set to false.
*
- * @any_usable will be set to true if there are any usable consoles.
+ * @any_usable will be set to true if there are any usable consoles
+ * in this context.
*
* Returns true when there was at least one usable console and a record was
* flushed. A returned false indicates there were no records to flush for any
@@ -3193,6 +3194,7 @@ static bool console_flush_one_record(bool do_cond_resched, u64 *next_seq, bool *
bool any_progress;
int cookie;
+ *any_usable = false;
any_progress = false;
printk_get_console_flush_type(&ft);
@@ -3229,7 +3231,7 @@ static bool console_flush_one_record(bool do_cond_resched, u64 *next_seq, bool *
* is already released.
*/
if (*handover)
- return false;
+ goto unusable;
/* Track the next of the highest seq flushed. */
if (printk_seq > *next_seq)
@@ -3241,7 +3243,7 @@ static bool console_flush_one_record(bool do_cond_resched, u64 *next_seq, bool *
/* Allow panic_cpu to take over the consoles safely. */
if (other_cpu_in_panic())
- goto abandon;
+ goto unusable_srcu;
if (do_cond_resched)
cond_resched();
@@ -3250,8 +3252,10 @@ static bool console_flush_one_record(bool do_cond_resched, u64 *next_seq, bool *
return any_progress;
-abandon:
+unusable_srcu:
console_srcu_read_unlock(cookie);
+unusable:
+ *any_usable = false;
return false;
}
@@ -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;
bool any_progress;
*next_seq = 0;
*handover = false;
do {
- any_progress = console_flush_one_record(do_cond_resched, next_seq, handover,
- &any_usable);
+ any_progress = console_flush_one_record(do_cond_resched, next_seq,
+ handover, &any_usable);
- if (*handover)
- return false;
-
- if (other_cpu_in_panic())
- return false;
} while (any_progress);
return any_usable;
--
2.34.1On 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
> @@ -3193,6 +3194,7 @@ static bool console_flush_one_record(bool do_cond_resched, u64 *next_seq, bool *
> bool any_progress;
> int cookie;
>
> + *any_usable = false;
Since it is expected that @next_seq and @handover are initialized by
their callers (if their callers are interested in the values), then I
would expect @any_usable to be initialized by the
caller. console_flush_one_record() never reads this variable.
> @@ -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.
> bool any_progress;
>
> *next_seq = 0;
> *handover = false;
>
> do {
> - any_progress = console_flush_one_record(do_cond_resched, next_seq, handover,
> - &any_usable);
> + any_progress = console_flush_one_record(do_cond_resched, next_seq,
> + handover, &any_usable);
>
Since the second line of the call to console_flush_one_record() already
has a ton of whitespace, I would remove the above blank line.
John
On Tue, 30 Sept 2025 at 13:59, John Ogness <john.ogness@linutronix.de> 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
> > @@ -3193,6 +3194,7 @@ static bool console_flush_one_record(bool do_cond_resched, u64 *next_seq, bool *
> > bool any_progress;
> > int cookie;
> >
> > + *any_usable = false;
>
> Since it is expected that @next_seq and @handover are initialized by
> their callers (if their callers are interested in the values), then I
> would expect @any_usable to be initialized by the
> caller. console_flush_one_record() never reads this variable.
Yes, that's correct. Perhaps the comments for the parameters should
indicate otherwise?
>
> > @@ -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.
At least that was the intent discussed here [1].
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? Thus semantically, the return value of
console_flush_one_record tells you that nothing bad happened and you
can call it again, and the benefit of calling it again depends on if
progress is being made (as determined by the caller through the
existing seq argument).
>
> > bool any_progress;
> >
> > *next_seq = 0;
> > *handover = false;
> >
> > do {
> > - any_progress = console_flush_one_record(do_cond_resched, next_seq, handover,
> > - &any_usable);
> > + any_progress = console_flush_one_record(do_cond_resched, next_seq,
> > + handover, &any_usable);
> >
>
> Since the second line of the call to console_flush_one_record() already
> has a ton of whitespace, I would remove the above blank line.
Sure.
>
> John
[1] https://lkml.org/lkml/2025/9/27/200
Andrew Murray
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
>> > @@ -3193,6 +3194,7 @@ static bool console_flush_one_record(bool do_cond_resched, u64 *next_seq, bool *
>> > bool any_progress;
>> > int cookie;
>> >
>> > + *any_usable = false;
>>
>> Since it is expected that @next_seq and @handover are initialized by
>> their callers (if their callers are interested in the values), then I
>> would expect @any_usable to be initialized by the
>> caller. console_flush_one_record() never reads this variable.
>
> Yes, that's correct. Perhaps the comments for the parameters should
> indicate otherwise?
They should clarify. For example, @next_seq is "valid only when this
function returns true" but @handover validitiy is not specified and is
also not related to the return value.
I would like to see the helper function provide clear usage. If the
caller is expected to initialize the parameters (which IMHO it should be
the case for all of the pointer parameters), then that should be
specified.
>> > @@ -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;
}
> 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? Thus semantically, the return value of
> console_flush_one_record tells you that nothing bad happened and you
> can call it again, and the benefit of calling it again depends on if
> progress is being made (as determined by the caller through the
> existing seq argument).
Sorry, I could not follow how this would work. It sounds like a
simplification. If you can make it work, go for it.
John
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
> >> > @@ -3193,6 +3194,7 @@ static bool console_flush_one_record(bool do_cond_resched, u64 *next_seq, bool *
> >> > bool any_progress;
> >> > int cookie;
> >> >
> >> > + *any_usable = false;
> >>
> >> Since it is expected that @next_seq and @handover are initialized by
> >> their callers (if their callers are interested in the values), then I
> >> would expect @any_usable to be initialized by the
> >> caller. console_flush_one_record() never reads this variable.
> >
> > Yes, that's correct. Perhaps the comments for the parameters should
> > indicate otherwise?
>
> They should clarify. For example, @next_seq is "valid only when this
> function returns true" but @handover validitiy is not specified and is
> also not related to the return value.
>
> I would like to see the helper function provide clear usage. If the
> caller is expected to initialize the parameters (which IMHO it should be
> the case for all of the pointer parameters), then that should be
> specified.
OK.
>
> >> > @@ -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 :|
>
> > 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? Thus semantically, the return value of
> > console_flush_one_record tells you that nothing bad happened and you
> > can call it again, and the benefit of calling it again depends on if
> > progress is being made (as determined by the caller through the
> > existing seq argument).
>
> Sorry, I could not follow how this would work. It sounds like a
> simplification. If you can make it work, go for it.
Against my patches, something like this:
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 2c9b9383df76..f38295cc3645 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -3186,16 +3186,13 @@ static inline void
printk_kthreads_check_locked(void) { }
*
* Requires the console_lock.
*/
-static bool console_flush_one_record(bool do_cond_resched, u64
*next_seq, bool *handover,
- bool *any_usable)
+static bool console_flush_one_record(bool do_cond_resched, u64
*next_seq, bool *handover)
{
struct console_flush_type ft;
struct console *con;
- bool any_progress;
int cookie;
- *any_usable = false;
- any_progress = false;
+ bool any_usable = false;
printk_get_console_flush_type(&ft);
@@ -3215,7 +3212,7 @@ static bool console_flush_one_record(bool
do_cond_resched, u64 *next_seq, bool *
if (!console_is_usable(con, flags, !do_cond_resched))
continue;
- *any_usable = true;
+ any_usable = true;
if (flags & CON_NBCON) {
progress = nbcon_legacy_emit_next_record(con,
handover, cookie,
@@ -3239,7 +3236,6 @@ static bool console_flush_one_record(bool
do_cond_resched, u64 *next_seq, bool *
if (!progress)
continue;
- any_progress = true;
/* Allow panic_cpu to take over the consoles safely. */
if (other_cpu_in_panic())
@@ -3250,12 +3246,11 @@ static bool console_flush_one_record(bool
do_cond_resched, u64 *next_seq, bool *
}
console_srcu_read_unlock(cookie);
- return any_progress;
+ return any_usable;
unusable_srcu:
console_srcu_read_unlock(cookie);
unusable:
- *any_usable = false;
return false;
}
@@ -3286,15 +3281,15 @@ static bool console_flush_all(bool
do_cond_resched, u64 *next_seq, bool *handove
{
bool any_usable;
bool any_progress;
+ u64 last_seq;
*next_seq = 0;
*handover = false;
do {
- any_progress =
console_flush_one_record(do_cond_resched, next_seq,
- handover, &any_usable);
-
- } while (any_progress);
+ last_seq = *next_seq;
+ any_usable = console_flush_one_record(do_cond_resched,
next_seq, handover);
+ } while (*next_seq > last_seq);
return any_usable;
}
@@ -3674,21 +3669,20 @@ static int legacy_kthread_func(void *unused)
wait_event_interruptible(legacy_wait,
legacy_kthread_should_wakeup());
+ u64 last_seq, next_seq = 0;
do {
- bool any_usable;
bool handover = false;
- u64 next_seq;
if (kthread_should_stop())
return 0;
console_lock();
- any_progress = console_flush_one_record(true, &next_seq,
- &handover, &any_usable);
+ last_seq = next_seq;
+ console_flush_one_record(true, &next_seq, &handover);
if (!handover)
__console_unlock();
- } while (any_progress);
+ } while (next_seq > last_seq);
goto wait_for_event;
}
--
2.34.1
This also has the benefit of removing code (and more could be removed
if the progress variable in console_flush_one_record is removed - i.e.
we could unconditionally bail on panic and resched each time).
Thanks,
Andrew Murray
© 2016 - 2025 Red Hat, Inc.