From f14879eae1c5afd899e63f3a459a8c06fc3da21d Mon Sep 17 00:00:00 2001 From: Nicolas Boichat Date: Wed, 3 Jan 2018 10:47:56 +0800 Subject: [PATCH] charger/isl923x: Protect CONTROL1 read-modify-write with a mutex CONTROL1 bits can be modified from multiple tasks: - charger_enable_otg_power (charger or pd task) - charger_discharge_on_ac (host command or console) - charger_enable/disable_psys (chipset task) - print_amon_bmon (console) Since we use I2C read, modify, then I2C write access pattern, there is a small chance of races between these accesses: let's protect them with a mutex. Also, the current code sometimes uses charger_get_option/set_option instead of manipulating CONTROL0 directly: fix those to regain a bit of the extra code size caused by the mutex. BRANCH=none BUG=b:67029560 TEST=Flash lux, battery charges, amon works fine. TEST=Flash elm, battery charges. Signed-off-by: Nicolas Boichat Change-Id: If375d9922db53dd582582bfa44d6218fe0b416ec Reviewed-on: https://chromium-review.googlesource.com/848486 Commit-Ready: Nicolas Boichat Tested-by: Nicolas Boichat Reviewed-by: Shawn N --- driver/charger/isl923x.c | 68 ++++++++++++++++++++++++++++------------ 1 file changed, 48 insertions(+), 20 deletions(-) diff --git a/driver/charger/isl923x.c b/driver/charger/isl923x.c index 503c689dca..e246f05ade 100644 --- a/driver/charger/isl923x.c +++ b/driver/charger/isl923x.c @@ -14,6 +14,7 @@ #include "hooks.h" #include "i2c.h" #include "isl923x.h" +#include "task.h" #include "timer.h" #include "util.h" @@ -31,6 +32,9 @@ static int learn_mode; +/* Mutex for CONTROL1 register, that can be updated from multiple tasks. */ +static struct mutex control1_mutex; + /* Charger parameters */ static const struct charger_info isl9237_charger_info = { .name = CHARGER_NAME, @@ -102,16 +106,23 @@ int charger_enable_otg_power(int enabled) { int rv, control1; + mutex_lock(&control1_mutex); + rv = raw_read16(ISL923X_REG_CONTROL1, &control1); if (rv) - return rv; + goto out; if (enabled) control1 |= ISL923X_C1_OTG; else control1 &= ~ISL923X_C1_OTG; - return raw_write16(ISL923X_REG_CONTROL1, control1); + rv = raw_write16(ISL923X_REG_CONTROL1, control1); + +out: + mutex_unlock(&control1_mutex); + + return rv; } /* @@ -308,14 +319,14 @@ static void isl923x_init(void) #ifdef CONFIG_CHARGE_RAMP_HW #ifdef CONFIG_CHARGER_ISL9237 - if (charger_get_option(®)) + if (raw_read16(ISL923X_REG_CONTROL0, ®)) goto init_fail; /* Set input voltage regulation reference voltage for charge ramp */ reg &= ~ISL9237_C0_VREG_REF_MASK; reg |= ISL9237_C0_VREG_REF_4200; - if (charger_set_option(reg)) + if (raw_write16(ISL923X_REG_CONTROL0, reg)) goto init_fail; #else /* !defined(CONFIG_CHARGER_ISL9237) */ /* @@ -329,13 +340,13 @@ static void isl923x_init(void) goto init_fail; #endif /* defined(CONFIG_CHARGER_ISL9237) */ #else /* !defined(CONFIG_CHARGE_RAMP_HW) */ - if (charger_get_option(®)) + if (raw_read16(ISL923X_REG_CONTROL0, ®)) goto init_fail; /* Disable voltage regulation loop to disable charge ramp */ reg |= ISL923X_C0_DISABLE_VREG; - if (charger_set_option(reg)) + if (raw_write16(ISL923X_REG_CONTROL0, reg)) goto init_fail; #endif /* defined(CONFIG_CHARGE_RAMP_HW) */ @@ -368,9 +379,11 @@ int charger_discharge_on_ac(int enable) int rv; int control1; + mutex_lock(&control1_mutex); + rv = raw_read16(ISL923X_REG_CONTROL1, &control1); if (rv) - return rv; + goto out; control1 &= ~ISL923X_C1_LEARN_MODE_AUTOEXIT; if (enable) @@ -381,6 +394,9 @@ int charger_discharge_on_ac(int enable) rv = raw_write16(ISL923X_REG_CONTROL1, control1); learn_mode = !rv && enable; + +out: + mutex_unlock(&control1_mutex); return rv; } @@ -392,7 +408,7 @@ int charger_set_hw_ramp(int enable) { int rv, reg; - rv = charger_get_option(®); + rv = raw_read16(ISL923X_REG_CONTROL0, ®); if (rv) return rv; @@ -402,7 +418,7 @@ int charger_set_hw_ramp(int enable) else reg |= ISL923X_C0_DISABLE_VREG; - return charger_set_option(reg); + return raw_write16(ISL923X_REG_CONTROL0, reg); } int chg_ramp_is_stable(void) @@ -440,6 +456,8 @@ static void charger_enable_psys(void) { int val; + mutex_lock(&control1_mutex); + /* * enable system power monitor PSYS function */ @@ -447,6 +465,8 @@ static void charger_enable_psys(void) val |= ISL923X_C1_ENABLE_PSYS; raw_write16(ISL923X_REG_CONTROL1, val); } + + mutex_unlock(&control1_mutex); } DECLARE_HOOK(HOOK_CHIPSET_STARTUP, charger_enable_psys, HOOK_PRIO_DEFAULT); @@ -454,6 +474,8 @@ static void charger_disable_psys(void) { int val; + mutex_lock(&control1_mutex); + /* * disable system power monitor PSYS function */ @@ -461,6 +483,8 @@ static void charger_disable_psys(void) val &= ~ISL923X_C1_ENABLE_PSYS; raw_write16(ISL923X_REG_CONTROL1, val); } + + mutex_unlock(&control1_mutex); } DECLARE_HOOK(HOOK_CHIPSET_SHUTDOWN, charger_disable_psys, HOOK_PRIO_DEFAULT); @@ -537,21 +561,25 @@ static int print_amon_bmon(enum amon_bmon amon, int direction, return ret; #endif + mutex_lock(&control1_mutex); + ret = i2c_read16(I2C_PORT_CHARGER, I2C_ADDR_CHARGER, ISL923X_REG_CONTROL1, ®); - if (ret) - return ret; + if (!ret) { + /* Switch between AMON/BMON */ + if (amon == AMON) + reg &= ~ISL923X_C1_SELECT_BMON; + else + reg |= ISL923X_C1_SELECT_BMON; - /* Switch between AMON/BMON */ - if (amon == AMON) - reg &= ~ISL923X_C1_SELECT_BMON; - else - reg |= ISL923X_C1_SELECT_BMON; + /* Enable monitor */ + reg &= ~ISL923X_C1_DISABLE_MON; + ret = i2c_write16(I2C_PORT_CHARGER, I2C_ADDR_CHARGER, + ISL923X_REG_CONTROL1, reg); + } + + mutex_unlock(&control1_mutex); - /* Enable monitor */ - reg &= ~ISL923X_C1_DISABLE_MON; - ret = i2c_write16(I2C_PORT_CHARGER, I2C_ADDR_CHARGER, - ISL923X_REG_CONTROL1, reg); if (ret) return ret;