To debug and validate nbcon ownership transitions, provide a new
macro debug_store() (enabled with DEBUG_NBCON) to log transition
information to the ringbuffer. If enabled, this macro only logs
to the ringbuffer and does not trigger any printing. This is to
avoid triggering recursive printing in the nbcon consoles.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 kernel/printk/printk_nbcon.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/kernel/printk/printk_nbcon.c b/kernel/printk/printk_nbcon.c
index bb379a4f6263..f9462b088439 100644
--- a/kernel/printk/printk_nbcon.c
+++ b/kernel/printk/printk_nbcon.c
@@ -10,6 +10,35 @@
  * the legacy style console_lock mechanism.
  */
 
+/*
+ * Define DEBUG_NBCON to allow for nbcon ownership transitions to be logged
+ * to the ringbuffer. The debug_store() macro only logs to the lockless
+ * ringbuffer and does not trigger any printing.
+ */
+#undef DEBUG_NBCON
+
+#ifdef DEBUG_NBCON
+/* Only write to ringbuffer. */
+int __debug_store(const char *fmt, ...)
+{
+	va_list args;
+	int r;
+
+	va_start(args, fmt);
+	r = vprintk_store(2, 7, NULL, fmt, args);
+	va_end(args);
+
+	return r;
+}
+#define debug_store(cond, fmt, ...)						\
+	do {									\
+		if (cond)							\
+			__debug_store(pr_fmt("DEBUG: " fmt), ##__VA_ARGS__)	\
+	} while (0)
+#else
+#define debug_store(cond, fmt, ...)
+#endif
+
 /**
  * nbcon_state_set - Helper function to set the console state
  * @con:	Console to update
-- 
2.39.2

On Fri 2023-07-28 02:08:27, John Ogness wrote:
> To debug and validate nbcon ownership transitions, provide a new
> macro debug_store() (enabled with DEBUG_NBCON) to log transition
> information to the ringbuffer. If enabled, this macro only logs
> to the ringbuffer and does not trigger any printing. This is to
> avoid triggering recursive printing in the nbcon consoles.
> 
> --- a/kernel/printk/printk_nbcon.c
> +++ b/kernel/printk/printk_nbcon.c
> @@ -10,6 +10,35 @@
>   * the legacy style console_lock mechanism.
>   */
>  
> +/*
> + * Define DEBUG_NBCON to allow for nbcon ownership transitions to be logged
> + * to the ringbuffer. The debug_store() macro only logs to the lockless
> + * ringbuffer and does not trigger any printing.
> + */
> +#undef DEBUG_NBCON
> +
> +#ifdef DEBUG_NBCON
> +/* Only write to ringbuffer. */
> +int __debug_store(const char *fmt, ...)
> +{
> +	va_list args;
> +	int r;
> +
> +	va_start(args, fmt);
> +	r = vprintk_store(2, 7, NULL, fmt, args);

I have never used the facility number before. It seems that they are
defined in /usr/include/sys/syslog.h, for example, see
https://sites.uclouvain.be/SystInfo/usr/include/sys/syslog.h.html

They are even somehow documented in "man 3 syslog", for example, see
https://linux.die.net/man/3/syslog

We should probably use one of the LOG_LOCALX numbers, e.g.

#define        LOG_LOCAL0        (16<<3)

Also, please use LOGLEVEL_DEBUG instead of the hard coded "7".

> +	va_end(args);
> +
> +	return r;
> +}
> +#define debug_store(cond, fmt, ...)						\
> +	do {									\
> +		if (cond)							\
> +			__debug_store(pr_fmt("DEBUG: " fmt), ##__VA_ARGS__)	\
> +	} while (0)
> +#else
> +#define debug_store(cond, fmt, ...)
> +#endif
> +
>  /**
>   * nbcon_state_set - Helper function to set the console state
>   * @con:	Console to update

Best Regards,
Petr

On 2023-07-28, Petr Mladek <pmladek@suse.com> wrote:
>> +	r = vprintk_store(2, 7, NULL, fmt, args);
>
> We should probably use one of the LOG_LOCALX numbers, e.g.
>
> #define        LOG_LOCAL0        (16<<3)
>
> Also, please use LOGLEVEL_DEBUG instead of the hard coded "7".

OK.

I am also open to dropping the function and its use. When developing the
ringbuffer I also had a similar function but never attempted to publish
it. It is useful for developing/testing/debugging, but once everything
works as it should it has no real purpose. I have no problems keeping
the debug stuff hidden in my own private toolbox.

John

On Fri 2023-07-28 23:07:08, John Ogness wrote:
> On 2023-07-28, Petr Mladek <pmladek@suse.com> wrote:
> >> +	r = vprintk_store(2, 7, NULL, fmt, args);
> >
> > We should probably use one of the LOG_LOCALX numbers, e.g.
> >
> > #define        LOG_LOCAL0        (16<<3)
> >
> > Also, please use LOGLEVEL_DEBUG instead of the hard coded "7".
> 
> OK.
> 
> I am also open to dropping the function and its use. When developing the
> ringbuffer I also had a similar function but never attempted to publish
> it. It is useful for developing/testing/debugging, but once everything
> works as it should it has no real purpose. I have no problems keeping
> the debug stuff hidden in my own private toolbox.

I do not mind to add this patch. It is actually pretty interesting
function. I wonder if it might inspire anyone for using this approach
for some other reasons.

Best Regards,
Petr

On 2023-07-28, John Ogness <john.ogness@linutronix.de> wrote:
> +/*
> + * Define DEBUG_NBCON to allow for nbcon ownership transitions to be logged
> + * to the ringbuffer. The debug_store() macro only logs to the lockless
> + * ringbuffer and does not trigger any printing.
> + */
> +#undef DEBUG_NBCON
> +
> +#ifdef DEBUG_NBCON
> +/* Only write to ringbuffer. */
> +int __debug_store(const char *fmt, ...)
> +{
> +	va_list args;
> +	int r;
> +
> +	va_start(args, fmt);
> +	r = vprintk_store(2, 7, NULL, fmt, args);
> +	va_end(args);
> +
> +	return r;
> +}
> +#define debug_store(cond, fmt, ...)						\
> +	do {									\
> +		if (cond)							\
> +			__debug_store(pr_fmt("DEBUG: " fmt), ##__VA_ARGS__)	\

Missing a semi-colon here. Wrapping this with a do-while was a
last-minute change requested by checkpatch.pl. Probably nobody would
notice because you must manually define DEBUG_NBCON by changing the
source code. Fixed for v3 (assuming Petr allows me to keep this
debugging code in place).

> +	} while (0)
> +#else
> +#define debug_store(cond, fmt, ...)
> +#endif

John