ectool: Don't acquire lock when dev interface is used

The /dev/cros_ec interface has a built-in mutex, thus we do not
need to use /run/lock to arbitrate access since we can assume other
tools (mosys, flashrom) also use the dev interface.

$ generate_logs
...
feedback/cbi_info
...
$ cat feedback/cbi_info
[0]
As integer: 1 (0x1)
As binary: 01 02
[1]
As integer: 3 (0x3)
As binary: 03
[2]
As integer: 103 (0x67)
As binary: 67 3a

Signed-off-by: Daisuke Nojiri <dnojiri@chromium.org>

BUG=chromium:849399
BRANCH=none
TEST=Verify 'ectool version' runs on Nami. Verify lock is acquired
when '--interface=lpc' is specified. Verify debugd can run cbi_info.

Change-Id: Id94317472917a974218bb137bda11fe5618a4b88
Reviewed-on: https://chromium-review.googlesource.com/1393729
Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com>
Tested-by: Daisuke Nojiri <dnojiri@chromium.org>
Reviewed-by: Aseda Aboagye <aaboagye@chromium.org>
(cherry picked from commit aed008f87c3c880edecf7608ab24eaa4bee1bc46)
Reviewed-on: https://chromium-review.googlesource.com/c/1398342
Commit-Queue: Daisuke Nojiri <dnojiri@chromium.org>
Reviewed-by: Daisuke Nojiri <dnojiri@chromium.org>

4 files changed, 60 insertions, 45 deletions

diff --git a/util/comm-host.c b/util/comm-host.c
index e459bdfaa8..e9e7fb1f5f 100644
--- a/util/comm-host.c
+++ b/util/comm-host.c
@@ -82,10 +82,33 @@ int ec_command(int command, int version,
 				indata, insize);
 }
 
-int comm_init(int interfaces, const char *device_name)
+int comm_init_alt(int interfaces, const char *device_name)
+{
+	if ((interfaces & COMM_SERVO) && comm_init_servo_spi &&
+	    !comm_init_servo_spi(device_name))
+		return 0;
+
+	/* Do not fallback to other communication methods if target is not a
+	 * cros_ec device */
+	if (!strcmp(CROS_EC_DEV_NAME, device_name)) {
+		/* Fallback to direct LPC on x86 */
+		if ((interfaces & COMM_LPC) && !comm_init_lpc())
+			return 0;
+
+		/* Fallback to direct i2c on ARM */
+		if ((interfaces & COMM_I2C) && !comm_init_i2c())
+			return 0;
+	}
+
+	/* Give up */
+	fprintf(stderr, "Unable to establish host communication\n");
+	return 1;
+}
+
+int comm_init_buffer(void)
 {
-	struct ec_response_get_protocol_info info;
 	int allow_large_buffer;
+	struct ec_response_get_protocol_info info;
 
 	/* Default memmap access */
 	ec_readmem = fake_readmem;
@@ -96,34 +119,6 @@ int comm_init(int interfaces, const char *device_name)
 		return 1;
 	}
 
-	/* Prefer new /dev method */
-	if ((interfaces & COMM_DEV) && comm_init_dev &&
-	    !comm_init_dev(device_name))
-		goto init_ok;
-
-	if ((interfaces & COMM_SERVO) && comm_init_servo_spi &&
-	    !comm_init_servo_spi(device_name))
-		goto init_ok;
-
-	/* Do not fallback to other communication methods if target is not a
-	 * cros_ec device */
-	if (strcmp(CROS_EC_DEV_NAME, device_name))
-		goto init_failed;
-
-	/* Fallback to direct LPC on x86 */
-	if ((interfaces & COMM_LPC) && comm_init_lpc && !comm_init_lpc())
-		goto init_ok;
-
-	/* Fallback to direct i2c on ARM */
-	if ((interfaces & COMM_I2C) && comm_init_i2c && !comm_init_i2c())
-		goto init_ok;
-
- init_failed:
-	/* Give up */
-	fprintf(stderr, "Unable to establish host communication\n");
-	return 1;
-
- init_ok:
 	/* Allocate shared I/O buffers */
 	ec_outbuf = malloc(ec_max_outsize);
 	ec_inbuf = malloc(ec_max_insize);
