lkml.org 
[lkml]   [2012]   [May]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH] w1: Introduce a slave mutex for serializing IO.
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;[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2012-05-09 04:01    [W:0.159 / U:0.004 seconds]
©2003-2014 Jasper Spaans. Advertise on this site