diff options
| author | Eric Yilun Lin <yllin@chromium.org> | 2022-04-11 13:59:50 +0800 |
|---|---|---|
| committer | Chromeos LUCI <chromeos-scoped@luci-project-accounts.iam.gserviceaccount.com> | 2022-04-12 02:56:14 +0000 |
| commit | 06bbb13141acef66ded7141cc6b6a6eaf9dbca53 (patch) | |
| tree | 996dc241eda9108c6a6757bb8801aeedde848db0 | |
| parent | 4a7cea931e8980ed1b4e431e8bc06f03137d54fd (diff) | |
| download | chrome-ec-06bbb13141acef66ded7141cc6b6a6eaf9dbca53.tar.gz | |
drive/charger/isl923x: Fix race condition with discharge_on_ac
This CL is basically a clone of CL:3579624 which fixes the same
issue on isl9241 driver. The below comment was copied from it:
There is a race condition with `learn_mode`. This variable is read and
then discharge_on_ac is disabled if it is not set. However it's possible
for isl9241_discharge_on_ac to get called after the branch is taken but
before discharge_on_ac is disabled. This can cause factory USB charging
tests to fail as discharge_on_ac is disabled improperly. This commit
protects the read and branch on `learn_mode` with a mutex to prevent the
above from happening.
BUG=b:214341758, b:206601685
TEST=zmake testall
BRANCH=asurada, cherry
Change-Id: I5d584b4b5f87e5f8a356e4f2bc8f6f37fd54250a
Signed-off-by: Eric Yilun Lin <yllin@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/3581823
Reviewed-by: Rob Barnes <robbarnes@google.com>
Commit-Queue: Eric Yilun Lin <yllin@google.com>
Tested-by: Eric Yilun Lin <yllin@google.com>
| -rw-r--r-- | driver/charger/isl923x.c | 37 |
1 files changed, 31 insertions, 6 deletions
diff --git a/driver/charger/isl923x.c b/driver/charger/isl923x.c index 7861ec95bc..c1e4f203fd 100644 --- a/driver/charger/isl923x.c +++ b/driver/charger/isl923x.c @@ -98,6 +98,7 @@ static int learn_mode; K_MUTEX_DEFINE(control1_mutex_isl923x); static enum ec_error_list isl923x_discharge_on_ac(int chgnum, int enable); +static enum ec_error_list isl923x_discharge_on_ac_weak_disable(int chgnum); /* Charger parameters */ static const struct charger_info isl9237_charger_info = { @@ -395,8 +396,7 @@ static enum ec_error_list isl923x_set_mode(int chgnum, int mode) * See crosbug.com/p/51196. Always disable learn mode unless it was set * explicitly. */ - if (!learn_mode) - rv = isl923x_discharge_on_ac(chgnum, 0); + rv = isl923x_discharge_on_ac_weak_disable(chgnum); /* ISL923X does not support inhibit mode setting. */ return rv; @@ -763,13 +763,15 @@ init_fail: CPRINTS("%s init failed!", CHARGER_NAME); } -static enum ec_error_list isl923x_discharge_on_ac(int chgnum, int enable) +/* + * Writes to ISL923X_REG_CONTROL1, unsafe as it does not lock + * control1_mutex_isl923x. + */ +static enum ec_error_list isl923x_discharge_on_ac_unsafe(int chgnum, int enable) { int rv; int control1; - mutex_lock(&control1_mutex_isl923x); - rv = raw_read16(chgnum, ISL923X_REG_CONTROL1, &control1); if (rv) goto out; @@ -782,9 +784,32 @@ static enum ec_error_list isl923x_discharge_on_ac(int chgnum, int enable) rv = raw_write16(chgnum, ISL923X_REG_CONTROL1, control1); - learn_mode = !rv && enable; + if (!rv) + learn_mode = enable; out: + return rv; +} + +static enum ec_error_list isl923x_discharge_on_ac(int chgnum, int enable) +{ + int rv; + + mutex_lock(&control1_mutex_isl923x); + rv = isl923x_discharge_on_ac_unsafe(chgnum, enable); + mutex_unlock(&control1_mutex_isl923x); + return rv; +} + +/* Disables discharge on ac only if it wasn't explicitly enabled. */ +static enum ec_error_list isl923x_discharge_on_ac_weak_disable(int chgnum) +{ + int rv = 0; + + mutex_lock(&control1_mutex_isl923x); + if (!learn_mode) + rv = isl923x_discharge_on_ac_unsafe(chgnum, 0); + mutex_unlock(&control1_mutex_isl923x); return rv; } |