[PATCH] Centralize the selection for debugging semaphores.

[PATCH] Centralize the selection for debugging semaphores.

[Robert P. J. Day]

  Centralize the kernel config option for debugging semaphores and
modify the macro for frv to use that config option instead.

Signed-off-by: Robert P. J. Day <rpjday@mindspring.com>

---

  at the moment, two architectures have support for debugging
semaphores:  alpha (with a local Kconfig.debug option), and frv (with
a hard-coded "SEMAPHORE_DEBUG" macro).

  it would seem to make more sense to simply have a universal kernel
config option for debugging semaphores in lib/Kconfig.debug, and let
any architecture work off of that.

  obviously, only two architectures would take advantage of it right
now, but the "help" screen for the new config option mentions that,
and it's trivial to add new architectures.


 arch/alpha/Kconfig.debug    |    8 --------
 arch/frv/kernel/semaphore.c |    2 +-
 include/asm-frv/semaphore.h |   14 ++++++--------
 lib/Kconfig.debug           |   11 +++++++++++
 4 files changed, 18 insertions(+), 17 deletions(-)

diff --git a/arch/alpha/Kconfig.debug b/arch/alpha/Kconfig.debug
index 36d0106..f45f28c 100644
--- a/arch/alpha/Kconfig.debug
+++ b/arch/alpha/Kconfig.debug
@@ -16,14 +16,6 @@ config DEBUG_RWLOCK
 	  too many attempts.  If you suspect a rwlock problem or a kernel
 	  hacker asks for this option then say Y.  Otherwise say N.

-config DEBUG_SEMAPHORE
-	bool "Semaphore debugging"
-	depends on DEBUG_KERNEL
-	help
-	  If you say Y here then semaphore processing will issue lots of
-	  verbose debugging messages.  If you suspect a semaphore problem or a
-	  kernel hacker asks for this option then say Y.  Otherwise say N.
-
 config ALPHA_LEGACY_START_ADDRESS
 	bool "Legacy kernel start address"
 	depends on ALPHA_GENERIC
diff --git a/arch/frv/kernel/semaphore.c b/arch/frv/kernel/semaphore.c
index f278cdf..8e182ce 100644
--- a/arch/frv/kernel/semaphore.c
+++ b/arch/frv/kernel/semaphore.c
@@ -19,7 +19,7 @@ struct sem_waiter {
 	struct task_struct	*task;
 };

