[Acked] [PATCH 1/1] power: bq27541 driver - serialize get_property

[Andy Whitcroft]

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 at 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 at 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 at canonical.com>

-apw