WARNING: mutexes are preferred for single holder semaphores

WARNING: mutexes are preferred for single holder semaphores

[Sean MacLennan]

This is a bit OT, but I got the warning in the subject from
checkpatch.pl for a piece of code. The code *is* using a mutex. Does it
actually mean I shouldn't use a mutex?

The code declares a global mutex:

	static DECLARE_MUTEX(list_lock);

The odds of two accesses to the list_lock at the same time are zero.
But it would be Very Bad(tm) if it did happen. Since the odds of
contention are near zero, the cost of the mutex is near zero, so I put
it in.

I think I can safely ignore the warning, but I want to make sure....

Cheers,
  Sean

Re: WARNING: mutexes are preferred for single holder semaphores

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 719 bytes --]

On Sun, 2008-05-04 at 20:41 -0400, Sean MacLennan wrote:
> This is a bit OT, but I got the warning in the subject from
> checkpatch.pl for a piece of code. The code *is* using a mutex. Does it
> actually mean I shouldn't use a mutex?
> 
> The code declares a global mutex:
> 
> 	static DECLARE_MUTEX(list_lock);

.. which is a semaphore :(  [see include/linux/semaphore.h]

I think you want DEFINE_MUTEX().

Yes, this is completely ridiculous.

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: WARNING: mutexes are preferred for single holder semaphores

[Sean MacLennan]

On Mon, 05 May 2008 11:06:55 +1000
"Michael Ellerman" <michael@ellerman.id.au> wrote:

> On Sun, 2008-05-04 at 20:41 -0400, Sean MacLennan wrote:
> > This is a bit OT, but I got the warning in the subject from
> > checkpatch.pl for a piece of code. The code *is* using a mutex.
> > Does it actually mean I shouldn't use a mutex?
> > 
> > The code declares a global mutex:
> > 
> > 	static DECLARE_MUTEX(list_lock);
> 
> .. which is a semaphore :(  [see include/linux/semaphore.h]
> 
> I think you want DEFINE_MUTEX().
> 
> Yes, this is completely ridiculous.
> 
> cheers
> 

Ok, that fixed it, once I changed all the up and down calls :p

Thanks.

Cheers,
   Sean

Re: WARNING: mutexes are preferred for single holder semaphores

[Paul Mackerras]

Sean MacLennan writes:

> This is a bit OT, but I got the warning in the subject from
> checkpatch.pl for a piece of code. The code *is* using a mutex. Does it
> actually mean I shouldn't use a mutex?

I don't require zero checkpatch warnings or errors on patches before I
accept them.  If what checkpatch is saying is rubbish, just ignore it.

Paul.

Re: WARNING: mutexes are preferred for single holder semaphores

[Benjamin Herrenschmidt]

On Sun, 2008-05-04 at 20:41 -0400, Sean MacLennan wrote:
> This is a bit OT, but I got the warning in the subject from
> checkpatch.pl for a piece of code. The code *is* using a mutex. Does it
> actually mean I shouldn't use a mutex?
> 
> The code declares a global mutex:
> 
> 	static DECLARE_MUTEX(list_lock);
> 
> The odds of two accesses to the list_lock at the same time are zero.
> But it would be Very Bad(tm) if it did happen. Since the odds of
> contention are near zero, the cost of the mutex is near zero, so I put
> it in.
> 
> I think I can safely ignore the warning, but I want to make sure....

Show us the code... It could be a bug in checkpatch or you using 
the wrong functions somewhere ...

Ben.

Re: WARNING: mutexes are preferred for single holder semaphores

[Sean MacLennan]

On Mon, 05 May 2008 13:38:39 +1000
"Benjamin Herrenschmidt" <benh@kernel.crashing.org> wrote:

> Show us the code... It could be a bug in checkpatch or you using 
> the wrong functions somewhere ...

The change to DEFINE_MUTEX and changing up/down to
mutex_lock/mutex_unlock solved the problem.

The code is GPLed but not currently available on the net. It is
basically a little driver that registers a character device and
then passes out the minor numbers to the PIKA board drivers.

It was written to isolate all the character/sysfs code to one place
since we have five drivers, and many debug drivers, that use it.

If anybody is really interested, I can post it here. I doubt it will
ever be submitted to the mainline kernel however.

Cheers,
   Sean

Re: WARNING: mutexes are preferred for single holder semaphores

[Christoph Hellwig]

On Mon, May 05, 2008 at 12:13:43PM +1000, Paul Mackerras wrote:
> Sean MacLennan writes:
> 
> > This is a bit OT, but I got the warning in the subject from
> > checkpatch.pl for a piece of code. The code *is* using a mutex. Does it
> > actually mean I shouldn't use a mutex?
> 
> I don't require zero checkpatch warnings or errors on patches before I
> accept them.  If what checkpatch is saying is rubbish, just ignore it.

Current versions don't spew much rubbish anymore, and this one certainly
isn't.

All your drivers are belong to us [was WARNING: mutexes are preferred for single holder semaphores]

[Detlev Zundel]

Hi Sean,

> The code is GPLed but not currently available on the net. It is
> basically a little driver that registers a character device and
> then passes out the minor numbers to the PIKA board drivers.
>
> It was written to isolate all the character/sysfs code to one place
> since we have five drivers, and many debug drivers, that use it.
>
> If anybody is really interested, I can post it here. I doubt it will
> ever be submitted to the mainline kernel however.

Just for the record and because it may be interesting to others, during
the Hannover Messe end of April, in the context of the OSADL Congress[1]
I attended a speech of Greg KH about driver development and citing from
memory he said:

"I am serious, please post all drivers that there are, even if you think
they are useless for other people.  We have a driver in mainline for a
device that I know only exists once in the whole world.

Having the drivers in the kernel gives the developers the chance to see
how the infrastructure is being used and to isolate good opportunities
for modularization benefiting the linux kernel.

[...]

It also happened that a driver once posted for what the customer
thought was a completely specific device of his own today supports lots
of different boards from at least four different manufacturers."

The wording is of course not exact but I hope I caught the spirit of
what Greg wanted to say.  So yes, please post the driver - maybe Greg KH
will tunnel it into mainline...

Cheers
  Detlev

[1] http://www.osadl.org/International-Congress-Open-Source-mee.hannover-2008-congress.0.html

PS: The OSADL pages should be updated in a few days to link to slides of
    the talks.

-- 
Ftpd never switches uid and euid, it uses setfsuid(2) instead. The
main reason is that uid switching has been exploited in several
breakins, but the sheer ugliness of uid switching counts too.
                                     -- pure-ftpd(8)
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu@denx.de

Re: All your drivers are belong to us [was WARNING: mutexes are preferredfor single holder semaphores]

[Sean MacLennan]

On Wed, 07 May 2008 16:36:30 +0200
"Detlev Zundel" <dzu@denx.de> wrote:

> It also happened that a driver once posted for what the customer
> thought was a completely specific device of his own today supports
> lots of different boards from at least four different manufacturers."
> 
> The wording is of course not exact but I hope I caught the spirit of
> what Greg wanted to say.  So yes, please post the driver - maybe Greg
> KH will tunnel it into mainline...

I agree with the sentiment. However, it is hard enough to get
legitimate drivers into the kernel. The drivers I mentioned do not
come close to following the Linux kernel coding spec.

Just to start, they use 2 character spaces for indentation :( They are
full of ifdefs since we need to support the whole gamut of 2.6 kernels
plus windows. We also must support multiple board versions in the same
files.

Cheers,
   Sean

Re: All your drivers are belong to us [was WARNING: mutexes are preferredfor single holder semaphores]

[Josh Boyer]

On Wed, 7 May 2008 17:42:51 -0400
Sean MacLennan <smaclennan@pikatech.com> wrote:

> On Wed, 07 May 2008 16:36:30 +0200
> "Detlev Zundel" <dzu@denx.de> wrote:
> 
> > It also happened that a driver once posted for what the customer
> > thought was a completely specific device of his own today supports
> > lots of different boards from at least four different manufacturers."
> > 
> > The wording is of course not exact but I hope I caught the spirit of
> > what Greg wanted to say.  So yes, please post the driver - maybe Greg
> > KH will tunnel it into mainline...
> 
> I agree with the sentiment. However, it is hard enough to get
> legitimate drivers into the kernel. The drivers I mentioned do not
> come close to following the Linux kernel coding spec.

Here's the cool part.  There's this Linux driver project:

http://www.linuxdriverproject.org/twiki/bin/view

where they can take drivers like that and clean them up.  At least to a
reasonable degree.  The benefit for you is that once it's cleaned up,
it can go into the mainline kernel and it will just be there in future
releases.  Helpful when you update.

Now, I can't say I've personally ever used that project but its mission
does seem pretty targeted towards groups that want to do the right
thing with respect to drivers.  Maybe you could give it a shot and tell
others about the experience.

josh

Re: All your drivers are belong to us [was WARNING: mutexes are preferredfor single holder semaphores]

[Sean MacLennan]

I have attached the driver in question below. This is *not*
representative of PIKA board drivers in general. This driver was
rewritten from scratch to work in 2.6.26, but still work on the 2.6.9
kernel in RedHatES 4.

Since it was rewritten, it matches the kernel coding specs rather than
PIKA's. Emacs users will recognize the comment at the bottom ;)

This is also a generally useful driver that does one specific thing. It
is also very small. The next smallest PIKA driver is close to 9,000
lines of code. Our largest is about 130,000 lines of code and lsmod
reports 1.6M in size.

The other board drivers are *very* specific to PIKA hardware, and are
not "chip" drivers as such. I suspect Greg was mainly talking about
chip drivers.

I use this driver all the time to write quick drivers. It handles all
the dirty work for you so you can quickly prototype character drivers.

And while the Linux driver project looks interesting, it doesn't really
help me. We still have to support all our customers, which includes
windows, and old Linux kernels. This means I would still have to
maintain the PIKA version of the driver separate from the driver
projects version.

Cheers,
   Sean

--- /dev/null	2005-11-20 22:22:37.000000000 -0500
+++ pikacore.h	2008-05-07 23:48:47.000000000 -0400
@@ -0,0 +1,39 @@
+/*
+ * PIKA Core Driver
+ *
+ * Copyright (c) 2008 PIKA Technologies
+ *   Sean MacLennan <smaclennan@pikatech.com>
+ */
+
+#ifndef PIKACORE_H
+#define PIKACORE_H
+
+#include <linux/module.h>
+#include <linux/fs.h>
+#include <linux/device.h>
+
+
+/* pikacore_register* returns a negative number on error.
+ * On success, it returns the minor number.
+ */
+int pikacore_register_board(struct file_operations *fops);
+int pikacore_register(int minor, char *name, struct file_operations *fops);
+int pikacore_unregister(int minor);
+int pikacore_device_attr(int minor, struct device_attribute *attr, void *data);
+
+
+/* Some compatibility changes. */
+
+#ifndef DEFINE_MUTEX
+#define DEFINE_MUTEX DECLARE_MUTEX
+#define mutex_lock down
+#define mutex_unlock up
+#endif
+
+#include <linux/interrupt.h>
+#ifndef IRQF_SHARED
+#define IRQF_SHARED (SA_SHIRQ | SA_INTERRUPT)
+#define IRQF_DISABLED 0
+#endif
+
+#endif
--- /dev/null	2005-11-20 22:22:37.000000000 -0500
+++ pikacore.c	2008-05-08 00:18:35.000000000 -0400
@@ -0,0 +1,261 @@
+/*
+ * PIKA Core Driver
+ *
+ * Copyright (c) 2008 PIKA Technologies
+ *   Sean MacLennan <smaclennan@pikatech.com>
+ *
+ * This driver handles all the details of creating a character device
+ * driver. It also allows us to reuse one major number for all the
+ * character device drivers.
+ */
+
+#include "pikacore.h"
+
+#include <linux/cdev.h>
+#include <linux/version.h>
+
+
+static int major;
+module_param(major, int, 0444);
+
+
+#if LINUX_VERSION_CODE < KERNEL_VERSION(2, 6, 13)
+/* This is really for RedHatES4 */
+#define OLD_KERNEL
+
+#define class_create class_simple_create
+#define class_destroy class_simple_destroy
+
+static inline struct class_device *device_create(struct class_simple *cls,
+						 void *parent, dev_t devt,
+						 const char *fmt, ...)
+{
+	va_list ap;
+	char tmpname[20];
+
+	va_start(ap, fmt);
+	vsnprintf(tmpname, sizeof(tmpname), fmt, ap);
+	va_end(ap);
+
+	return class_simple_device_add(cls, devt, parent, tmpname);
+}
+
+#define device_destroy(a, b) class_simple_device_remove(b)
+
+#define device class_device
+static struct class_simple *pikacore_class;
+#else
+static struct class *pikacore_class;
+#endif
+
+
+struct pk_coredevice  {
+	struct list_head list;
+	int minor;
+	struct cdev cdev;
+	struct device *device;
+};
+
+static LIST_HEAD(pikacore_list);
+static DEFINE_MUTEX(pikacore_mtx);
+
+#define MAX_MINORS 64
+static unsigned long pikacore_minors[MAX_MINORS / 8 / sizeof(unsigned long)];
+
+
+int pikacore_register(int minor, char *name, struct file_operations *fops)
+{
+	struct pk_coredevice *pika;
+	dev_t dev;
+	int err;
+
+	mutex_lock(&pikacore_mtx);
+
+	if (minor == 0)
+		minor = find_first_zero_bit(pikacore_minors, MAX_MINORS);
+
+	if (minor >= MAX_MINORS ||
+	    test_and_set_bit(minor, pikacore_minors)) {
+		mutex_unlock(&pikacore_mtx);
+		return -ENOENT;
+	}
+
+	mutex_unlock(&pikacore_mtx);
+
+	pika = kzalloc(sizeof(struct pk_coredevice), GFP_KERNEL);
+	if (!pika)
+		return -ENOMEM;
+
+	INIT_LIST_HEAD(&pika->list);
+	pika->minor = minor;
+	dev = MKDEV(major, minor);
+
+	if (name)
+		pika->device = device_create(pikacore_class, NULL, dev,
+					     "%s", name);
+	else
+		pika->device = device_create(pikacore_class, NULL, dev,
+					     "pika%d", minor);
+	if (IS_ERR(pika->device)) {
+		err = PTR_ERR(pika->device);
+		goto out;
+	}
+
+	cdev_init(&pika->cdev, fops);
+	err = cdev_add(&pika->cdev, dev, 1);
+	if (err)
+		goto out;
+
+	mutex_lock(&pikacore_mtx);
+	list_add(&pika->list, &pikacore_list);
+	mutex_unlock(&pikacore_mtx);
+
+	printk(KERN_INFO "pikacore: registered %d.%d\n", major, minor);
+
+	return minor;
+
+ out:
+	if (pika->device)
+		device_destroy(pikacore_class, MKDEV(major, minor));
+
+	kfree(pika);
+	return err;
+}
+EXPORT_SYMBOL(pikacore_register);
+
+int pikacore_register_board(struct file_operations *fops)
+{
+	return pikacore_register(0, NULL, fops);
+}
+EXPORT_SYMBOL(pikacore_register_board);
+
+/* You must have pikacore_mtx held */
+static inline struct pk_coredevice *find_by_minor(int minor)
+{
+	struct pk_coredevice *pika;
+
+	list_for_each_entry(pika, &pikacore_list, list)
+		if (pika->minor == minor)
+			return pika;
+
+	return NULL;
+}
+
+int pikacore_unregister(int minor)
+{
+	struct pk_coredevice *pika;
+
+	mutex_lock(&pikacore_mtx);
+
+	pika = find_by_minor(minor);
+	if (!pika) {
+		mutex_unlock(&pikacore_mtx);
+		return -EINVAL;
+	}
+
+	list_del(&pika->list);
+
+	clear_bit(minor, pikacore_minors);
+
+	mutex_unlock(&pikacore_mtx);
+
+	printk(KERN_INFO "pikacore: remove %d.%d\n", major, minor);
+
+	device_destroy(pikacore_class, MKDEV(major, minor));
+
+	cdev_del(&pika->cdev);
+
+	kfree(pika);
+
+	return 0;
+}
+EXPORT_SYMBOL(pikacore_unregister);
+
+int pikacore_device_attr(int minor, struct device_attribute *attr, void *data)
+{
+#ifdef OLD_KERNEL
+	return -ENOSYS;
+#else
+	struct pk_coredevice *pika;
+	int rc;
+
+	mutex_lock(&pikacore_mtx);
+
+	pika = find_by_minor(minor);
+	if (!pika) {
+		printk(KERN_WARNING "%s: %d is not a pika device\n",
+		       __func__, minor);
+		rc = -ENOENT;
+		goto out;
+	}
+
+	if (data) {
+		void *cur = dev_get_drvdata(pika->device);
+		if (cur == NULL)
+			dev_set_drvdata(pika->device, data);
+		else if (cur != data) {
+			printk(KERN_WARNING "%s: data already set!!!\n",
+			       __func__);
+			rc = -EINVAL;
+			goto out;
+		}
+	}
+
+	rc = device_create_file(pika->device, attr);
+
+out:
+	mutex_unlock(&pikacore_mtx);
+
+	return rc;
+#endif
+}
+EXPORT_SYMBOL(pikacore_device_attr);
+
+static int __init pikacore_init(void)
+{
+	int rc;
+	dev_t devt;
+
+	pikacore_class = class_create(THIS_MODULE, "pika");
+	if (IS_ERR(pikacore_class))
+		return PTR_ERR(pikacore_class);
+
+	if (major) {
+		/* user specified the major */
+		devt = MKDEV(major, 0);
+		rc = register_chrdev_region(devt, MAX_MINORS, "pikacore");
+	} else
+		/* dynamic */
+		rc = alloc_chrdev_region(&devt, 0, MAX_MINORS, "pikacore");
+	if (rc) {
+		printk(KERN_ERR "pikacore: Unable to register (%d).\n", rc);
+		class_destroy(pikacore_class);
+		return rc;
+	}
+
+	major = MAJOR(devt);
+
+	return 0;
+}
+module_init(pikacore_init);
+
+void __exit pikacore_exit(void)
+{
+	unregister_chrdev_region(MKDEV(major, 0), MAX_MINORS);
+
+	class_destroy(pikacore_class);
+}
+module_exit(pikacore_exit);
+
+MODULE_DESCRIPTION("pikacore - core class driver");
+MODULE_AUTHOR("Sean MacLennan");
+MODULE_LICENSE("GPL");
+
+/*
+ * Force kernel coding standard on PIKA code
+ * Local Variables:
+ * tab-width: 8
+ * c-basic-offset: 8
+ * indent-tabs-mode: t
+ * End:
+ */

Re: All your drivers are belong to us

[Detlev Zundel]

Hi,

> On Wed, 7 May 2008 17:42:51 -0400
> Sean MacLennan <smaclennan@pikatech.com> wrote:
>
>> On Wed, 07 May 2008 16:36:30 +0200
>> "Detlev Zundel" <dzu@denx.de> wrote:
>> 
>> > It also happened that a driver once posted for what the customer
>> > thought was a completely specific device of his own today supports
>> > lots of different boards from at least four different manufacturers."
>> > 
>> > The wording is of course not exact but I hope I caught the spirit of
>> > what Greg wanted to say.  So yes, please post the driver - maybe Greg
>> > KH will tunnel it into mainline...
>> 
>> I agree with the sentiment. However, it is hard enough to get
>> legitimate drivers into the kernel. The drivers I mentioned do not
>> come close to following the Linux kernel coding spec.
>
> Here's the cool part.  There's this Linux driver project:
>
> http://www.linuxdriverproject.org/twiki/bin/view
>
> where they can take drivers like that and clean them up.  At least to a
> reasonable degree.  The benefit for you is that once it's cleaned up,
> it can go into the mainline kernel and it will just be there in future
> releases.  Helpful when you update.

Yes, I should have mentioned this also in my original mail.  If I
remember correctly, Greg mentioned that in this project, he has *300*
developers waiting to do something.  None of the companies previously
claiming Linux had a driver problem showed up even when he contacted
them directly....

> Now, I can't say I've personally ever used that project but its mission
> does seem pretty targeted towards groups that want to do the right
> thing with respect to drivers.  Maybe you could give it a shot and tell
> others about the experience.

This would definitely be interesting.

Cheers
  Detlev

-- 
-- Question authority!
-- Yeah, says who?
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu@denx.de