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

[Dan Raymond <draymond@foxvalley.net>]

On 8/23/2023 2:30 AM, Ilpo Järvinen wrote:

> Thanks, looks useful (although it might have challenge in tracing hw
> during early init).

I suppose there would need to be a mechanism to enable tracing by 
default (kernel cmd line?)  Is the UART driver even used very early in 
the boot process?

>> +struct reg_event {
>> +	uint32_t  cycle_lo;  /* CPU cycle count (lower 32-bits) */
>> +	uint16_t  cycle_hi;  /* CPU cycle count (upper 16-bits) */
>> +	uint8_t   access;    /* write flag + register number */
>> +	uint8_t   data;      /* register data */
> Some HW-specific registers are larger than 8 bits.

Not for 8250/16550?  Currently this feature only supports those and it 
also relies on the TSC which is an x86 thing.

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

>
>> +		num_bytes = (size < len) ? size : len;
> min_t().

Done.

>
>> +	
>> +	if (error)
>> +		return total_bytes ? total_bytes : error;
>> +	return total_bytes;
> So this is same as:
> 	if (!total_bytes)
> 		return error;
>
> 	return total_bytes;

OK, I simplified this.

> For in-kernel interfaces, use u64 and u32, uintxx_t is for userspace
> interactions.

Done.

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

>> +
>> +	cpu_cycle      = rdtsc();
>> +	event.cycle_lo = (uint32_t)cpu_cycle;
>> +	event.cycle_hi = (uint16_t)(cpu_cycle >> 32);
>> +	event.access   = (write ? 0x08 : 0x00) | reg;
> Use defines for these literals.
>
>> +static noinline void queue_free(struct uart_port *port, bool force)
>> +{
>> +	struct uart_debug *uart_debug = port->private_data;
>> +	struct reg_queue *queue = &uart_debug->register_queue;
>> +
>> +	if (force || queue->read_idx == queue->write_idx) {
> Why cannot the only place where force=true just reset the indexes before
> making the call so no force parameter is required? ...I think there's a
> bug anyway with the indexes not getting properly reset in that case.

Only the queue_xxx() functions read or write the queue structure.  The 
indices are reset below when we memset() the entire structure to 0.

>
>> +		vfree(queue->buf);
>> +		memset(queue, 0, sizeof(*queue));
>> +	}
>> ...
>> +	} else if (num_events) {
>> +		reg = event.access & 0x07;
>> +		sym = event.access & 0x08 ? out_regs[reg] : in_regs[reg];
> Some uarts have registers beyond 0x07 so this doesn't seem enough.
> It would be nice if the driver could provide alternative set of names for
> the registers.

I'll have to look into how difficult it would be to support other UARTs 
besides 8250/16550.

>> +	while (trace_size >>= 1)
>> +		rounded_size <<= 1;  /* round down to nearest power of 2 */
> Comment is certainly misplaces as it's the whole while loop which
> calculates that.

Fixed.

>> +/*
>> + *  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?

> It would make this look cleaner if you split the double assignment.

Done.

>> +
>> +	uart_debug->trace_size = 4096;
> SZ_4K

Done.

Thanks for looking at this.  I've created another patch that 
incorporates most of your suggestions.