kgdb-bugreport

Re: [Kgdb-bugreport] [PATCH] kdb: Replace deprecated strcpy() with strscpy()

[Doug A. <dia...@ch...>]

Hi,

On Fri, Jul 18, 2025 at 2:40 PM Thorsten Blum <tho...@li...v> wrote:
>
> strcpy() is deprecated; use strscpy() instead.
>
> Link: https://github.com/KSPP/linux/issues/88
> Signed-off-by: Thorsten Blum <tho...@li...v>
> ---
>  kernel/debug/kdb/kdb_support.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

nit: Since this only covers things in the file `kdb_support.c` and not
everything in kernel/debug/kdb, perhaps that should be in the subject
line? Maybe "kdb: Replace deprecated strcpy() with strscpy() in
kdb_strdup()"?

Other than that, this looks fine to me.

Reviewed-by: Douglas Anderson <dia...@ch...>

Re: [Kgdb-bugreport] [PATCH] kernel: debug: Replace deprecated strcpy() with strscpy()

[Doug A. <dia...@ch...>]

Hi,

On Fri, Jul 18, 2025 at 12:32 PM Thorsten Blum <tho...@li...v> wrote:
>
> strcpy() is deprecated; use strscpy() instead.
>
> Link: https://github.com/KSPP/linux/issues/88
> Signed-off-by: Thorsten Blum <tho...@li...v>
> ---
>  kernel/debug/gdbstub.c | 29 +++++++++++++++--------------
>  1 file changed, 15 insertions(+), 14 deletions(-)

nit: Since this only covers things in the file `gdbstub.c` and not
everything in kernel/debug, perhaps that should be in the subject
line? Maybe "kernel: debug: gdbstub: Replace deprecated strcpy() with
strscpy()"?

Other than that, this looks fine to me.

Reviewed-by: Douglas Anderson <dia...@ch...>

Re: [Kgdb-bugreport] [PATCH 2/2] kdb: Adapt kdb_msg_write to work with NBCON consoles

[John O. <joh...@li...>]

