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.
