Re: [RFC] mmiotrace full patch, preview 1

[corbet@lwn.net (Jonathan Corbet)]

Hey, Pekka,

A couple of little things I noticed...

> +static int post_kmmio_handler(unsigned long condition, struct pt_regs *regs)
> +{
> +	int ret = 0;
> +	struct kmmio_probe *probe;
> +	struct kmmio_fault_page *faultpage;
> +	struct kmmio_context *ctx = &get_cpu_var(kmmio_ctx);
> +
> +	if (!ctx->active)
> +		goto out;

Should that text read something like:

	if (condition != DIE_TRAP || !ctx->active)

Presumably you won't be active if something else is going wrong, but one
never knows.

> +int register_kmmio_probe(struct kmmio_probe *p)
> +{
> +	int ret = 0;
> +	unsigned long size = 0;
> +
> +	spin_lock_irq(&kmmio_lock);
> +	kmmio_count++;
> +	if (get_kmmio_probe(p->addr)) {
> +		ret = -EEXIST;
> +		goto out;
> +	}

That only checks the first page; if the probed region partially overlaps
another one found later in memory, the registration will succeed.

Maybe you want to decrement kmmio_count if you decide to return -EEXIST
(or hold off on the increment until after the test)?

In general, I worry about what happens if an interrupt handler generates
traced MMIO traffic while a fault handler is active.  It looks a lot
like the "all hell breaks loose" scenario mentioned in the comments.  Is
there some way of preventing that which I missed?  Otherwise, maybe,
should the ioremap() wrappers take an additional argument, being an IRQ
to disable while trace handlers are active?

jon