kodama: overwrite bad battery params to known good value

Kodama's bitbang driver fails randomly, and there's no way to notify
kernel side that bitbang read failed (batt->flags does not propagate
into kernel).

Thus, if any value in batt_params is bad, replace it with a cached
good value, to make sure we never send random numbers to kernel side.

BUG=b:144195782
TEST=Modify smart battery driver to make sb_read has 50% fail rate,
     and monitor /sys/class/power_supply/sbs*/*, make sure the bad values
     does not observable in kernel.
BRANCH=kukui

Change-Id: Idf4691eb743f1ef785593b308b8f07a34e5ea642
Signed-off-by: Ting Shen <phoenixshen@google.com>
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/1943637
Reviewed-by: Eric Yilun Lin <yllin@chromium.org>
Reviewed-by: Daisuke Nojiri <dnojiri@chromium.org>
Commit-Queue: Ting Shen <phoenixshen@chromium.org>
Tested-by: Ting Shen <phoenixshen@chromium.org>

5 files changed, 88 insertions, 0 deletions

diff --git a/board/kodama/battery.c b/board/kodama/battery.c
index 7b7a3d7cb6..4328ff02d8 100644
--- a/board/kodama/battery.c
+++ b/board/kodama/battery.c
@@ -11,6 +11,7 @@
 #include "driver/charger/rt946x.h"
 #include "gpio.h"
 #include "power.h"
+#include "timer.h"
 #include "usb_pd.h"
 #include "util.h"
 
@@ -81,6 +82,79 @@ enum battery_present battery_hw_present(void)
 	return gpio_get_level(GPIO_EC_BATT_PRES_ODL) ? BP_NO : BP_YES;
 }
 
+static void fix_single_param(int flag, int *cached, int *curr)
+{
+	if (flag)
+		*curr = *cached;
+	else
+		*cached = *curr;
+}
+
+#define CACHE_INVALIDATION_TIME_US (5 * SECOND)
+
+/*
+ * b:144195782: bitbang fails randomly, and there's no way to
+ * notify kernel side that bitbang read failed.
+ * Thus, if any value in batt_params is bad, replace it with a cached
+ * good value, to make sure we never send random numbers to kernel
+ * side.
+ */
+__override void board_battery_compensate_params(struct batt_params *batt)
+{
+	static struct batt_params batt_cache = { 0 };
+	static timestamp_t deadline;
+
+	/*
+	 * If battery keeps failing for 5 seconds, stop hiding the error and
+	 * report back to host.
+	 */
+	if (batt->flags & BATT_FLAG_BAD_ANY) {
+		if (timestamp_expired(deadline, NULL))
+			return;
+	} else {
+		deadline.val = get_time().val + CACHE_INVALIDATION_TIME_US;
+	}
+
+	/* return cached values for at most CACHE_INVALIDATION_TIME_US */
+	fix_single_param(batt->flags & BATT_FLAG_BAD_STATE_OF_CHARGE,
+			&batt_cache.state_of_charge,
+			&batt->state_of_charge);
+	fix_single_param(batt->flags & BATT_FLAG_BAD_VOLTAGE,
+			&batt_cache.voltage,
+			&batt->voltage);
+	fix_single_param(batt->flags & BATT_FLAG_BAD_CURRENT,
+			&batt_cache.current,
+			&batt->current);
+	fix_single_param(batt->flags & BATT_FLAG_BAD_DESIRED_VOLTAGE,
+			&batt_cache.desired_voltage,
+			&batt->desired_voltage);
+	fix_single_param(batt->flags & BATT_FLAG_BAD_DESIRED_CURRENT,
+			&batt_cache.desired_current,
+			&batt->desired_current);
+	fix_single_param(batt->flags & BATT_FLAG_BAD_REMAINING_CAPACITY,
+			&batt_cache.remaining_capacity,
+			&batt->remaining_capacity);
+	fix_single_param(batt->flags & BATT_FLAG_BAD_FULL_CAPACITY,
+			&batt_cache.full_capacity,
+			&batt->full_capacity);
+	fix_single_param(batt->flags & BATT_FLAG_BAD_STATUS,
+			&batt_cache.status,
+			&batt->status);
+	fix_single_param(batt->flags & BATT_FLAG_BAD_TEMPERATURE,
+			&batt_cache.temperature,
+			&batt->temperature);
+	/*
+	 * If battery_compensate_params() didn't calculate display_charge
+	 * for us, also update it with last good value.
+	 */
+	fix_single_param(batt->display_charge == 0,
+			&batt_cache.display_charge,
+			&batt->display_charge);
+
+	/* remove bad flags after applying cached values */
+	batt->flags &= ~BATT_FLAG_BAD_ANY;
+}
+
 int charger_profile_override(struct charge_state_data *curr)
 {
 	const struct battery_info *batt_info = battery_get_info();
diff --git a/common/battery.c b/common/battery.c
index d55d4d393e..850d703954 100644
--- a/common/battery.c
+++ b/common/battery.c
@@ -615,6 +615,10 @@ void battery_compensate_params(struct batt_params *batt)
 		batt->display_charge = 0;
 }
 
+__overridable void board_battery_compensate_params(struct batt_params *batt)
+{
+}
+
 __attribute__((weak)) int get_battery_manufacturer_name(char *dest, int size)
 {
 	strzcpy(dest, "<unkn>", size);
diff --git a/driver/battery/smart.c b/driver/battery/smart.c
index 14106664cb..bd9487642f 100644
--- a/driver/battery/smart.c
+++ b/driver/battery/smart.c
@@ -443,6 +443,7 @@ void battery_get_params(struct batt_params *batt)
 #ifdef HAS_TASK_HOSTCMD
 	/* if there is no host, we don't care about compensation */
 	battery_compensate_params(&batt_new);
+	board_battery_compensate_params(&batt_new);
 #endif
 
 	/* Update visible battery parameters */
diff --git a/include/battery.h b/include/battery.h
index 7052e2d663..a41cd9da40 100644
--- a/include/battery.h
+++ b/include/battery.h
@@ -445,4 +445,9 @@ extern struct i2c_stress_test_dev battery_i2c_stress_test_dev;
  */
 void battery_compensate_params(struct batt_params *batt);
 
+/**
+ * board-specific battery_compensate_params
+ */
+__override_proto void board_battery_compensate_params(struct batt_params *batt);
+
 #endif /* __CROS_EC_BATTERY_H */
diff --git a/test/battery_get_params_smart.c b/test/battery_get_params_smart.c
index e2a3a81309..465d667f48 100644
--- a/test/battery_get_params_smart.c
+++ b/test/battery_get_params_smart.c
@@ -24,6 +24,10 @@ void battery_compensate_params(struct batt_params *batt)
 {
 }
 
+void board_battery_compensate_params(struct batt_params *batt)
+{
+}
+
 static void reset_and_fail_on(int first, int last)
 {
 	/* We're not initializing the fake battery, so everything reads zero */