Messages in this thread Patch in this message |  | | | Date | Wed, 9 May 2012 11:43:49 +1000 | | From | NeilBrown <> | | Subject | Re: [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] |  |