tcpci: allow get_chip_info callers to update the cache

Some drivers massage retrieved chip info in assorted ways (usually
because the chips misbehave), but it's both confusing and inefficient
how that interacts with cached chip info because the consuming driver
needs to always interpose itself between possibly-cached values and its
caller, usually requiring another layer of caching.

This adds a new tcpci_get_chip_info_mutable() function that works like
tcpci_get_chip_info(), but the caller can provide a function that
modifies the cached data in order to do any required massaging. A
callback is used rather than an output pointer to give the
implementation freedom to change its behavior more, such as by adding
locking to prevent the latent concurrency bugs that currently lurk in
it.

This also fixes a bug in the previous implementation where partial data
would be cached if an error occurred in reading certain TCPC registers.

BUG=b:244502337
TEST=make buildall; zmake build -a; twister
BRANCH=none

Change-Id: Ia3dcac109eb22bf0326e3fd1722aa64fe9f73f50
Signed-off-by: Peter Marheine <pmarheine@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/4179431
Reviewed-by: Jeremy Bettis <jbettis@chromium.org>
Reviewed-by: Abe Levkoy <alevkoy@chromium.org>

4 files changed, 131 insertions, 44 deletions

diff --git a/driver/tcpm/tcpci.c b/driver/tcpm/tcpci.c
index 84fdbe8cbc..6c6e7a01c1 100644
--- a/driver/tcpm/tcpci.c
+++ b/driver/tcpm/tcpci.c
@@ -1375,67 +1375,82 @@ int tcpci_get_vbus_voltage(int port, int *vbus)
 	return EC_SUCCESS;
 }
 
-/*
- * This call will wake up the TCPC if it is in low power mode upon accessing the
- * i2c bus (but the pd state machine should put it back into low power mode).
- *
- * Once it's called, the chip info will be stored in cache, which can be
- * accessed by tcpm_get_chip_info without worrying about chip states.
- */
-int tcpci_get_chip_info(int port, int live,
-			struct ec_response_pd_chip_info_v1 *chip_info)
+int tcpci_get_chip_info_mutable(
+	int port, int live, struct ec_response_pd_chip_info_v1 *const chip_info,
+	int (*const mutator)(int port, bool live,
+			     struct ec_response_pd_chip_info_v1 *cached))
 {
 	static struct ec_response_pd_chip_info_v1
 		cached_info[CONFIG_USB_PD_PORT_MAX_COUNT];
-	struct ec_response_pd_chip_info_v1 *i;
+	struct ec_response_pd_chip_info_v1 *info;
 	int error;
-	int val;
 
 	if (port >= board_get_usb_pd_port_count())
 		return EC_ERROR_INVAL;
 
-	i = &cached_info[port];
+	info = &cached_info[port];
+
+	/* Fetch live data if nothing is cached or live data was requested */
+	if (!info->vendor_id || live) {
+		int vendor_id, product_id, device_id;
 
-	/* If already cached && live data is not asked, return cached value */
-	if (i->vendor_id && !live) {
 		/*
-		 * If chip_info is NULL, chip info will be stored in cache and
-		 * can be read later by another call.
+		 * The cache is no longer valid because we're fetching. Avoid
+		 * storing the new vendor ID until we've actually succeeded so
+		 * future invocations won't see partial data and assume it's a
+		 * valid cache.
 		 */
-		if (chip_info)
-			memcpy(chip_info, i, sizeof(*i));
-
-		return EC_SUCCESS;
-	}
-
-	error = tcpc_read16(port, TCPC_REG_VENDOR_ID, &val);
-	if (error)
-		return error;
-	i->vendor_id = val;
+		info->vendor_id = 0;
+		error = tcpc_read16(port, TCPC_REG_VENDOR_ID, &vendor_id);
+		if (error)
+			return error;
 
-	error = tcpc_read16(port, TCPC_REG_PRODUCT_ID, &val);
-	if (error)
-		return error;
-	i->product_id = val;
+		error = tcpc_read16(port, TCPC_REG_PRODUCT_ID, &product_id);
+		if (error)
+			return error;
+		info->product_id = product_id;
 
-	error = tcpc_read16(port, TCPC_REG_BCD_DEV, &val);
-	if (error)
-		return error;
-	i->device_id = val;
+		error = tcpc_read16(port, TCPC_REG_BCD_DEV, &device_id);
+		if (error)
+			return error;
+		info->device_id = device_id;
 
+		/*
+		 * This varies chip to chip; more specific driver code is
+		 * expected to override this value if it can by providing a
+		 * mutator.
+		 */
+		info->fw_version_number = -1;
+
+		info->vendor_id = vendor_id;
+		if (mutator != NULL) {
+			error = mutator(port, live, info);
+			if (error) {
+				/*
+				 * Mutator needs to have a complete view, but if
+				 * it fails the cache is invalidated.
+				 */
+				info->vendor_id = 0;
+				return error;
+			}
+		}
+	}
 	/*
-	 * This varies chip to chip; more specific driver code is expected to
-	 * override this value if it can.
+	 * If chip_info is NULL, this invocation will ensure the cache is fresh
+	 * but return nothing.
 	 */
-	i->fw_version_number = -1;
-
-	/* Copy the cached value to return if chip_info is not NULL */
 	if (chip_info)
-		memcpy(chip_info, i, sizeof(*i));
+		memcpy(chip_info, info, sizeof(*info));
 
 	return EC_SUCCESS;
 }
 
