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>

18 files changed, 156 insertions, 153 deletions

diff --git a/board/glados/battery.c b/board/glados/battery.c
index 2372affb64..016d225950 100644
--- a/board/glados/battery.c
+++ b/board/glados/battery.c
@@ -47,9 +47,9 @@ int board_cut_off_battery(void)
 	buf[2] = PARAM_CUT_OFF_HIGH;
 
 	i2c_lock(I2C_PORT_BATTERY, 1);
-	rv = i2c_xfer(I2C_PORT_BATTERY, BATTERY_ADDR, buf, 3, NULL, 0,
+	rv = i2c_xfer_unlocked(I2C_PORT_BATTERY, BATTERY_ADDR, buf, 3, NULL, 0,
 		      I2C_XFER_SINGLE);
-	rv |= i2c_xfer(I2C_PORT_BATTERY, BATTERY_ADDR, buf, 3, NULL, 0,
+	rv |= i2c_xfer_unlocked(I2C_PORT_BATTERY, BATTERY_ADDR, buf, 3, NULL, 0,
 		       I2C_XFER_SINGLE);
 	i2c_lock(I2C_PORT_BATTERY, 0);
 
diff --git a/board/mchpevb1/battery.c b/board/mchpevb1/battery.c
index 2372affb64..016d225950 100644
--- a/board/mchpevb1/battery.c
+++ b/board/mchpevb1/battery.c
@@ -47,9 +47,9 @@ int board_cut_off_battery(void)
 	buf[2] = PARAM_CUT_OFF_HIGH;
 
 	i2c_lock(I2C_PORT_BATTERY, 1);
-	rv = i2c_xfer(I2C_PORT_BATTERY, BATTERY_ADDR, buf, 3, NULL, 0,
+	rv = i2c_xfer_unlocked(I2C_PORT_BATTERY, BATTERY_ADDR, buf, 3, NULL, 0,
 		      I2C_XFER_SINGLE);
-	rv |= i2c_xfer(I2C_PORT_BATTERY, BATTERY_ADDR, buf, 3, NULL, 0,
+	rv |= i2c_xfer_unlocked(I2C_PORT_BATTERY, BATTERY_ADDR, buf, 3, NULL, 0,
 		       I2C_XFER_SINGLE);
 	i2c_lock(I2C_PORT_BATTERY, 0);
 
diff --git a/board/oak/battery.c b/board/oak/battery.c
index a3fe0cd452..a868e20bc0 100644
--- a/board/oak/battery.c
+++ b/board/oak/battery.c
@@ -57,9 +57,9 @@ static int cutoff(void)
 	buf[2] = PARAM_CUT_OFF_HIGH;
 
 	i2c_lock(I2C_PORT_BATTERY, 1);
-	rv = i2c_xfer(I2C_PORT_BATTERY, BATTERY_ADDR, buf, 3, NULL, 0,
+	rv = i2c_xfer_unlocked(I2C_PORT_BATTERY, BATTERY_ADDR, buf, 3, NULL, 0,
 		      I2C_XFER_SINGLE);
-	rv |= i2c_xfer(I2C_PORT_BATTERY, BATTERY_ADDR, buf, 3, NULL, 0,
+	rv |= i2c_xfer_unlocked(I2C_PORT_BATTERY, BATTERY_ADDR, buf, 3, NULL, 0,
 		       I2C_XFER_SINGLE);
 	i2c_lock(I2C_PORT_BATTERY, 0);
 
diff --git a/board/samus/battery.c b/board/samus/battery.c
index 64cf417ecc..f3c78e9d96 100644
--- a/board/samus/battery.c
+++ b/board/samus/battery.c
@@ -290,9 +290,9 @@ int board_cut_off_battery(void)
 	buf[2] = PARAM_CUT_OFF_HIGH;
 
 	i2c_lock(I2C_PORT_BATTERY, 1);
-	rv = i2c_xfer(I2C_PORT_BATTERY, BATTERY_ADDR, buf, 3, NULL, 0,
+	rv = i2c_xfer_unlocked(I2C_PORT_BATTERY, BATTERY_ADDR, buf, 3, NULL, 0,
 		      I2C_XFER_SINGLE);
-	rv |= i2c_xfer(I2C_PORT_BATTERY, BATTERY_ADDR, buf, 3, NULL, 0,
+	rv |= i2c_xfer_unlocked(I2C_PORT_BATTERY, BATTERY_ADDR, buf, 3, NULL, 0,
 		      I2C_XFER_SINGLE);
 	i2c_lock(I2C_PORT_BATTERY, 0);
 
diff --git a/board/servo_micro/board.c b/board/servo_micro/board.c
index 5887d559ae..355c091a77 100644
--- a/board/servo_micro/board.c
+++ b/board/servo_micro/board.c
@@ -231,12 +231,13 @@ static int ite_i2c_read_register(uint8_t register_offset, uint8_t *output)
 	 */
 	int ret;
 	/* Tell the ITE EC which register we want to read. */
-	ret = i2c_xfer(I2C_PORT_MASTER, ITE_DFU_I2C_CMD_ADDR, &register_offset,
-		sizeof(register_offset), NULL, 0, I2C_XFER_SINGLE);
+	ret = i2c_xfer_unlocked(I2C_PORT_MASTER, ITE_DFU_I2C_CMD_ADDR,
+		&register_offset, sizeof(register_offset),
+		NULL, 0, I2C_XFER_SINGLE);
 	if (ret != EC_SUCCESS)
 		return ret;
 	/* Read in the 1 byte register value. */
-	ret = i2c_xfer(I2C_PORT_MASTER, ITE_DFU_I2C_DATA_ADDR, NULL, 0,
+	ret = i2c_xfer_unlocked(I2C_PORT_MASTER, ITE_DFU_I2C_DATA_ADDR, NULL, 0,
 		output, sizeof(*output), I2C_XFER_SINGLE);
 	return ret;
 }
diff --git a/board/sweetberry/board.c b/board/sweetberry/board.c
index 8640cfd87f..2d94f06f76 100644
--- a/board/sweetberry/board.c
+++ b/board/sweetberry/board.c
@@ -121,8 +121,6 @@ static void board_init(void)
 	uint8_t tmp;
 
 	/* i2c 0 has a tendancy to get wedged. TODO(nsanders): why? */
-	i2c_lock(0, 1);
-	i2c_xfer(0, 0, NULL, 0, &tmp, 1, I2C_XFER_SINGLE);
-	i2c_lock(0, 0);
+	i2c_xfer(0, 0, NULL, 0, &tmp, 1);
 }
 DECLARE_HOOK(HOOK_INIT, board_init, HOOK_PRIO_DEFAULT);
diff --git a/chip/stm32/usb_power.c b/chip/stm32/usb_power.c
index cc4f723322..7e4206e83b 100644
--- a/chip/stm32/usb_power.c
+++ b/chip/stm32/usb_power.c
@@ -411,10 +411,7 @@ uint16_t ina2xx_readagain(uint8_t port, uint8_t addr)
 	int res;
 	uint16_t val;
 
-	i2c_lock(port, 1);
-	res = i2c_xfer(port, addr, NULL, 0, (uint8_t *)&val, sizeof(uint16_t),
-		      I2C_XFER_SINGLE);
-	i2c_lock(port, 0);
+	res = i2c_xfer(port, addr, NULL, 0, (uint8_t *)&val, sizeof(uint16_t));
 
 	if (res) {
 		CPRINTS("INA2XX I2C readagain failed p:%d a:%02x",
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);
diff --git a/driver/tcpm/fusb302.c b/driver/tcpm/fusb302.c
index f89f47896e..da5d32e206 100644
--- a/driver/tcpm/fusb302.c
+++ b/driver/tcpm/fusb302.c
@@ -322,9 +322,7 @@ static int fusb302_send_message(int port, uint16_t header, const uint32_t *data,
 	buf[buf_pos++] = FUSB302_TKN_TXON;
 
 	/* burst write for speed! */
-	tcpc_lock(port, 1);
-	rv = tcpc_xfer(port, buf, buf_pos, 0, 0, I2C_XFER_SINGLE);
-	tcpc_lock(port, 0);
+	rv = tcpc_xfer(port, buf, buf_pos, 0, 0);
 
 	return rv;
 }
@@ -727,7 +725,7 @@ static int fusb302_tcpm_get_message(int port, uint32_t *payload, int *head)
 		 * PART 1 OF BURST READ: Write in register address.
 		 * Issue a START, no STOP.
 		 */
-		rv = tcpc_xfer(port, buf, 1, 0, 0, I2C_XFER_START);
+		rv = tcpc_xfer_unlocked(port, buf, 1, 0, 0, I2C_XFER_START);
 
 		/*
 		 * PART 2 OF BURST READ: Read up to the header.
@@ -736,7 +734,7 @@ static int fusb302_tcpm_get_message(int port, uint32_t *payload, int *head)
 		 * and determine how many more bytes we need to read.
 		 * TODO: Check token to ensure valid packet.
 		 */
-		rv |= tcpc_xfer(port, 0, 0, buf, 3, I2C_XFER_START);
+		rv |= tcpc_xfer_unlocked(port, 0, 0, buf, 3, I2C_XFER_START);
 
 		/* Grab the header */
 		*head = (buf[1] & 0xFF);
@@ -750,7 +748,7 @@ static int fusb302_tcpm_get_message(int port, uint32_t *payload, int *head)
 		 * No START, but do issue a STOP at the end.
 		 * add 4 to len to read CRC out
 		 */
-		rv |= tcpc_xfer(port, 0, 0, buf, len+4, I2C_XFER_STOP);
+		rv |= tcpc_xfer_unlocked(port, 0, 0, buf, len+4, I2C_XFER_STOP);
 
 		tcpc_lock(port, 0);
 	} while (!rv && PACKET_IS_GOOD_CRC(*head) &&
diff --git a/driver/tcpm/tcpci.c b/driver/tcpm/tcpci.c
index f9760a0aed..aec5c8a329 100644
--- a/driver/tcpm/tcpci.c
+++ b/driver/tcpm/tcpci.c
@@ -106,14 +106,26 @@ int tcpc_write_block(int port, int reg, const uint8_t *out, int size)
 }
 
 int tcpc_xfer(int port, const uint8_t *out, int out_size,
+			uint8_t *in, int in_size)
+{
+	int rv;
+	/* Dispatching to tcpc_xfer_unlocked reduces code size growth. */
+	tcpc_lock(port, 1);
+	rv = tcpc_xfer_unlocked(port, out, out_size, in, in_size,
+				I2C_XFER_SINGLE);
+	tcpc_lock(port, 0);
+	return rv;
+}
+
+int tcpc_xfer_unlocked(int port, const uint8_t *out, int out_size,
 			    uint8_t *in, int in_size, int flags)
 {
-	int rv = i2c_xfer(tcpc_config[port].i2c_host_port,
+	int rv = i2c_xfer_unlocked(tcpc_config[port].i2c_host_port,
 			  tcpc_config[port].i2c_slave_addr, out, out_size,
 			  in, in_size, flags);
 	if (rv && pd_device_in_low_power(port)) {
 		pd_wait_for_wakeup(port);
-		rv = i2c_xfer(tcpc_config[port].i2c_host_port,
+		rv = i2c_xfer_unlocked(tcpc_config[port].i2c_host_port,
 			      tcpc_config[port].i2c_slave_addr, out, out_size,
 			      in, in_size, flags);
 	}
diff --git a/driver/tcpm/tcpm.h b/driver/tcpm/tcpm.h
index cc360ba75d..0ea0bf47a7 100644
--- a/driver/tcpm/tcpm.h
+++ b/driver/tcpm/tcpm.h
@@ -51,10 +51,18 @@ static inline int tcpc_read16(int port, int reg, int *val)
 }
 
 static inline int tcpc_xfer(int port, const uint8_t *out, int out_size,
-			    uint8_t *in, int in_size, int flags)
+			    uint8_t *in, int in_size)
 {
 	return i2c_xfer(tcpc_config[port].i2c_host_port,
 			tcpc_config[port].i2c_slave_addr, out, out_size, in,
+			in_size);
+}
+
+static inline int tcpc_xfer_unlocked(int port, const uint8_t *out, int out_size,
+			    uint8_t *in, int in_size, int flags)
+{
+	return i2c_xfer_unlocked(tcpc_config[port].i2c_host_port,
+			tcpc_config[port].i2c_slave_addr, out, out_size, in,
 			in_size, flags);
 }
 
@@ -79,6 +87,8 @@ int tcpc_read16(int port, int reg, int *val);
 int tcpc_read_block(int port, int reg, uint8_t *in, int size);
 int tcpc_write_block(int port, int reg, const uint8_t *out, int size);
 int tcpc_xfer(int port, const uint8_t *out, int out_size,
+		uint8_t *in, int in_size);
+int tcpc_xfer_unlocked(int port, const uint8_t *out, int out_size,
 		uint8_t *in, int in_size, int flags);
 
 #endif /* CONFIG_USB_PD_TCPC_LOW_POWER */
diff --git a/driver/touchpad_elan.c b/driver/touchpad_elan.c
index e1f95f3949..b39ef4fecc 100644
--- a/driver/touchpad_elan.c
+++ b/driver/touchpad_elan.c
@@ -112,36 +112,25 @@ const int pressure_div = 1024;
 static int elan_tp_read_cmd(uint16_t reg, uint16_t *val)
 {
 	uint8_t buf[2];
-	int rv;
 
 	buf[0] = reg;
 	buf[1] = reg >> 8;
 
-	i2c_lock(CONFIG_TOUCHPAD_I2C_PORT, 1);
-	rv = i2c_xfer(CONFIG_TOUCHPAD_I2C_PORT, CONFIG_TOUCHPAD_I2C_ADDR,
-		      buf, sizeof(buf), (uint8_t *)val, sizeof(*val),
-		      I2C_XFER_SINGLE);
-	i2c_lock(CONFIG_TOUCHPAD_I2C_PORT, 0);
-
-	return rv;
+	return i2c_xfer(CONFIG_TOUCHPAD_I2C_PORT, CONFIG_TOUCHPAD_I2C_ADDR,
+		      buf, sizeof(buf), (uint8_t *)val, sizeof(*val));
 }
 
 static int elan_tp_write_cmd(uint16_t reg, uint16_t val)
 {
 	uint8_t buf[4];
-	int rv;
 
 	buf[0] = reg;
 	buf[1] = reg >> 8;
 	buf[2] = val;
 	buf[3] = val >> 8;
 
-	i2c_lock(CONFIG_TOUCHPAD_I2C_PORT, 1);
-	rv = i2c_xfer(CONFIG_TOUCHPAD_I2C_PORT, CONFIG_TOUCHPAD_I2C_ADDR,
-		      buf, sizeof(buf), NULL, 0, I2C_XFER_SINGLE);
-	i2c_lock(CONFIG_TOUCHPAD_I2C_PORT, 0);
-
-	return rv;
+	return i2c_xfer(CONFIG_TOUCHPAD_I2C_PORT, CONFIG_TOUCHPAD_I2C_ADDR,
+		      buf, sizeof(buf), NULL, 0);
 }
 
 /* Power is on by default. */
@@ -201,10 +190,8 @@ static int elan_tp_read_report(void)
 	/* Compute and save timestamp early in case another interrupt comes. */
 	timestamp = irq_ts / USB_HID_TOUCHPAD_TIMESTAMP_UNIT;
 
-	i2c_lock(CONFIG_TOUCHPAD_I2C_PORT, 1);
 	rv = i2c_xfer(CONFIG_TOUCHPAD_I2C_PORT, CONFIG_TOUCHPAD_I2C_ADDR,
-		      NULL, 0, tp_buf, ETP_I2C_REPORT_LEN, I2C_XFER_SINGLE);
-	i2c_lock(CONFIG_TOUCHPAD_I2C_PORT, 0);
+		      NULL, 0, tp_buf, ETP_I2C_REPORT_LEN);
 
 	if (rv) {
 		CPRINTS("read report error (%d)", rv);
@@ -288,10 +275,8 @@ static void elan_tp_init(void)
 
 	elan_tp_write_cmd(ETP_I2C_STAND_CMD, ETP_I2C_RESET);
 	msleep(100);
-	i2c_lock(CONFIG_TOUCHPAD_I2C_PORT, 1);
 	rv = i2c_xfer(CONFIG_TOUCHPAD_I2C_PORT, CONFIG_TOUCHPAD_I2C_ADDR,
-		      NULL, 0, val, sizeof(val), I2C_XFER_SINGLE);
-	i2c_lock(CONFIG_TOUCHPAD_I2C_PORT, 0);
+		      NULL, 0, val, sizeof(val));
 
 	CPRINTS("reset rv %d buf=%04x", rv, *((uint16_t *)val));
 	if (rv)
@@ -480,11 +465,8 @@ static int touchpad_update_page(const uint8_t *data)
 	page_store[FW_PAGE_SIZE + 2 + 0] = checksum & 0xff;
 	page_store[FW_PAGE_SIZE + 2 + 1] = (checksum >> 8) & 0xff;
 
-	i2c_lock(CONFIG_TOUCHPAD_I2C_PORT, 1);
 	rv = i2c_xfer(CONFIG_TOUCHPAD_I2C_PORT, CONFIG_TOUCHPAD_I2C_ADDR,
-		      page_store, sizeof(page_store), NULL, 0,
-		      I2C_XFER_SINGLE);
-	i2c_lock(CONFIG_TOUCHPAD_I2C_PORT, 0);
+		      page_store, sizeof(page_store), NULL, 0);
 	if (rv)
 		return rv;
 	msleep(20);
@@ -647,12 +629,10 @@ int touchpad_debug(const uint8_t *param, unsigned int param_size,
 			memset(buffer, 0, buffer_size);
 		}
 
-		i2c_lock(CONFIG_TOUCHPAD_I2C_PORT, 1);
 		rv = i2c_xfer(CONFIG_TOUCHPAD_I2C_PORT,
 			      CONFIG_TOUCHPAD_I2C_ADDR,
 			      &param[offset], write_length,
-			      buffer, read_length, I2C_XFER_SINGLE);
-		i2c_lock(CONFIG_TOUCHPAD_I2C_PORT, 0);
+			      buffer, read_length);
 
 		if (rv)
 			return EC_RES_BUS_ERROR;
diff --git a/include/i2c.h b/include/i2c.h
index 0bd47fd24f..8ae21566cb 100644
--- a/include/i2c.h
+++ b/include/i2c.h
@@ -83,7 +83,7 @@ extern struct i2c_stress_test i2c_stress_tests[];
 extern const int i2c_test_dev_used;
 #endif
 
-/* Flags for i2c_xfer() */
+/* Flags for i2c_xfer_unlocked() */
 #define I2C_XFER_START (1 << 0)  /* Start smbus session from idle state */
 #define I2C_XFER_STOP (1 << 1)  /* Terminate smbus session with stop bit */
 #define I2C_XFER_SINGLE (I2C_XFER_START | I2C_XFER_STOP)  /* One transaction */
@@ -91,10 +91,8 @@ extern const int i2c_test_dev_used;
 /**
  * Transmit one block of raw data, then receive one block of raw data. However,
  * received data might be capped at CONFIG_I2C_CHIP_MAX_READ_SIZE if
- * CONFIG_I2C_XFER_LARGE_READ is not defined.
- *
- * This is a wrapper function for chip_i2c_xfer(), a low-level chip-dependent
- * function. It must be called between i2c_lock(port, 1) and i2c_lock(port, 0).
+ * CONFIG_I2C_XFER_LARGE_READ is not defined.  The transfer is strictly atomic,
+ * by locking the I2C port and performing an I2C_XFER_SINGLE transfer.
  *
  * @param port		Port to access
  * @param slave_addr	Slave device address
@@ -102,11 +100,20 @@ extern const int i2c_test_dev_used;
  * @param out_size	Number of bytes to send
  * @param in		Destination buffer for received data
  * @param in_size	Number of bytes to receive
- * @param flags		Flags (see I2C_XFER_* above)
  * @return EC_SUCCESS, or non-zero if error.
  */
 int i2c_xfer(int port, int slave_addr, const uint8_t *out, int out_size,
-	     uint8_t *in, int in_size, int flags);
+	     uint8_t *in, int in_size);
+
+/**
+ * Same as i2c_xfer, but the bus is not implicitly locked.  It must be called
+ * between i2c_lock(port, 1) and i2c_lock(port, 0).
+ *
+ * @param flags		Flags (see I2C_XFER_* above)
+ */
+int i2c_xfer_unlocked(int port, int slave_addr,
+		      const uint8_t *out, int out_size,
+		      uint8_t *in, int in_size, int flags);
 
 #define I2C_LINE_SCL_HIGH (1 << 0)
 #define I2C_LINE_SDA_HIGH (1 << 1)