Messages in this thread Patch in this message | | | Date | Wed, 2 May 2012 16:26:27 +1000 | From | NeilBrown <> | Subject | Re: [PATCH] w1: Introduce a slave mutex for serializing IO. |
| |
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; }[unhandled content-type:application/pgp-signature] | |