+int tcpci_get_chip_info(int port, int live,
+			struct ec_response_pd_chip_info_v1 *chip_info)
+{
+	return tcpci_get_chip_info_mutable(port, live, chip_info, NULL);
+}
+
 /*
  * Dissociate from the TCPC.
  */
diff --git a/include/driver/tcpm/tcpci.h b/include/driver/tcpm/tcpci.h
index 4879f7dad1..eb2b4e7f9f 100644
--- a/include/driver/tcpm/tcpci.h
+++ b/include/driver/tcpm/tcpci.h
@@ -340,8 +340,46 @@ int tcpci_tcpm_mux_set(const struct usb_mux *me, mux_state_t mux_state,
 		       bool *ack_required);
 int tcpci_tcpm_mux_get(const struct usb_mux *me, mux_state_t *mux_state);
 int tcpci_tcpm_mux_enter_low_power(const struct usb_mux *me);
+
+/**
+ * Get the TCPC chip information (chip IDs, etc) for the given port.
+ *
+ * The returned value is cached internally, so subsequent calls to this function
+ * will not access the TCPC. If live is true, data will be fetched from the TCPC
+ * regardless of whether any cached data is available.
+ *
+ * If chip_info is NULL, this will ensure the cache is up to date but avoid
+ * writing the output chip_info.
+ *
+ * If the TCPC is accessed (live data is retrieved), this will wake the chip up
+ * from low power mode on I2C access. It is expected that the USB-PD state
+ * machine will return it to low power mode as appropriate afterward.
+ *
+ * Returns EC_SUCCESS or an error; chip_info is not updated on error.
+ */
 int tcpci_get_chip_info(int port, int live,
 			struct ec_response_pd_chip_info_v1 *chip_info);
+
+/**
+ * Equivalent to tcpci_get_chip_info, but allows the caller to modify the cache
+ * if new data is fetched.
+ *
+ * If mutator is non-NULL and data is read from the TCPC (either because live
+ * data is requested or nothing was cached), then it is called with a pointer
+ * to the cached information for the port. It can then make changes to the
+ * cached data (for example correcting IDs that are known to be reported
+ * incorrectly by some chips, possibly requiring more communication with the
+ * TCPC). Any error returned by mutator causes this function to return with the
+ * same error.
+ *
+ * If mutator writes through the cached data pointer, those changes will be
+ * retained until live data is requested again.
+ */
+int tcpci_get_chip_info_mutable(
+	int port, int live, struct ec_response_pd_chip_info_v1 *chip_info,
+	int (*mutator)(int port, bool live,
+		       struct ec_response_pd_chip_info_v1 *cached));
+
 int tcpci_get_vbus_voltage(int port, int *vbus);
 bool tcpci_tcpm_get_snk_ctrl(int port);
 int tcpci_tcpm_set_snk_ctrl(int port, int enable);
