i2c: Split i2c_xfer into locked/unlocked versions.

Rename i2c_xfer to i2c_xfer_unlocked.  Audit all users of i2c_xfer to
see if they can use simple locking semantics or require their own
locking.  Since locked accesses are only safe for I2C_XFER_SINGLE
transactions, remove the flags parameter from i2c_xfer.  This also makes
the audit a bit easier.

Some remaining applications hold the bus locked across several
transactions that would otherwise be atomic, and a few others implement
complex I2C transactions in multiple commands.

Add a (nondeterministic) verification on the I2C port locking
correctness.  This will catch all statically incorrect locking patterns,
although dynamically incorrect locking patterns may not be caught.
Related: Revise the i2c_port_active_count to be a bitmap of the active
ports instead of a count of the active ports.  The EC's mutex does not
provide an is_locked() primitive, and we would prefer not to have one.

- board/glados: Custom locking for battery reset
- board/mchpevb1: Custom locking for battery reset
- board/oak: Custom locking for battery reset
- board/samus: Custom locking for lightbar reset
- board/sweetberry: simple locking
- board/servo_micro: Custom locking for funky i2c transfers
- capsense: simple locking
- host_command_master: multi-command transactions
- lb_common: manual locking to support samus power sequence
- smbus: multi-command transactions
- usb_i2c: simple locking
- driver/tcpm/fusb302: simple locking and multi-command transactions
- driver/tcpm/tcpi: Forward _unlocked and implicitly locked interface to
  fusb302
- driver/touchpad_elan: simple locking

BUG=chromium:871851
BRANCH=none
TEST=buildall; very careful audit
TEST=grunt clamshell and Coral clamshell, test boot, battery charging,
PD communication, and TCPC port low-power mode.
Signed-off-by: Jonathan Brandmeyer <jbrandmeyer@chromium.org>

Change-Id: Ieabf22bcab42780bdb994fca3ced5d8c62519d56
Reviewed-on: https://chromium-review.googlesource.com/1169913
Commit-Ready: Jonathan Brandmeyer <jbrandmeyer@chromium.org>
Tested-by: Jonathan Brandmeyer <jbrandmeyer@chromium.org>
Reviewed-by: Randall Spangler <rspangler@chromium.org>
Reviewed-by: Jett Rink <jettrink@chromium.org>

1 files changed, 70 insertions, 57 deletions

diff --git a/common/i2c_master.c b/common/i2c_master.c
index aa6340610c..e02a479aeb 100644
--- a/common/i2c_master.c
+++ b/common/i2c_master.c
@@ -34,9 +34,27 @@
 #endif
 
 static struct mutex port_mutex[I2C_CONTROLLER_COUNT];
-static uint32_t i2c_port_active_count;
+/* A bitmap of the controllers which are currently servicing a request. */
+static uint32_t i2c_port_active_list;
+BUILD_ASSERT(I2C_CONTROLLER_COUNT < 32);
 static uint8_t port_protected[I2C_CONTROLLER_COUNT];
 
