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

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

[NeilBrown]

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


w1 devices need a mutex to serial IO.
Most use master->mutex.

However that is used for other purposes and they can conflict.

In particular master->mutex is held while w1_attach_slave_device is
called.

For bq27000, this registers a 'powersupply' device which tries to
read the current status.  The attempt to read will cause a deadlock on
master->mutex.

So create a new per-slave mutex and use that for serializing IO for
bq27000.

Signed-off-by: NeilBrown <neilb@suse.de>
---
 drivers/w1/slaves/w1_bq27000.c |    4 ++--
 drivers/w1/w1.c                |    1 +
 drivers/w1/w1.h                |    1 +
 3 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/w1/slaves/w1_bq27000.c b/drivers/w1/slaves/w1_bq27000.c
index 52ad812..8755478 100644
--- a/drivers/w1/slaves/w1_bq27000.c
+++ b/drivers/w1/slaves/w1_bq27000.c
@@ -31,10 +31,10 @@ 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);
 
-	mutex_lock(&sl->master->mutex);
+	mutex_lock(&sl->mutex);
 	w1_write_8(sl->master, HDQ_CMD_READ | reg);
 	val = w1_read_8(sl->master);
-	mutex_unlock(&sl->master->mutex);
+	mutex_unlock(&sl->mutex);
 
 	return val;
 }
diff --git a/drivers/w1/w1.c b/drivers/w1/w1.c
index 9761950..2173f83 100644
--- a/drivers/w1/w1.c
+++ b/drivers/w1/w1.c
@@ -688,6 +688,7 @@ static int w1_attach_slave_device(struct w1_master *dev, struct w1_reg_num *rn)
 	memcpy(&sl->reg_num, rn, sizeof(sl->reg_num));
 	atomic_set(&sl->refcnt, 0);
 	init_completion(&sl->released);
+	mutex_init(&sl->mutex);
 
 	spin_lock(&w1_flock);
 	f = w1_family_registered(rn->family);
diff --git a/drivers/w1/w1.h b/drivers/w1/w1.h
index 4d012ca..8c8be38 100644
--- a/drivers/w1/w1.h
+++ b/drivers/w1/w1.h
@@ -75,6 +75,7 @@ struct w1_slave
 	void			*family_data;
 	struct device		dev;
 	struct completion	released;
+	struct mutex		mutex;
 };
 
 typedef void (*w1_slave_found_callback)(struct w1_master *, u64);
-- 
1.7.10


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

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

[Evgeniy Polyakov]

On Wed, Apr 25, 2012 at 12:49:14PM +1000, NeilBrown (neilb@suse.de) wrote:
> w1 devices need a mutex to serial IO.
> Most use master->mutex.
> 
> However that is used for other purposes and they can conflict.
> 
> In particular master->mutex is held while w1_attach_slave_device is
> called.
> 
> For bq27000, this registers a 'powersupply' device which tries to
> read the current status.  The attempt to read will cause a deadlock on
> master->mutex.
> 
> So create a new per-slave mutex and use that for serializing IO for
> bq27000.

How will this protect against master doing search for example?
It performs a number of read/write operations which are not allowed to
be interrupted by outside read.

-- 
	Evgeniy Polyakov

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

[NeilBrown]

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

On Wed, 2 May 2012 01:39:58 +0400 Evgeniy Polyakov <zbr@ioremap.net> wrote:

> On Wed, Apr 25, 2012 at 12:49:14PM +1000, NeilBrown (neilb@suse.de) wrote:
> > w1 devices need a mutex to serial IO.
> > Most use master->mutex.
> > 
> > However that is used for other purposes and they can conflict.
> > 
> > In particular master->mutex is held while w1_attach_slave_device is
> > called.
> > 
> > For bq27000, this registers a 'powersupply' device which tries to
> > read the current status.  The attempt to read will cause a deadlock on
> > master->mutex.
> > 
> > So create a new per-slave mutex and use that for serializing IO for
> > bq27000.
> 
> How will this protect against master doing search for example?
> It performs a number of read/write operations which are not allowed to
> be interrupted by outside read.
> 

Yes, you are right.  That was broken, I see it now.

Take takes me back to the deadlock.
To be precise:
  while scanning the w1 bus - with master->mutex held - w1_attach_slave_device
  eventually calls into bq27000_battery_probe  which calls
    power_supply_register -> device_add -> kobject_uevent_env

  and the to get all the data for the uevent, power_supply_uevent calls
  the get_property callback which tries to read from the w1 bus and so
  needs the master->mutex.  Which is held.  Deadlock.

So options seems to be:
 - drop the lock while attaching slave devices
 - create a list of slave devices, then attach them after the bus scan has
   finished.
 - have device_add run the kobject_uevent in a separate task (work_queue)
 - or maybe the following which feels ugly but is easy.  Mark the bq27000 as
   not ready until after the power_supply_register, and if get_property is 
   called before the device is ready, return ENODATA.
   Simple and works.  Maybe horrible.
   power_supply_register will have scheduled a power_supply_changed() which
   will poll

Thoughts?

Thanks,
NeilBrown


