From 06bbb13141acef66ded7141cc6b6a6eaf9dbca53 Mon Sep 17 00:00:00 2001 From: Eric Yilun Lin Date: Mon, 11 Apr 2022 13:59:50 +0800 Subject: 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 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/3581823 Reviewed-by: Rob Barnes Commit-Queue: Eric Yilun Lin Tested-by: Eric Yilun Lin --- driver/charger/isl923x.c | 37 +++++++++++++++++++++++++++++++------ 1 file 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; } -- cgit v1.2.1