diff --git a/zephyr/test/drivers/default/src/tcpci.c b/zephyr/test/drivers/default/src/tcpci.c
index f648b43a96..90d5d3fb65 100644
--- a/zephyr/test/drivers/default/src/tcpci.c
+++ b/zephyr/test/drivers/default/src/tcpci.c
@@ -200,6 +200,11 @@ ZTEST(tcpci, test_generic_tcpci_get_chip_info)
 		emul_tcpci_generic_get_i2c_common_data(emul);
 
 	test_tcpci_get_chip_info(emul, common_data, USBC_PORT_C0);
+
+	zassert_equal(EC_ERROR_INVAL,
+		      tcpci_get_chip_info(board_get_usb_pd_port_count(), false,
+					  NULL),
+		      "get_chip_info should return INVAL for an invalid port");
 }
 
 /** Test TCPCI enter low power mode */
diff --git a/zephyr/test/drivers/default/src/tcpci_test_common.c b/zephyr/test/drivers/default/src/tcpci_test_common.c
index 06f03d4834..01f1cd944c 100644
--- a/zephyr/test/drivers/default/src/tcpci_test_common.c
+++ b/zephyr/test/drivers/default/src/tcpci_test_common.c
@@ -900,6 +900,19 @@ void test_tcpci_drp_toggle(const struct emul *emul,
 			TCPC_REG_COMMAND_LOOK4CONNECTION);
 }
 
+static int test_tcpci_get_chip_info_mutator_device_id(
+	int port, bool live, struct ec_response_pd_chip_info_v1 *cached)
+{
+	cached->device_id = 0xbeef;
+	return EC_SUCCESS;
+}
+
+static int test_tcpci_get_chip_info_mutator_fail(
+	int port, bool live, struct ec_response_pd_chip_info_v1 *cached)
+{
+	return EC_ERROR_UNCHANGED;
+}
+
 /** Test TCPCI get chip info */
 void test_tcpci_get_chip_info(const struct emul *emul,
 			      struct i2c_common_emul_data *common_data,
@@ -913,13 +926,16 @@ void test_tcpci_get_chip_info(const struct emul *emul,
 	i2c_common_emul_set_read_fail_reg(common_data, TCPC_REG_VENDOR_ID);
 	zassert_equal(EC_ERROR_INVAL, drv->get_chip_info(port, 1, &info));
 
-	/* Test error on failed product id get */
+	/*
+	 * Test error on failed product id get. Cache should not be valid
+	 * because the previous call to get_chip_info failed.
+	 */
 	i2c_common_emul_set_read_fail_reg(common_data, TCPC_REG_PRODUCT_ID);
-	zassert_equal(EC_ERROR_INVAL, drv->get_chip_info(port, 1, &info));
+	zassert_equal(EC_ERROR_INVAL, drv->get_chip_info(port, 0, &info));
 
 	/* Test error on failed BCD get */
-	i2c_common_emul_set_read_fail_reg(common_data, TCPC_REG_VENDOR_ID);
-	zassert_equal(EC_ERROR_INVAL, drv->get_chip_info(port, 1, &info));
+	i2c_common_emul_set_read_fail_reg(common_data, TCPC_REG_BCD_DEV);
+	zassert_equal(EC_ERROR_INVAL, drv->get_chip_info(port, 0, &info));
 	i2c_common_emul_set_read_fail_reg(common_data,
 					  I2C_COMMON_EMUL_NO_FAIL_REG);
 
@@ -948,6 +964,19 @@ void test_tcpci_get_chip_info(const struct emul *emul,
 	zassert_equal(vendor, info.vendor_id);
 	zassert_equal(product, info.product_id);
 	zassert_equal(bcd, info.device_id);
+
+	/* Test providing a callback to modify cached data. */
+	zassert_equal(EC_SUCCESS,
+		      tcpci_get_chip_info_mutable(
+			      port, 1, &info,
+			      test_tcpci_get_chip_info_mutator_device_id));
+	zassert_equal(0xbeef, info.device_id);
+
+	/* Errors from the mutator get bubbled up. */
+	zassert_equal(EC_ERROR_UNCHANGED,
+		      tcpci_get_chip_info_mutable(
+			      port, 1, &info,
+			      test_tcpci_get_chip_info_mutator_fail));
 }
 
 /** Test TCPCI enter low power mode */