diff --git a/drivers/power/bq27x00_battery.c b/drivers/power/bq27x00_battery.c
index 298de4b..bd89494 100644
--- a/drivers/power/bq27x00_battery.c
+++ b/drivers/power/bq27x00_battery.c
@@ -107,6 +107,7 @@ struct bq27x00_device_info {
 
 	struct bq27x00_access_methods bus;
 
+	int ready;
 	struct mutex lock;
 };
 
@@ -500,6 +501,8 @@ static int bq27x00_battery_get_property(struct power_supply *psy,
 	int ret = 0;
 	struct bq27x00_device_info *di = to_bq27x00_device_info(psy);
 
+	if (!di->ready)
+		return -ENODATA;
 	mutex_lock(&di->lock);
 	if (time_is_before_jiffies(di->last_update + 5 * HZ)) {
 		cancel_delayed_work_sync(&di->work);
@@ -583,6 +586,7 @@ static int bq27x00_powersupply_init(struct bq27x00_device_info *di)
 	di->bat.num_properties = ARRAY_SIZE(bq27x00_battery_props);
 	di->bat.get_property = bq27x00_battery_get_property;
 	di->bat.external_power_changed = bq27x00_external_power_changed;
+	di->ready = 0;
 
 	INIT_DELAYED_WORK(&di->work, bq27x00_battery_poll);
 	mutex_init(&di->lock);
@@ -592,10 +596,12 @@ static int bq27x00_powersupply_init(struct bq27x00_device_info *di)
 		dev_err(di->dev, "failed to register battery: %d\n", ret);
 		return ret;
 	}
+	di->ready = 1;
 
 	dev_info(di->dev, "support ver. %s enabled\n", DRIVER_VERSION);
 
-	bq27x00_update(di);
+	di->last_update = jiffies - 10*HZ;
+	schedule_delayed_work(&di->work, 1);
 
 	return 0;
 }

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

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

[Evgeniy Polyakov]

On Wed, May 02, 2012 at 04:26:27PM +1000, NeilBrown (neilb@suse.de) wrote:
> Take takes me back to the deadlock.
> To be precise:
>   while scanning the w1 bus - with master->mutex held - w1_attach_slave_device
>   eventually calls into bq27000_battery_probe  which calls
>     power_supply_register -> device_add -> kobject_uevent_env
> 
>   and the to get all the data for the uevent, power_supply_uevent calls
>   the get_property callback which tries to read from the w1 bus and so
>   needs the master->mutex.  Which is held.  Deadlock.
> 
> So options seems to be:
>  - drop the lock while attaching slave devices
>  - create a list of slave devices, then attach them after the bus scan has
>    finished.
>  - have device_add run the kobject_uevent in a separate task (work_queue)
>  - or maybe the following which feels ugly but is easy.  Mark the bq27000 as
>    not ready until after the power_supply_register, and if get_property is 
>    called before the device is ready, return ENODATA.
>    Simple and works.  Maybe horrible.
>    power_supply_register will have scheduled a power_supply_changed() which
>    will poll

You can check mutex owner - and if it is you who holds it, then dig into
low-level function without lock.

-- 
	Evgeniy Polyakov

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

[NeilBrown]

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

On Thu, 3 May 2012 21:58:57 +0400 Evgeniy Polyakov <zbr@ioremap.net> wrote:

> On Wed, May 02, 2012 at 04:26:27PM +1000, NeilBrown (neilb@suse.de) wrote:
> > Take takes me back to the deadlock.
> > To be precise:
> >   while scanning the w1 bus - with master->mutex held - w1_attach_slave_device
> >   eventually calls into bq27000_battery_probe  which calls
> >     power_supply_register -> device_add -> kobject_uevent_env
> > 
> >   and the to get all the data for the uevent, power_supply_uevent calls
> >   the get_property callback which tries to read from the w1 bus and so
> >   needs the master->mutex.  Which is held.  Deadlock.
> > 
> > So options seems to be:
> >  - drop the lock while attaching slave devices
> >  - create a list of slave devices, then attach them after the bus scan has
> >    finished.
> >  - have device_add run the kobject_uevent in a separate task (work_queue)
> >  - or maybe the following which feels ugly but is easy.  Mark the bq27000 as
> >    not ready until after the power_supply_register, and if get_property is 
> >    called before the device is ready, return ENODATA.
> >    Simple and works.  Maybe horrible.
> >    power_supply_register will have scheduled a power_supply_changed() which
> >    will poll
> 
> You can check mutex owner - and if it is you who holds it, then dig into
> low-level function without lock.
> 

Interesting idea.  However:

