diff mbox series

[printk,v2,2/8] printk: Provide debug_store() for nbcon debugging

Message ID 20230728000233.50887-3-john.ogness@linutronix.de
State New
Headers
Series wire up nbcon consoles |

Commit Message

John Ogness July 28, 2023, 12:02 a.m. UTC 
  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(+)

Comments

John Ogness July 28, 2023, 9:52 a.m. UTC | #1 
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
Petr Mladek July 28, 2023, 3:22 p.m. UTC | #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
John Ogness July 28, 2023, 9:01 p.m. UTC | #3 
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
Petr Mladek July 31, 2023, 3:43 p.m. UTC | #4 
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
diff mbox series

Patch

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