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

[Lu Baolu <baolu.lu@linux.intel.com>]

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