* Re: [tip:x86/mm] x86, mm: fault.c, simplify kmmio_fault()
[not found] <tip-b814d41f0987c7648d7ed07471258101c95c026b@kernel.org>
@ 2009-02-20 23:38 ` Pekka Paalanen
2009-02-22 9:26 ` Ingo Molnar
0 siblings, 1 reply; 2+ messages in thread
From: Pekka Paalanen @ 2009-02-20 23:38 UTC (permalink / raw)
To: hpa, mingo, torvalds, pq, tglx, mingo, linux-kernel
On Fri, 20 Feb 2009 22:40:09 GMT
Ingo Molnar <mingo@elte.hu> wrote:
> Author: Ingo Molnar <mingo@elte.hu>
> AuthorDate: Fri, 20 Feb 2009 22:32:10 +0100
> Commit: Ingo Molnar <mingo@elte.hu>
> CommitDate: Sat, 21 Feb 2009 00:09:42 +0100
>
> x86, mm: fault.c, simplify kmmio_fault()
>
> Impact: cleanup
>
> Remove an #ifdef from kmmio_fault() - we can do this by
> providing default implementations for is_kmmio_active()
> and kmmio_handler(). The compiler optimizes it all away
> in the !CONFIG_MMIOTRACE case.
>
> Also, while at it, clean up mmiotrace.h a bit:
>
> - standard header guards
> - standard vertical spaces for structure definitions
>
> No code changed (both with mmiotrace on and off in the config):
>
> text data bss dec hex filename
> 2947 12 12 2971 b9b fault.o.before
> 2947 12 12 2971 b9b fault.o.after
>
> Cc: Pekka Paalanen <pq@iki.fi>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Ingo Molnar <mingo@elte.hu>
>
Acked-by: Pekka Paalanen <pq@iki.fi>
except for one minor detail below.
>
> ---
> arch/x86/mm/fault.c | 5 ++-
> include/linux/mmiotrace.h | 78 +++++++++++++++++++++++++++------------------
> 2 files changed, 50 insertions(+), 33 deletions(-)
>
> diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
> index 3e36614..fe99af4 100644
> --- a/arch/x86/mm/fault.c
> +++ b/arch/x86/mm/fault.c
> @@ -55,13 +55,14 @@ enum x86_pf_error_code {
> PF_INSTR = 1 << 4,
> };
>
> +/*
> + * (returns 0 if mmiotrace is disabled)
Returns 0 if the fault was not handled in mmiotrace.
> + */
> static inline int kmmio_fault(struct pt_regs *regs, unsigned long addr)
> {
> -#ifdef CONFIG_MMIOTRACE
> if (unlikely(is_kmmio_active()))
> if (kmmio_handler(regs, addr) == 1)
> return -1;
> -#endif
> return 0;
> }
>
> diff --git a/include/linux/mmiotrace.h b/include/linux/mmiotrace.h
> index 139d7c8..3d1b7bd 100644
> --- a/include/linux/mmiotrace.h
> +++ b/include/linux/mmiotrace.h
> @@ -1,5 +1,5 @@
> -#ifndef MMIOTRACE_H
> -#define MMIOTRACE_H
> +#ifndef _LINUX_MMIOTRACE_H
> +#define _LINUX_MMIOTRACE_H
>
> #include <linux/types.h>
> #include <linux/list.h>
> @@ -13,28 +13,34 @@ typedef void (*kmmio_post_handler_t)(struct kmmio_probe *,
> unsigned long condition, struct pt_regs *);
>
> struct kmmio_probe {
> - struct list_head list; /* kmmio internal list */
> - unsigned long addr; /* start location of the probe point */
> - unsigned long len; /* length of the probe region */
> - kmmio_pre_handler_t pre_handler; /* Called before addr is executed. */
> - kmmio_post_handler_t post_handler; /* Called after addr is executed */
> - void *private;
> + /* kmmio internal list: */
> + struct list_head list;
> + /* start location of the probe point: */
> + unsigned long addr;
> + /* length of the probe region: */
> + unsigned long len;
> + /* Called before addr is executed: */
> + kmmio_pre_handler_t pre_handler;
> + /* Called after addr is executed: */
> + kmmio_post_handler_t post_handler;
> + void *private;
> };
>
> +extern unsigned int kmmio_count;
> +
> +extern int register_kmmio_probe(struct kmmio_probe *p);
> +extern void unregister_kmmio_probe(struct kmmio_probe *p);
> +
> +#ifdef CONFIG_MMIOTRACE
> /* kmmio is active by some kmmio_probes? */
> static inline int is_kmmio_active(void)
> {
> - extern unsigned int kmmio_count;
> return kmmio_count;
> }
>
> -extern int register_kmmio_probe(struct kmmio_probe *p);
> -extern void unregister_kmmio_probe(struct kmmio_probe *p);
> -
> /* Called from page fault handler. */
> extern int kmmio_handler(struct pt_regs *regs, unsigned long addr);
>
> -#ifdef CONFIG_MMIOTRACE
> /* Called from ioremap.c */
> extern void mmiotrace_ioremap(resource_size_t offset, unsigned long size,
> void __iomem *addr);
> @@ -43,7 +49,17 @@ extern void mmiotrace_iounmap(volatile void __iomem *addr);
> /* For anyone to insert markers. Remember trailing newline. */
> extern int mmiotrace_printk(const char *fmt, ...)
> __attribute__ ((format (printf, 1, 2)));
> -#else
> +#else /* !CONFIG_MMIOTRACE: */
> +static inline int is_kmmio_active(void)
> +{
> + return 0;
> +}
> +
> +static inline int kmmio_handler(struct pt_regs *regs, unsigned long addr)
> +{
> + return 0;
> +}
> +
> static inline void mmiotrace_ioremap(resource_size_t offset,
> unsigned long size, void __iomem *addr)
> {
> @@ -63,28 +79,28 @@ static inline int mmiotrace_printk(const char *fmt, ...)
> #endif /* CONFIG_MMIOTRACE */
>
> enum mm_io_opcode {
> - MMIO_READ = 0x1, /* struct mmiotrace_rw */
> - MMIO_WRITE = 0x2, /* struct mmiotrace_rw */
> - MMIO_PROBE = 0x3, /* struct mmiotrace_map */
> - MMIO_UNPROBE = 0x4, /* struct mmiotrace_map */
> - MMIO_UNKNOWN_OP = 0x5, /* struct mmiotrace_rw */
> + MMIO_READ = 0x1, /* struct mmiotrace_rw */
> + MMIO_WRITE = 0x2, /* struct mmiotrace_rw */
> + MMIO_PROBE = 0x3, /* struct mmiotrace_map */
> + MMIO_UNPROBE = 0x4, /* struct mmiotrace_map */
> + MMIO_UNKNOWN_OP = 0x5, /* struct mmiotrace_rw */
> };
>
> struct mmiotrace_rw {
> - resource_size_t phys; /* PCI address of register */
> - unsigned long value;
> - unsigned long pc; /* optional program counter */
> - int map_id;
> - unsigned char opcode; /* one of MMIO_{READ,WRITE,UNKNOWN_OP} */
> - unsigned char width; /* size of register access in bytes */
> + resource_size_t phys; /* PCI address of register */
> + unsigned long value;
> + unsigned long pc; /* optional program counter */
> + int map_id;
> + unsigned char opcode; /* one of MMIO_{READ,WRITE,UNKNOWN_OP} */
> + unsigned char width; /* size of register access in bytes */
> };
>
> struct mmiotrace_map {
> - resource_size_t phys; /* base address in PCI space */
> - unsigned long virt; /* base virtual address */
> - unsigned long len; /* mapping size */
> - int map_id;
> - unsigned char opcode; /* MMIO_PROBE or MMIO_UNPROBE */
> + resource_size_t phys; /* base address in PCI space */
> + unsigned long virt; /* base virtual address */
> + unsigned long len; /* mapping size */
> + int map_id;
> + unsigned char opcode; /* MMIO_PROBE or MMIO_UNPROBE */
> };
>
> /* in kernel/trace/trace_mmiotrace.c */
> @@ -94,4 +110,4 @@ extern void mmio_trace_rw(struct mmiotrace_rw *rw);
> extern void mmio_trace_mapping(struct mmiotrace_map *map);
> extern int mmio_trace_printk(const char *fmt, va_list args);
>
> -#endif /* MMIOTRACE_H */
> +#endif /* _LINUX_MMIOTRACE_H */
>
--
Pekka Paalanen
http://www.iki.fi/pq/
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [tip:x86/mm] x86, mm: fault.c, simplify kmmio_fault()
2009-02-20 23:38 ` [tip:x86/mm] x86, mm: fault.c, simplify kmmio_fault() Pekka Paalanen
@ 2009-02-22 9:26 ` Ingo Molnar
0 siblings, 0 replies; 2+ messages in thread
From: Ingo Molnar @ 2009-02-22 9:26 UTC (permalink / raw)
To: Pekka Paalanen; +Cc: hpa, mingo, torvalds, tglx, linux-kernel
* Pekka Paalanen <pq@iki.fi> wrote:
> On Fri, 20 Feb 2009 22:40:09 GMT
> Ingo Molnar <mingo@elte.hu> wrote:
>
> > Author: Ingo Molnar <mingo@elte.hu>
> > AuthorDate: Fri, 20 Feb 2009 22:32:10 +0100
> > Commit: Ingo Molnar <mingo@elte.hu>
> > CommitDate: Sat, 21 Feb 2009 00:09:42 +0100
> >
> > x86, mm: fault.c, simplify kmmio_fault()
> >
> > Impact: cleanup
> >
> > Remove an #ifdef from kmmio_fault() - we can do this by
> > providing default implementations for is_kmmio_active()
> > and kmmio_handler(). The compiler optimizes it all away
> > in the !CONFIG_MMIOTRACE case.
> >
> > Also, while at it, clean up mmiotrace.h a bit:
> >
> > - standard header guards
> > - standard vertical spaces for structure definitions
> >
> > No code changed (both with mmiotrace on and off in the config):
> >
> > text data bss dec hex filename
> > 2947 12 12 2971 b9b fault.o.before
> > 2947 12 12 2971 b9b fault.o.after
> >
> > Cc: Pekka Paalanen <pq@iki.fi>
> > Cc: Linus Torvalds <torvalds@linux-foundation.org>
> > Signed-off-by: Ingo Molnar <mingo@elte.hu>
> >
>
> Acked-by: Pekka Paalanen <pq@iki.fi>
> except for one minor detail below.
>
> >
> > ---
> > arch/x86/mm/fault.c | 5 ++-
> > include/linux/mmiotrace.h | 78 +++++++++++++++++++++++++++------------------
> > 2 files changed, 50 insertions(+), 33 deletions(-)
> >
> > diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
> > index 3e36614..fe99af4 100644
> > --- a/arch/x86/mm/fault.c
> > +++ b/arch/x86/mm/fault.c
> > @@ -55,13 +55,14 @@ enum x86_pf_error_code {
> > PF_INSTR = 1 << 4,
> > };
> >
> > +/*
> > + * (returns 0 if mmiotrace is disabled)
>
> Returns 0 if the fault was not handled in mmiotrace.
ok, i've updated the comment. I did not mean the comment to be
an exclusive statement, but in hindsight it's indeed ambiguous.
Ingo
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2009-02-22 9:26 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <tip-b814d41f0987c7648d7ed07471258101c95c026b@kernel.org>
2009-02-20 23:38 ` [tip:x86/mm] x86, mm: fault.c, simplify kmmio_fault() Pekka Paalanen
2009-02-22 9:26 ` Ingo Molnar
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox