Rename DECLARE_MUTEX to DEFINE_SEMAPHORE

PATCH In review Marcin Slusarz 4 participants 9 messages Oct 26, 2008 – Oct 29, 2008

Subsystems (8)

Arm (32-bit) Sound Arm sub-architectures Staging +4 more subsystems

lore

Share & tools

lore

Marcin Slusarz

author

2008-10-26 12:09 UTC #0 lore raw

PATCH 1/1 Rename DECLARE_MUTEX to DEFINE_SEMAPHORE

DECLARE_MUTEX is doubly misleading name (it actually _defines_ struct _semaphore_ initialized to 1) and it can be confused with DEFINE_MUTEX (which defines real struct mutex). Rename it.

Signed-off-by Marcin Slusarz <[email protected]>

Cc Matthew Wilcox <[email protected]>

Cc Andrew Morton <[email protected]>

Cc Linus Torvalds <[email protected]>

---
 arch/arm/mach-lh7a40x/clocks.c      |    2 +-
 drivers/macintosh/adb.c             |    2 +-
 drivers/staging/go7007/go7007-i2c.c |    4 ++--
 include/linux/semaphore.h           |    2 +-
 kernel/printk.c                     |    4 ++--
 lib/dynamic_printk.c                |    2 +-
 scripts/checkpatch.pl               |    2 +-
 sound/soc/s3c24xx/s3c2443-ac97.c    |    2 +-
 8 files changed, 10 insertions(+), 10 deletions(-)

arch/arm/mach-lh7a40x/clocks.c +1 -1

index 4fb23ac..909ab4e 100644
@@ -80,7 +80,7 @@ unsigned int pclkfreq_get (void)
 /* ----- */
 
 static LIST_HEAD(clocks);
-static DECLARE_MUTEX(clocks_sem);
+static DEFINE_SEMAPHORE(clocks_sem);
 
 struct clk *clk_get (struct device *dev, const char *id)
 {

drivers/macintosh/adb.c +1 -1

index 23741ce..4d9b203 100644
@@ -83,7 +83,7 @@ static struct adb_driver *adb_controller;
 BLOCKING_NOTIFIER_HEAD(adb_client_list);
 static int adb_got_sleep;
 static int adb_inited;
-static DECLARE_MUTEX(adb_probe_mutex);
+static DEFINE_SEMAPHORE(adb_probe_mutex);
 static int sleepy_trackpad;
 static int autopoll_devs;
 int __adb_probe_sync;

drivers/staging/go7007/go7007-i2c.c +2 -2

index cd55b76..0c88e3c 100644
@@ -56,7 +56,7 @@ struct wis_i2c_client_driver {
 };
 
 static LIST_HEAD(i2c_client_drivers);
-static DECLARE_MUTEX(i2c_client_driver_list_lock);
+static DEFINE_SEMAPHORE(i2c_client_driver_list_lock);
 
 /* Client drivers register here by their I2C driver ID */
 int wis_i2c_add_driver(unsigned int id, found_proc found_proc)

@@ -129,7 +129,7 @@ int wis_i2c_probe_device(struct i2c_adapter *adapter,
 
 /* There is only one I2C port on the TW2804 that feeds all four GO7007 VIPs
  * on the Adlink PCI-MPG24, so access is shared between all of them. */
-static DECLARE_MUTEX(adlink_mpg24_i2c_lock);
+static DEFINE_SEMAPHORE(adlink_mpg24_i2c_lock);
 
 static int go7007_i2c_xfer(struct go7007 *go, u16 addr, int read,
 		u16 command, int flags, u8 *data)

include/linux/semaphore.h +1 -1

index 7415839..c3657f3 100644
@@ -26,7 +26,7 @@ struct semaphore {
 	.wait_list	= LIST_HEAD_INIT((name).wait_list),		\
 }
 
-#define DECLARE_MUTEX(name)	\
+#define DEFINE_SEMAPHORE(name)	\
 	struct semaphore name = __SEMAPHORE_INITIALIZER(name, 1)
 
 static inline void sema_init(struct semaphore *sem, int val)

kernel/printk.c +2 -2

index 6341af7..132f9b9 100644
@@ -72,8 +72,8 @@ EXPORT_SYMBOL(oops_in_progress);
  * provides serialisation for access to the entire console
  * driver system.
  */
-static DECLARE_MUTEX(console_sem);
-static DECLARE_MUTEX(secondary_console_sem);
+static DEFINE_SEMAPHORE(console_sem);
+static DEFINE_SEMAPHORE(secondary_console_sem);
 struct console *console_drivers;
 EXPORT_SYMBOL_GPL(console_drivers);

lib/dynamic_printk.c +1 -1

index d640f87..a3d154e 100644
@@ -34,7 +34,7 @@ static struct hlist_head module_table[DEBUG_HASH_TABLE_SIZE] =
 	{ [0 ... DEBUG_HASH_TABLE_SIZE-1] = HLIST_HEAD_INIT };
 static struct hlist_head module_table2[DEBUG_HASH_TABLE_SIZE] =
 	{ [0 ... DEBUG_HASH_TABLE_SIZE-1] = HLIST_HEAD_INIT };
-static DECLARE_MUTEX(debug_list_mutex);
+static DEFINE_SEMAPHORE(debug_list_mutex);
 
 /* dynamic_printk_enabled, and dynamic_printk_enabled2 are bitmasks in which
  * bit n is set to 1 if any modname hashes into the bucket n, 0 otherwise. They

scripts/checkpatch.pl +1 -1

index f88bb3e..d2e24d6 100755
@@ -2428,7 +2428,7 @@ sub process {
 		}
 
 # check for semaphores used as mutexes
-		if ($line =~ /^.\s*(DECLARE_MUTEX|init_MUTEX)\s*\(/) {
+		if ($line =~ /^.\s*(DEFINE_SEMAPHORE|init_MUTEX)\s*\(/) {
 			WARN("mutexes are preferred for single holder semaphores\n" . $herecurr);
 		}
 # check for semaphores used as mutexes

sound/soc/s3c24xx/s3c2443-ac97.c +1 -1

index 19c5c3c..97dd922 100644
@@ -46,7 +46,7 @@ static struct s3c24xx_ac97_info s3c24xx_ac97;
 
 static DECLARE_COMPLETION(ac97_completion);
 static u32 codec_ready;
-static DECLARE_MUTEX(ac97_mutex);
+static DEFINE_SEMAPHORE(ac97_mutex);
 
 static unsigned short s3c2443_ac97_read(struct snd_ac97 *ac97,
 	unsigned short reg)

Signature · 1 line

1.5.6.4

Peter Zijlstra

2008-10-26 13:39 UTC #1 lore raw

Quoted 3 lines · Marcin Slusarz

DECLARE_MUTEX is doubly misleading name (it actually _defines_ struct _semaphore_ initialized to 1) and it can be confused with DEFINE_MUTEX (which defines real struct mutex). Rename it.

I'd prefer DEFINE_BINARY_SEM or somesuch

Leon Woestenberg

2008-10-26 14:11 UTC #2 lore raw

Hello,

Quoted 7 lines · Peter Zijlstra

Quoted 3 lines · Marcin Slusarz
DECLARE_MUTEX is doubly misleading name (it actually _defines_ struct _semaphore_ initialized to 1) and it can be confused with DEFINE_MUTEX (which defines real struct mutex). Rename it.
I'd prefer DEFINE_BINARY_SEM or somesuch

But it is not a binary semaphore, or is it?

Also, at least under 2.6.24,

#define DECLARE_MUTEX(name) __DECLARE_SEMAPHORE_GENERIC(name,1)

thus the DECLARE is wrong there as well?

Regards,

Signature · 1 line

Leon

Peter Zijlstra

2008-10-26 14:41 UTC #3 lore raw

Quoted 8 lines · Leon Woestenberg

Hello,
Quoted 5 lines · Peter Zijlstra
Quoted 3 lines · Marcin Slusarz
DECLARE_MUTEX is doubly misleading name (it actually _defines_ struct _semaphore_ initialized to 1) and it can be confused with DEFINE_MUTEX (which defines real struct mutex). Rename it.

Quoted 5 lines

Quoted 2 lines
I'd prefer DEFINE_BINARY_SEM or somesuch
But it is not a binary semaphore, or is it?
#define DECLARE_MUTEX(name) __DECLARE_SEMAPHORE_GENERIC(name,1)

I thought that 1 made it a binary sem.

Leon Woestenberg

2008-10-26 15:20 UTC #4 lore raw

Hello,

Quoted 7 lines · Peter Zijlstra

Quoted 6 lines · Leon Woestenberg
Quoted 5 lines · Peter Zijlstra
Quoted 3 lines · Marcin Slusarz
DECLARE_MUTEX is doubly misleading name (it actually _defines_ struct _semaphore_ initialized to 1) and it can be confused with DEFINE_MUTEX (which defines real struct mutex). Rename it.

Quoted 8 lines

Quoted 5 lines
Quoted 2 lines
I'd prefer DEFINE_BINARY_SEM or somesuch
But it is not a binary semaphore, or is it?
#define DECLARE_MUTEX(name) __DECLARE_SEMAPHORE_GENERIC(name,1)
I thought that 1 made it a binary sem.

I thought it only initialized the semaphore count to 1, but not disallowing it to be upped. Maybe am I just plain wrong, and I should be zapped to kernelnewbies instead. :-/

Regards,

Signature · 1 line

Leon

Marcin Slusarz

author

2008-10-26 15:27 UTC #5 lore raw

In reply to Peter Zijlstra

Quoted 6 lines · Peter Zijlstra

Quoted 3 lines · Marcin Slusarz
DECLARE_MUTEX is doubly misleading name (it actually _defines_ struct _semaphore_ initialized to 1) and it can be confused with DEFINE_MUTEX (which defines real struct mutex). Rename it.
I'd prefer DEFINE_BINARY_SEM or somesuch

Sounds better.

---
Subject: [PATCH] Rename DECLARE_MUTEX to DEFINE_BINARY_SEM

DECLARE_MUTEX is doubly misleading name (it actually _defines_ struct _semaphore_ initialized to 1) and it can be confused with DEFINE_MUTEX (which defines real struct mutex). Rename it.

Signed-off-by Marcin Slusarz <[email protected]>

Cc Matthew Wilcox <[email protected]>

Cc Andrew Morton <[email protected]>

Cc Linus Torvalds <[email protected]>

Cc Peter Zijlstra <[email protected]>

---
 arch/arm/mach-lh7a40x/clocks.c      |    2 +-
 drivers/macintosh/adb.c             |    2 +-
 drivers/staging/go7007/go7007-i2c.c |    4 ++--
 include/linux/semaphore.h           |    5 +++--
 kernel/printk.c                     |    4 ++--
 lib/dynamic_printk.c                |    2 +-
 scripts/checkpatch.pl               |    2 +-
 sound/soc/s3c24xx/s3c2443-ac97.c    |    2 +-
 8 files changed, 12 insertions(+), 11 deletions(-)

arch/arm/mach-lh7a40x/clocks.c +1 -1

index 4fb23ac..de27e81 100644
@@ -80,7 +80,7 @@ unsigned int pclkfreq_get (void)
 /* ----- */
 
 static LIST_HEAD(clocks);
-static DECLARE_MUTEX(clocks_sem);
+static DEFINE_BINARY_SEM(clocks_sem);
 
 struct clk *clk_get (struct device *dev, const char *id)
 {

drivers/macintosh/adb.c +1 -1

index 23741ce..7cf07c2 100644
@@ -83,7 +83,7 @@ static struct adb_driver *adb_controller;
 BLOCKING_NOTIFIER_HEAD(adb_client_list);
 static int adb_got_sleep;
 static int adb_inited;
-static DECLARE_MUTEX(adb_probe_mutex);
+static DEFINE_BINARY_SEM(adb_probe_mutex);
 static int sleepy_trackpad;
 static int autopoll_devs;
 int __adb_probe_sync;

drivers/staging/go7007/go7007-i2c.c +2 -2

index cd55b76..6f698af 100644
@@ -56,7 +56,7 @@ struct wis_i2c_client_driver {
 };
 
 static LIST_HEAD(i2c_client_drivers);
-static DECLARE_MUTEX(i2c_client_driver_list_lock);
+static DEFINE_BINARY_SEM(i2c_client_driver_list_lock);
 
 /* Client drivers register here by their I2C driver ID */
 int wis_i2c_add_driver(unsigned int id, found_proc found_proc)

@@ -129,7 +129,7 @@ int wis_i2c_probe_device(struct i2c_adapter *adapter,
 
 /* There is only one I2C port on the TW2804 that feeds all four GO7007 VIPs
  * on the Adlink PCI-MPG24, so access is shared between all of them. */
-static DECLARE_MUTEX(adlink_mpg24_i2c_lock);
+static DEFINE_BINARY_SEM(adlink_mpg24_i2c_lock);
 
 static int go7007_i2c_xfer(struct go7007 *go, u16 addr, int read,
 		u16 command, int flags, u8 *data)

include/linux/semaphore.h +3 -2

index 7415839..824b423 100644
@@ -26,8 +26,9 @@ struct semaphore {
 	.wait_list	= LIST_HEAD_INIT((name).wait_list),		\
 }
 
-#define DECLARE_MUTEX(name)	\
-	struct semaphore name = __SEMAPHORE_INITIALIZER(name, 1)
+#define DEFINE_SEMAPHORE(name, init)	\
+	struct semaphore name = __SEMAPHORE_INITIALIZER(name, init)
+#define DEFINE_BINARY_SEM(name) DEFINE_SEMAPHORE(name, 1)
 
 static inline void sema_init(struct semaphore *sem, int val)
 {

kernel/printk.c +2 -2

index 6341af7..2a2281b 100644
@@ -72,8 +72,8 @@ EXPORT_SYMBOL(oops_in_progress);
  * provides serialisation for access to the entire console
  * driver system.
  */
-static DECLARE_MUTEX(console_sem);
-static DECLARE_MUTEX(secondary_console_sem);
+static DEFINE_BINARY_SEM(console_sem);
+static DEFINE_BINARY_SEM(secondary_console_sem);
 struct console *console_drivers;
 EXPORT_SYMBOL_GPL(console_drivers);

lib/dynamic_printk.c +1 -1

index d640f87..0b3dbdb 100644
@@ -34,7 +34,7 @@ static struct hlist_head module_table[DEBUG_HASH_TABLE_SIZE] =
 	{ [0 ... DEBUG_HASH_TABLE_SIZE-1] = HLIST_HEAD_INIT };
 static struct hlist_head module_table2[DEBUG_HASH_TABLE_SIZE] =
 	{ [0 ... DEBUG_HASH_TABLE_SIZE-1] = HLIST_HEAD_INIT };
-static DECLARE_MUTEX(debug_list_mutex);
+static DEFINE_BINARY_SEM(debug_list_mutex);
 
 /* dynamic_printk_enabled, and dynamic_printk_enabled2 are bitmasks in which
  * bit n is set to 1 if any modname hashes into the bucket n, 0 otherwise. They

scripts/checkpatch.pl +1 -1

index f88bb3e..91462b0 100755
@@ -2428,7 +2428,7 @@ sub process {
 		}
 
 # check for semaphores used as mutexes
-		if ($line =~ /^.\s*(DECLARE_MUTEX|init_MUTEX)\s*\(/) {
+		if ($line =~ /^.\s*(DEFINE_BINARY_SEM|init_MUTEX)\s*\(/) {
 			WARN("mutexes are preferred for single holder semaphores\n" . $herecurr);
 		}
 # check for semaphores used as mutexes

sound/soc/s3c24xx/s3c2443-ac97.c +1 -1

index 19c5c3c..8980111 100644
@@ -46,7 +46,7 @@ static struct s3c24xx_ac97_info s3c24xx_ac97;
 
 static DECLARE_COMPLETION(ac97_completion);
 static u32 codec_ready;
-static DECLARE_MUTEX(ac97_mutex);
+static DEFINE_BINARY_SEM(ac97_mutex);
 
 static unsigned short s3c2443_ac97_read(struct snd_ac97 *ac97,
 	unsigned short reg)

Signature · 1 line

1.5.6.4

Christoph Hellwig

2008-10-26 16:41 UTC #6 lore raw

In reply to Marcin Slusarz

Quoted 3 lines · Marcin Slusarz

DECLARE_MUTEX is doubly misleading name (it actually _defines_ struct _semaphore_ initialized to 1) and it can be confused with DEFINE_MUTEX (which defines real struct mutex). Rename it.

NACK. It describes reasonably well what it does. Of course that's a little confusing as we now have a specialized primitive to just do that.

If the name annoys you enough so that you want to get rid of it help converting the remaining instances to struct mutex and DEFINE_MUTEX().

Marcin Slusarz

author

2008-10-29 21:17 UTC #7 lore raw

Quoted 10 lines · Christoph Hellwig

Quoted 3 lines · Marcin Slusarz
DECLARE_MUTEX is doubly misleading name (it actually _defines_ struct _semaphore_ initialized to 1) and it can be confused with DEFINE_MUTEX (which defines real struct mutex). Rename it.
NACK. It describes reasonably well what it does. Of course that's a little confusing as we now have a specialized primitive to just do that.
If the name annoys you enough so that you want to get rid of it help converting the remaining instances to struct mutex and DEFINE_MUTEX().

Changing the name of DECLARE_MUTEX (to something which tells us what it really does) will increase visibility of semaphore uses, don't you think? And more visibility means more patches removing its uses.

Marcin

Christoph Hellwig

2008-10-29 21:23 UTC #8 lore raw

Quoted 3 lines · Marcin Slusarz

Changing the name of DECLARE_MUTEX (to something which tells us what it really does) will increase visibility of semaphore uses, don't you think? And more visibility means more patches removing its uses.

What it primarily would is cause change churn. Really, if you want to do something useful try getting rid of it instead.