struct mutex {
	/* 1: unlocked, 0: locked, negative: locked, possible waiters */
	atomic_t		count;
	spinlock_t		wait_lock;
	struct list_head	wait_list;
#if defined(CONFIG_DEBUG_MUTEXES) || defined(CONFIG_SMP)
	struct task_struct	*owner;
#endif


You can only check the owner on SMP builds, or when debugging is enabled.
So I don't think that approach can work.

Thanks,
NeilBrown

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

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

[Evgeniy Polyakov]

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.

-- 
	Evgeniy Polyakov

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

[NeilBrown]

[-- 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 --]

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

[Evgeniy Polyakov]

On Wed, May 09, 2012 at 11:43:49AM +1000, NeilBrown (neilb@suse.de) wrote:
> 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.

I only came to idea of not allowing sysfs in ->probe() and instead
introduce new callback, which will initialize 'external' to w1 core
parts without holding master's mutex

-- 
	Evgeniy Polyakov

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

[NeilBrown]

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

On Tue, 15 May 2012 05:26:34 +0400 Evgeniy Polyakov <zbr@ioremap.net> wrote:

> On Wed, May 09, 2012 at 11:43:49AM +1000, NeilBrown (neilb@suse.de) wrote:
> > 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.
> 
> I only came to idea of not allowing sysfs in ->probe() and instead
> introduce new callback, which will initialize 'external' to w1 core
> parts without holding master's mutex
> 

The more I think about it, the more I think that we really need to split the
lock in two.  That is the only way to avoid a potential deadlock with sysfs.

What do you think of the following?  It works for me but I cannot test the
other battery controllers or masters.

NeilBrown


From: NeilBrown <neilb@suse.de>
Date: Fri, 18 May 2012 15:59:52 +1000
Subject: [PATCH] W1: split master mutex to avoid deadlocks.

The 'mutex' in struct w1_master is use for two very different
purposes.

Firstly it protects various data structures such as the list of all
slaves.

Secondly it protects the w1 buss against concurrent accesses.

This can lead to deadlocks when the ->probe code called while adding a
slave needs to talk on the bus, as is the case for power_supply
devices.
ds2780 and ds2781 drivers contain a work around to track which
process hold the lock simply to avoid this deadlock.  bq27000 doesn't
have that work around and so deadlocks.

There are other possible deadlocks involving sysfs.
When removing a device the sysfs s_active lock is held, so the lock
that protects the slave list must take precedence over s_active.
However when access power_supply attributes via sysfs, the s_active
lock must take precedence over the lock that protects accesses to
the bus.

So to avoid deadlocks between w1 slaves and sysfs, these must be
two separate locks.  Making them separate means that the work around
in ds2780 and ds2781 can be removed.

So this patch:
 - adds a new mutex: "bus_mutex" which serialises access to the bus.
 - takes mutex in w1_search and ds1wm_search while they access
   the bus for searching.  The mutex is dropped before calling the
   callback which adds the slave.
 - changes all slaves to use bus_mutex instead of mutex to
   protect access to the bus
 - removes w1_ds2790_io_nolock and w1_ds2781_io_nolock, and the
   related code from drivers/power/ds278[01]_battery.c which
   calls them.

Signed-off-by: NeilBrown <neilb@suse.de>

diff --git a/drivers/power/ds2780_battery.c b/drivers/power/ds2780_battery.c
index de31cae..74fad94 100644
--- a/drivers/power/ds2780_battery.c
+++ b/drivers/power/ds2780_battery.c
@@ -39,7 +39,6 @@ struct ds2780_device_info {
 	struct device *dev;
 	struct power_supply bat;
 	struct device *w1_dev;
-	struct task_struct *mutex_holder;
 };
 
 enum current_types {
@@ -64,10 +63,7 @@ static inline struct power_supply *to_power_supply(struct device *dev)
 static inline int ds2780_battery_io(struct ds2780_device_info *dev_info,
 	char *buf, int addr, size_t count, int io)
 {
-	if (dev_info->mutex_holder == current)
-		return w1_ds2780_io_nolock(dev_info->w1_dev, buf, addr, count, io);
-	else
-		return w1_ds2780_io(dev_info->w1_dev, buf, addr, count, io);
+	return w1_ds2780_io(dev_info->w1_dev, buf, addr, count, io);
 }
 
