Messages in this thread | | | Subject | Re: [RFC] mmiotrace full patch, preview 1 | From | (Jonathan Corbet) | Date | Tue, 26 Feb 2008 10:20:08 -0700 |
| |
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
| |