Patchwork [1/1] power: bq27541 driver - serialize get_property

login
register
mail settings
Submitter Alex Hornung
Date Jan. 29, 2013, 2:29 p.m.
Message ID <39a5ee5b03a884334640e9c65bd8ec7a@alexhornung.com>
Download mbox | patch
Permalink /patch/216566/
State New
Headers show

Comments

Alex Hornung - Jan. 29, 2013, 2:29 p.m.
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(-)
Tim Gardner - Jan. 29, 2013, 7:20 p.m.
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
Andy Whitcroft - Jan. 29, 2013, 7:52 p.m.
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
Leann Ogasawara - Jan. 29, 2013, 7:57 p.m.
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/
>
>
Alex Hornung - Jan. 29, 2013, 8:03 p.m.
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
Andy Whitcroft - Jan. 29, 2013, 8:26 p.m.
Applied to ubuntu-nexus7.

-apw
Alex Hornung - Jan. 29, 2013, 9:31 p.m.
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
Oliver Grawert - Jan. 30, 2013, 8:32 a.m.
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

Patch

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");