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

[NeilBrown <neilb@suse.de>]

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