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

[Mathias Nyman]

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