@@ -154,5 +149,4 @@ int comm_init(int interfaces, const char *device_name)
 	}
 
 	return 0;
-
 }
diff --git a/util/comm-host.h b/util/comm-host.h
index bf6921d17d..11f81b4950 100644
--- a/util/comm-host.h
+++ b/util/comm-host.h
@@ -35,13 +35,27 @@ enum comm_interface {
 };
 
 /**
- * Perform initializations needed for subsequent requests
+ * Initialize alternative interfaces
  *
  * @param interfaces	Interfaces to try; use COMM_ALL to try all of them.
  * @param device_name For DEV option, the device file to use.
  * @return 0 in case of success, or error code.
  */
-int comm_init(int interfaces, const char *device_name);
+int comm_init_alt(int interfaces, const char *device_name);
+
+/**
+ * Initialize dev interface
+ *
+ * @return 0 in case of success, or error code.
+ */
+int comm_init_dev(const char *device_name);
+
+/**
+ * Initialize input & output buffers
+ *
+ * @return 0 in case of success, or error code.
+ */
+int comm_init_buffer(void);
 
 /**
  * Send a command to the EC.  Returns the length of output data returned (0 if
diff --git a/util/ec_sb_firmware_update.c b/util/ec_sb_firmware_update.c
index 54b7ff1567..09067b6928 100644
--- a/util/ec_sb_firmware_update.c
+++ b/util/ec_sb_firmware_update.c
@@ -748,7 +748,7 @@ void usage(char *argv[])
 
 int main(int argc, char *argv[])
 {
-	int rv = 0, interfaces = COMM_ALL;
+	int rv = 0;
 	int op = OP_UNKNOWN;
 	uint8_t val = 0;
 
@@ -767,14 +767,14 @@ int main(int argc, char *argv[])
 		return -1;
 	}
 
-	if (acquire_gec_lock(GEC_LOCK_TIMEOUT_SECS) < 0) {
-		printf("Could not acquire GEC lock.\n");
+	if (comm_init_dev(NULL)) {
+		printf("Couldn't initialize /dev.\n");
 		return -1;
 	}
 
-	if (comm_init(interfaces, NULL)) {
-		printf("Couldn't find EC\n");
-		goto out;
+	if (comm_init_buffer()) {
+		fprintf(stderr, "Couldn't initialize buffers\n");
+		return -1;
 	}
 
 	fw_update.flags = 0;
@@ -836,7 +836,6 @@ int main(int argc, char *argv[])
 	if (fw_update.flags & F_POWERD_DISABLED)
 		rv |= restore_power_management();
 out:
-	release_gec_lock();
 	if (rv)
 		return -1;
 	else
diff --git a/util/ectool.c b/util/ectool.c
index f918c0b7eb..c9ac36c686 100644
--- a/util/ectool.c
+++ b/util/ectool.c
@@ -8598,13 +8598,21 @@ int main(int argc, char *argv[])
 		exit(1);
 	}
 
-	if (acquire_gec_lock(GEC_LOCK_TIMEOUT_SECS) < 0) {
-		fprintf(stderr, "Could not acquire GEC lock.\n");
-		exit(1);
+	/* Prefer /dev method, which supports built-in mutex */
+	if (!(interfaces & COMM_DEV) || comm_init_dev(device_name)) {
+		/* If dev is excluded or isn't supported, find alternative */
+		if (acquire_gec_lock(GEC_LOCK_TIMEOUT_SECS) < 0) {
+			fprintf(stderr, "Could not acquire GEC lock.\n");
+			exit(1);
+		}
+		if (comm_init_alt(interfaces, device_name)) {
+			fprintf(stderr, "Couldn't find EC\n");
+			goto out;
+		}
 	}
 
-	if (comm_init(interfaces, device_name)) {
-		fprintf(stderr, "Couldn't find EC\n");
+	if (comm_init_buffer()) {
+		fprintf(stderr, "Couldn't initialize buffers\n");
 		goto out;
 	}