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>

6 files changed, 91 insertions, 91 deletions

diff --git a/common/capsense.c b/common/capsense.c
index bc54d5aae3..2e34651573 100644
--- a/common/capsense.c
+++ b/common/capsense.c
@@ -24,10 +24,8 @@ static int capsense_read_bitmask(void)
 	int rv;
 	uint8_t val = 0;
 
-	i2c_lock(I2C_PORT_CAPSENSE, 1);
 	rv = i2c_xfer(I2C_PORT_CAPSENSE, CAPSENSE_I2C_ADDR,
-		      0, 0, &val, 1, I2C_XFER_SINGLE);
-	i2c_lock(I2C_PORT_CAPSENSE, 0);
+		      0, 0, &val, 1);
 
 	if (rv)
 		CPRINTS("%s failed: error %d", __func__, rv);
diff --git a/common/host_command_master.c b/common/host_command_master.c
index c567271688..9ba8ba64ab 100644
--- a/common/host_command_master.c
+++ b/common/host_command_master.c
@@ -80,7 +80,7 @@ static int pd_host_command_internal(int command, int version,
 	 */
 	i2c_lock(I2C_PORT_PD_MCU, 1);
 	i2c_set_timeout(I2C_PORT_PD_MCU, PD_HOST_COMMAND_TIMEOUT_US);
-	ret = i2c_xfer(I2C_PORT_PD_MCU, CONFIG_USB_PD_I2C_SLAVE_ADDR,
+	ret = i2c_xfer_unlocked(I2C_PORT_PD_MCU, CONFIG_USB_PD_I2C_SLAVE_ADDR,
 			&req_buf[0], outsize + sizeof(rq) + 1, &resp_buf[0],
 			2, I2C_XFER_START);
 	i2c_set_timeout(I2C_PORT_PD_MCU, 0);
@@ -94,7 +94,7 @@ static int pd_host_command_internal(int command, int version,
 
 	if (resp_len > (insize + sizeof(rs))) {
 		/* Do a dummy read to generate stop condition */
-		i2c_xfer(I2C_PORT_PD_MCU, CONFIG_USB_PD_I2C_SLAVE_ADDR,
+		i2c_xfer_unlocked(I2C_PORT_PD_MCU, CONFIG_USB_PD_I2C_SLAVE_ADDR,
 			0, 0, &resp_buf[2], 1, I2C_XFER_STOP);
 		i2c_lock(I2C_PORT_PD_MCU, 0);
 		CPRINTF("[%T response size is too large %d > %d]\n",
@@ -103,7 +103,8 @@ static int pd_host_command_internal(int command, int version,
 	}
 
 	/* Receive remaining data */
-	ret = i2c_xfer(I2C_PORT_PD_MCU, CONFIG_USB_PD_I2C_SLAVE_ADDR, 0, 0,
+	ret = i2c_xfer_unlocked(I2C_PORT_PD_MCU, CONFIG_USB_PD_I2C_SLAVE_ADDR,
+			0, 0,
 			&resp_buf[2], resp_len, I2C_XFER_STOP);
 	i2c_lock(I2C_PORT_PD_MCU, 0);
 	if (ret) {
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);
diff --git a/common/lb_common.c b/common/lb_common.c
index 2ffba93b2f..f800a6a65a 100644
--- a/common/lb_common.c
+++ b/common/lb_common.c
@@ -120,7 +120,7 @@ static inline void controller_write(int ctrl_num, uint8_t reg, uint8_t val)
 	buf[0] = reg;
 	buf[1] = val;
 	ctrl_num = ctrl_num % ARRAY_SIZE(i2c_addr);
-	i2c_xfer(I2C_PORT_LIGHTBAR, i2c_addr[ctrl_num], buf, 2, 0, 0,
+	i2c_xfer_unlocked(I2C_PORT_LIGHTBAR, i2c_addr[ctrl_num], buf, 2, 0, 0,
 		 I2C_XFER_SINGLE);
 }
 
@@ -130,8 +130,8 @@ static inline uint8_t controller_read(int ctrl_num, uint8_t reg)
 	int rv;
 
 	ctrl_num = ctrl_num % ARRAY_SIZE(i2c_addr);
-	rv = i2c_xfer(I2C_PORT_LIGHTBAR, i2c_addr[ctrl_num], &reg, 1, buf, 1,
-		      I2C_XFER_SINGLE);
+	rv = i2c_xfer_unlocked(I2C_PORT_LIGHTBAR, i2c_addr[ctrl_num],
+			&reg, 1, buf, 1, I2C_XFER_SINGLE);
 	return rv ? 0 : buf[0];
 }
 
diff --git a/common/smbus.c b/common/smbus.c
index 1c612983d0..2694eba08a 100644
--- a/common/smbus.c
+++ b/common/smbus.c
@@ -20,9 +20,6 @@ int smbus_write_word(uint8_t i2c_port, uint8_t slave_addr,
 			uint8_t smbus_cmd, uint16_t d16)
 {
 	uint8_t buf[5];
-	int rv;
-
-	i2c_lock(i2c_port, 1);
 
 	/* Command sequence for CRC calculation */
 	buf[0] = slave_addr;
@@ -30,11 +27,7 @@ int smbus_write_word(uint8_t i2c_port, uint8_t slave_addr,
 	buf[2] = d16 & 0xff;
 	buf[3] = (d16 >> 8) & 0xff;
 	buf[4] = crc8(buf, 4);
-	rv = i2c_xfer(i2c_port, slave_addr,
-		      buf + 1, 4, NULL, 0, I2C_XFER_SINGLE);
-
-	i2c_lock(i2c_port, 0);
-	return rv;
+	return i2c_xfer(i2c_port, slave_addr, buf + 1, 4, NULL, 0);
 }
 
 /* Write up to SMBUS_MAX_BLOCK_SIZE bytes using smbus block access protocol */
@@ -52,20 +45,21 @@ int smbus_write_block(uint8_t i2c_port, uint8_t slave_addr,
 	i2c_lock(i2c_port, 1);
 
 	/* Send command + length */
-	rv = i2c_xfer(i2c_port, slave_addr,
+	rv = i2c_xfer_unlocked(i2c_port, slave_addr,
 		      buf + 1, 2, NULL, 0, I2C_XFER_START);
 	if (rv != EC_SUCCESS)
 		goto smbus_write_block_done;
 
 	/* Send data */
-	rv = i2c_xfer(i2c_port, slave_addr, data, len, NULL, 0, 0);
+	rv = i2c_xfer_unlocked(i2c_port, slave_addr, data, len, NULL, 0, 0);
 	if (rv != EC_SUCCESS)
 		goto smbus_write_block_done;
 
 	/* Send CRC */
 	buf[0] = crc8(buf, 3);
 	buf[0] = crc8_arg(data, len, buf[0]);
-	rv = i2c_xfer(i2c_port, slave_addr, buf, 1, NULL, 0, I2C_XFER_STOP);
+	rv = i2c_xfer_unlocked(i2c_port, slave_addr, buf, 1, NULL, 0,
+			I2C_XFER_STOP);
 
 smbus_write_block_done:
 	i2c_lock(i2c_port, 0);
@@ -86,11 +80,8 @@ int smbus_read_word(uint8_t i2c_port, uint8_t slave_addr,
 	buf[2] = slave_addr | 0x1;
 	crc = crc8(buf, 3);
 
-	i2c_lock(i2c_port, 1);
-
 	/* Read data bytes + CRC byte */
-	rv = i2c_xfer(i2c_port, slave_addr,
-		&smbus_cmd, 1, buf, 3, I2C_XFER_SINGLE);
+	rv = i2c_xfer(i2c_port, slave_addr, &smbus_cmd, 1, buf, 3);
 
 	/* Verify CRC */
 	if (crc8_arg(buf, 2, crc) != buf[2])
@@ -101,7 +92,6 @@ int smbus_read_word(uint8_t i2c_port, uint8_t slave_addr,
 	else
 		*p16 = 0;
 
-	i2c_lock(i2c_port, 0);
 	return rv;
 }
 
@@ -122,7 +112,7 @@ int smbus_read_block(uint8_t i2c_port, uint8_t slave_addr,
 	i2c_lock(i2c_port, 1);
 
 	/* First read size from slave */
-	rv = i2c_xfer(i2c_port, slave_addr,
+	rv = i2c_xfer_unlocked(i2c_port, slave_addr,
 		      &smbus_cmd, 1, buf + 3, 1, I2C_XFER_START);
 	if (rv != EC_SUCCESS)
 			goto smbus_read_block_done;
@@ -140,12 +130,13 @@ int smbus_read_block(uint8_t i2c_port, uint8_t slave_addr,
 	}
 
 	/* Now read back all bytes */
-	rv = i2c_xfer(i2c_port, slave_addr, NULL, 0, data, read_len, 0);
+	rv = i2c_xfer_unlocked(i2c_port, slave_addr,
+			NULL, 0, data, read_len, 0);
 	if (rv)
 		goto smbus_read_block_done;
 
 	/* Read CRC + verify */
-	rv = i2c_xfer(i2c_port, slave_addr,
+	rv = i2c_xfer_unlocked(i2c_port, slave_addr,
 		      NULL, 0, buf, 1, I2C_XFER_STOP);
 	if (do_crc && crc8_arg(data, read_len, crc) != buf[0])
 		rv = EC_ERROR_CRC;
diff --git a/common/usb_i2c.c b/common/usb_i2c.c
index 4ceb4fe031..a92c411f1b 100644
--- a/common/usb_i2c.c
+++ b/common/usb_i2c.c
@@ -85,7 +85,6 @@ void usb_i2c_execute(struct usb_i2c_config const *config)
 	int write_count     = (config->buffer[1] >> 0) & 0xff;
 	int read_count      = (config->buffer[1] >> 8) & 0xff;
 	int offset          = 0;    /* Offset for extended reading header. */
-	int port;
 
 	config->buffer[0] = 0;
 	config->buffer[1] = 0;
@@ -117,14 +116,12 @@ void usb_i2c_execute(struct usb_i2c_config const *config)
 		 * knows about.  It should behave closer to
 		 * EC_CMD_I2C_PASSTHRU, which can protect ports and ranges.
 		 */
-		port = i2c_ports[portindex].port;
-		i2c_lock(port, 1);
-		ret = i2c_xfer(port, slave_addr,
+		ret = i2c_xfer(i2c_ports[portindex].port,
+				 slave_addr,
 				 (uint8_t *)(config->buffer + 2) + offset,
 				 write_count,
 				 (uint8_t *)(config->buffer + 2),
-				 read_count, I2C_XFER_SINGLE);
-		i2c_lock(port, 0);
+				 read_count);
 		config->buffer[0] = usb_i2c_map_error(ret);
 	}
 	usb_i2c_write_packet(config, read_count + 4);