| Message ID | 39a5ee5b03a884334640e9c65bd8ec7a@alexhornung.com |
|---|---|
| State | New |
| Headers | show |
On 01/29/2013 07:29 AM, Alex Hornung wrote: > Hi, > > this patch fixes bug 1093543[1]. Explanation is further down, as per > commit message. > > It is not obvious to me what your upstream repository (that includes the > bq27541 driver) is. The MAINTAINERS file also includes no information on > this file. > Alex - our source can be found at git://kernel.ubuntu.com/ubuntu/ubuntu-nexus7.git The current release is always in the master branch. master-next is our working branch. The origin is based on https://android.googlesource.com/kernel/tegra/ android-tegra3-grouper-3.1-jb-mr1
On Tue, Jan 29, 2013 at 02:29:54PM +0000, Alex Hornung wrote: > Hi, > > this patch fixes bug 1093543[1]. Explanation is further down, as per > commit message. > > It is not obvious to me what your upstream repository (that includes > the bq27541 driver) is. The MAINTAINERS file also includes no > information on this file. > > I would suspect it's android kernel/tegra[2], but I'm not sure since > it only contains release branches (and an empty master branch). If > it is, which one is the branch you based your master branch on? If > not, which one is it? I'd like to get the patch upstream. > > > * Currently, get_property() and its callees are not thread-safe, since > they share some global state, which they access without > serialization > or guaranteed atomicity. > > * get_property() can be called in a reentrant fashion from either > several concurrent sysfs accesses, or a sysfs access concurrently > with > an access from power_supply_core.c, which in turn are triggered from > a work queue in the driver itself. > > * This fixes bogus readings affecting the capacity and the charge_now > values - possibly others as well. > > > Cheers, > Alex > > > [1]: https://bugs.launchpad.net/ubuntu-nexus7/+bug/1093543 > [2]: https://android.googlesource.com/kernel/tegra/ > From 0164dc98a311137bdda9c39dc20cfcb89079ac12 Mon Sep 17 00:00:00 2001 > From: Alex Hornung <alex@alexhornung.com> > Date: Mon, 28 Jan 2013 20:43:08 +0000 > Subject: [PATCH 1/1] power: bq27541 driver - serialize get_property > > * Currently, get_property() and its callees are not thread-safe, since > they share some global state, which they access without serialization > or guaranteed atomicity. > > * get_property() can be called in a reentrant fashion from either > several concurrent sysfs accesses, or a sysfs access concurrently with > an access from power_supply_core.c, which in turn are triggered from > a work queue in the driver itself. > > * This fixes bogus readings affecting the capacity and the charge_now > values - possibly others as well. > > Signed-off-by: Alex Hornung <alex@alexhornung.com> Most of the data accessed here are atomic sized objects and so it is unlikely that overlapping accesses would get you anything other than an old but valid value. That said the values are obtained by i2c bit banging and indeed there really ought to be locking to prevent overlapping transcations interlieving. > --- > drivers/power/bq27541_battery.c | 15 +++++++++++---- > 1 file changed, 11 insertions(+), 4 deletions(-) > > diff --git a/drivers/power/bq27541_battery.c b/drivers/power/bq27541_battery.c > index 9a86cc7..9ce5c2d 100755 > --- a/drivers/power/bq27541_battery.c > +++ b/drivers/power/bq27541_battery.c > @@ -34,6 +34,7 @@ > #include <linux/delay.h> > #include <linux/timer.h> > #include <linux/interrupt.h> > +#include <linux/mutex.h> > #include <asm/unaligned.h> > #include <linux/miscdevice.h> > #include <mach/gpio.h> > @@ -265,7 +266,7 @@ static struct bq27541_device_info { > unsigned int old_temperature; > unsigned int temp_err; > unsigned int prj_id; > - spinlock_t lock; > + struct mutex lock; > } *bq27541_device; > > static int bq27541_read_i2c(u8 reg, int *rt_value, int b_single) > @@ -689,6 +690,9 @@ static int bq27541_get_property(struct power_supply *psy, > union power_supply_propval *val) > { > u8 count; > + > + mutex_lock(&bq27541_device->lock); > + > switch (psp) { > case POWER_SUPPLY_PROP_PRESENT: > case POWER_SUPPLY_PROP_HEALTH: > @@ -717,19 +721,21 @@ static int bq27541_get_property(struct power_supply *psy, > } > > if (bq27541_get_psp(count, psp, val)) > - return -EINVAL; > + goto error; > break; > > default: > dev_err(&bq27541_device->client->dev, > "%s: INVALID property psp=%u\n", __func__,psp); > - return -EINVAL; > + goto error; > } > > + mutex_unlock(&bq27541_device->lock); > return 0; > > error: > > + mutex_unlock(&bq27541_device->lock); > return -EINVAL; > } > > @@ -757,6 +763,8 @@ static int bq27541_probe(struct i2c_client *client, > bq27541_device->shutdown_disable = 1; > bq27541_device->cap_zero_count = 0; > > + mutex_init(&bq27541_device->lock); > + > for (i = 0; i < ARRAY_SIZE(bq27541_supply); i++) { > ret = power_supply_register(&client->dev, &bq27541_supply[i]); > if (ret) { > @@ -775,7 +783,6 @@ static int bq27541_probe(struct i2c_client *client, > INIT_DELAYED_WORK(&bq27541_device->shutdown_en_work, shutdown_enable_set); > cancel_delayed_work(&bq27541_device->status_poll_work); > > - spin_lock_init(&bq27541_device->lock); > wake_lock_init(&bq27541_device->low_battery_wake_lock, WAKE_LOCK_SUSPEND, "low_battery_detection"); > wake_lock_init(&bq27541_device->cable_wake_lock, WAKE_LOCK_SUSPEND, "cable_state_changed"); > > -- > 1.7.10.4 This is probabally a rather large hammer for the task, but it does seem to prevent parallel i2c transactions and serialise the data as well. Alex, I assume you are upstreaming this in parallel. Overall assuming this passes testing it seems that it should be low risk. Acked-by: Andy Whitcroft <apw@canonical.com> -apw
Acked-by: Leann Ogasawara <leann.ogasawara@canonical.com> On 01/29/2013 06:29 AM, Alex Hornung wrote: > Hi, > > this patch fixes bug 1093543[1]. Explanation is further down, as per > commit message. > > It is not obvious to me what your upstream repository (that includes > the bq27541 driver) is. The MAINTAINERS file also includes no > information on this file. > > I would suspect it's android kernel/tegra[2], but I'm not sure since > it only contains release branches (and an empty master branch). If it > is, which one is the branch you based your master branch on? If not, > which one is it? I'd like to get the patch upstream. > > > * Currently, get_property() and its callees are not thread-safe, since > they share some global state, which they access without serialization > or guaranteed atomicity. > > * get_property() can be called in a reentrant fashion from either > several concurrent sysfs accesses, or a sysfs access concurrently with > an access from power_supply_core.c, which in turn are triggered from > a work queue in the driver itself. > > * This fixes bogus readings affecting the capacity and the charge_now > values - possibly others as well. > > > Cheers, > Alex > > > [1]: https://bugs.launchpad.net/ubuntu-nexus7/+bug/1093543 > [2]: https://android.googlesource.com/kernel/tegra/ > >
Hi Andy, thanks. On 29/01/13 19:52, Andy Whitcroft wrote: > This is probabally a rather large hammer for the task, but it does seem > to prevent parallel i2c transactions and serialise the data as well. It sure is. Fact is that the driver is a mess as it stands, and could do with a complete rewrite. However, that is more effort than I am willing to put into this. > Alex, I assume you are upstreaming this in parallel. Yes, now that I know which one the upstream that your kernel is based on is, I'll get the patches upstream - that is, once I figure out how that "repo" script submits things into android's gerrit. Same goes for the other patch. > Overall assuming this passes testing it seems that it should be low > risk. Gema did some test runs on this - it gets rid of the spurious values she was getting, and everything else works as before. Cheers, Alex
Applied to ubuntu-nexus7. -apw
On 29/01/13 19:52, Andy Whitcroft wrote:
> Alex, I assume you are upstreaming this in parallel.
It's submitted upstream here:
https://android-review.googlesource.com/#/c/50742/
Cheers,
Alex
hi, Am Dienstag, den 29.01.2013, 14:29 +0000 schrieb Alex Hornung: > Hi, > > this patch fixes bug 1093543[1]. Explanation is further down, as per > commit message. i tested the patch yesterday and didnt see any change in behavior for 1093543. i left the battery draining completely twice while using the patch, both times teh erratic behavior persisted and i got bombed with notifications around 30% battery left ... ciao oli
diff --git a/drivers/power/bq27541_battery.c b/drivers/power/bq27541_battery.c index 9a86cc7..9ce5c2d 100755 --- a/drivers/power/bq27541_battery.c +++ b/drivers/power/bq27541_battery.c @@ -34,6 +34,7 @@ #include <linux/delay.h> #include <linux/timer.h> #include <linux/interrupt.h> +#include <linux/mutex.h> #include <asm/unaligned.h> #include <linux/miscdevice.h> #include <mach/gpio.h> @@ -265,7 +266,7 @@ static struct bq27541_device_info { unsigned int old_temperature; unsigned int temp_err; unsigned int prj_id; - spinlock_t lock; + struct mutex lock; } *bq27541_device; static int bq27541_read_i2c(u8 reg, int *rt_value, int b_single) @@ -689,6 +690,9 @@ static int bq27541_get_property(struct power_supply *psy, union power_supply_propval *val) { u8 count; + + mutex_lock(&bq27541_device->lock); + switch (psp) { case POWER_SUPPLY_PROP_PRESENT: case POWER_SUPPLY_PROP_HEALTH: @@ -717,19 +721,21 @@ static int bq27541_get_property(struct power_supply *psy, } if (bq27541_get_psp(count, psp, val)) - return -EINVAL; + goto error; break; default: dev_err(&bq27541_device->client->dev, "%s: INVALID property psp=%u\n", __func__,psp); - return -EINVAL; + goto error; } + mutex_unlock(&bq27541_device->lock); return 0; error: + mutex_unlock(&bq27541_device->lock); return -EINVAL; } @@ -757,6 +763,8 @@ static int bq27541_probe(struct i2c_client *client, bq27541_device->shutdown_disable = 1; bq27541_device->cap_zero_count = 0; + mutex_init(&bq27541_device->lock); + for (i = 0; i < ARRAY_SIZE(bq27541_supply); i++) { ret = power_supply_register(&client->dev, &bq27541_supply[i]); if (ret) { @@ -775,7 +783,6 @@ static int bq27541_probe(struct i2c_client *client, INIT_DELAYED_WORK(&bq27541_device->shutdown_en_work, shutdown_enable_set); cancel_delayed_work(&bq27541_device->status_poll_work); - spin_lock_init(&bq27541_device->lock); wake_lock_init(&bq27541_device->low_battery_wake_lock, WAKE_LOCK_SUSPEND, "low_battery_detection"); wake_lock_init(&bq27541_device->cable_wake_lock, WAKE_LOCK_SUSPEND, "cable_state_changed");
Hi, this patch fixes bug 1093543[1]. Explanation is further down, as per commit message. It is not obvious to me what your upstream repository (that includes the bq27541 driver) is. The MAINTAINERS file also includes no information on this file. I would suspect it's android kernel/tegra[2], but I'm not sure since it only contains release branches (and an empty master branch). If it is, which one is the branch you based your master branch on? If not, which one is it? I'd like to get the patch upstream. * Currently, get_property() and its callees are not thread-safe, since they share some global state, which they access without serialization or guaranteed atomicity. * get_property() can be called in a reentrant fashion from either several concurrent sysfs accesses, or a sysfs access concurrently with an access from power_supply_core.c, which in turn are triggered from a work queue in the driver itself. * This fixes bogus readings affecting the capacity and the charge_now values - possibly others as well. Cheers, Alex [1]: https://bugs.launchpad.net/ubuntu-nexus7/+bug/1093543 [2]: https://android.googlesource.com/kernel/tegra/ From 0164dc98a311137bdda9c39dc20cfcb89079ac12 Mon Sep 17 00:00:00 2001 From: Alex Hornung <alex@alexhornung.com> Date: Mon, 28 Jan 2013 20:43:08 +0000 Subject: [PATCH 1/1] power: bq27541 driver - serialize get_property * Currently, get_property() and its callees are not thread-safe, since they share some global state, which they access without serialization or guaranteed atomicity. * get_property() can be called in a reentrant fashion from either several concurrent sysfs accesses, or a sysfs access concurrently with an access from power_supply_core.c, which in turn are triggered from a work queue in the driver itself. * This fixes bogus readings affecting the capacity and the charge_now values - possibly others as well. Signed-off-by: Alex Hornung <alex@alexhornung.com> --- drivers/power/bq27541_battery.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)