Re: [PATCH v7 04/10] usb: dbc: add debug buffer

From: Mathias Nyman
Date: Thu Feb 18 2016 - 06:37:36 EST


On 26.01.2016 14:58, Lu Baolu wrote:
"printk" is not suitable for dbc debugging especially when console
is in usage. This patch adds a debug buffer in dbc driver and puts
the debug messages in this local buffer. The debug buffer could be
dumped whenever the console is not in use. This part of code will
not be visible unless DBC_DEBUG is defined.

Signed-off-by: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
---
drivers/usb/early/xhci-dbc.c | 62 ++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 60 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/early/xhci-dbc.c b/drivers/usb/early/xhci-dbc.c
index 41ce116..6855048 100644
--- a/drivers/usb/early/xhci-dbc.c
+++ b/drivers/usb/early/xhci-dbc.c
@@ -32,8 +32,64 @@ static struct xdbc_state xdbc_stat;
static struct xdbc_state *xdbcp = &xdbc_stat;

#ifdef DBC_DEBUG
-/* place holder */
-#define xdbc_trace printk
+#define XDBC_DEBUG_BUF_SIZE (PAGE_SIZE * 32)

Does it really need to be this huge? minimum 4096 * 32 ~ 128k
The kernel ring buffer is about the same size (16k - 256k)

+#define MSG_MAX_LINE 128

with 128 characters per line this would fit ~1000 lines

+static char xdbc_debug_buf[XDBC_DEBUG_BUF_SIZE];
+static void xdbc_trace(const char *fmt, ...)
+{
+ int i, size;
+ va_list args;
+ static int pos;
+ char temp_buf[MSG_MAX_LINE];
+
+ if (pos >= XDBC_DEBUG_BUF_SIZE - 1)
+ return;
+
+ memset(temp_buf, 0, MSG_MAX_LINE);
+ va_start(args, fmt);
+ vsnprintf(temp_buf, MSG_MAX_LINE - 1, fmt, args);
+ va_end(args);
+
+ i = 0;
+ size = strlen(temp_buf);
+ while (i < size) {
+ xdbc_debug_buf[pos] = temp_buf[i];
+ pos++;
+ i++;
+
+ if (pos >= XDBC_DEBUG_BUF_SIZE - 1)
+ break;
+ }

how about something like:

size = min(XDBC_DEBUG_BUF_SIZE - pos, size)
memcpy(xdbc_debug_buf + pos, temp_buf, size)
pos += size;

(might need some "-1" and off by one checking..)

+}
+
+static void xdbc_dump_debug_buffer(void)
+{
+ int index = 0;
+ int count = 0;
+ char dump_buf[MSG_MAX_LINE];
+
+ xdbc_trace("The end of DbC trace buffer\n");
+ pr_notice("DBC debug buffer:\n");
+ memset(dump_buf, 0, MSG_MAX_LINE);
+
+ while (index < XDBC_DEBUG_BUF_SIZE) {
+ if (!xdbc_debug_buf[index])
+ break;
+
+ if (xdbc_debug_buf[index] == '\n' ||
+ count >= MSG_MAX_LINE - 1) {
+ pr_notice("DBC: @%08x %s\n", index, dump_buf);

Is showing the he index (position in debug buffer) useful here?

+ memset(dump_buf, 0, MSG_MAX_LINE);
+ count = 0;
+ } else {
+ dump_buf[count] = xdbc_debug_buf[index];
+ count++;
+ }
+
+ index++;
+ }

So we have one huge buffer that xdbc keeps on filling as the initialization progresses.
It is never emptied, or overwritten (circular).
When dumped it always dumps the whole thing, copying one character
at a time.

As this is only used for debugging during xdbc development/debugging, and never enabled
even if xdbc early printk is used, I don't think optimization really matters.

Perhaps take a look if we really need PAGE_SIZE * 32 bytes, is xdbc driver even nearly
writing that much debug data.
-Mathias