On 2025-07-13, Marcos Paulo de Souza <mpd...@su...> wrote:
> diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
> index 9b11b10b120cf07e451a7a4d92ce50f9a6c066b2..3b7365c11d06b01d487767fd89f1081da10dd2ed 100644
> --- a/kernel/debug/kdb/kdb_io.c
> +++ b/kernel/debug/kdb/kdb_io.c
> @@ -558,6 +558,25 @@ static int kdb_search_string(char *searched, char *searchfor)
>  	return 0;
>  }
>  
> +static struct nbcon_context *nbcon_acquire_ctxt(struct console *con,
> +					struct nbcon_write_context *wctxt,
> +					char *msg, int msg_len)
> +{
> +	struct nbcon_context *ctxt = &ACCESS_PRIVATE(wctxt, ctxt);
> +
> +	ctxt->console		    = con;
> +	ctxt->spinwait_max_us	    = 0;
> +	ctxt->prio		    = NBCON_PRIO_EMERGENCY;
> +	ctxt->allow_unsafe_takeover = false;
> +	wctxt->outbuf		    = msg;
> +	wctxt->len		    = msg_len;
> +
> +	if (!nbcon_context_try_acquire(ctxt))
> +		return NULL;
> +
> +	return ctxt;

This function is grabbing a reference to a private member and returning
it, thus exposing internals. Can we instead create a proper API in
kernel/printk/nbcon.c for kdb?

For example, take a look at:

nbcon_device_try_acquire() and nbcon_device_release()

We could have something similar for kdb, such as:

bool *nbcon_kdb_try_acquire(struct nbcon_write_context *wctxt,
			    struct console *con, char *msg, int msg_len);

void nbcon_kdb_release(struct nbcon_write_context *wctxt);

> +}
> +
>  static void kdb_msg_write(const char *msg, int msg_len)
>  {
>  	struct console *c;
> @@ -589,12 +608,26 @@ static void kdb_msg_write(const char *msg, int msg_len)
>  	 */
>  	cookie = console_srcu_read_lock();
>  	for_each_console_srcu(c) {
> -		if (!(console_srcu_read_flags(c) & CON_ENABLED))
> +		struct nbcon_write_context wctxt = { };
> +		struct nbcon_context *ctxt;

With the above suggestion we do not need @ctxt.

> +		short flags = console_srcu_read_flags(c);
> +
> +		if (!console_is_usable(c, flags, true))
>  			continue;
>  		if (c == dbg_io_ops->cons)
>  			continue;
> -		if (!c->write)
> -			continue;
> +
> +		/*
> +		 * Do not continue if the console is NBCON and the context
> +		 * can't be acquired.
> +		 */
> +		if (flags & CON_NBCON) {
> +			ctxt = nbcon_acquire_ctxt(c, &wctxt, (char *)msg,
> +						  msg_len);
> +			if (!ctxt)
> +				continue;

And this becomes:

			if (!nbcon_kdb_try_acquire(&wctxt, c, (char *)msg, msg_len))
				continue;
> +		}
> +
>  		/*
>  		 * Set oops_in_progress to encourage the console drivers to
>  		 * disregard their internal spin locks: in the current calling
> @@ -605,7 +638,12 @@ static void kdb_msg_write(const char *msg, int msg_len)
>  		 * for this calling context.
>  		 */
>  		++oops_in_progress;
> -		c->write(c, msg, msg_len);
> +		if (flags & CON_NBCON) {
> +			c->write_atomic(c, &wctxt);
> +			nbcon_context_release(ctxt);

And this becomes:

			nbcon_kdb_release(&wctxt);

> +		} else {
> +			c->write(c, msg, msg_len);
> +		}
>  		--oops_in_progress;
>  		touch_nmi_watchdog();
>  	}

John Ogness

Re: [Kgdb-bugreport] [PATCH 1/2] printk: nbcon: Export console_is_usage and other nbcon symbols

[John O. <joh...@li...>]

On 2025-07-13, Marcos Paulo de Souza <mpd...@su...> wrote:
> Function nbcon_context_try_acquire, nbcon_context_relase and
> console_is_usable are going to be used in the next patch.

The nbcon_context is supposed is not meant to be exposed like this. I
would prefer creating a proper interface rather than having kdb code
directly modifying internal structures. I will provide more details in
the response to the 2nd patch of this series.

John Ogness

Re: [Kgdb-bugreport] [PATCH 1/2] printk: nbcon: Export console_is_usage and other nbcon symbols

[kernel t. r. <lk...@in...>]

Hi Marcos,

kernel test robot noticed the following build errors:

[auto build test ERROR on d0b3b7b22dfa1f4b515fd3a295b3fd958f9e81af]

url:    https://github.com/intel-lab-lkp/linux/commits/Marcos-Paulo-de-Souza/printk-nbcon-Export-console_is_usage-and-other-nbcon-symbols/20250713-131106
base:   d0b3b7b22dfa1f4b515fd3a295b3fd958f9e81af
patch link:    https://lore.kernel.org/r/20250713-nbcon-kgdboc-v1-1-51eccd9247a8%40suse.com
patch subject: [PATCH 1/2] printk: nbcon: Export console_is_usage and other nbcon symbols
config: i386-buildonly-randconfig-004-20250713 (https://download.01.org/0day-ci/archive/20250713/202...@in.../config)
compiler: gcc-12 (Debian 12.2.0-14+deb12u1) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250713/202...@in.../reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lk...@in...>
| Closes: https://lore.kernel.org/oe-kbuild-all/202...@in.../

All error/warnings (new ones prefixed by >>):

   In file included from kernel/panic.c:32:
   include/linux/console.h: In function 'nbcon_context_try_acquire':
>> include/linux/console.h:653:53: warning: no return statement in function returning non-void [-Wreturn-type]
     653 | static inline bool nbcon_context_try_acquire(struct nbcon_context *ctxt) { }
         |                                                     ^~~~~~~~~~~~~
--
   In file included from kernel/printk/printk.c:26:
   include/linux/console.h: In function 'nbcon_context_try_acquire':
>> include/linux/console.h:653:53: warning: no return statement in function returning non-void [-Wreturn-type]
     653 | static inline bool nbcon_context_try_acquire(struct nbcon_context *ctxt) { }
         |                                                     ^~~~~~~~~~~~~
   In file included from kernel/printk/printk.c:62:
   kernel/printk/internal.h: At top level:
>> kernel/printk/internal.h:165:20: error: redefinition of 'console_is_usable'
     165 | static inline bool console_is_usable(struct console *con, short flags,
         |                    ^~~~~~~~~~~~~~~~~
   include/linux/console.h:661:20: note: previous definition of 'console_is_usable' with type 'bool(struct console *, short int,  bool)' {aka '_Bool(struct console *, short int,  _Bool)'}
     661 | static inline bool console_is_usable(struct console *con, short flags,
         |                    ^~~~~~~~~~~~~~~~~


vim +/console_is_usable +165 kernel/printk/internal.h

6b93bb41f6eaa1 Thomas Gleixner 2023-09-16  164  
6cb58cfebb2932 John Ogness     2024-09-04 @165  static inline bool console_is_usable(struct console *con, short flags,
6cb58cfebb2932 John Ogness     2024-09-04  166  				     bool use_atomic) { return false; }
864c25c83d834b John Ogness     2024-08-20  167  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [Kgdb-bugreport] [PATCH 1/2] printk: nbcon: Export console_is_usage and other nbcon symbols

[kernel t. r. <lk...@in...>]

Hi Marcos,

kernel test robot noticed the following build warnings:

[auto build test WARNING on d0b3b7b22dfa1f4b515fd3a295b3fd958f9e81af]

url:    https://github.com/intel-lab-lkp/linux/commits/Marcos-Paulo-de-Souza/printk-nbcon-Export-console_is_usage-and-other-nbcon-symbols/20250713-131106
base:   d0b3b7b22dfa1f4b515fd3a295b3fd958f9e81af
patch link:    https://lore.kernel.org/r/20250713-nbcon-kgdboc-v1-1-51eccd9247a8%40suse.com
patch subject: [PATCH 1/2] printk: nbcon: Export console_is_usage and other nbcon symbols
config: x86_64-buildonly-randconfig-001-20250713 (https://download.01.org/0day-ci/archive/20250713/202...@in.../config)
compiler: clang version 20.1.8 (https://github.com/llvm/llvm-project 87f0227cb60147a26a1eeb4fb06e3b505e9c7261)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250713/202...@in.../reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lk...@in...>
| Closes: https://lore.kernel.org/oe-kbuild-all/202...@in.../

All warnings (new ones prefixed by >>):

   In file included from drivers/char/tpm/tpm_tis.c:21:
   In file included from include/linux/pnp.h:16:
>> include/linux/console.h:653:76: warning: non-void function does not return a value [-Wreturn-type]
     653 | static inline bool nbcon_context_try_acquire(struct nbcon_context *ctxt) { }
         |                                                                            ^
   In file included from drivers/char/tpm/tpm_tis.c:29:
   In file included from drivers/char/tpm/tpm.h:28:
   include/linux/tpm_eventlog.h:167:6: warning: variable 'mapping_size' set but not used [-Wunused-but-set-variable]
     167 |         int mapping_size;
         |             ^
   2 warnings generated.


vim +653 include/linux/console.h

   651	
   652	#else
 > 653	static inline bool nbcon_context_try_acquire(struct nbcon_context *ctxt) { }
   654	static inline void nbcon_context_release(struct nbcon_context *ctxt) { }
   655	static inline void nbcon_cpu_emergency_enter(void) { }
   656	static inline void nbcon_cpu_emergency_exit(void) { }
   657	static inline bool nbcon_can_proceed(struct nbcon_write_context *wctxt) { return false; }
   658	static inline bool nbcon_enter_unsafe(struct nbcon_write_context *wctxt) { return false; }
   659	static inline bool nbcon_exit_unsafe(struct nbcon_write_context *wctxt) { return false; }
   660	static inline void nbcon_reacquire_nobuf(struct nbcon_write_context *wctxt) { }
   661	static inline bool console_is_usable(struct console *con, short flags,
   662					     bool use_atomic) { return false; }
   663	#endif
   664	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [Kgdb-bugreport] [PATCH 2/7] printk: Use consoles_suspended flag when suspending/resuming all consoles

[Petr M. <pm...@su...>]

On Tue 2025-06-24 13:10:25, John Ogness wrote:
> On 2025-06-24, Petr Mladek <pm...@su...> wrote:
> >> > Variant C:
> >> > ==========
> >> >
> >> > Remove even @flags parameter from console_is_usable() and read both
> >> > values there directly.
> >> >
> >> > Many callers read @flags only because they call console_is_usable().
> >> > The change would simplify the code.
> >> >
> >> > But there are few exceptions:
> >> >
> >> >    2. Another exception is __pr_flush() where console_is_usable() is
> >> >       called twice with @use_atomic set "true" and "false".
> >> >
> >> >       We would want to read "con->flags" only once here. A solution
> >> >       would be to add a parameter to check both con->write_atomic
> >> >       and con->write_thread in a single call.
> >> 
> >> Or it could become a bitmask of printing types to check:
> >> 
> >> #define ATOMIC_PRINTING 0x1
> >> #define NONATOMIC_PRINTING 0x2
> >> 
> >> and then __pr_flush() looks like:
> >> 
> >> if (!console_is_usable(c, flags, ATOMIC_PRINTING|NONATOMIC_PRINTING)
> >
> > I like this. It will help even in all other cases when one mode is needed.
> > I mean that, for example:
> >
> >    console_is_usable(c, flags, ATOMIC_PRINTING)
> >
> > is more self-explaining than
> >
> >    console_is_usable(c, flags, true)
> 
> After I wrote that suggestion, I decided that the naming is not
> good. There is always confusion about what "atomic printing" means. For
> that reason the parameter was changed to "use_atomic". Basically we are
> specifying which callback to use and not the purpose. It is a bit tricky
> because legacy consoles do not have an atomic callback, i.e. the
> parameter only has meaning for nbcon consoles.
> 
> Perhaps these macros would be more suitable:
> 
> #define NBCON_USE_ATOMIC 0x1
> #define NBCON_USE_THREAD 0x2

I personally prefer this variant.

> or
> 
> #define NBCON_USE_WRITE_ATOMIC 0x1
> #define NBCON_USE_WRITE_THREAD 0x2

This one sounds more precise but it very long.

> or
> 
> #define NBCON_ATOMIC_CB 0x1
> #define NBCON_THREAD_CB 0x2
> 
> or
> 
> #define NBCON_ATOMIC_FUNC 0x1
> #define NBCON_THREAD_FUNC 0x2
> 
> Hopefully that gives Petr enough ideas that he can come up with good
> naming. ;-)

I thought about better names yesterday but I somehow did not have
inspiration ;-) Thanks for coming with the variants.

Best Regards,
Petr

Re: [Kgdb-bugreport] [PATCH 2/7] printk: Use consoles_suspended flag when suspending/resuming all consoles

[John O. <joh...@li...>]

On 2025-06-24, Petr Mladek <pm...@su...> wrote:
>> > Variant C:
>> > ==========
>> >
>> > Remove even @flags parameter from console_is_usable() and read both
>> > values there directly.
>> >
>> > Many callers read @flags only because they call console_is_usable().
>> > The change would simplify the code.
>> >
>> > But there are few exceptions:
>> >
>> >    2. Another exception is __pr_flush() where console_is_usable() is
>> >       called twice with @use_atomic set "true" and "false".
>> >
>> >       We would want to read "con->flags" only once here. A solution
>> >       would be to add a parameter to check both con->write_atomic
>> >       and con->write_thread in a single call.
>> 
>> Or it could become a bitmask of printing types to check:
>> 
>> #define ATOMIC_PRINTING 0x1
>> #define NONATOMIC_PRINTING 0x2
>> 
>> and then __pr_flush() looks like:
>> 
>> if (!console_is_usable(c, flags, ATOMIC_PRINTING|NONATOMIC_PRINTING)
>
> I like this. It will help even in all other cases when one mode is needed.
> I mean that, for example:
>
>    console_is_usable(c, flags, ATOMIC_PRINTING)
>
> is more self-explaining than
>
>    console_is_usable(c, flags, true)

After I wrote that suggestion, I decided that the naming is not
good. There is always confusion about what "atomic printing" means. For
that reason the parameter was changed to "use_atomic". Basically we are
specifying which callback to use and not the purpose. It is a bit tricky
because legacy consoles do not have an atomic callback, i.e. the
parameter only has meaning for nbcon consoles.

Perhaps these macros would be more suitable:

#define NBCON_USE_ATOMIC 0x1
#define NBCON_USE_THREAD 0x2

or

#define NBCON_USE_WRITE_ATOMIC 0x1
#define NBCON_USE_WRITE_THREAD 0x2

or

#define NBCON_ATOMIC_CB 0x1
#define NBCON_THREAD_CB 0x2

or

#define NBCON_ATOMIC_FUNC 0x1
#define NBCON_THREAD_FUNC 0x2

Hopefully that gives Petr enough ideas that he can come up with good
naming. ;-)

John

Re: [Kgdb-bugreport] [PATCH 2/7] printk: Use consoles_suspended flag when suspending/resuming all consoles

[Petr M. <pm...@su...>]

On Fri 2025-06-20 16:49:07, John Ogness wrote:
> On 2025-06-13, Petr Mladek <pm...@su...> wrote:
> >> diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
> >> index fd12efcc4aeda8883773d9807bc215f6e5cdf71a..72de12396e6f1bc5234acfdf6dcc393acf88d216 100644
> >> --- a/kernel/printk/nbcon.c
> >> +++ b/kernel/printk/nbcon.c
> >> @@ -1147,7 +1147,7 @@ static bool nbcon_kthread_should_wakeup(struct console *con, struct nbcon_contex
> >>  	cookie = console_srcu_read_lock();
> >>  
> >>  	flags = console_srcu_read_flags(con);
> >> -	if (console_is_usable(con, flags, false)) {
> >> +	if (console_is_usable(con, flags, false, consoles_suspended)) {
> >
> > The new global console_suspended value has the be synchronized the
> > same way as the current CON_SUSPENDED per-console flag.
> > It means that the value must be:
> >
> >   + updated only under console_list_lock together with
> >     synchronize_rcu().
> >
> >   + read using READ_ONCE() under console_srcu_read_lock()
> 
> Yes.
> 
> > I am going to propose more solutions because no one is obviously
> > the best one.
> 
> [...]
> 
> > Variant C:
> > ==========
> >
> > Remove even @flags parameter from console_is_usable() and read both
> > values there directly.
> >
> > Many callers read @flags only because they call console_is_usable().
> > The change would simplify the code.
> >
> > But there are few exceptions:
> >
> >    2. Another exception is __pr_flush() where console_is_usable() is
> >       called twice with @use_atomic set "true" and "false".
> >
> >       We would want to read "con->flags" only once here. A solution
> >       would be to add a parameter to check both con->write_atomic
> >       and con->write_thread in a single call.
> 
> Or it could become a bitmask of printing types to check:
> 
> #define ATOMIC_PRINTING 0x1
> #define NONATOMIC_PRINTING 0x2
> 
> and then __pr_flush() looks like:
> 
> if (!console_is_usable(c, flags, ATOMIC_PRINTING|NONATOMIC_PRINTING)

I like this. It will help even in all other cases when one mode is needed.
I mean that, for example:

   console_is_usable(c, flags, ATOMIC_PRINTING)

is more self-explaining than

   console_is_usable(c, flags, true)

Best Regards,
Petr

Re: [Kgdb-bugreport] [PATCH 5/7] arch: um: kmsg_dump: Don't check for CON_ENABLED

[John O. <joh...@li...>]

On 2025-06-16, Petr Mladek <pm...@su...> wrote:
> On Fri 2025-06-06 23:53:47, Marcos Paulo de Souza wrote:
>> All consoles found on for_each_console are registered, meaning that all of
>> them are CON_ENABLED. The code tries to find an active console, so check if the
>> console is not suspended instead.
>> 
>> Signed-off-by: Marcos Paulo de Souza <mpd...@su...>
>> ---
>>  arch/um/kernel/kmsg_dump.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/arch/um/kernel/kmsg_dump.c b/arch/um/kernel/kmsg_dump.c
>> index 4190211752726593dd2847f66efd9d3a61cea982..f3025b2a813453f479d720618c630bee135d4e08 100644
>> --- a/arch/um/kernel/kmsg_dump.c
>> +++ b/arch/um/kernel/kmsg_dump.c
>> @@ -31,7 +31,7 @@ static void kmsg_dumper_stdout(struct kmsg_dumper *dumper,
>>  		 * expected to output the crash information.
>>  		 */
>>  		if (strcmp(con->name, "ttynull") != 0 &&
>> -		    (console_srcu_read_flags(con) & CON_ENABLED)) {
>> +		    (console_srcu_read_flags(con) & CON_SUSPENDED) == 0) {
>>  			break;
>
> I think that we should actually replace the check of the
> CON_ENABLE/CON_SUSPENDED flag with
>
> 		is_console_usable(con, console_srcu_read_flags(con), true)
>
> And it should be done at the beginning of the patchset before
> changing the semantic of the flags.
>
> Motivation:
>
> There is the following comment at the beginning of the function:
>
> 	/*
> 	 * If no consoles are available to output crash information, dump
> 	 * the kmsg buffer to stdout.
> 	 */
>
> The if-condition checks for:
>
>   + "ttynull" because this special console does not show any messages
>     by definition
>
>   + disabled/suspended consoles; note that this patchset is replacing
>     CON_ENABLED with CON_SUSPENDED flag because it the state is
>     changed during suspend.
>
> But it should check also for:
>
>   + whether the console is NBCON_console and does not have con->write_atomic
>     because such a console would not be able to show the messages
>     in panic().
>
> And it should also check the global "consoles_suspended" flag. Because
> consoles won't show anything when it is set.
>
> And all these is already done by "is_console_usable()" except for
> the check of "ttynull" which is very special.
>
> How does the sound, please?

FWIW, I agree with all these points.

John

Re: [Kgdb-bugreport] [PATCH 2/7] printk: Use consoles_suspended flag when suspending/resuming all consoles

[John O. <joh...@li...>]

On 2025-06-13, Petr Mladek <pm...@su...> wrote:
>> diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
>> index fd12efcc4aeda8883773d9807bc215f6e5cdf71a..72de12396e6f1bc5234acfdf6dcc393acf88d216 100644
>> --- a/kernel/printk/nbcon.c
>> +++ b/kernel/printk/nbcon.c
>> @@ -1147,7 +1147,7 @@ static bool nbcon_kthread_should_wakeup(struct console *con, struct nbcon_contex
>>  	cookie = console_srcu_read_lock();
>>  
>>  	flags = console_srcu_read_flags(con);
>> -	if (console_is_usable(con, flags, false)) {
>> +	if (console_is_usable(con, flags, false, consoles_suspended)) {
>
> The new global console_suspended value has the be synchronized the
> same way as the current CON_SUSPENDED per-console flag.
> It means that the value must be:
>
>   + updated only under console_list_lock together with
>     synchronize_rcu().
>
>   + read using READ_ONCE() under console_srcu_read_lock()

Yes.

> I am going to propose more solutions because no one is obviously
> the best one.

[...]

> Variant C:
> ==========
>
> Remove even @flags parameter from console_is_usable() and read both
> values there directly.
>
> Many callers read @flags only because they call console_is_usable().
> The change would simplify the code.
>
> But there are few exceptions:
>
>   1. __nbcon_atomic_flush_pending(), console_flush_all(),
>      and legacy_kthread_should_wakeup() pass @flags to
>      console_is_usable() and also check CON_NBCON flag.
>
>      But CON_NBCON flag is special. It is statically initialized
>      and never set/cleared at runtime. It can be checked without
>      READ_ONCE(). Well, we still might want to be sure that
>      the struct console can't disappear.
>
>      IMHO, this can be solved by a helper function:
>
> 	/**
> 	 * console_srcu_is_nbcon - Locklessly check whether the console is nbcon
> 	 * @con:	struct console pointer of console to check
> 	 *
> 	 * Requires console_srcu_read_lock to be held, which implies that @con might
> 	 * be a registered console. The purpose of holding console_srcu_read_lock is
> 	 * to guarantee that no exit/cleanup routines will run if the console
> 	 * is currently undergoing unregistration.
> 	 *
> 	 * If the caller is holding the console_list_lock or it is _certain_ that
> 	 * @con is not and will not become registered, the caller may read
> 	 * @con->flags directly instead.
> 	 *
> 	 * Context: Any context.
> 	 * Return: True when CON_NBCON flag is set.
> 	 */
> 	static inline bool console_is_nbcon(const struct console *con)
> 	{
> 		WARN_ON_ONCE(!console_srcu_read_lock_is_held());
>
> 		/*
> 		 * The CON_NBCON flag is statically initialized and is never
> 		 * set or cleared at runtime.
> 		return data_race(con->flags & CON_NBCON);
> 	}

Agreed.

>    2. Another exception is __pr_flush() where console_is_usable() is
>       called twice with @use_atomic set "true" and "false".
>
>       We would want to read "con->flags" only once here. A solution
>       would be to add a parameter to check both con->write_atomic
>       and con->write_thread in a single call.

Or it could become a bitmask of printing types to check:

#define ATOMIC_PRINTING 0x1
#define NONATOMIC_PRINTING 0x2

and then __pr_flush() looks like:

if (!console_is_usable(c, flags, ATOMIC_PRINTING|NONATOMIC_PRINTING)

>       But it might actually be enough to check is with the "false"
>       value because "con->write_thread()" is mandatory for nbcon
>       consoles. And legacy consoles do not distinguish atomic mode.

A bit tricky, but you are right.

>
>
> Variant D:
> ==========
>
> We need to distinguish the global and per-console "suspended" flag
> because they might be nested. But we could use a separate flag
> for the global setting.
>
> I mean that:
>
>     + console_suspend() would set CON_SUSPENDED flag
>     + console_suspend_all() would set CON_SUSPENDED_ALL flag
>
> They both will be in con->flags.
>
> Pros:
>
>     + It is easy to implement.
>
> Cons:
>
>     + It feels a bit ugly.
>
>
> My opinion:
> ===========
>
> I personally prefer the variant C because:
>
>   + Removes one parameter from console_is_usable().
>
>   + The lockless synchronization of both global and per-console
>     flags is hidden in console_is_usable().
>
>   + The global console_suspended flag will be stored in global
>     variable (in compare with variant D).
>
> What do you think, please?
>
> Best Regards,
> Petr
>
>
> PS: The commit message and the cover letter should better explain
>     the background of this change.
>
>     It would be great if the cover letter described the bigger
>     picture, especially the history of the console_suspended,
>     CON_SUSPENDED, and CON_ENABLED flags. It might use info
>     from
>     https://lore.kernel.org/lkml/Zyo...@pa.../
>     and maybe even this link.
>
>     Also this commit message should mention that it partly reverts
>     the commit 9e70a5e109a4a233678 ("printk: Add per-console
>     suspended state"). But it is not simple revert because
>     we need to preserve the synchronization using
>     the console_list_lock for writing and SRCU for reading.

-- 
John Ogness
Linutronix GmbH | Bahnhofstraße 3 | D-88690 Uhldingen-Mühlhofen
Phone: +49 7556 25 999 20; Fax.: +49 7556 25 999 99

Hinweise zum Datenschutz finden Sie hier (Information on data privacy
can be found here): https://linutronix.de/legal/data-protection.php

Linutronix GmbH | Firmensitz (Registered Office): Uhldingen-Mühlhofen |
Registergericht (Registration Court): Amtsgericht Freiburg i.Br., HRB700
806 | Geschäftsführer (Managing Directors): Harry Demas, Heinz Egger,
Thomas Gleixner, Yin Sorrell, Jeffrey Schneiderman

Re: [Kgdb-bugreport] [PATCH 7/7] printk: Don't check for CON_ENABLED on console_unblank

[Geert U. <ge...@li...>]

Hi Petr,

On Mon, 16 Jun 2025 at 17:27, Petr Mladek <pm...@su...> wrote:
> On Fri 2025-06-06 23:53:49, Marcos Paulo de Souza wrote:
> > All consoles found on for_each_console_srcu are registered, meaning that all of
> > them are already CON_ENABLEDed.
> >
> > Signed-off-by: Marcos Paulo de Souza <mpd...@su...>
> > ---
> >  kernel/printk/printk.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> > index 658acf92aa3d2a3d1e294b7e17e5ee96d8169afe..8074a0f73691cfc5f637361048097ace1545c7c0 100644
> > --- a/kernel/printk/printk.c
> > +++ b/kernel/printk/printk.c
> > @@ -3360,7 +3360,7 @@ void console_unblank(void)
> >               if (flags & CON_SUSPENDED)
> >                       continue;
> >
> > -             if ((flags & CON_ENABLED) && c->unblank) {
> > +             if (c->unblank) {
>
> It might actually make sense to check is_console_usable() here.

My first thought was: one more to convert to a better name!
But the actual function is already called console_is_usable() ;-)

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@li...

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [Kgdb-bugreport] [PATCH 7/7] printk: Don't check for CON_ENABLED on console_unblank

[Petr M. <pm...@su...>]

On Fri 2025-06-06 23:53:49, Marcos Paulo de Souza wrote:
> All consoles found on for_each_console_srcu are registered, meaning that all of
> them are already CON_ENABLEDed.
> 
> Signed-off-by: Marcos Paulo de Souza <mpd...@su...>
> ---
>  kernel/printk/printk.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 658acf92aa3d2a3d1e294b7e17e5ee96d8169afe..8074a0f73691cfc5f637361048097ace1545c7c0 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -3360,7 +3360,7 @@ void console_unblank(void)
>  		if (flags & CON_SUSPENDED)
>  			continue;
>  
> -		if ((flags & CON_ENABLED) && c->unblank) {
> +		if (c->unblank) {

It might actually make sense to check is_console_usable() here.
The reason is similar as for the 5th and 6th patch, see
https://lore.kernel.org/r/aFA...@pa...
https://lore.kernel.org/r/aFA...@pa...


>  			found_unblank = true;
>  			break;
>  		}
> @@ -3402,7 +3402,7 @@ void console_unblank(void)
>  		if (flags & CON_SUSPENDED)
>  			continue;
>  
> -		if ((flags & CON_ENABLED) && c->unblank)
> +		if (c->unblank)
>  			c->unblank();

Same here.

>  	}
>  	console_srcu_read_unlock(cookie);
> 
> -- 
> 2.49.0

Re: [Kgdb-bugreport] [PATCH 6/7] debug: kgd_io: Don't check for CON_ENABLED

[Petr M. <pm...@su...>]

On Fri 2025-06-06 23:53:48, Marcos Paulo de Souza wrote:
> All consoles found on for_each_console_srcu are registered, meaning that all of
> them are CON_ENABLED. The code tries to find an active console, so check if the
> console is not suspended instead.
> 
> Signed-off-by: Marcos Paulo de Souza <mpd...@su...>
> ---
>  kernel/debug/kdb/kdb_io.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
> index 9b11b10b120cf07e451a7a4d92ce50f9a6c066b2..cdc1ee81d7332a9a00b967af719939f438f26cef 100644
> --- a/kernel/debug/kdb/kdb_io.c
> +++ b/kernel/debug/kdb/kdb_io.c
> @@ -589,7 +589,7 @@ static void kdb_msg_write(const char *msg, int msg_len)
>  	 */
>  	cookie = console_srcu_read_lock();
>  	for_each_console_srcu(c) {
> -		if (!(console_srcu_read_flags(c) & CON_ENABLED))
> +		if (console_srcu_read_flags(c) & CON_SUSPENDED)
>  			continue;

I think that this is similar to the 5th patch. We should check
here is_console_usable(con, console_srcu_read_flags(c), true)
because it checks more conditions:

     + the global console_suspended flag. The consoles drivers should
       not be used when it is set...

     + whether NBCON console driver has con->write_atomic

and we should also fix kdb_msg_write() to actually use
con->write_atomic() when it is a NBCON console driver.
There is hard-coded con->write() at the moment.

But it might get more complicated. It would be nice to do it correctly
and use con->write_atomit() only when nbcon_context_try_acquire()
succeeds. We probably should use a context with NBCON_PRIO_EMERGENCY.

And this should be fixed at the beginning of the patchset because
it actually fixes the support of the new NBCON console drivers.

Best Regards,
Petr

>  		if (c == dbg_io_ops->cons)
>  			continue;
> 
> -- 
> 2.49.0

Re: [Kgdb-bugreport] [PATCH 5/7] arch: um: kmsg_dump: Don't check for CON_ENABLED

[Petr M. <pm...@su...>]

On Fri 2025-06-06 23:53:47, Marcos Paulo de Souza wrote:
> All consoles found on for_each_console are registered, meaning that all of
> them are CON_ENABLED. The code tries to find an active console, so check if the
> console is not suspended instead.
> 
> Signed-off-by: Marcos Paulo de Souza <mpd...@su...>
> ---
>  arch/um/kernel/kmsg_dump.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/um/kernel/kmsg_dump.c b/arch/um/kernel/kmsg_dump.c
> index 4190211752726593dd2847f66efd9d3a61cea982..f3025b2a813453f479d720618c630bee135d4e08 100644
> --- a/arch/um/kernel/kmsg_dump.c
> +++ b/arch/um/kernel/kmsg_dump.c
> @@ -31,7 +31,7 @@ static void kmsg_dumper_stdout(struct kmsg_dumper *dumper,
>  		 * expected to output the crash information.
>  		 */
>  		if (strcmp(con->name, "ttynull") != 0 &&
> -		    (console_srcu_read_flags(con) & CON_ENABLED)) {
> +		    (console_srcu_read_flags(con) & CON_SUSPENDED) == 0) {
>  			break;

I think that we should actually replace the check of the
CON_ENABLE/CON_SUSPENDED flag with

		is_console_usable(con, console_srcu_read_flags(con), true)

And it should be done at the beginning of the patchset before
changing the semantic of the flags.

Motivation:

There is the following comment at the beginning of the function:

	/*
	 * If no consoles are available to output crash information, dump
	 * the kmsg buffer to stdout.
	 */

The if-condition checks for:

  + "ttynull" because this special console does not show any messages
    by definition

  + disabled/suspended consoles; note that this patchset is replacing
    CON_ENABLED with CON_SUSPENDED flag because it the state is
    changed during suspend.

But it should check also for:

  + whether the console is NBCON_console and does not have con->write_atomic
    because such a console would not be able to show the messages
    in panic().

And it should also check the global "consoles_suspended" flag. Because
consoles won't show anything when it is set.

And all these is already done by "is_console_usable()" except for
the check of "ttynull" which is very special.

How does the sound, please?

Best Regards,
Petr

>  		}
>  	}
> 
> -- 
> 2.49.0

Re: [Kgdb-bugreport] [PATCH 4/7] drivers: serial: kgdboc: Check CON_SUSPENDED instead of CON_ENABLED

[Doug A. <dia...@ch...>]

Hi,

On Fri, Jun 13, 2025 at 3:52 AM Petr Mladek <pm...@su...> wrote:
>
> On Thu 2025-06-12 16:16:09, Doug Anderson wrote:
> > Hi,
> >
> > On Thu, Jun 12, 2025 at 6:57 AM Petr Mladek <pm...@su...> wrote:
> > >
> > > > > > > @@ -577,7 +577,8 @@ static int __init kgdboc_earlycon_init(char
> > > > > > > *opt)
> > > > > > >         console_list_lock();
> > > > > > >         for_each_console(con) {
> > > > > > >                 if (con->write && con->read &&
> > > > > > > -                   (con->flags & (CON_BOOT | CON_ENABLED)) &&
> > > > > > > +                   (con->flags & CON_BOOT) &&
> > > > > > > +                   ((con->flags & CON_SUSPENDED) == 0) &&
> > > > > >
> > > > > > I haven't tried running the code, so I could easily be mistaken,
> > > > > > but...
> > > > > >
> > > > > > ...the above doesn't seem like the correct conversion. The old
> > > > > > expression was:
> > > > > >
> > > > > > (con->flags & (CON_BOOT | CON_ENABLED))
> > > > > >
> > > It is easy to get confused by the register_console() code. And
> > > it has been even worse some years ago.
> > >
> > > Anyway, the current code sets CON_ENABLED for all registered
> > > consoles, including CON_BOOT consoles. The flag is cleared only
> > > by console_suspend()[*] or unregister_console().
> > >
> > > IMHO, kgdboc_earlycon_init() does not need to care about
> > > console_suspend() because the kernel could not be suspended
> > > during boot. Does this makes sense?
> >
> > Yeah, makes sense to me.
> >
> > > Resume:
> > >
> > > I would remove the check of CON_ENABLED or CON_SUSPENDED
> > > from kgdboc_earlycon_init() completely.
> > >
> > > IMHO, we should keep the check of CON_BOOT because we do not
> > > want to register "normal" console drivers as kgdboc_earlycon_io_ops.
> > > It is later removed by kgdboc_earlycon_deinit(). I guess
> > > that the code is not ready to handle a takeover from normal
> > > to normal (even the same) console driver.
> >
> > I'm not sure I understand your last sentence there. In general the
> > code handling all of the possible handoff (or lack of handoff) cases
> > between kgdboc_earlycon and regular kgdboc is pretty wacky. At one
> > point I thought through it all and tried to test as many scenarios as
> > I could and I seem to remember trying to model some of the behavior on
> > how earlycon worked. If something looks broken here then let me know.
>
> Later update: The code is safe. The scenario below does not exist,
>               see the "WAIT:" section below.
>
>
> I am not familiar with the kgdb init code. I thought about the
> following scenario:
>
> 1. kgdboc_earlycon_init() registers some struct console via
>
>         kgdboc_earlycon_io_ops.cons = con;
>         pr_info("Going to register kgdb with earlycon '%s'\n", con->name);
>         if (kgdb_register_io_module(&kgdboc_earlycon_io_ops) != 0) {
>
>    and sets
>
>                 earlycon_orig_exit = con->exit;
>                 con->exit = kgdboc_earlycon_deferred_exit;
>
>
> 2. Later, configure_kgdboc() would find some "preferred" console
>    and register it via
>
>         for_each_console_srcu(cons) {
>                 int idx;
>                 if (cons->device && cons->device(cons, &idx) == p &&
>                     idx == tty_line) {
>                         kgdboc_io_ops.cons = cons;
> [...]
>         err = kgdb_register_io_module(&kgdboc_io_ops);
>
>    , where kgdb_register_io_module would call deinit for the
>    previously registered kgdboc_earlycon_io_ops:
>
>         if (old_dbg_io_ops) {
>                 old_dbg_io_ops->deinit();
>                 return 0;
>         }
>
>    , where kgdboc_earlycon_deinit() might call the .exit() callback
>
>                 kgdboc_earlycon_io_ops.cons->exit(kgdboc_earlycon_io_ops.cons);
>
>
> BANG: If both "kgdboc_earlycon_io_ops" and "kgdboc_io_ops" pointed to
>       the same struct console then this might call .exit() callback
>       for a console which is still being used.
>
>       But I am wrong, see below.
>
> WAIT:
>
> I have got all the pieces together when writing this mail).
> I see that the code is safe, namely:
>
> static void kgdboc_earlycon_deinit(void)
> {
>         if (!kgdboc_earlycon_io_ops.cons)
>                 return;
>
>         if (kgdboc_earlycon_io_ops.cons->exit == kgdboc_earlycon_deferred_exit)
>                 /*
>                  * kgdboc_earlycon is exiting but original boot console exit
>                  * was never called (AKA kgdboc_earlycon_deferred_exit()
>                  * didn't ever run).  Undo our trap.
>                  */
>                 kgdboc_earlycon_io_ops.cons->exit = earlycon_orig_exit;
>         else if (kgdboc_earlycon_io_ops.cons->exit)
>                 /*
>                  * We skipped calling the exit() routine so we could try to
>                  * keep using the boot console even after it went away.  We're
>                  * finally done so call the function now.
>                  */
>                 kgdboc_earlycon_io_ops.cons->exit(kgdboc_earlycon_io_ops.cons);
>
>         kgdboc_earlycon_io_ops.cons = NULL;
> }
>
> It calls kgdboc_earlycon_io_ops.cons->exit() only when
> unregister_console() tried to call the original con->exit()
> which was reassigned to kgdboc_earlycon_deferred_exit()...
>
> Updated resume:
>
> It looks to me that even normal console can be used by
> kgdboc_earlycon_io_ops and we could remove even the check
> of the CON_BOOT flag.
>
> My expectation:
>
> If a "struct console" is registered then the driver is used
> by printk() and it should be safe even for kgdboc_earlycon,
> as long as it has both "con->write" and "con->read" callbacks.
>
> So the check in kgdboc_earlycon_init() might be:
>
>         for_each_console(con) {
>                 if (con->write && con->read &&
>                     (!opt || !opt[0] || strcmp(con->name, opt) == 0))
>                         break;
>         }
>
> Heh, I hope that you were able to follow my thoughts and I did not
> create even bigger confusion.

I didn't go back to the code and trace through every step you made,
but it sounds like the code looks OK even if it's a bit convoluted
(sorry about that! at least it's commented!).

I agree with your final suggestion for the check. That has the side
effect of also being less of a change from what we had before. Because
of how the code was written before, it would have allowed any console
because it checked for "enabled or boot" and all consoles were
enabled. So just getting rid of the check for flags seems reasonable
to me.

-Doug

Re: [Kgdb-bugreport] [PATCH 2/7] printk: Use consoles_suspended flag when suspending/resuming all consoles

[Petr M. <pm...@su...>]

On Fri 2025-06-06 23:53:44, Marcos Paulo de Souza wrote:
> Instead of update a per-console CON_SUSPENDED flag, use the console_list
> locks to protect this flag. This is also applied to console_is_usable
> functions, which now also checks if consoles_suspend is set.
> 
> Signed-off-by: Marcos Paulo de Souza <mpd...@su...>
> ---
>  kernel/printk/internal.h |  7 ++++++-
>  kernel/printk/nbcon.c    |  8 ++++----
>  kernel/printk/printk.c   | 23 ++++++++++-------------
>  3 files changed, 20 insertions(+), 18 deletions(-)
> 
> diff --git a/kernel/printk/internal.h b/kernel/printk/internal.h
> index 48a24e7b309db20fdd7419f7aeda68ea7c79fd80..752101904f44b13059b6a922519d88e24c9f32c0 100644
> --- a/kernel/printk/internal.h
> +++ b/kernel/printk/internal.h
> @@ -118,8 +118,12 @@ void nbcon_kthreads_wake(void);
>   * which can also play a role in deciding if @con can be used to print
>   * records.
>   */
> -static inline bool console_is_usable(struct console *con, short flags, bool use_atomic)
> +static inline bool console_is_usable(struct console *con, short flags,
> +				     bool use_atomic, bool consoles_suspended)
>  {
> +	if (consoles_suspended)
> +		return false;
> +
>  	if (!(flags & CON_ENABLED))
>  		return false;
>  
> @@ -212,6 +216,7 @@ extern bool have_boot_console;
>  extern bool have_nbcon_console;
>  extern bool have_legacy_console;
>  extern bool legacy_allow_panic_sync;
> +extern bool consoles_suspended;
>  
>  /**
>   * struct console_flush_type - Define available console flush methods
> diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
> index fd12efcc4aeda8883773d9807bc215f6e5cdf71a..72de12396e6f1bc5234acfdf6dcc393acf88d216 100644
> --- a/kernel/printk/nbcon.c
> +++ b/kernel/printk/nbcon.c
> @@ -1147,7 +1147,7 @@ static bool nbcon_kthread_should_wakeup(struct console *con, struct nbcon_contex
>  	cookie = console_srcu_read_lock();
>  
>  	flags = console_srcu_read_flags(con);
> -	if (console_is_usable(con, flags, false)) {
> +	if (console_is_usable(con, flags, false, consoles_suspended)) {

The new global console_suspended value has the be synchronized the
same way as the current CON_SUSPENDED per-console flag.
It means that the value must be:

  + updated only under console_list_lock together with
    synchronize_rcu().

  + read using READ_ONCE() under console_srcu_read_lock()


I am going to propose more solutions because no one is obviously
the best one.

Variant A:
=========

Create a helper functions, similar to
console_srcu_read_flags() and console_srcu_write_flags():

Something like:

static inline bool console_srcu_read_consoles_suspended()
{
	WARN_ON_ONCE(!console_srcu_read_lock_is_held());

	/*
	 * The READ_ONCE() matches the WRITE_ONCE() when the value
	 * is modified console_srcu_write_consoles_suspended().
	 */
	return data_race(READ_ONCE(consoles_suspended));
}

static inline void console_srcu_write_consoles_suspended(bool suspended)
{
	lockdep_assert_console_list_lock_held();

	/* This matches the READ_ONCE() in console_srcu_read_consoles_suspended(). */
	WRITE_ONCE(consoles_suspended, suspended);
}

This has the drawback that most console_is_usable() callers would need
to get and pass both variables, for example:

--- a/kernel/printk/nbcon.c
+++ b/kernel/printk/nbcon.c
@@ -1137,6 +1137,7 @@ static bool nbcon_emit_one(struct nbcon_write_context *wctxt, bool use_atomic)
  */
 static bool nbcon_kthread_should_wakeup(struct console *con, struct nbcon_context *ctxt)
 {
+	bool cons_suspended;
 	bool ret = false;
 	short flags;
 	int cookie;
@@ -1147,7 +1148,8 @@ static bool nbcon_kthread_should_wakeup(struct console *con, struct nbcon_contex
 	cookie = console_srcu_read_lock();
 
 	flags = console_srcu_read_flags(con);
-	if (console_is_usable(con, flags, false)) {
+	cons_suspended = console_srcu_read_consoles_suspended();
+	if (console_is_usable(con, flags, false, cons_suspended)) {
 		/* Bring the sequence in @ctxt up to date */
 		ctxt->seq = nbcon_seq_read(con);

Pros:

   + always correct

Cons:

   + not user friendly



Variant B:
==========

Do not pass @consoles_suspended as a parameter. Instead, read it
in console_us_usable() directly.

I do not like this because it is not consistent with the con->flags
handling and it is not clear why.


Variant C:
==========

Remove even @flags parameter from console_is_usable() and read both
values there directly.

Many callers read @flags only because they call console_is_usable().
The change would simplify the code.

But there are few exceptions:

  1. __nbcon_atomic_flush_pending(), console_flush_all(),
     and legacy_kthread_should_wakeup() pass @flags to
     console_is_usable() and also check CON_NBCON flag.

     But CON_NBCON flag is special. It is statically initialized
     and never set/cleared at runtime. It can be checked without
     READ_ONCE(). Well, we still might want to be sure that
     the struct console can't disappear.

     IMHO, this can be solved by a helper function:

	/**
	 * console_srcu_is_nbcon - Locklessly check whether the console is nbcon
	 * @con:	struct console pointer of console to check
	 *
	 * Requires console_srcu_read_lock to be held, which implies that @con might
	 * be a registered console. The purpose of holding console_srcu_read_lock is
	 * to guarantee that no exit/cleanup routines will run if the console
	 * is currently undergoing unregistration.
	 *
	 * If the caller is holding the console_list_lock or it is _certain_ that
	 * @con is not and will not become registered, the caller may read
	 * @con->flags directly instead.
	 *
	 * Context: Any context.
	 * Return: True when CON_NBCON flag is set.
	 */
	static inline bool console_is_nbcon(const struct console *con)
	{
		WARN_ON_ONCE(!console_srcu_read_lock_is_held());

		/*
		 * The CON_NBCON flag is statically initialized and is never
		 * set or cleared at runtime.
		return data_race(con->flags & CON_NBCON);
	}


   2. Another exception is __pr_flush() where console_is_usable() is
      called twice with @use_atomic set "true" and "false".

      We would want to read "con->flags" only once here. A solution
      would be to add a parameter to check both con->write_atomic
      and con->write_thread in a single call.

      But it might actually be enough to check is with the "false"
      value because "con->write_thread()" is mandatory for nbcon
      consoles. And legacy consoles do not distinguish atomic mode.


Variant D:
==========

We need to distinguish the global and per-console "suspended" flag
because they might be nested. But we could use a separate flag
for the global setting.

I mean that:

    + console_suspend() would set CON_SUSPENDED flag
    + console_suspend_all() would set CON_SUSPENDED_ALL flag

They both will be in con->flags.

Pros:

    + It is easy to implement.

Cons:

    + It feels a bit ugly.


My opinion:
===========

I personally prefer the variant C because:

  + Removes one parameter from console_is_usable().

  + The lockless synchronization of both global and per-console
    flags is hidden in console_is_usable().

  + The global console_suspended flag will be stored in global
    variable (in compare with variant D).

What do you think, please?

Best Regards,
Petr


PS: The commit message and the cover letter should better explain
    the background of this change.

    It would be great if the cover letter described the bigger
    picture, especially the history of the console_suspended,
    CON_SUSPENDED, and CON_ENABLED flags. It might use info
    from
    https://lore.kernel.org/lkml/Zyo...@pa.../
    and maybe even this link.

    Also this commit message should mention that it partly reverts
    the commit 9e70a5e109a4a233678 ("printk: Add per-console
    suspended state"). But it is not simple revert because
    we need to preserve the synchronization using
    the console_list_lock for writing and SRCU for reading.

Re: [Kgdb-bugreport] [PATCH 4/7] drivers: serial: kgdboc: Check CON_SUSPENDED instead of CON_ENABLED

[Petr M. <pm...@su...>]

On Thu 2025-06-12 16:16:09, Doug Anderson wrote:
> Hi,
> 
> On Thu, Jun 12, 2025 at 6:57 AM Petr Mladek <pm...@su...> wrote:
> >
> > > > > > @@ -577,7 +577,8 @@ static int __init kgdboc_earlycon_init(char
> > > > > > *opt)
> > > > > >         console_list_lock();
> > > > > >         for_each_console(con) {
> > > > > >                 if (con->write && con->read &&
> > > > > > -                   (con->flags & (CON_BOOT | CON_ENABLED)) &&
> > > > > > +                   (con->flags & CON_BOOT) &&
> > > > > > +                   ((con->flags & CON_SUSPENDED) == 0) &&
> > > > >
> > > > > I haven't tried running the code, so I could easily be mistaken,
> > > > > but...
> > > > >
> > > > > ...the above doesn't seem like the correct conversion. The old
> > > > > expression was:
> > > > >
> > > > > (con->flags & (CON_BOOT | CON_ENABLED))
> > > > >
> > It is easy to get confused by the register_console() code. And
> > it has been even worse some years ago.
> >
> > Anyway, the current code sets CON_ENABLED for all registered
> > consoles, including CON_BOOT consoles. The flag is cleared only
> > by console_suspend()[*] or unregister_console().
> >
> > IMHO, kgdboc_earlycon_init() does not need to care about
> > console_suspend() because the kernel could not be suspended
> > during boot. Does this makes sense?
> 
> Yeah, makes sense to me.
> 
> > Resume:
> >
> > I would remove the check of CON_ENABLED or CON_SUSPENDED
> > from kgdboc_earlycon_init() completely.
> >
> > IMHO, we should keep the check of CON_BOOT because we do not
> > want to register "normal" console drivers as kgdboc_earlycon_io_ops.
> > It is later removed by kgdboc_earlycon_deinit(). I guess
> > that the code is not ready to handle a takeover from normal
> > to normal (even the same) console driver.
> 
> I'm not sure I understand your last sentence there. In general the
> code handling all of the possible handoff (or lack of handoff) cases
> between kgdboc_earlycon and regular kgdboc is pretty wacky. At one
> point I thought through it all and tried to test as many scenarios as
> I could and I seem to remember trying to model some of the behavior on
> how earlycon worked. If something looks broken here then let me know.

Later update: The code is safe. The scenario below does not exist,
	      see the "WAIT:" section below.


I am not familiar with the kgdb init code. I thought about the
following scenario:

1. kgdboc_earlycon_init() registers some struct console via

	kgdboc_earlycon_io_ops.cons = con;
	pr_info("Going to register kgdb with earlycon '%s'\n", con->name);
	if (kgdb_register_io_module(&kgdboc_earlycon_io_ops) != 0) {

   and sets

		earlycon_orig_exit = con->exit;
		con->exit = kgdboc_earlycon_deferred_exit;


2. Later, configure_kgdboc() would find some "preferred" console
   and register it via

	for_each_console_srcu(cons) {
		int idx;
		if (cons->device && cons->device(cons, &idx) == p &&
		    idx == tty_line) {
			kgdboc_io_ops.cons = cons;
[...]
	err = kgdb_register_io_module(&kgdboc_io_ops);

   , where kgdb_register_io_module would call deinit for the
   previously registered kgdboc_earlycon_io_ops:

	if (old_dbg_io_ops) {
		old_dbg_io_ops->deinit();
		return 0;
	}

   , where kgdboc_earlycon_deinit() might call the .exit() callback

		kgdboc_earlycon_io_ops.cons->exit(kgdboc_earlycon_io_ops.cons);


BANG: If both "kgdboc_earlycon_io_ops" and "kgdboc_io_ops" pointed to
      the same struct console then this might call .exit() callback
      for a console which is still being used.

      But I am wrong, see below.

WAIT:

I have got all the pieces together when writing this mail).
I see that the code is safe, namely:

static void kgdboc_earlycon_deinit(void)
{
	if (!kgdboc_earlycon_io_ops.cons)
		return;

	if (kgdboc_earlycon_io_ops.cons->exit == kgdboc_earlycon_deferred_exit)
		/*
		 * kgdboc_earlycon is exiting but original boot console exit
		 * was never called (AKA kgdboc_earlycon_deferred_exit()
		 * didn't ever run).  Undo our trap.
		 */
		kgdboc_earlycon_io_ops.cons->exit = earlycon_orig_exit;
	else if (kgdboc_earlycon_io_ops.cons->exit)
		/*
		 * We skipped calling the exit() routine so we could try to
		 * keep using the boot console even after it went away.  We're
		 * finally done so call the function now.
		 */
		kgdboc_earlycon_io_ops.cons->exit(kgdboc_earlycon_io_ops.cons);

	kgdboc_earlycon_io_ops.cons = NULL;
}

It calls kgdboc_earlycon_io_ops.cons->exit() only when
unregister_console() tried to call the original con->exit()
which was reassigned to kgdboc_earlycon_deferred_exit()...

Updated resume:

It looks to me that even normal console can be used by
kgdboc_earlycon_io_ops and we could remove even the check
of the CON_BOOT flag.

My expectation:

If a "struct console" is registered then the driver is used
by printk() and it should be safe even for kgdboc_earlycon,
as long as it has both "con->write" and "con->read" callbacks.

So the check in kgdboc_earlycon_init() might be:

	for_each_console(con) {
		if (con->write && con->read &&
		    (!opt || !opt[0] || strcmp(con->name, opt) == 0))
			break;
	}

Heh, I hope that you were able to follow my thoughts and I did not
create even bigger confusion.

Best Regards,
Petr

Re: [Kgdb-bugreport] [PATCH 4/7] drivers: serial: kgdboc: Check CON_SUSPENDED instead of CON_ENABLED

[Doug A. <dia...@ch...>]

Hi,

On Thu, Jun 12, 2025 at 6:57 AM Petr Mladek <pm...@su...> wrote:
>
> > > > > @@ -577,7 +577,8 @@ static int __init kgdboc_earlycon_init(char
> > > > > *opt)
> > > > >         console_list_lock();
> > > > >         for_each_console(con) {
> > > > >                 if (con->write && con->read &&
> > > > > -                   (con->flags & (CON_BOOT | CON_ENABLED)) &&
> > > > > +                   (con->flags & CON_BOOT) &&
> > > > > +                   ((con->flags & CON_SUSPENDED) == 0) &&
> > > >
> > > > I haven't tried running the code, so I could easily be mistaken,
> > > > but...
> > > >
> > > > ...the above doesn't seem like the correct conversion. The old
> > > > expression was:
> > > >
> > > > (con->flags & (CON_BOOT | CON_ENABLED))
> > > >
> > > > That would evaluate to non-zero (true) if the console was _either_
> > > > "boot" or "enabled".
> > > >
> > > > The new expression is is:
> > > >
> > > > (con->flags & CON_BOOT) && ((con->flags & CON_SUSPENDED) == 0)
> > > >
> > > > That's only true if the console is _both_ "boot" and "not suspended".
> > >
> > > My idea here was that the users of for_each_console would find the
> > > first available console, and by available I would expect them to be
> > > usable. In this case, is there any value for kgdboc to use a console
> > > that is suspended? Would it work in this case?
> > >
> > > I never really used kgdboc, but only checking if the console was
> > > enabled (which it's always the case here) was something that needed to
> > > be fixed.
> > >
> > > Maybe I'm missing something here as well, so please let me know if I
> > > should remove the new check.
> >
> > So it's been 5 years since I wrote the code, but reading that I was
> > checking for:
> >
> >   (con->flags & (CON_BOOT | CON_ENABLED))
> >
> > Makes me believe that this was the case when I wrote the code:
> > 1. Early boot consoles (earlycon) were not marked as CON_ENABLED but
> > were instead marked as CON_BOOT.
> > 2. Once consoles became non-early they were moved to CON_ENABLED.
> >
> > ...and the code was basically looking for any consoles with a matching
> > name that were either boot consoles or normal/enabled consoles.
> >
> > Is that a plausible theory? It's also possible that I just was
> > confused when I code things up and that I really meant to write:
>
> It is easy to get confused by the register_console() code. And
> it has been even worse some years ago.
>
> Anyway, the current code sets CON_ENABLED for all registered
> consoles, including CON_BOOT consoles. The flag is cleared only
> by console_suspend()[*] or unregister_console().
>
> IMHO, kgdboc_earlycon_init() does not need to care about
> console_suspend() because the kernel could not be suspended
> during boot. Does this makes sense?

Yeah, makes sense to me.


> Also it does not need to care about unregister_console() because
> the unregistered console won't be listed by for_each_console().
>
> [*] The 1st patch in this patchset updates console_suspend()
>     to set CON_SUSPENDED instead of clearing CON_ENABLED.
>     It helps to make it clear that it is suspend-related.
>
>
> Resume:
>
> I would remove the check of CON_ENABLED or CON_SUSPENDED
> from kgdboc_earlycon_init() completely.
>
> IMHO, we should keep the check of CON_BOOT because we do not
> want to register "normal" console drivers as kgdboc_earlycon_io_ops.
> It is later removed by kgdboc_earlycon_deinit(). I guess
> that the code is not ready to handle a takeover from normal
> to normal (even the same) console driver.

I'm not sure I understand your last sentence there. In general the
code handling all of the possible handoff (or lack of handoff) cases
between kgdboc_earlycon and regular kgdboc is pretty wacky. At one
point I thought through it all and tried to test as many scenarios as
I could and I seem to remember trying to model some of the behavior on
how earlycon worked. If something looks broken here then let me know.


> To make it clear, I would use:
>
>         for_each_console(con) {
>                 if (con->write && con->read &&
>                     (con->flags & CON_BOOT) &&
>                     (!opt || !opt[0] || strcmp(con->name, opt) == 0))
>                         break;
>         }
>
> And I would do this change as the 1st patch in the patchset
> before we change the flag modified by console_suspend().

Seems OK to me.

-Doug

Re: [Kgdb-bugreport] [PATCH 4/7] drivers: serial: kgdboc: Check CON_SUSPENDED instead of CON_ENABLED

[Petr M. <pm...@su...>]

On Tue 2025-06-10 16:18:22, Doug Anderson wrote:
> Hi,
> 
> On Tue, Jun 10, 2025 at 1:03 PM Marcos Paulo de Souza
> <mpd...@su...> wrote:
> >
> > On Mon, 2025-06-09 at 13:13 -0700, Doug Anderson wrote:
> > > Hi,
> > >
> > > On Fri, Jun 6, 2025 at 7:54 PM Marcos Paulo de Souza
> > > <mpd...@su...> wrote:
> > > >
> > > > All consoles found on for_each_console are registered, meaning that
> > > > all of
> > > > them are CON_ENABLED. The code tries to find an active console, so
> > > > check if the
> > > > console is not suspended instead.
> > > >
> > > > Signed-off-by: Marcos Paulo de Souza <mpd...@su...>
> > > > ---
> > > >  drivers/tty/serial/kgdboc.c | 3 ++-
> > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/tty/serial/kgdboc.c
> > > > b/drivers/tty/serial/kgdboc.c
> > > > index
> > > > 85f6c5a76e0fff556f86f0d45ebc5aadf5b191e8..af6d2208b8ddb82d62f33292b
> > > > 006b2923583a0d2 100644
> > > > --- a/drivers/tty/serial/kgdboc.c
> > > > +++ b/drivers/tty/serial/kgdboc.c
> > > > @@ -577,7 +577,8 @@ static int __init kgdboc_earlycon_init(char
> > > > *opt)
> > > >         console_list_lock();
> > > >         for_each_console(con) {
> > > >                 if (con->write && con->read &&
> > > > -                   (con->flags & (CON_BOOT | CON_ENABLED)) &&
> > > > +                   (con->flags & CON_BOOT) &&
> > > > +                   ((con->flags & CON_SUSPENDED) == 0) &&
> > >
> > > I haven't tried running the code, so I could easily be mistaken,
> > > but...
> > >
> > > ...the above doesn't seem like the correct conversion. The old
> > > expression was:
> > >
> > > (con->flags & (CON_BOOT | CON_ENABLED))
> > >
> > > That would evaluate to non-zero (true) if the console was _either_
> > > "boot" or "enabled".
> > >
> > > The new expression is is:
> > >
> > > (con->flags & CON_BOOT) && ((con->flags & CON_SUSPENDED) == 0)
> > >
> > > That's only true if the console is _both_ "boot" and "not suspended".
> >
> > My idea here was that the users of for_each_console would find the
> > first available console, and by available I would expect them to be
> > usable. In this case, is there any value for kgdboc to use a console
> > that is suspended? Would it work in this case?
> >
> > I never really used kgdboc, but only checking if the console was
> > enabled (which it's always the case here) was something that needed to
> > be fixed.
> >
> > Maybe I'm missing something here as well, so please let me know if I
> > should remove the new check.
> 
> So it's been 5 years since I wrote the code, but reading that I was
> checking for:
> 
>   (con->flags & (CON_BOOT | CON_ENABLED))
> 
> Makes me believe that this was the case when I wrote the code:
> 1. Early boot consoles (earlycon) were not marked as CON_ENABLED but
> were instead marked as CON_BOOT.
> 2. Once consoles became non-early they were moved to CON_ENABLED.
> 
> ...and the code was basically looking for any consoles with a matching
> name that were either boot consoles or normal/enabled consoles.
> 
> Is that a plausible theory? It's also possible that I just was
> confused when I code things up and that I really meant to write:

It is easy to get confused by the register_console() code. And
it has been even worse some years ago.

Anyway, the current code sets CON_ENABLED for all registered
consoles, including CON_BOOT consoles. The flag is cleared only
by console_suspend()[*] or unregister_console().

IMHO, kgdboc_earlycon_init() does not need to care about
console_suspend() because the kernel could not be suspended
during boot. Does this makes sense?

Also it does not need to care about unregister_console() because
the unregistered console won't be listed by for_each_console().

[*] The 1st patch in this patchset updates console_suspend()
    to set CON_SUSPENDED instead of clearing CON_ENABLED.
    It helps to make it clear that it is suspend-related.


Resume:

I would remove the check of CON_ENABLED or CON_SUSPENDED
from kgdboc_earlycon_init() completely.

IMHO, we should keep the check of CON_BOOT because we do not
want to register "normal" console drivers as kgdboc_earlycon_io_ops.
It is later removed by kgdboc_earlycon_deinit(). I guess
that the code is not ready to handle a takeover from normal
to normal (even the same) console driver.

To make it clear, I would use:

	for_each_console(con) {
		if (con->write && con->read &&
		    (con->flags & CON_BOOT) &&
		    (!opt || !opt[0] || strcmp(con->name, opt) == 0))
			break;
	}

And I would do this change as the 1st patch in the patchset
before we change the flag modified by console_suspend().

Best Regards,
Petr

Re: [Kgdb-bugreport] [PATCH 1/7] printk: Make console_{suspend, resume} handle CON_SUSPENDED

[Petr M. <pm...@su...>]

On Fri 2025-06-06 23:53:43, Marcos Paulo de Souza wrote:
> Since commit 9e70a5e109a4 ("printk: Add per-console suspended state") the
> CON_SUSPENDED flag was introced, and this flag was being checked on
> console_is_usable function, which returns false if the console is suspended.
> 
> No functional changes.

I double checked potential functional changes. In particular, I
checked where the CON_ENABLED and CON_SUSPENDED flags were used.

Both flags seems to have the same effect in most situations,
for example, in console_is_usable() or console_unblank().

But there seems to be two exceptions: kdb_msg_write() and
show_cons_active(). These two functions check only
the CON_ENABLED flag. And they think that the console is
usable when the flag is set.

The change in this patch would change the behavior of the two
functions during suspend. It is later fixed by the 3rd and 4th
patch. But it might cause regressions during bisections.

It is probably not a big deal because the system is not much
usable during the suspend anyway. But still, I would feel more
comfortable if we prevented the "temporary" regression.

I see two possibilities:

   1. Merge the 3rd and 4th patch into this one. It would change
      the semantic in a single patch.

   2. First update kdb_msg_write() and show_cons_active()
      to check both CON_ENABLE and CON_SUSPENDED flags.

The 1st solution probably makes more sense because we are going
to remove the CON_ENABLE flag in the end. And even the merged
patch is small enough.

Best Regards,
Petr

Re: [Kgdb-bugreport] [PATCH 3/7] drivers: tty: Check CON_SUSPENDED instead of CON_ENABLED

[Petr M. <pm...@su...>]

On Fri 2025-06-06 23:53:45, Marcos Paulo de Souza wrote:
> All consoles found on for_each_console are registered, meaning that all of
> them are CON_ENABLED. The code tries to find an active console, so check if the
> console is not suspended instead.

This patch "fixes" a behavior change caused by the 1st patch. Please,
merge it into the 1st patch to avoid regressions when bisecting.

Best Regards,
Petr

Re: [Kgdb-bugreport] [PATCH][next] kdb: remove redundant check for scancode 0xe0

[Doug A. <dia...@ch...>]

Hi,

On Wed, May 7, 2025 at 3:44 AM Colin Ian King <col...@gm...> wrote:
>
> The check for scancode 0xe0 is always false because earlier on
> the scan code is masked with 0x7f so there are never going to
> be values greater than 0x7f. Remove the redundant check.
>
> Signed-off-by: Colin Ian King <col...@gm...>
> ---
>  kernel/debug/kdb/kdb_keyboard.c | 3 ---
>  1 file changed, 3 deletions(-)
>
> diff --git a/kernel/debug/kdb/kdb_keyboard.c b/kernel/debug/kdb/kdb_keyboard.c
> index 3a74604fdb8a..386d30e530b7 100644
> --- a/kernel/debug/kdb/kdb_keyboard.c
> +++ b/kernel/debug/kdb/kdb_keyboard.c
> @@ -145,9 +145,6 @@ int kdb_get_kbd_char(void)
>                 return CTRL('F');
>         }
>
> -       if (scancode == 0xe0)
> -               return -1;
> -

What a glorious bit of undocumented code. I agree that this is dead
code and should be safe to remove.

Reviewed-by: Douglas Anderson <dia...@ch...>

Re: [Kgdb-bugreport] [PATCH 4/7] drivers: serial: kgdboc: Check CON_SUSPENDED instead of CON_ENABLED

[Doug A. <dia...@ch...>]

Hi,

On Tue, Jun 10, 2025 at 1:03 PM Marcos Paulo de Souza
<mpd...@su...> wrote:
>
> On Mon, 2025-06-09 at 13:13 -0700, Doug Anderson wrote:
> > Hi,
> >
> > On Fri, Jun 6, 2025 at 7:54 PM Marcos Paulo de Souza
> > <mpd...@su...> wrote:
> > >
> > > All consoles found on for_each_console are registered, meaning that
> > > all of
> > > them are CON_ENABLED. The code tries to find an active console, so
> > > check if the
> > > console is not suspended instead.
> > >
> > > Signed-off-by: Marcos Paulo de Souza <mpd...@su...>
> > > ---
> > >  drivers/tty/serial/kgdboc.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/tty/serial/kgdboc.c
> > > b/drivers/tty/serial/kgdboc.c
> > > index
> > > 85f6c5a76e0fff556f86f0d45ebc5aadf5b191e8..af6d2208b8ddb82d62f33292b
> > > 006b2923583a0d2 100644
> > > --- a/drivers/tty/serial/kgdboc.c
> > > +++ b/drivers/tty/serial/kgdboc.c
> > > @@ -577,7 +577,8 @@ static int __init kgdboc_earlycon_init(char
> > > *opt)
> > >         console_list_lock();
> > >         for_each_console(con) {
> > >                 if (con->write && con->read &&
> > > -                   (con->flags & (CON_BOOT | CON_ENABLED)) &&
> > > +                   (con->flags & CON_BOOT) &&
> > > +                   ((con->flags & CON_SUSPENDED) == 0) &&
> >
> > I haven't tried running the code, so I could easily be mistaken,
> > but...
> >
> > ...the above doesn't seem like the correct conversion. The old
> > expression was:
> >
> > (con->flags & (CON_BOOT | CON_ENABLED))
> >
> > That would evaluate to non-zero (true) if the console was _either_
> > "boot" or "enabled".
> >
> > The new expression is is:
> >
> > (con->flags & CON_BOOT) && ((con->flags & CON_SUSPENDED) == 0)
> >
> > That's only true if the console is _both_ "boot" and "not suspended".
>
> My idea here was that the users of for_each_console would find the
> first available console, and by available I would expect them to be
> usable. In this case, is there any value for kgdboc to use a console
> that is suspended? Would it work in this case?
>
> I never really used kgdboc, but only checking if the console was
> enabled (which it's always the case here) was something that needed to
> be fixed.
>
> Maybe I'm missing something here as well, so please let me know if I
> should remove the new check.

So it's been 5 years since I wrote the code, but reading that I was
checking for:

  (con->flags & (CON_BOOT | CON_ENABLED))

Makes me believe that this was the case when I wrote the code:
1. Early boot consoles (earlycon) were not marked as CON_ENABLED but
were instead marked as CON_BOOT.
2. Once consoles became non-early they were moved to CON_ENABLED.

...and the code was basically looking for any consoles with a matching
name that were either boot consoles or normal/enabled consoles.

Is that a plausible theory? It's also possible that I just was
confused when I code things up and that I really meant to write:

((con->flags & (CON_BOOT | CON_ENABLED)) == (CON_BOOT | CON_ENABLED))

...AKA that I wanted consoles that were BOOT _and_ ENABLED.

In any case, I booted up the current mainline and I put a printout here. I saw:

[    0.000000] kgdboc: DOUG: console qcom_geni has flags 0x0000000f

So that means that both BOOT and ENABLED were set. That makes me feel
like it's plausible that I was confused and really meant BOOT _and_
ENABLED. I didn't spend time trying to figure out how to build/boot an
old kernel to check, though.

In any case, given my test then I think your change should be fine.
Given that it does change the boolean logic, it seems like that
deserves a mention in the commit message.

-Doug

Re: [Kgdb-bugreport] [RFC PATCH 2/2] serial: kgdboc: convert to use faux_device

[Doug A. <dia...@ch...>]

Hi,

On Tue, May 6, 2025 at 5:51 AM Roxana Nicolescu
<nic...@pr...> wrote:
>
> The kgdboc uses a "fake" platform device to handle tty drivers showing
> up late. In case the tty device is not detected during probe, it will
> return EPROBE_DEFER which means the probe will be called later when the
> tty device might be there. Before this, the kgdboc driver
> would be initialized early in the process (useful for early boot
> debugging) but then the tty device wouldn't be there, and retry wouldn't be
> done later. For a better explanation, see commit
> '68e55f61c138: ("kgdboc: Use a platform device to handle tty drivers
> showing up late")'.
>
> This replaces the platform_device usage with faux_device which was
> introduced recently for scenarios like this, where there is not real
> platform device needed. Moreover, it makes the code cleaner than before.
>
> Signed-off-by: Roxana Nicolescu <nic...@pr...>
> ---
>  drivers/tty/serial/kgdboc.c | 50 +++++++++++--------------------------
>  1 file changed, 14 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/tty/serial/kgdboc.c b/drivers/tty/serial/kgdboc.c
> index 85f6c5a76e0f..d1ffe6186685 100644
> --- a/drivers/tty/serial/kgdboc.c
> +++ b/drivers/tty/serial/kgdboc.c
> @@ -21,7 +21,7 @@
>  #include <linux/input.h>
>  #include <linux/irq_work.h>
>  #include <linux/module.h>
> -#include <linux/platform_device.h>
> +#include <linux/device/faux.h>
>  #include <linux/serial_core.h>
>
>  #define MAX_CONFIG_LEN         40
> @@ -42,7 +42,7 @@ static int kgdboc_use_kms;  /* 1 if we use kernel mode switching */
>  static struct tty_driver       *kgdb_tty_driver;
>  static int                     kgdb_tty_line;
>
> -static struct platform_device *kgdboc_pdev;
> +static struct faux_device *kgdboc_fdev;
>
>  #if IS_BUILTIN(CONFIG_KGDB_SERIAL_CONSOLE)
>  static struct kgdb_io          kgdboc_earlycon_io_ops;
> @@ -259,7 +259,7 @@ static int configure_kgdboc(void)
>         return err;
>  }
>
> -static int kgdboc_probe(struct platform_device *pdev)
> +static int kgdboc_probe(struct faux_device *fdev)
>  {
>         int ret = 0;
>
> @@ -276,47 +276,26 @@ static int kgdboc_probe(struct platform_device *pdev)
>         return ret;
>  }
>
> -static struct platform_driver kgdboc_platform_driver = {
> +struct faux_device_ops kgdboc_driver = {

nit: s/kgdboc_driver/kgdboc_faux_ops/ ?

Other than that, this seems reasonable to me. I guess I'd assume that
Greg would chime in at some point since patch #1 in this series would
need to go through him.

-Doug