 static inline int ds2780_read8(struct ds2780_device_info *dev_info, u8 *val,
@@ -779,7 +775,6 @@ static int __devinit ds2780_battery_probe(struct platform_device *pdev)
 	dev_info->bat.properties	= ds2780_battery_props;
 	dev_info->bat.num_properties	= ARRAY_SIZE(ds2780_battery_props);
 	dev_info->bat.get_property	= ds2780_battery_get_property;
-	dev_info->mutex_holder		= current;
 
 	ret = power_supply_register(&pdev->dev, &dev_info->bat);
 	if (ret) {
@@ -809,8 +804,6 @@ static int __devinit ds2780_battery_probe(struct platform_device *pdev)
 		goto fail_remove_bin_file;
 	}
 
-	dev_info->mutex_holder = NULL;
-
 	return 0;
 
 fail_remove_bin_file:
@@ -830,8 +823,6 @@ static int __devexit ds2780_battery_remove(struct platform_device *pdev)
 {
 	struct ds2780_device_info *dev_info = platform_get_drvdata(pdev);
 
-	dev_info->mutex_holder = current;
-
 	/* remove attributes */
 	sysfs_remove_group(&dev_info->bat.dev->kobj, &ds2780_attr_group);
 
diff --git a/drivers/power/ds2781_battery.c b/drivers/power/ds2781_battery.c
index ca0d653..9943e4d 100644
--- a/drivers/power/ds2781_battery.c
+++ b/drivers/power/ds2781_battery.c
@@ -37,7 +37,6 @@ struct ds2781_device_info {
 	struct device *dev;
 	struct power_supply bat;
 	struct device *w1_dev;
-	struct task_struct *mutex_holder;
 };
 
 enum current_types {
@@ -62,11 +61,7 @@ static inline struct power_supply *to_power_supply(struct device *dev)
 static inline int ds2781_battery_io(struct ds2781_device_info *dev_info,
 	char *buf, int addr, size_t count, int io)
 {
-	if (dev_info->mutex_holder == current)
-		return w1_ds2781_io_nolock(dev_info->w1_dev, buf, addr,
-				count, io);
-	else
-		return w1_ds2781_io(dev_info->w1_dev, buf, addr, count, io);
+	return w1_ds2781_io(dev_info->w1_dev, buf, addr, count, io);
 }
 
 int w1_ds2781_read(struct ds2781_device_info *dev_info, char *buf,
@@ -783,7 +778,6 @@ static int __devinit ds2781_battery_probe(struct platform_device *pdev)
 	dev_info->bat.properties	= ds2781_battery_props;
 	dev_info->bat.num_properties	= ARRAY_SIZE(ds2781_battery_props);
 	dev_info->bat.get_property	= ds2781_battery_get_property;
-	dev_info->mutex_holder		= current;
 
 	ret = power_supply_register(&pdev->dev, &dev_info->bat);
 	if (ret) {
@@ -813,8 +807,6 @@ static int __devinit ds2781_battery_probe(struct platform_device *pdev)
 		goto fail_remove_bin_file;
 	}
 
-	dev_info->mutex_holder = NULL;
-
 	return 0;
 
 fail_remove_bin_file:
@@ -834,8 +826,6 @@ static int __devexit ds2781_battery_remove(struct platform_device *pdev)
 {
 	struct ds2781_device_info *dev_info = platform_get_drvdata(pdev);
 
-	dev_info->mutex_holder = current;
-
 	/* remove attributes */
 	sysfs_remove_group(&dev_info->bat.dev->kobj, &ds2781_attr_group);
 
diff --git a/drivers/w1/masters/ds1wm.c b/drivers/w1/masters/ds1wm.c
index a0c8965..530a2d3 100644
--- a/drivers/w1/masters/ds1wm.c
+++ b/drivers/w1/masters/ds1wm.c
@@ -334,7 +334,9 @@ static void ds1wm_search(void *data, struct w1_master *master_dev,
 			return;
 		}
 
+		mutex_lock(&master_dev->bus_mutex);
 		if (ds1wm_reset(ds1wm_data)) {
+			mutex_unlock(&master_dev->bus_mutex);
 			dev_dbg(&ds1wm_data->pdev->dev,
 				"pass: %d reset error (or no slaves)\n", pass);
 			break;
@@ -387,6 +389,7 @@ static void ds1wm_search(void *data, struct w1_master *master_dev,
 
 		}
 		if (ds1wm_data->read_error) {
+			mutex_unlock(&master_dev->bus_mutex);
 			dev_err(&ds1wm_data->pdev->dev,
 				"pass: %d read error, retrying\n", pass);
 			break;
@@ -400,6 +403,7 @@ static void ds1wm_search(void *data, struct w1_master *master_dev,
 		dev_dbg(&ds1wm_data->pdev->dev,
 			"pass: %d resetting bus\n", pass);
 		ds1wm_reset(ds1wm_data);
+		mutex_unlock(&master_dev->bus_mutex);
 		if ((r_prime & ((u64)1 << 63)) && (d & ((u64)1 << 63))) {
 			dev_err(&ds1wm_data->pdev->dev,
 				"pass: %d bus error, retrying\n", pass);
diff --git a/drivers/w1/slaves/w1_bq27000.c b/drivers/w1/slaves/w1_bq27000.c
index 52ad812..773dca5 100644
--- a/drivers/w1/slaves/w1_bq27000.c
+++ b/drivers/w1/slaves/w1_bq27000.c
@@ -31,10 +31,10 @@ 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);
 
-	mutex_lock(&sl->master->mutex);
+	mutex_lock(&sl->master->bus_mutex);
 	w1_write_8(sl->master, HDQ_CMD_READ | reg);
 	val = w1_read_8(sl->master);
-	mutex_unlock(&sl->master->mutex);
+	mutex_unlock(&sl->master->bus_mutex);
 
 	return val;
 }
diff --git a/drivers/w1/slaves/w1_ds2408.c b/drivers/w1/slaves/w1_ds2408.c
index 7c8cdb8..485dc34 100644
--- a/drivers/w1/slaves/w1_ds2408.c
+++ b/drivers/w1/slaves/w1_ds2408.c
@@ -52,11 +52,11 @@ static int _read_reg(struct w1_slave *sl, u8 address, unsigned char* buf)
 	if (!buf)
 		return -EINVAL;
 
-	mutex_lock(&sl->master->mutex);
+	mutex_lock(&sl->master->bus_mutex);
 	dev_dbg(&sl->dev, "mutex locked");
 
 	if (w1_reset_select_slave(sl)) {
-		mutex_unlock(&sl->master->mutex);
+		mutex_unlock(&sl->master->bus_mutex);
 		return -EIO;
 	}
 
@@ -66,7 +66,7 @@ static int _read_reg(struct w1_slave *sl, u8 address, unsigned char* buf)
 	w1_write_block(sl->master, wrbuf, 3);
 	*buf = w1_read_8(sl->master);
 
-	mutex_unlock(&sl->master->mutex);
+	mutex_unlock(&sl->master->bus_mutex);
 	dev_dbg(&sl->dev, "mutex unlocked");
 	return 1;
 }
@@ -165,7 +165,7 @@ static ssize_t w1_f29_write_output(
 		return -EFAULT;
 
 	dev_dbg(&sl->dev, "locking mutex for write_output");
-	mutex_lock(&sl->master->mutex);
+	mutex_lock(&sl->master->bus_mutex);
 	dev_dbg(&sl->dev, "mutex locked");
 
 	if (w1_reset_select_slave(sl))
@@ -200,14 +200,14 @@ static ssize_t w1_f29_write_output(
 		/* read the result of the READ_PIO_REGS command */
 		if (w1_read_8(sl->master) == *buf) {
 			/* success! */
-			mutex_unlock(&sl->master->mutex);
+			mutex_unlock(&sl->master->bus_mutex);
 			dev_dbg(&sl->dev,
 				"mutex unlocked, retries:%d", retries);
 			return 1;
 		}
 	}
 error:
-	mutex_unlock(&sl->master->mutex);
+	mutex_unlock(&sl->master->bus_mutex);
 	dev_dbg(&sl->dev, "mutex unlocked in error, retries:%d", retries);
 
 	return -EIO;
@@ -228,7 +228,7 @@ static ssize_t w1_f29_write_activity(
 	if (count != 1 || off != 0)
 		return -EFAULT;
 
-	mutex_lock(&sl->master->mutex);
+	mutex_lock(&sl->master->bus_mutex);
 
 	if (w1_reset_select_slave(sl))
 		goto error;
@@ -236,7 +236,7 @@ static ssize_t w1_f29_write_activity(
 	while (retries--) {
 		w1_write_8(sl->master, W1_F29_FUNC_RESET_ACTIVITY_LATCHES);
 		if (w1_read_8(sl->master) == W1_F29_SUCCESS_CONFIRM_BYTE) {
-			mutex_unlock(&sl->master->mutex);
+			mutex_unlock(&sl->master->bus_mutex);
 			return 1;
 		}
 		if (w1_reset_resume_command(sl->master))
@@ -244,7 +244,7 @@ static ssize_t w1_f29_write_activity(
 	}
 
 error:
-	mutex_unlock(&sl->master->mutex);
+	mutex_unlock(&sl->master->bus_mutex);
 	return -EIO;
 }
 
@@ -263,7 +263,7 @@ static ssize_t w1_f29_write_status_control(
 	if (count != 1 || off != 0)
 		return -EFAULT;
 
-	mutex_lock(&sl->master->mutex);
+	mutex_lock(&sl->master->bus_mutex);
 
 	if (w1_reset_select_slave(sl))
 		goto error;
@@ -285,12 +285,12 @@ static ssize_t w1_f29_write_status_control(
 		w1_write_block(sl->master, w1_buf, 3);
 		if (w1_read_8(sl->master) == *buf) {
 			/* success! */
-			mutex_unlock(&sl->master->mutex);
+			mutex_unlock(&sl->master->bus_mutex);
 			return 1;
 		}
 	}
 error:
-	mutex_unlock(&sl->master->mutex);
+	mutex_unlock(&sl->master->bus_mutex);
 
 	return -EIO;
 }
diff --git a/drivers/w1/slaves/w1_ds2423.c b/drivers/w1/slaves/w1_ds2423.c
index 7a7dbe5..40a10b5 100644
--- a/drivers/w1/slaves/w1_ds2423.c
+++ b/drivers/w1/slaves/w1_ds2423.c
@@ -66,7 +66,7 @@ static ssize_t w1_counter_read(struct device *device,
 	wrbuf[0]	= 0xA5;
 	wrbuf[1]	= rom_addr & 0xFF;
 	wrbuf[2]	= rom_addr >> 8;
-	mutex_lock(&dev->mutex);
+	mutex_lock(&dev->bus_mutex);
 	if (!w1_reset_select_slave(sl)) {
 		w1_write_block(dev, wrbuf, 3);
 		read_byte_count = 0;
@@ -124,7 +124,7 @@ static ssize_t w1_counter_read(struct device *device,
 	} else {
 		c -= snprintf(out_buf + PAGE_SIZE - c, c, "Connection error");
 	}
-	mutex_unlock(&dev->mutex);
+	mutex_unlock(&dev->bus_mutex);
 	return PAGE_SIZE - c;
 }
 
diff --git a/drivers/w1/slaves/w1_ds2431.c b/drivers/w1/slaves/w1_ds2431.c
index 84e2410..984b303 100644
--- a/drivers/w1/slaves/w1_ds2431.c
+++ b/drivers/w1/slaves/w1_ds2431.c
@@ -107,7 +107,7 @@ static ssize_t w1_f2d_read_bin(struct file *filp, struct kobject *kobj,
 	if (count == 0)
 		return 0;
 
-	mutex_lock(&sl->master->mutex);
+	mutex_lock(&sl->master->bus_mutex);
 
 	/* read directly from the EEPROM in chunks of W1_F2D_READ_MAXLEN */
 	while (todo > 0) {
@@ -126,7 +126,7 @@ static ssize_t w1_f2d_read_bin(struct file *filp, struct kobject *kobj,
 		off += W1_F2D_READ_MAXLEN;
 	}
 
-	mutex_unlock(&sl->master->mutex);
+	mutex_unlock(&sl->master->bus_mutex);
 
 	return count;
 }
@@ -214,7 +214,7 @@ static ssize_t w1_f2d_write_bin(struct file *filp, struct kobject *kobj,
 	if (count == 0)
 		return 0;
 
-	mutex_lock(&sl->master->mutex);
+	mutex_lock(&sl->master->bus_mutex);
 
 	/* Can only write data in blocks of the size of the scratchpad */
 	addr = off;
@@ -259,7 +259,7 @@ static ssize_t w1_f2d_write_bin(struct file *filp, struct kobject *kobj,
 	}
 
 out_up:
-	mutex_unlock(&sl->master->mutex);
+	mutex_unlock(&sl->master->bus_mutex);
 
 	return count;
 }
diff --git a/drivers/w1/slaves/w1_ds2433.c b/drivers/w1/slaves/w1_ds2433.c
index 0f7b8f9..85f2cdb 100644
--- a/drivers/w1/slaves/w1_ds2433.c
+++ b/drivers/w1/slaves/w1_ds2433.c
@@ -107,7 +107,7 @@ static ssize_t w1_f23_read_bin(struct file *filp, struct kobject *kobj,
 	if ((count = w1_f23_fix_count(off, count, W1_EEPROM_SIZE)) == 0)
 		return 0;
 
-	mutex_lock(&sl->master->mutex);
+	mutex_lock(&sl->master->bus_mutex);
 
 #ifdef CONFIG_W1_SLAVE_DS2433_CRC
 
@@ -138,7 +138,7 @@ static ssize_t w1_f23_read_bin(struct file *filp, struct kobject *kobj,
 #endif	/* CONFIG_W1_SLAVE_DS2433_CRC */
 
 out_up:
-	mutex_unlock(&sl->master->mutex);
+	mutex_unlock(&sl->master->bus_mutex);
 
 	return count;
 }
@@ -233,7 +233,7 @@ static ssize_t w1_f23_write_bin(struct file *filp, struct kobject *kobj,
 	}
 #endif	/* CONFIG_W1_SLAVE_DS2433_CRC */
 
-	mutex_lock(&sl->master->mutex);
+	mutex_lock(&sl->master->bus_mutex);
 
 	/* Can only write data to one page at a time */
 	idx = 0;
@@ -251,7 +251,7 @@ static ssize_t w1_f23_write_bin(struct file *filp, struct kobject *kobj,
 	}
 
 out_up:
-	mutex_unlock(&sl->master->mutex);
+	mutex_unlock(&sl->master->bus_mutex);
 
 	return count;
 }
diff --git a/drivers/w1/slaves/w1_ds2760.c b/drivers/w1/slaves/w1_ds2760.c
index 5754c9a..aa7bd5f 100644
--- a/drivers/w1/slaves/w1_ds2760.c
+++ b/drivers/w1/slaves/w1_ds2760.c
@@ -31,7 +31,7 @@ static int w1_ds2760_io(struct device *dev, char *buf, int addr, size_t count,
 	if (!dev)
 		return 0;
 
-	mutex_lock(&sl->master->mutex);
+	mutex_lock(&sl->master->bus_mutex);
 
 	if (addr > DS2760_DATA_SIZE || addr < 0) {
 		count = 0;
@@ -54,7 +54,7 @@ static int w1_ds2760_io(struct device *dev, char *buf, int addr, size_t count,
 	}
 
 out:
-	mutex_unlock(&sl->master->mutex);
+	mutex_unlock(&sl->master->bus_mutex);
 
 	return count;
 }
@@ -76,14 +76,14 @@ static int w1_ds2760_eeprom_cmd(struct device *dev, int addr, int cmd)
 	if (!dev)
 		return -EINVAL;
 
-	mutex_lock(&sl->master->mutex);
+	mutex_lock(&sl->master->bus_mutex);
 
 	if (w1_reset_select_slave(sl) == 0) {
 		w1_write_8(sl->master, cmd);
 		w1_write_8(sl->master, addr);
 	}
 
-	mutex_unlock(&sl->master->mutex);
+	mutex_unlock(&sl->master->bus_mutex);
 	return 0;
 }
 
diff --git a/drivers/w1/slaves/w1_ds2780.c b/drivers/w1/slaves/w1_ds2780.c
index 39f78c0..7b09307 100644
--- a/drivers/w1/slaves/w1_ds2780.c
+++ b/drivers/w1/slaves/w1_ds2780.c
@@ -60,30 +60,16 @@ int w1_ds2780_io(struct device *dev, char *buf, int addr, size_t count,
 	if (!dev)
 		return -ENODEV;
 
-	mutex_lock(&sl->master->mutex);
+	mutex_lock(&sl->master->bus_mutex);
 
 	ret = w1_ds2780_do_io(dev, buf, addr, count, io);
 
-	mutex_unlock(&sl->master->mutex);
+	mutex_unlock(&sl->master->bus_mutex);
 
 	return ret;
 }
 EXPORT_SYMBOL(w1_ds2780_io);
 
-int w1_ds2780_io_nolock(struct device *dev, char *buf, int addr, size_t count,
-			int io)
-{
-	int ret;
-
-	if (!dev)
-		return -ENODEV;
-
-	ret = w1_ds2780_do_io(dev, buf, addr, count, io);
-
-	return ret;
-}
-EXPORT_SYMBOL(w1_ds2780_io_nolock);
-
 int w1_ds2780_eeprom_cmd(struct device *dev, int addr, int cmd)
 {
 	struct w1_slave *sl = container_of(dev, struct w1_slave, dev);
@@ -91,14 +77,14 @@ int w1_ds2780_eeprom_cmd(struct device *dev, int addr, int cmd)
 	if (!dev)
 		return -EINVAL;
 
-	mutex_lock(&sl->master->mutex);
+	mutex_lock(&sl->master->bus_mutex);
 
 	if (w1_reset_select_slave(sl) == 0) {
 		w1_write_8(sl->master, cmd);
 		w1_write_8(sl->master, addr);
 	}
 
-	mutex_unlock(&sl->master->mutex);
+	mutex_unlock(&sl->master->bus_mutex);
 	return 0;
 }
 EXPORT_SYMBOL(w1_ds2780_eeprom_cmd);
diff --git a/drivers/w1/slaves/w1_ds2780.h b/drivers/w1/slaves/w1_ds2780.h
index 7373793..a1fba79 100644
--- a/drivers/w1/slaves/w1_ds2780.h
+++ b/drivers/w1/slaves/w1_ds2780.h
@@ -124,8 +124,6 @@
 
 extern int w1_ds2780_io(struct device *dev, char *buf, int addr, size_t count,
 			int io);
-extern int w1_ds2780_io_nolock(struct device *dev, char *buf, int addr,
-			size_t count, int io);
 extern int w1_ds2780_eeprom_cmd(struct device *dev, int addr, int cmd);
 
 #endif /* !_W1_DS2780_H */
diff --git a/drivers/w1/slaves/w1_ds2781.c b/drivers/w1/slaves/w1_ds2781.c
index 0d0c798..877daf7 100644
--- a/drivers/w1/slaves/w1_ds2781.c
+++ b/drivers/w1/slaves/w1_ds2781.c
@@ -58,30 +58,16 @@ int w1_ds2781_io(struct device *dev, char *buf, int addr, size_t count,
 	if (!dev)
 		return -ENODEV;
 
-	mutex_lock(&sl->master->mutex);
+	mutex_lock(&sl->master->bus_mutex);
 
 	ret = w1_ds2781_do_io(dev, buf, addr, count, io);
 
-	mutex_unlock(&sl->master->mutex);
+	mutex_unlock(&sl->master->bus_mutex);
 
 	return ret;
 }
 EXPORT_SYMBOL(w1_ds2781_io);
 
-int w1_ds2781_io_nolock(struct device *dev, char *buf, int addr, size_t count,
-			int io)
-{
-	int ret;
-
-	if (!dev)
-		return -ENODEV;
-
-	ret = w1_ds2781_do_io(dev, buf, addr, count, io);
-
-	return ret;
-}
-EXPORT_SYMBOL(w1_ds2781_io_nolock);
-
 int w1_ds2781_eeprom_cmd(struct device *dev, int addr, int cmd)
 {
 	struct w1_slave *sl = container_of(dev, struct w1_slave, dev);
@@ -89,14 +75,14 @@ int w1_ds2781_eeprom_cmd(struct device *dev, int addr, int cmd)
 	if (!dev)
 		return -EINVAL;
 
-	mutex_lock(&sl->master->mutex);
+	mutex_lock(&sl->master->bus_mutex);
 
 	if (w1_reset_select_slave(sl) == 0) {
 		w1_write_8(sl->master, cmd);
 		w1_write_8(sl->master, addr);
 	}
 
-	mutex_unlock(&sl->master->mutex);
+	mutex_unlock(&sl->master->bus_mutex);
 	return 0;
 }
 EXPORT_SYMBOL(w1_ds2781_eeprom_cmd);
diff --git a/drivers/w1/slaves/w1_ds2781.h b/drivers/w1/slaves/w1_ds2781.h
index 82bc664..557dfb0 100644
--- a/drivers/w1/slaves/w1_ds2781.h
+++ b/drivers/w1/slaves/w1_ds2781.h
@@ -129,8 +129,6 @@
 
 extern int w1_ds2781_io(struct device *dev, char *buf, int addr, size_t count,
 			int io);
-extern int w1_ds2781_io_nolock(struct device *dev, char *buf, int addr,
-			size_t count, int io);
 extern int w1_ds2781_eeprom_cmd(struct device *dev, int addr, int cmd);
 
 #endif /* !_W1_DS2781_H */
diff --git a/drivers/w1/slaves/w1_therm.c b/drivers/w1/slaves/w1_therm.c
index ff29ae7..d90062b 100644
--- a/drivers/w1/slaves/w1_therm.c
+++ b/drivers/w1/slaves/w1_therm.c
@@ -179,7 +179,7 @@ static ssize_t w1_therm_read(struct device *device,
 	int i, max_trying = 10;
 	ssize_t c = PAGE_SIZE;
 
-	i = mutex_lock_interruptible(&dev->mutex);
+	i = mutex_lock_interruptible(&dev->bus_mutex);
 	if (i != 0)
 		return i;
 
@@ -207,19 +207,19 @@ static ssize_t w1_therm_read(struct device *device,
 			w1_write_8(dev, W1_CONVERT_TEMP);
 
 			if (external_power) {
-				mutex_unlock(&dev->mutex);
+				mutex_unlock(&dev->bus_mutex);
 
 				sleep_rem = msleep_interruptible(tm);
 				if (sleep_rem != 0)
 					return -EINTR;
 
-				i = mutex_lock_interruptible(&dev->mutex);
+				i = mutex_lock_interruptible(&dev->bus_mutex);
 				if (i != 0)
 					return i;
 			} else if (!w1_strong_pullup) {
 				sleep_rem = msleep_interruptible(tm);
 				if (sleep_rem != 0) {
-					mutex_unlock(&dev->mutex);
+					mutex_unlock(&dev->bus_mutex);
 					return -EINTR;
 				}
 			}
@@ -258,7 +258,7 @@ static ssize_t w1_therm_read(struct device *device,
 
 	c -= snprintf(buf + PAGE_SIZE - c, c, "t=%d\n",
 		w1_convert_temp(rom, sl->family->fid));
-	mutex_unlock(&dev->mutex);
+	mutex_unlock(&dev->bus_mutex);
 
 	return PAGE_SIZE - c;
 }
diff --git a/drivers/w1/w1.c b/drivers/w1/w1.c
index 9761950..31af83e 100644
--- a/drivers/w1/w1.c
+++ b/drivers/w1/w1.c
@@ -887,16 +887,21 @@ void w1_search(struct w1_master *dev, u8 search_type, w1_slave_found_callback cb
 		 *
 		 * Return 0 - device(s) present, 1 - no devices present.
 		 */
+		mutex_lock(&dev->bus_mutex);
 		if (w1_reset_bus(dev)) {
+			mutex_unlock(&dev->bus_mutex);
 			dev_dbg(&dev->dev, "No devices present on the wire.\n");
 			break;
 		}
 
 		/* Do fast search on single slave bus */
 		if (dev->max_slave_count == 1) {
+			int rv;
 			w1_write_8(dev, W1_READ_ROM);
+			rv = w1_read_block(dev, (u8 *)&rn, 8);
+			mutex_unlock(&dev->bus_mutex);
 
-			if (w1_read_block(dev, (u8 *)&rn, 8) == 8 && rn)
+			if (rv == 8 && rn)
 				cb(dev, rn);
 
 			break;
@@ -929,10 +934,12 @@ void w1_search(struct w1_master *dev, u8 search_type, w1_slave_found_callback cb
 			rn |= (tmp64 << i);
 
 			if (kthread_should_stop()) {
+				mutex_unlock(&dev->bus_mutex);
 				dev_dbg(&dev->dev, "Abort w1_search\n");
 				return;
 			}
 		}
+		mutex_unlock(&dev->bus_mutex);
 
 		if ( (triplet_ret & 0x03) != 0x03 ) {
 			if ( (desc_bit == last_zero) || (last_zero < 0))
diff --git a/drivers/w1/w1.h b/drivers/w1/w1.h
index 4d012ca..45908e5 100644
--- a/drivers/w1/w1.h
+++ b/drivers/w1/w1.h
@@ -180,6 +180,7 @@ struct w1_master
 
 	struct task_struct	*thread;
 	struct mutex		mutex;
+	struct mutex		bus_mutex;
 
 	struct device_driver	*driver;
 	struct device		dev;
diff --git a/drivers/w1/w1_int.c b/drivers/w1/w1_int.c
index 6828835..5314341 100644
--- a/drivers/w1/w1_int.c
+++ b/drivers/w1/w1_int.c
@@ -76,6 +76,7 @@ static struct w1_master * w1_alloc_dev(u32 id, int slave_count, int slave_ttl,
 
 	INIT_LIST_HEAD(&dev->slist);
 	mutex_init(&dev->mutex);
+	mutex_init(&dev->bus_mutex);
 
 	memcpy(&dev->dev, device, sizeof(struct device));
 	dev_set_name(&dev->dev, "w1_bus_master%u", dev->id);

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

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

[Evgeniy Polyakov]

Hi

Sorry for *that* long delay, this mail was somehow dropped.

On Fri, May 18, 2012 at 04:05:07PM +1000, NeilBrown (neilb@suse.de) wrote:
> So to avoid deadlocks between w1 slaves and sysfs, these must be
> two separate locks.  Making them separate means that the work around
> in ds2780 and ds2781 can be removed.
> 
> So this patch:
>  - adds a new mutex: "bus_mutex" which serialises access to the bus.
>  - takes mutex in w1_search and ds1wm_search while they access
>    the bus for searching.  The mutex is dropped before calling the
>    callback which adds the slave.
>  - changes all slaves to use bus_mutex instead of mutex to
>    protect access to the bus
>  - removes w1_ds2790_io_nolock and w1_ds2781_io_nolock, and the
>    related code from drivers/power/ds278[01]_battery.c which
>    calls them.

Greg, can you put it into the tree when merge window opens (sorry, I do
not really know release cycle dates). Since other drivers basically are
not affected (only mutex name changed for them), it should be ok, but
this new feature should be told for all new device.

Also if deadlock will be caught somewhere in w1 in current tree (which
is possible, although unlikely - w1 users are usually embedded devices
where new sysfs files are created only at startup time), we can direct
authors to this thread.

> Signed-off-by: NeilBrown <neilb@suse.de>

Acked-by: Evgeniy Polyakov <zbr@ioremap.net>

-- 
	Evgeniy Polyakov