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@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)
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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
