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

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 5136 bytes --]

On Thu, 24 Aug 2023, Dan Raymond wrote:
> 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?

I mainly meant the time when the driver is initialized, when moving from 
univ8250 -> the actual 8250 driver for the particular HW. It's one of the 
points of interest.

I don't know if the tracing side has something more "standard" for this 
already and since you're looking to that already, it's good to take 
notice if there's something.

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

Some drivers for 8250 variants have such registers. Not that they're 
common so it's perhaps not a big deal.

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

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

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

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

Ah, I see.

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


-- 
 i.