Re: [PATCH] w1: Introduce a slave mutex for serializing IO.

[NeilBrown <neilb@suse.de>]

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

On Fri, 4 May 2012 01:27:06 +0400 Evgeniy Polyakov <zbr@ioremap.net> wrote:

> On Fri, May 04, 2012 at 07:08:38AM +1000, NeilBrown (neilb@suse.de) wrote:
> > You can only check the owner on SMP builds, or when debugging is enabled.
> > So I don't think that approach can work.
> 
> You can store owner in master device and protect with mutex itself.
> On non-smp systems it can not be preempted, so can be checked without
> mutex.
> 

I tried that - or something a lot like it.  Patch below.

However lockdep didn't like it.  There are ordering problems between this
mutex and and sysfs's s_active.

When you access battery properies via sysfs, the sysfs lock is taken first,
then the master->mutex.
When w1_reconnect_slaves calls through to device_del and sys_addrm_finish,
the mutex is held while the sysfs lock is wanted.

So we might need to come up with something more clever.

I haven't had a chance to look really deeply into this yet.  Hopefully when I
do I'll find something clever and let you know.

Thanks,
NeilBrown



diff --git a/drivers/w1/slaves/w1_bq27000.c b/drivers/w1/slaves/w1_bq27000.c
index 52ad812..83ebaad 100644
--- a/drivers/w1/slaves/w1_bq27000.c
+++ b/drivers/w1/slaves/w1_bq27000.c
@@ -30,11 +30,14 @@ static int w1_bq27000_read(struct device *dev, unsigned int reg)
 {
 	u8 val;
 	struct w1_slave *sl = container_of(dev->parent, struct w1_slave, dev);
+	bool own_mutex = (sl->master->mutex_owner == current);
 
-	mutex_lock(&sl->master->mutex);
+	if (!own_mutex)
+		mutex_lock(&sl->master->mutex);
 	w1_write_8(sl->master, HDQ_CMD_READ | reg);
 	val = w1_read_8(sl->master);
-	mutex_unlock(&sl->master->mutex);
+	if (!own_mutex)
+		mutex_unlock(&sl->master->mutex);
 
 	return val;
 }
diff --git a/drivers/w1/w1.c b/drivers/w1/w1.c
index 9761950..97de03d 100644
--- a/drivers/w1/w1.c
+++ b/drivers/w1/w1.c
@@ -616,7 +616,13 @@ static int __w1_attach_slave_device(struct w1_slave *sl)
 	dev_dbg(&sl->dev, "%s: registering %s as %p.\n", __func__,
 		dev_name(&sl->dev), sl);
 
+	/* device_register might end up asking the slave to
+	 * access the bus, so we must let it know that it
+	 * already holds the lock.
+	 */
+	sl->master->mutex_owner = current;
 	err = device_register(&sl->dev);
+	sl->master->mutex_owner = NULL;
 	if (err < 0) {
 		dev_err(&sl->dev,
 			"Device registration [%s] failed. err=%d\n",
diff --git a/drivers/w1/w1.h b/drivers/w1/w1.h
index 4d012ca..ebb157c 100644
--- a/drivers/w1/w1.h
+++ b/drivers/w1/w1.h
@@ -180,6 +180,13 @@ struct w1_master
 
 	struct task_struct	*thread;
 	struct mutex		mutex;
+	/* The mutex_owner owns the mutex and so does not
+	 * need to take it again (and doing so would deadlock).
+	 * This is important when registering a device while holding
+	 * the mutex as the slave might need to access the bus as part
+	 * of registration.
+	 */
+	struct task_struct	*mutex_owner;
 
 	struct device_driver	*driver;
 	struct device		dev;

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]