Re: [RFC] mmiotrace full patch, preview 1
On Mon, 25 Feb 2008 14:49:22 -0800 Andrew Morton <[EMAIL PROTECTED]> wrote: > Please feed the diff through scritps/checkpatch.pl and consider addressing > the things which it finds. I checked that, but I didn't think any of them were worth fixing. And since this is a work in progress and a in a state which I do not want committed yet, I did not sign this patch. mmio-mod.c is still a mess. > WARNING: Use of volatile is usually wrong: see > Documentation/volatile-considered-harmful.txt > #1097: FILE: arch/x86/mm/mmio-mod.c:437: > +void iounmap_trace(volatile void __iomem *addr) I believe the 'volatile' belongs here, it is also in the declaration of iounmap() in arch/x86/mm/ioremap.c. > WARNING: externs should be avoided in .c files > #1766: FILE: arch/x86/mm/testmmiotrace.c:7: > +extern void __iomem *ioremap_nocache_trace(unsigned long offset, > > WARNING: Use of volatile is usually wrong: see > Documentation/volatile-considered-harmful.txt > #1768: FILE: arch/x86/mm/testmmiotrace.c:9: > +extern void iounmap_trace(volatile void __iomem *addr); > > WARNING: externs should be avoided in .c files > #1768: FILE: arch/x86/mm/testmmiotrace.c:9: > +extern void iounmap_trace(volatile void __iomem *addr); These are all in testmmiotrace.c which calls the traced ioremap functions directly. These direct calls will go away and it will call the normal ioremap functions. > WARNING: Use of volatile is usually wrong: see > Documentation/volatile-considered-harmful.txt > #1790: FILE: arch/x86/mm/testmmiotrace.c:31: > + volatile unsigned int v; Since testmmiotrace does not use v for anything other than return value of ioread8/16/32(), I wanted to prevent the compiler from optimizing it away. Now thinking it again, ioread*() must guarantee that the read is always executed. I'll remove v altogether. Patch for the other issues you mentioned is brewing. Thanks. -- Pekka Paalanen http://www.iki.fi/pq/ -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] mmiotrace full patch, preview 1
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 = _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(_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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] mmiotrace full patch, preview 1
On Mon, Feb 25, 2008 at 09:42:00PM -0500, Pavel Roskin wrote: > It looks like a mutex, it acts like a mutex, but it isn't a mutex, it's a > trap for the unwary. Weird. I was annoyed by it before; now I see a > fellow developer actually getting into that trap. > > I'd say, rename DECLARE_MUTEX to DECLARE_SEMAPHORE and let external code be > fixed one way or another (i.e. stick with the "mutex" name or stick with > the semaphore functionality if it's really needed). It's a semaphore used as mutex. Until we got struct mutex this was perfectly fine and now we're phasing it out and will hopefully get rid of it soon. It just takes some time to convert all users. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] mmiotrace full patch, preview 1
On Tue, Feb 26, 2008 at 11:49:48AM +0100, Ingo Molnar wrote: > > * Andy Whitcroft <[EMAIL PROTECTED]> wrote: > > > Ok, so that would be the following, work for everyone? > > > > WARNING: mutexes are preferred for single holder semaphores > > #1: FILE: Z95.c:1: > > + DECLARE_MUTEX(); > > > > WARNING: mutexes are preferred for single holder semaphores > > #3: FILE: Z95.c:3: > > + init_MUTEX(); > > yeah. > > Acked-by: Ingo Molnar <[EMAIL PROTECTED]> > > also i guess init_MUTEX_LOCKED() should emit a "this should be a > completion" warning. Thats easy enough. Though your tone here implies its less definatly wrong than the other use forms. Do we want gentle language here? "consider using a completion" > i guess non-DEFINE_SPINLOCK old-style spinlock definition: > > spinlock_t lock = SPIN_LOCK_UNLOCKED; > > should emit a 'use DEFINE_SPINLOCK' warning as well? Those (SPIN_LOCK_UNLOCKED & RW_LOCK_UNLOCKED) we already pick up and indicate are deprecated. -apw -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] mmiotrace full patch, preview 1
* Andy Whitcroft <[EMAIL PROTECTED]> wrote: > Ok, so that would be the following, work for everyone? > > WARNING: mutexes are preferred for single holder semaphores > #1: FILE: Z95.c:1: > + DECLARE_MUTEX(); > > WARNING: mutexes are preferred for single holder semaphores > #3: FILE: Z95.c:3: > + init_MUTEX(); yeah. Acked-by: Ingo Molnar <[EMAIL PROTECTED]> also i guess init_MUTEX_LOCKED() should emit a "this should be a completion" warning. i guess non-DEFINE_SPINLOCK old-style spinlock definition: spinlock_t lock = SPIN_LOCK_UNLOCKED; should emit a 'use DEFINE_SPINLOCK' warning as well? Ingo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] mmiotrace full patch, preview 1
On Mon, Feb 25, 2008 at 06:34:49PM -0500, Christoph Hellwig wrote: > On Mon, Feb 25, 2008 at 02:49:22PM -0800, Andrew Morton wrote: > > the things which it finds. > > > > > +static DECLARE_MUTEX(kmmio_init_mutex); > > > > That's not a mutex. > > > > > + down(_init_mutex); > > > > It's a semaphore. Please do convert it to a mutex. > > > > Andy, I'd say that addition of new semaphores is worth a warning - they're > > rarely legitimate. > > I'm not sure that any semaphore should be a warning, but the initializer > for semaphore used as binary mutex (DECLARE_MUTEX and init_MUTEX) are > worth it. Ok, so that would be the following, work for everyone? WARNING: mutexes are preferred for single holder semaphores #1: FILE: Z95.c:1: + DECLARE_MUTEX(); WARNING: mutexes are preferred for single holder semaphores #3: FILE: Z95.c:3: + init_MUTEX(); -apw -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] mmiotrace full patch, preview 1
On Mon, Feb 25, 2008 at 09:42:00PM -0500, Pavel Roskin wrote: > Quoting Christoph Hellwig <[EMAIL PROTECTED]>: > > >On Mon, Feb 25, 2008 at 02:49:22PM -0800, Andrew Morton wrote: > >>the things which it finds. > >> > >>> +static DECLARE_MUTEX(kmmio_init_mutex); > >> > >>That's not a mutex. > >> > >>> + down(_init_mutex); > >> > >>It's a semaphore. Please do convert it to a mutex. > >> > >>Andy, I'd say that addition of new semaphores is worth a warning - they're > >>rarely legitimate. > > > >I'm not sure that any semaphore should be a warning, but the initializer > >for semaphore used as binary mutex (DECLARE_MUTEX and init_MUTEX) are > >worth it. > > It looks like a mutex, it acts like a mutex, but it isn't a mutex, > it's a trap for the unwary. Weird. I was annoyed by it before; now I > see a fellow developer actually getting into that trap. > > I'd say, rename DECLARE_MUTEX to DECLARE_SEMAPHORE and let external > code be fixed one way or another (i.e. stick with the "mutex" name or > stick with the semaphore functionality if it's really needed). I like the fact that in evey architecture its defined as: #define DECLARE_MUTEX(name) __DECLARE_SEMAPHORE_GENERIC(name,1) -apw -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] mmiotrace full patch, preview 1
On Mon, Feb 25, 2008 at 09:42:00PM -0500, Pavel Roskin wrote: Quoting Christoph Hellwig [EMAIL PROTECTED]: On Mon, Feb 25, 2008 at 02:49:22PM -0800, Andrew Morton wrote: the things which it finds. +static DECLARE_MUTEX(kmmio_init_mutex); That's not a mutex. + down(kmmio_init_mutex); It's a semaphore. Please do convert it to a mutex. Andy, I'd say that addition of new semaphores is worth a warning - they're rarely legitimate. I'm not sure that any semaphore should be a warning, but the initializer for semaphore used as binary mutex (DECLARE_MUTEX and init_MUTEX) are worth it. It looks like a mutex, it acts like a mutex, but it isn't a mutex, it's a trap for the unwary. Weird. I was annoyed by it before; now I see a fellow developer actually getting into that trap. I'd say, rename DECLARE_MUTEX to DECLARE_SEMAPHORE and let external code be fixed one way or another (i.e. stick with the mutex name or stick with the semaphore functionality if it's really needed). I like the fact that in evey architecture its defined as: #define DECLARE_MUTEX(name) __DECLARE_SEMAPHORE_GENERIC(name,1) -apw -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] mmiotrace full patch, preview 1
* Andy Whitcroft [EMAIL PROTECTED] wrote: Ok, so that would be the following, work for everyone? WARNING: mutexes are preferred for single holder semaphores #1: FILE: Z95.c:1: + DECLARE_MUTEX(foo); WARNING: mutexes are preferred for single holder semaphores #3: FILE: Z95.c:3: + init_MUTEX(foo); yeah. Acked-by: Ingo Molnar [EMAIL PROTECTED] also i guess init_MUTEX_LOCKED() should emit a this should be a completion warning. i guess non-DEFINE_SPINLOCK old-style spinlock definition: spinlock_t lock = SPIN_LOCK_UNLOCKED; should emit a 'use DEFINE_SPINLOCK' warning as well? Ingo -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] mmiotrace full patch, preview 1
On Mon, Feb 25, 2008 at 06:34:49PM -0500, Christoph Hellwig wrote: On Mon, Feb 25, 2008 at 02:49:22PM -0800, Andrew Morton wrote: the things which it finds. +static DECLARE_MUTEX(kmmio_init_mutex); That's not a mutex. + down(kmmio_init_mutex); It's a semaphore. Please do convert it to a mutex. Andy, I'd say that addition of new semaphores is worth a warning - they're rarely legitimate. I'm not sure that any semaphore should be a warning, but the initializer for semaphore used as binary mutex (DECLARE_MUTEX and init_MUTEX) are worth it. Ok, so that would be the following, work for everyone? WARNING: mutexes are preferred for single holder semaphores #1: FILE: Z95.c:1: + DECLARE_MUTEX(foo); WARNING: mutexes are preferred for single holder semaphores #3: FILE: Z95.c:3: + init_MUTEX(foo); -apw -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] mmiotrace full patch, preview 1
On Tue, Feb 26, 2008 at 11:49:48AM +0100, Ingo Molnar wrote: * Andy Whitcroft [EMAIL PROTECTED] wrote: Ok, so that would be the following, work for everyone? WARNING: mutexes are preferred for single holder semaphores #1: FILE: Z95.c:1: + DECLARE_MUTEX(foo); WARNING: mutexes are preferred for single holder semaphores #3: FILE: Z95.c:3: + init_MUTEX(foo); yeah. Acked-by: Ingo Molnar [EMAIL PROTECTED] also i guess init_MUTEX_LOCKED() should emit a this should be a completion warning. Thats easy enough. Though your tone here implies its less definatly wrong than the other use forms. Do we want gentle language here? consider using a completion i guess non-DEFINE_SPINLOCK old-style spinlock definition: spinlock_t lock = SPIN_LOCK_UNLOCKED; should emit a 'use DEFINE_SPINLOCK' warning as well? Those (SPIN_LOCK_UNLOCKED RW_LOCK_UNLOCKED) we already pick up and indicate are deprecated. -apw -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] mmiotrace full patch, preview 1
On Mon, Feb 25, 2008 at 09:42:00PM -0500, Pavel Roskin wrote: It looks like a mutex, it acts like a mutex, but it isn't a mutex, it's a trap for the unwary. Weird. I was annoyed by it before; now I see a fellow developer actually getting into that trap. I'd say, rename DECLARE_MUTEX to DECLARE_SEMAPHORE and let external code be fixed one way or another (i.e. stick with the mutex name or stick with the semaphore functionality if it's really needed). It's a semaphore used as mutex. Until we got struct mutex this was perfectly fine and now we're phasing it out and will hopefully get rid of it soon. It just takes some time to convert all users. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] mmiotrace full patch, preview 1
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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] mmiotrace full patch, preview 1
On Mon, 25 Feb 2008 14:49:22 -0800 Andrew Morton [EMAIL PROTECTED] wrote: Please feed the diff through scritps/checkpatch.pl and consider addressing the things which it finds. I checked that, but I didn't think any of them were worth fixing. And since this is a work in progress and a in a state which I do not want committed yet, I did not sign this patch. mmio-mod.c is still a mess. WARNING: Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt #1097: FILE: arch/x86/mm/mmio-mod.c:437: +void iounmap_trace(volatile void __iomem *addr) I believe the 'volatile' belongs here, it is also in the declaration of iounmap() in arch/x86/mm/ioremap.c. WARNING: externs should be avoided in .c files #1766: FILE: arch/x86/mm/testmmiotrace.c:7: +extern void __iomem *ioremap_nocache_trace(unsigned long offset, WARNING: Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt #1768: FILE: arch/x86/mm/testmmiotrace.c:9: +extern void iounmap_trace(volatile void __iomem *addr); WARNING: externs should be avoided in .c files #1768: FILE: arch/x86/mm/testmmiotrace.c:9: +extern void iounmap_trace(volatile void __iomem *addr); These are all in testmmiotrace.c which calls the traced ioremap functions directly. These direct calls will go away and it will call the normal ioremap functions. WARNING: Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt #1790: FILE: arch/x86/mm/testmmiotrace.c:31: + volatile unsigned int v; Since testmmiotrace does not use v for anything other than return value of ioread8/16/32(), I wanted to prevent the compiler from optimizing it away. Now thinking it again, ioread*() must guarantee that the read is always executed. I'll remove v altogether. Patch for the other issues you mentioned is brewing. Thanks. -- Pekka Paalanen http://www.iki.fi/pq/ -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] mmiotrace full patch, preview 1
Quoting Christoph Hellwig <[EMAIL PROTECTED]>: On Mon, Feb 25, 2008 at 02:49:22PM -0800, Andrew Morton wrote: the things which it finds. > +static DECLARE_MUTEX(kmmio_init_mutex); That's not a mutex. > + down(_init_mutex); It's a semaphore. Please do convert it to a mutex. Andy, I'd say that addition of new semaphores is worth a warning - they're rarely legitimate. I'm not sure that any semaphore should be a warning, but the initializer for semaphore used as binary mutex (DECLARE_MUTEX and init_MUTEX) are worth it. It looks like a mutex, it acts like a mutex, but it isn't a mutex, it's a trap for the unwary. Weird. I was annoyed by it before; now I see a fellow developer actually getting into that trap. I'd say, rename DECLARE_MUTEX to DECLARE_SEMAPHORE and let external code be fixed one way or another (i.e. stick with the "mutex" name or stick with the semaphore functionality if it's really needed). -- Regards, Pavel Roskin -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] mmiotrace full patch, preview 1
On Mon, Feb 25, 2008 at 02:49:22PM -0800, Andrew Morton wrote: > the things which it finds. > > > +static DECLARE_MUTEX(kmmio_init_mutex); > > That's not a mutex. > > > + down(_init_mutex); > > It's a semaphore. Please do convert it to a mutex. > > Andy, I'd say that addition of new semaphores is worth a warning - they're > rarely legitimate. I'm not sure that any semaphore should be a warning, but the initializer for semaphore used as binary mutex (DECLARE_MUTEX and init_MUTEX) are worth it. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] mmiotrace full patch, preview 1
On Sun, 24 Feb 2008 19:03:23 +0200 Pekka Paalanen <[EMAIL PROTECTED]> wrote: > arch/x86/Kconfig.debug | 33 +++ > arch/x86/mm/Makefile|5 + > arch/x86/mm/fault.c | 13 + > arch/x86/mm/kmmio.c | 541 > +++ > arch/x86/mm/mmio-mod.c | 541 > +++ > arch/x86/mm/pageattr.c |1 + > arch/x86/mm/pf_in.c | 489 ++ > arch/x86/mm/pf_in.h | 39 +++ > arch/x86/mm/testmmiotrace.c | 76 ++ > include/linux/mmiotrace.h | 104 + Please feed the diff through scritps/checkpatch.pl and consider addressing the things which it finds. > +static DECLARE_MUTEX(kmmio_init_mutex); That's not a mutex. > + down(_init_mutex); It's a semaphore. Please do convert it to a mutex. Andy, I'd say that addition of new semaphores is worth a warning - they're rarely legitimate. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] mmiotrace full patch, preview 1
On Sun, 24 Feb 2008 19:03:23 +0200 Pekka Paalanen [EMAIL PROTECTED] wrote: arch/x86/Kconfig.debug | 33 +++ arch/x86/mm/Makefile|5 + arch/x86/mm/fault.c | 13 + arch/x86/mm/kmmio.c | 541 +++ arch/x86/mm/mmio-mod.c | 541 +++ arch/x86/mm/pageattr.c |1 + arch/x86/mm/pf_in.c | 489 ++ arch/x86/mm/pf_in.h | 39 +++ arch/x86/mm/testmmiotrace.c | 76 ++ include/linux/mmiotrace.h | 104 + Please feed the diff through scritps/checkpatch.pl and consider addressing the things which it finds. +static DECLARE_MUTEX(kmmio_init_mutex); That's not a mutex. + down(kmmio_init_mutex); It's a semaphore. Please do convert it to a mutex. Andy, I'd say that addition of new semaphores is worth a warning - they're rarely legitimate. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] mmiotrace full patch, preview 1
On Mon, Feb 25, 2008 at 02:49:22PM -0800, Andrew Morton wrote: the things which it finds. +static DECLARE_MUTEX(kmmio_init_mutex); That's not a mutex. + down(kmmio_init_mutex); It's a semaphore. Please do convert it to a mutex. Andy, I'd say that addition of new semaphores is worth a warning - they're rarely legitimate. I'm not sure that any semaphore should be a warning, but the initializer for semaphore used as binary mutex (DECLARE_MUTEX and init_MUTEX) are worth it. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] mmiotrace full patch, preview 1
Quoting Christoph Hellwig [EMAIL PROTECTED]: On Mon, Feb 25, 2008 at 02:49:22PM -0800, Andrew Morton wrote: the things which it finds. +static DECLARE_MUTEX(kmmio_init_mutex); That's not a mutex. + down(kmmio_init_mutex); It's a semaphore. Please do convert it to a mutex. Andy, I'd say that addition of new semaphores is worth a warning - they're rarely legitimate. I'm not sure that any semaphore should be a warning, but the initializer for semaphore used as binary mutex (DECLARE_MUTEX and init_MUTEX) are worth it. It looks like a mutex, it acts like a mutex, but it isn't a mutex, it's a trap for the unwary. Weird. I was annoyed by it before; now I see a fellow developer actually getting into that trap. I'd say, rename DECLARE_MUTEX to DECLARE_SEMAPHORE and let external code be fixed one way or another (i.e. stick with the mutex name or stick with the semaphore functionality if it's really needed). -- Regards, Pavel Roskin -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] mmiotrace full patch, preview 1
Hi Pekka.
A small nitpick..
>
> +config MMIOTRACE_HOOKS
> + bool
> + default n
n is default so no need to be explicit.
If you prefer being explcit then use:
def_bool n
> +config MMIOTRACE
> + tristate "Memory mapped IO tracing"
> + depends on DEBUG_KERNEL && RELAY && DEBUG_FS
> + select MMIOTRACE_HOOKS
> + default n
Same here.
> + help
> + This will build a kernel module called mmiotrace.
> + Making this a built-in is heavily discouraged.
> +
> + Mmiotrace traces Memory Mapped I/O access and is meant for debugging
> + and reverse engineering. The kernel module offers wrapped
> + versions of the ioremap family of functions. The driver to be traced
> + must be modified to call these wrappers. A user space program is
> + required to collect the MMIO data.
> +
> + See http://nouveau.freedesktop.org/wiki/MmioTrace
> + If you are not helping to develop drivers, say N.
> +
> +config MMIOTRACE_TEST
> + tristate "Test module for mmiotrace"
> + depends on MMIOTRACE && m
> + default n
> + help
> + This is a dumb module for testing mmiotrace. It is very dangerous
> + as it will write garbage to IO memory starting at a given address.
> + However, it should be safe to use on e.g. unused portion of VRAM.
> +
> + Say N, unless you absolutely know what you are doing.
> +
> #
> # IO delay types:
> #
> diff --git a/arch/x86/mm/Makefile b/arch/x86/mm/Makefile
> index 9832910..e182f6d 100644
> --- a/arch/x86/mm/Makefile
> +++ b/arch/x86/mm/Makefile
> @@ -3,3 +3,8 @@ include ${srctree}/arch/x86/mm/Makefile_32
> else
> include ${srctree}/arch/x86/mm/Makefile_64
> endif
> +
> +obj-$(CONFIG_MMIOTRACE_HOOKS)+= kmmio.o
> +obj-$(CONFIG_MMIOTRACE) += mmiotrace.o
> +mmiotrace-objs := pf_in.o mmio-mod.o
Do not use '-objs' - '-y' is preferred as below:
> +mmiotrace-y := pf_in.o mmio-mod.o
Sam
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] mmiotrace full patch, preview 1
Hi Pekka.
A small nitpick..
+config MMIOTRACE_HOOKS
+ bool
+ default n
n is default so no need to be explicit.
If you prefer being explcit then use:
def_bool n
+config MMIOTRACE
+ tristate Memory mapped IO tracing
+ depends on DEBUG_KERNEL RELAY DEBUG_FS
+ select MMIOTRACE_HOOKS
+ default n
Same here.
+ help
+ This will build a kernel module called mmiotrace.
+ Making this a built-in is heavily discouraged.
+
+ Mmiotrace traces Memory Mapped I/O access and is meant for debugging
+ and reverse engineering. The kernel module offers wrapped
+ versions of the ioremap family of functions. The driver to be traced
+ must be modified to call these wrappers. A user space program is
+ required to collect the MMIO data.
+
+ See http://nouveau.freedesktop.org/wiki/MmioTrace
+ If you are not helping to develop drivers, say N.
+
+config MMIOTRACE_TEST
+ tristate Test module for mmiotrace
+ depends on MMIOTRACE m
+ default n
+ help
+ This is a dumb module for testing mmiotrace. It is very dangerous
+ as it will write garbage to IO memory starting at a given address.
+ However, it should be safe to use on e.g. unused portion of VRAM.
+
+ Say N, unless you absolutely know what you are doing.
+
#
# IO delay types:
#
diff --git a/arch/x86/mm/Makefile b/arch/x86/mm/Makefile
index 9832910..e182f6d 100644
--- a/arch/x86/mm/Makefile
+++ b/arch/x86/mm/Makefile
@@ -3,3 +3,8 @@ include ${srctree}/arch/x86/mm/Makefile_32
else
include ${srctree}/arch/x86/mm/Makefile_64
endif
+
+obj-$(CONFIG_MMIOTRACE_HOOKS)+= kmmio.o
+obj-$(CONFIG_MMIOTRACE) += mmiotrace.o
+mmiotrace-objs := pf_in.o mmio-mod.o
Do not use '-objs' - '-y' is preferred as below:
+mmiotrace-y := pf_in.o mmio-mod.o
Sam
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
