Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1337289 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v7 04/10] usb: dbc: add debug buffer Mathias Nyman <mathias.nyman@linux.intel.com> - 2016-02-18 12:40 +0100
    Re: [PATCH v7 04/10] usb: dbc: add debug buffer Lu Baolu <baolu.lu@linux.intel.com> - 2016-02-19 07:40 +0100

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

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@linux.intel.com>
> ---
>   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

[toc] | [next] | [standalone]

#1337922

On 02/18/2016 07:43 PM, Mathias Nyman wrote:
> 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@linux.intel.com>
>> ---
>>   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)

This debug buffer is only used when DBC_DEBUG is defined.

128k is a little large. I will change it to 16k.

>
>> +#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..)

Yes. This looks better.

>
>> +}
>> +
>> +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?

It helps to check the debug log especially when it shows the hardware
registers or memory block.

>
>
>> +            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.

Yes, it's only a helper for the persons who develop and debug this driver.

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

I will reduce the debug buffer size.

>  
> -Mathias
>
>

Thanks for your time.

Regards,
-Baolu

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web