-#if SEMAPHORE_DEBUG
+#ifdef CONFIG_DEBUG_SEMAPHORE
 void semtrace(struct semaphore *sem, const char *str)
 {
 	if (sem->debug)
diff --git a/include/asm-frv/semaphore.h b/include/asm-frv/semaphore.h
index 907c5c3..0958652 100644
--- a/include/asm-frv/semaphore.h
+++ b/include/asm-frv/semaphore.h
@@ -20,8 +20,6 @@
 #include <linux/spinlock.h>
 #include <linux/rwsem.h>

-#define SEMAPHORE_DEBUG		0
-
 /*
  * the semaphore definition
  * - if counter is >0 then there are tokens available on the semaphore for down to collect
@@ -32,12 +30,12 @@ struct semaphore {
 	unsigned		counter;
 	spinlock_t		wait_lock;
 	struct list_head	wait_list;
-#if SEMAPHORE_DEBUG
+#ifdef CONFIG_DEBUG_SEMAPHORE
 	unsigned		__magic;
 #endif
 };

-#if SEMAPHORE_DEBUG
+#ifdef CONFIG_DEBUG_SEMAPHORE
 # define __SEM_DEBUG_INIT(name) , (long)&(name).__magic
 #else
 # define __SEM_DEBUG_INIT(name)
@@ -76,7 +74,7 @@ static inline void down(struct semaphore *sem)
 {
 	unsigned long flags;

-#if SEMAPHORE_DEBUG
+#ifdef CONFIG_DEBUG_SEMAPHORE
 	CHECK_MAGIC(sem->__magic);
 #endif

@@ -95,7 +93,7 @@ static inline int down_interruptible(struct semaphore *sem)
 	unsigned long flags;
 	int ret = 0;

-#if SEMAPHORE_DEBUG
+#ifdef CONFIG_DEBUG_SEMAPHORE
 	CHECK_MAGIC(sem->__magic);
 #endif

@@ -119,7 +117,7 @@ static inline int down_trylock(struct semaphore *sem)
 	unsigned long flags;
 	int success = 0;

-#if SEMAPHORE_DEBUG
+#ifdef CONFIG_DEBUG_SEMAPHORE
 	CHECK_MAGIC(sem->__magic);
 #endif

@@ -136,7 +134,7 @@ static inline void up(struct semaphore *sem)
 {
 	unsigned long flags;

-#if SEMAPHORE_DEBUG
+#ifdef CONFIG_DEBUG_SEMAPHORE
 	CHECK_MAGIC(sem->__magic);
 #endif

diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 5c26818..17c4524 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -181,6 +181,17 @@ config DEBUG_MUTEXES
 	 This feature allows mutex semantics violations to be detected and
 	 reported.

+config DEBUG_SEMAPHORE
+	bool "Semaphore debugging"
+	depends on DEBUG_KERNEL
+	default n
+	help
+	  If you say Y here then semaphore processing will issue lots of
+	  verbose debugging messages.  If you suspect a semaphore problem or a
+	  kernel hacker asks for this option then say Y.  Otherwise say N.
+
+	  At the moment, this is implemented only by alpha and frv.
+
 config DEBUG_RWSEMS
 	bool "RW-sem debugging: basic checks"
 	depends on DEBUG_KERNEL

-- 
========================================================================
Robert P. J. Day
Linux Consulting, Training and Annoying Kernel Pedantry
Waterloo, Ontario, CANADA

http://www.fsdev.dreamhosters.com/wiki/index.php?title=Main_Page
========================================================================

Re: [PATCH] Centralize the selection for debugging semaphores.

[Michal Piotrowski]

On 31/01/07, Robert P. J. Day <rpjday@mindspring.com> wrote:
>
>   Centralize the kernel config option for debugging semaphores and
> modify the macro for frv to use that config option instead.
>
> Signed-off-by: Robert P. J. Day <rpjday@mindspring.com>
[..]
> +config DEBUG_SEMAPHORE
> +       bool "Semaphore debugging"
> +       depends on DEBUG_KERNEL
> +       default n
> +       help
> +         If you say Y here then semaphore processing will issue lots of
> +         verbose debugging messages.  If you suspect a semaphore problem or a
> +         kernel hacker asks for this option then say Y.  Otherwise say N.
> +
> +         At the moment, this is implemented only by alpha and frv.

IMHO this option should stay in arch/{alpha,frv}/Kconfig.debug

Regards,
Michal

-- 
Michal K. K. Piotrowski
LTG - Linux Testers Group
(http://www.stardust.webpages.pl/ltg/)

Re: [PATCH] Centralize the selection for debugging semaphores.

[Robert P. J. Day]

On Wed, 31 Jan 2007, Michal Piotrowski wrote:

> On 31/01/07, Robert P. J. Day <rpjday@mindspring.com> wrote:
> >
> >   Centralize the kernel config option for debugging semaphores and
> > modify the macro for frv to use that config option instead.
> >
> > Signed-off-by: Robert P. J. Day <rpjday@mindspring.com>
> [..]
> > +config DEBUG_SEMAPHORE
> > +       bool "Semaphore debugging"
> > +       depends on DEBUG_KERNEL
> > +       default n
> > +       help
> > +         If you say Y here then semaphore processing will issue lots of
> > +         verbose debugging messages.  If you suspect a semaphore problem or
> > a
> > +         kernel hacker asks for this option then say Y.  Otherwise say N.
> > +
> > +         At the moment, this is implemented only by alpha and frv.
>
> IMHO this option should stay in arch/{alpha,frv}/Kconfig.debug

any particular reason?  and note that it's currently *not* in
arch/frv/Kconfig.debug -- it's hard-coded with a macro name that's not
the same as the config option, and i don't really see the advantage of
having each architecture implement precisely the same feature in
different and incompatible ways.  but i'm willing to be convinced
otherwise.

rday

-- 
========================================================================
Robert P. J. Day
Linux Consulting, Training and Annoying Kernel Pedantry
Waterloo, Ontario, CANADA

http://www.fsdev.dreamhosters.com/wiki/index.php?title=Main_Page
========================================================================

Re: [PATCH] Centralize the selection for debugging semaphores.

[Michal Piotrowski]

On 31/01/07, Robert P. J. Day <rpjday@mindspring.com> wrote:
> On Wed, 31 Jan 2007, Michal Piotrowski wrote:
>
> > On 31/01/07, Robert P. J. Day <rpjday@mindspring.com> wrote:
> > >
> > >   Centralize the kernel config option for debugging semaphores and
> > > modify the macro for frv to use that config option instead.
> > >
> > > Signed-off-by: Robert P. J. Day <rpjday@mindspring.com>
> > [..]
> > > +config DEBUG_SEMAPHORE
> > > +       bool "Semaphore debugging"
> > > +       depends on DEBUG_KERNEL
> > > +       default n
> > > +       help
> > > +         If you say Y here then semaphore processing will issue lots of
> > > +         verbose debugging messages.  If you suspect a semaphore problem or
> > > a
> > > +         kernel hacker asks for this option then say Y.  Otherwise say N.
> > > +
> > > +         At the moment, this is implemented only by alpha and frv.
> >
> > IMHO this option should stay in arch/{alpha,frv}/Kconfig.debug
>
> any particular reason?

Only this
"At the moment, this is implemented only by alpha and frv."

It depends on architecture.

> and note that it's currently *not* in
> arch/frv/Kconfig.debug -- it's hard-coded with a macro name that's not
> the same as the config option, and i don't really see the advantage of
> having each architecture implement precisely the same feature in
> different and incompatible ways.  but i'm willing to be convinced
> otherwise.
>
> rday

Regards,
Michal

-- 
Michal K. K. Piotrowski
LTG - Linux Testers Group
(http://www.stardust.webpages.pl/ltg/)

Re: [PATCH] Centralize the selection for debugging semaphores.

[Robert P. J. Day]

On Wed, 31 Jan 2007, Michal Piotrowski wrote:

> On 31/01/07, Robert P. J. Day <rpjday@mindspring.com> wrote:
> > On Wed, 31 Jan 2007, Michal Piotrowski wrote:
> >
> > > On 31/01/07, Robert P. J. Day <rpjday@mindspring.com> wrote:
> > > >
> > > >   Centralize the kernel config option for debugging semaphores and
> > > > modify the macro for frv to use that config option instead.
> > > >
> > > > Signed-off-by: Robert P. J. Day <rpjday@mindspring.com>
> > > [..]
> > > > +config DEBUG_SEMAPHORE
> > > > +       bool "Semaphore debugging"
> > > > +       depends on DEBUG_KERNEL
> > > > +       default n
> > > > +       help
> > > > +         If you say Y here then semaphore processing will issue lots of
> > > > +         verbose debugging messages.  If you suspect a semaphore
> > problem or
> > > > a
> > > > +         kernel hacker asks for this option then say Y.  Otherwise say
> > N.
> > > > +
> > > > +         At the moment, this is implemented only by alpha and frv.
> > >
> > > IMHO this option should stay in arch/{alpha,frv}/Kconfig.debug
> >
> > any particular reason?
>
> Only this
> "At the moment, this is implemented only by alpha and frv."
>
> It depends on architecture.

  yes, i realize that, but surely there's a cleaner way to implement
something that's supported by only *some* of the architectures rather
than duplicating the code for every one of those architectures.

  one possibility is to have the global config option look like this:

config DEBUG_SEMAPHORE
	bool "Semaphore debugging"
	depends on DEBUG_KERNEL && HAVE_SEMAPHORE_DEBUGGING
				   ^^^^^^^^^^^^^^^^^^^^^^^^
        default n
	...

  at that point, any architecture that implements semaphore debugging
and wants to advertise that simply needs to add this at the top of its
Kconfig.debug file:

config HAVE_SEMAPHORE_DEBUGGING
        def_bool y

  that was just off the top of my head, but it seems to work.  more
generally, though, surely this sort of thing has come up before --
wanting to advertise an identical user-configurable option only for
certain architectures.  how is that done *now* without rampant code
duplication?  or is it?

rday

-- 
========================================================================
Robert P. J. Day
Linux Consulting, Training and Annoying Kernel
Pedantry Waterloo, Ontario, CANADA

http://www.fsdev.dreamhosters.com/wiki/index.php?title=Main_Page
========================================================================