Re: [PATCH] tty/serial: create debugfs interface for UART register tracing

[Dan Raymond <draymond@foxvalley.net>]

On 8/24/2023 6:13 AM, Ilpo Järvinen wrote:

>> Currently this feature only supports those and it also
>> relies on the TSC which is an x86 thing.
> 
> I wonder why you have to rely on that. Why e.g. ktime_get_boottime() is 
> not enough for this usecase?

I was motivated to make the tracing as unobtrusive as possible and it seemed
like the rdtsc instruction would be very fast and very precise.  I'll have to
profile ktime_get_boottime() to see how much overhead that adds.

>>>> +		ptr = uart_debug->line + uart_debug->offset;
>>>> +		len = strlen(ptr);
>>> Why you need to calculate length? Shouldn't queue_remove() be able to return
>>> this information?
>>
>> Yes, we can return the string length from queue_remove() but we still need to
>> call strlen() to accommodate all code paths.  The user might call read() with
>> a very small buffer and that requires us to advance ptr past the beginning of
>> the string on subsequent calls.
> 
> I still find it hard to believe you could not keep track of it all 
> without strlen(), snprintf() returns the number of chars it wrote to 
> uart_debug->line so it should be that length - uart_debug->offset?

True.  I could store the total string length and trade a little more memory and
a little more complexity for a little less CPU time.  Do you think it's a good
tradeoff?  The strlen() call should be pretty fast since the string is a maximum
of 27 characters.  Also, this is in the trace consumer path where performance
is not as important as the trace producer path.
 
>>>> +	static uint64_t cpu_freq;  /* cycles per second */
>>>> +	uint32_t h, m, s, us;
>>>> +
>>>> +	if (cpu_freq == 0)
>>>> +		cpu_freq = arch_freq_get_on_cpu(0) * 1000ULL;
>>>> +
>>>> +	s = div64_u64_rem(cpu_cycles, cpu_freq, &cpu_cycles);
>>>> +	us = div64_u64(cpu_cycles * 1000 * 1000 + 500 * 1000, cpu_freq);
>>>> +
>>>> +	m = s / 60; s = s % 60;
>>>> +	h = m / 60; m = m % 60;
>>>> +
>>>> +	snprintf(buf, size, "%02d:%02d:%02d.%06u", h, m, s, us);
>>> seconds.us is enough. If some additional formatting is to happen, it
>>> should be done in userspace.
>>
>> I can see your point.  If the user does want to reformat this it will be
>> easier to start with the format you suggested.  Is this a general rule for
>> kernel space?
> 
> I don't know if there's a rule. But having had to parse those :: inputs 
> way too many times in the past, I have very little love for that format 
> being forced on user ;-).

OK, I've changed the format from "hh:mm:ss:mmmuuu" to "ssssssss:mmmuuu".

>>>> +/*
>>>> + *  Create the debugfs interface.  This should be called during port
>>>> registration after
>>>> + *  port->name, port->serial_in, and port->serial_out have been
>>>> initialized.
>>>> We are
>>>> + *  using port->private_data to store a pointer to our data structure.
>>>> That
>>>> field appears
>>>> + *  to be otherwise unused.  If this is wrong we will need to create a
>>>> new
>>>> field.
>>>> + */
>>>> +void uart_debug_create(struct uart_port *port)
>>>> +{
>>>> +	struct uart_debug *uart_debug;
>>>> +	struct dentry *dir;
>>>> +
>>>> +	uart_debug = port->private_data = kzalloc(sizeof(struct uart_debug),
>>>> GFP_KERNEL);
>>> How about the drivers which use port->private_data?
>>
>> It didn't look like this field was used.  Was I wrong about this?
> 
> ~/linux/uart/drivers/tty/serial/8250$ git grep 'private_data =' | wc -l
> 20
> 
> There are multiple 8250 variant drivers using it.
> 
> Some also come with additional registers so it's all relevant also in 
> serial/8250/ domain.

OK, I've added a new field named 'debug_data' and I'll use that instead of
'private_data'.  My latest changes are in the v3 patch.