+/**
+ * Non-deterministically test the lock status of the port.  If another task
+ * has locked the port and the caller is accessing it illegally, then this test
+ * will incorrectly return true.  However, callers which failed to statically
+ * lock the port will fail quickly.
+ */
+static int i2c_port_is_locked(int port)
+{
+#ifdef CONFIG_I2C_MULTI_PORT_CONTROLLER
+	/* Test the controller, not the port */
+	port = i2c_port_to_controller(port);
+#endif
+	return (i2c_port_active_list >> port) & 1;
+}
+
+
 const struct i2c_port_t *get_i2c_port(int port)
 {
 	int i;
@@ -98,12 +116,18 @@ static int i2c_xfer_no_retry(int port, int slave_addr, const uint8_t *out,
 }
 #endif /* CONFIG_I2C_XFER_LARGE_READ */
 
-int i2c_xfer(int port, int slave_addr, const uint8_t *out, int out_size,
-	     uint8_t *in, int in_size, int flags)
+int i2c_xfer_unlocked(int port, int slave_addr,
+		      const uint8_t *out, int out_size,
+		      uint8_t *in, int in_size, int flags)
 {
 	int i;
 	int ret = EC_SUCCESS;
 
+	if (!i2c_port_is_locked(port)) {
+		CPUTS("Access I2C without lock!");
+		return EC_ERROR_INVAL;
+	}
+
 	for (i = 0; i <= CONFIG_I2C_NACK_RETRY_COUNT; i++) {
 #ifdef CONFIG_I2C_XFER_LARGE_READ
 		ret = i2c_xfer_no_retry(port, slave_addr, out, out_size, in,
@@ -118,6 +142,19 @@ int i2c_xfer(int port, int slave_addr, const uint8_t *out, int out_size,
 	return ret;
 }
 
+int i2c_xfer(int port, int slave_addr, const uint8_t *out, int out_size,
+	     uint8_t *in, int in_size)
+{
+	int rv;
+
+	i2c_lock(port, 1);
+	rv = i2c_xfer_unlocked(port, slave_addr, out, out_size, in, in_size,
+			I2C_XFER_SINGLE);
+	i2c_lock(port, 0);
+
+	return rv;
+}
+
 void i2c_lock(int port, int lock)
 {
 #ifdef CONFIG_I2C_MULTI_PORT_CONTROLLER
@@ -133,7 +170,7 @@ void i2c_lock(int port, int lock)
 		/* Disable interrupt during changing counter for preemption. */
 		interrupt_disable();
 
-		i2c_port_active_count++;
+		i2c_port_active_list |= 1 << port;
 		/* Ec cannot enter sleep if there's any i2c port active. */
 		disable_sleep(SLEEP_MASK_I2C_MASTER);
 
@@ -141,9 +178,9 @@ void i2c_lock(int port, int lock)
 	} else {
 		interrupt_disable();
 
-		i2c_port_active_count--;
+		i2c_port_active_list &= ~(1 << port);
 		/* Once there is no i2c port active, enable sleep bit of i2c. */
-		if (!i2c_port_active_count)
+		if (!i2c_port_active_list)
 			enable_sleep(SLEEP_MASK_I2C_MASTER);
 
 		interrupt_enable();
@@ -168,10 +205,7 @@ int i2c_read32(int port, int slave_addr, int offset, int *data)
 
 	reg = offset & 0xff;
 	/* I2C read 32-bit word: transmit 8-bit offset, and read 32bits */
-	i2c_lock(port, 1);
-	rv = i2c_xfer(port, slave_addr, &reg, 1, buf, sizeof(uint32_t),
-		      I2C_XFER_SINGLE);
-	i2c_lock(port, 0);
+	rv = i2c_xfer(port, slave_addr, &reg, 1, buf, sizeof(uint32_t));
 
 	if (rv)
 		return rv;
@@ -188,7 +222,6 @@ int i2c_read32(int port, int slave_addr, int offset, int *data)
 
 int i2c_write32(int port, int slave_addr, int offset, int data)
 {
-	int rv;
 	uint8_t buf[1 + sizeof(uint32_t)];
 
 	buf[0] = offset & 0xff;
@@ -205,12 +238,8 @@ int i2c_write32(int port, int slave_addr, int offset, int data)
 		buf[4] = (data >> 24) & 0xff;
 	}
 
-	i2c_lock(port, 1);
-	rv = i2c_xfer(port, slave_addr, buf, sizeof(uint32_t) + 1, NULL, 0,
-		      I2C_XFER_SINGLE);
-	i2c_lock(port, 0);
-
-	return rv;
+	return i2c_xfer(port, slave_addr, buf, sizeof(uint32_t) + 1,
+			NULL, 0);
 }
 
 int i2c_read16(int port, int slave_addr, int offset, int *data)
@@ -220,10 +249,7 @@ int i2c_read16(int port, int slave_addr, int offset, int *data)
 
 	reg = offset & 0xff;
 	/* I2C read 16-bit word: transmit 8-bit offset, and read 16bits */
-	i2c_lock(port, 1);
-	rv = i2c_xfer(port, slave_addr, &reg, 1, buf, sizeof(uint16_t),
-		      I2C_XFER_SINGLE);
-	i2c_lock(port, 0);
+	rv = i2c_xfer(port, slave_addr, &reg, 1, buf, sizeof(uint16_t));
 
 	if (rv)
 		return rv;
@@ -238,7 +264,6 @@ int i2c_read16(int port, int slave_addr, int offset, int *data)
 
 int i2c_write16(int port, int slave_addr, int offset, int data)
 {
-	int rv;
 	uint8_t buf[1 + sizeof(uint16_t)];
 
 	buf[0] = offset & 0xff;
@@ -251,45 +276,32 @@ int i2c_write16(int port, int slave_addr, int offset, int data)
 		buf[2] = (data >> 8) & 0xff;
 	}
 
-	i2c_lock(port, 1);
-	rv = i2c_xfer(port, slave_addr, buf, 1 + sizeof(uint16_t), NULL, 0,
-		      I2C_XFER_SINGLE);
-	i2c_lock(port, 0);
-
-	return rv;
+	return i2c_xfer(port, slave_addr, buf, 1 + sizeof(uint16_t), NULL, 0);
 }
 
 int i2c_read8(int port, int slave_addr, int offset, int *data)
 {
 	int rv;
-	/* We use buf[1] here so it's aligned for DMA on STM32 */
-	uint8_t reg, buf[1];
+	uint8_t reg = offset;
+	uint8_t buf;
 
 	reg = offset;
 
-	i2c_lock(port, 1);
-	rv = i2c_xfer(port, slave_addr, &reg, 1, buf, 1, I2C_XFER_SINGLE);
-	i2c_lock(port, 0);
-
+	rv = i2c_xfer(port, slave_addr, &reg, 1, &buf, 1);
 	if (!rv)
-		*data = buf[0];
+		*data = buf;
 
 	return rv;
 }
 
 int i2c_write8(int port, int slave_addr, int offset, int data)
 {
-	int rv;
 	uint8_t buf[2];
 
 	buf[0] = offset;
 	buf[1] = data;
 
-	i2c_lock(port, 1);
-	rv = i2c_xfer(port, slave_addr, buf, 2, 0, 0, I2C_XFER_SINGLE);
-	i2c_lock(port, 0);
-
-	return rv;
+	return i2c_xfer(port, slave_addr, buf, 2, 0, 0);
 }
 
 int i2c_read_string(int port, int slave_addr, int offset, uint8_t *data,
@@ -305,15 +317,19 @@ int i2c_read_string(int port, int slave_addr, int offset, uint8_t *data,
 	 * Send device reg space offset, and read back block length.  Keep this
 	 * session open without a stop.
 	 */
-	rv = i2c_xfer(port, slave_addr, &reg, 1, &block_length, 1,
+	rv = i2c_xfer_unlocked(port, slave_addr, &reg, 1, &block_length, 1,
 		      I2C_XFER_START);
-	if (rv)
+	if (rv) {
+		/* Dummy read for the stop bit */
+		i2c_xfer_unlocked(port, slave_addr, 0, 0, &reg, 1,
+				  I2C_XFER_STOP);
 		goto exit;
+	}
 
 	if (len && block_length > (len - 1))
 		block_length = len - 1;
 
-	rv = i2c_xfer(port, slave_addr, 0, 0, data, block_length,
+	rv = i2c_xfer_unlocked(port, slave_addr, 0, 0, data, block_length,
 		      I2C_XFER_STOP);
 	data[block_length] = 0;
 
@@ -328,10 +344,7 @@ int i2c_read_block(int port, int slave_addr, int offset, uint8_t *data,
 	int rv;
 	uint8_t reg_address = offset;
 
-	i2c_lock(port, 1);
-	rv = i2c_xfer(port, slave_addr, &reg_address, 1, data, len,
-			I2C_XFER_SINGLE);
-	i2c_lock(port, 0);
+	rv = i2c_xfer(port, slave_addr, &reg_address, 1, data, len);
 	return rv;
 }
 
@@ -348,9 +361,9 @@ int i2c_write_block(int port, int slave_addr, int offset, const uint8_t *data,
 	 * order to have a better chance at sending out the stop bit.
 	 */
 	i2c_lock(port, 1);
-	rv0 = i2c_xfer(port, slave_addr, &reg_address, 1, NULL, 0,
+	rv0 = i2c_xfer_unlocked(port, slave_addr, &reg_address, 1, NULL, 0,
 		      I2C_XFER_START);
-	rv1 = i2c_xfer(port, slave_addr, data, len, NULL, 0,
+	rv1 = i2c_xfer_unlocked(port, slave_addr, data, len, NULL, 0,
 		      I2C_XFER_STOP);
 	i2c_lock(port, 0);
 
@@ -711,8 +724,10 @@ static int i2c_command_passthru(struct host_cmd_handler_args *args)
 #endif
 			if (!port_is_locked)
 				i2c_lock(params->port, (port_is_locked = 1));
-			rv = i2c_xfer(params->port, addr, out, write_len,
-				      &resp->data[in_len], read_len, xferflags);
+			rv = i2c_xfer_unlocked(params->port, addr,
+					       out, write_len,
+					       &resp->data[in_len], read_len,
+					       xferflags);
 		}
 
 		if (rv) {
@@ -844,7 +859,8 @@ static void scan_bus(int port, const char *desc)
 		ccputs(".");
 
 		/* Do a single read */
-		if (!i2c_xfer(port, a, NULL, 0, &tmp, 1, I2C_XFER_SINGLE))
+		if (!i2c_xfer_unlocked(port, a, NULL, 0, &tmp, 1,
+				       I2C_XFER_SINGLE))
 			ccprintf("\n  0x%02x", a);
 	}
 
@@ -925,10 +941,7 @@ static int command_i2cxfer(int argc, char **argv)
 		if (argc < 6 || v < 0 || v > sizeof(data))
 			return EC_ERROR_PARAM5;
 
-		i2c_lock(port, 1);
-		rv = i2c_xfer(port, slave_addr,
-			      &offset, 1, data, v, I2C_XFER_SINGLE);
-		i2c_lock(port, 0);
+		rv = i2c_xfer(port, slave_addr, &offset, 1, data, v);
 
 		if (!rv)
 			ccprintf("Data: %.*h\n", v, data);