[PATCH v2 0/5] platform/chrome: Fix MEC concurrency problems for Framework Laptop

[PATCH v2 0/5] platform/chrome: Fix MEC concurrency problems for Framework Laptop

[Ben Walsh]

Framework Laptops with the Microchip EC have a problem where the EC
"stops working" after a while. Symptoms include the Fn key not
working, and "bad packet checksum" errors appearing in the system log.

The problem is caused by ACPI code which accesses the Microchip EC
(MEC) memory using the Microchip EMI protocol. It uses an AML mutex to
prevent concurrent access. But the cros_ec_lpc driver is not aware of
this mutex. The ACPI code and LPC driver both attempt to talk to the
EC at the same time, messing up communication with the EC.

The solution is to have the cros_ec_lpc_mec code find and use the AML
mutex. But to make it all work we have to do a few more things:

  * Allow the cros_ec_lpc_mec code to return error codes in case it
    can't lock the mutex.

  * Have the cros_ec_lpc code find the correct ACPI device (PNP0C09)
    and then the AML mutex itself.

  * Use the quirks mechanism to specify the AML mutex name.

The code has been tested on an 11th-generation Intel Framework Laptop
(with Microchip EC) and an AMD Framework Laptop (without Microchip
EC). It has _not_ been tested on any Chromebook devices.

Changes in v2:

  * Put ACPI id in a quirk, not the acpi_match_table.
  * Add return code check for mutex unlock.
  * Remove "in_range" check which belongs in a separate patch.
  * Use "ret" not "sum" variable (same value but clearer).
  * Remove excessive error logging.
  * Rebase to latest ChromeOS for-next branch.

Ben Walsh (5):
  platform/chrome: cros_ec_lpc: MEC access can return error code
  platform/chrome: cros_ec_lpc: MEC access can use an AML mutex
  platform/chrome: cros_ec_lpc: Add a new quirk for ACPI id
  platform/chrome: cros_ec_lpc: Add a new quirk for AML mutex
  platform/chrome: cros_ec_lpc: Add quirks for Framework Laptop

 drivers/platform/chrome/cros_ec_lpc.c      | 210 +++++++++++++++------
 drivers/platform/chrome/cros_ec_lpc_mec.c  |  87 ++++++++-
 drivers/platform/chrome/cros_ec_lpc_mec.h  |  18 +-
 drivers/platform/chrome/wilco_ec/mailbox.c |  22 ++-
 4 files changed, 264 insertions(+), 73 deletions(-)


base-commit: bc3e45258096f2ea2116302abefde4b1cb9bc3c1
-- 
2.45.1

[PATCH v2 1/5] platform/chrome: cros_ec_lpc: MEC access can return error code

[Ben Walsh]

cros_ec_lpc_io_bytes_mec was returning a u8 checksum of all bytes
read/written, which didn't leave room to indicate errors. Change this
u8 to an int where negative values indicate an error, and non-negative
values are the checksum as before.

Signed-off-by: Ben Walsh <ben@jubnut.com>
---
 drivers/platform/chrome/cros_ec_lpc.c      | 130 ++++++++++++++-------
 drivers/platform/chrome/cros_ec_lpc_mec.c  |   9 +-
 drivers/platform/chrome/cros_ec_lpc_mec.h  |   7 +-
 drivers/platform/chrome/wilco_ec/mailbox.c |  22 ++--
 4 files changed, 110 insertions(+), 58 deletions(-)

diff --git a/drivers/platform/chrome/cros_ec_lpc.c b/drivers/platform/chrome/cros_ec_lpc.c
index ddfbfec44f4c..6663141a07d4 100644
--- a/drivers/platform/chrome/cros_ec_lpc.c
+++ b/drivers/platform/chrome/cros_ec_lpc.c
@@ -62,14 +62,16 @@ struct cros_ec_lpc {
 
 /**
  * struct lpc_driver_ops - LPC driver operations
- * @read: Copy length bytes from EC address offset into buffer dest. Returns
- *        the 8-bit checksum of all bytes read.
- * @write: Copy length bytes from buffer msg into EC address offset. Returns
- *         the 8-bit checksum of all bytes written.
+ * @read: Copy length bytes from EC address offset into buffer dest.
+ *        Returns a negative error code on error, or the 8-bit checksum
+ *        of all bytes read.
+ * @write: Copy length bytes from buffer msg into EC address offset.
+ *         Returns a negative error code on error, or the 8-bit checksum
+ *         of all bytes written.
  */
 struct lpc_driver_ops {
-	u8 (*read)(unsigned int offset, unsigned int length, u8 *dest);
-	u8 (*write)(unsigned int offset, unsigned int length, const u8 *msg);
+	int (*read)(unsigned int offset, unsigned int length, u8 *dest);
+	int (*write)(unsigned int offset, unsigned int length, const u8 *msg);
 };
 
 static struct lpc_driver_ops cros_ec_lpc_ops = { };
@@ -78,10 +80,10 @@ static struct lpc_driver_ops cros_ec_lpc_ops = { };
  * A generic instance of the read function of struct lpc_driver_ops, used for
  * the LPC EC.
  */
-static u8 cros_ec_lpc_read_bytes(unsigned int offset, unsigned int length,
-				 u8 *dest)
+static int cros_ec_lpc_read_bytes(unsigned int offset, unsigned int length,
+				  u8 *dest)
 {
-	int sum = 0;
+	u8 sum = 0;
 	int i;
 
 	for (i = 0; i < length; ++i) {
@@ -97,10 +99,10 @@ static u8 cros_ec_lpc_read_bytes(unsigned int offset, unsigned int length,
  * A generic instance of the write function of struct lpc_driver_ops, used for
  * the LPC EC.
  */
-static u8 cros_ec_lpc_write_bytes(unsigned int offset, unsigned int length,
-				  const u8 *msg)
+static int cros_ec_lpc_write_bytes(unsigned int offset, unsigned int length,
+				   const u8 *msg)
 {
-	int sum = 0;
+	u8 sum = 0;
 	int i;
 
 	for (i = 0; i < length; ++i) {
@@ -116,8 +118,8 @@ static u8 cros_ec_lpc_write_bytes(unsigned int offset, unsigned int length,
  * An instance of the read function of struct lpc_driver_ops, used for the
  * MEC variant of LPC EC.
  */
-static u8 cros_ec_lpc_mec_read_bytes(unsigned int offset, unsigned int length,
-				     u8 *dest)
+static int cros_ec_lpc_mec_read_bytes(unsigned int offset, unsigned int length,
+				      u8 *dest)
 {
 	int in_range = cros_ec_lpc_mec_in_range(offset, length);
 
@@ -135,8 +137,8 @@ static u8 cros_ec_lpc_mec_read_bytes(unsigned int offset, unsigned int length,
  * An instance of the write function of struct lpc_driver_ops, used for the
  * MEC variant of LPC EC.
  */
-static u8 cros_ec_lpc_mec_write_bytes(unsigned int offset, unsigned int length,
-				      const u8 *msg)
+static int cros_ec_lpc_mec_write_bytes(unsigned int offset, unsigned int length,
+				       const u8 *msg)
 {
 	int in_range = cros_ec_lpc_mec_in_range(offset, length);
 
@@ -154,11 +156,14 @@ static int ec_response_timed_out(void)
 {
 	unsigned long one_second = jiffies + HZ;
 	u8 data;
+	int ret;
 
 	usleep_range(200, 300);
 	do {
-		if (!(cros_ec_lpc_ops.read(EC_LPC_ADDR_HOST_CMD, 1, &data) &
-		    EC_LPC_STATUS_BUSY_MASK))
+		ret = cros_ec_lpc_ops.read(EC_LPC_ADDR_HOST_CMD, 1, &data);
+		if (ret < 0)
+			return ret;
+		if (!(data & EC_LPC_STATUS_BUSY_MASK))
 			return 0;
 		usleep_range(100, 200);
 	} while (time_before(jiffies, one_second));
@@ -179,28 +184,41 @@ static int cros_ec_pkt_xfer_lpc(struct cros_ec_device *ec,
 		goto done;
 
 	/* Write buffer */
-	cros_ec_lpc_ops.write(EC_LPC_ADDR_HOST_PACKET, ret, ec->dout);
+	ret = cros_ec_lpc_ops.write(EC_LPC_ADDR_HOST_PACKET, ret, ec->dout);
+	if (ret < 0)
+		goto done;
 
 	/* Here we go */
 	sum = EC_COMMAND_PROTOCOL_3;
-	cros_ec_lpc_ops.write(EC_LPC_ADDR_HOST_CMD, 1, &sum);
+	ret = cros_ec_lpc_ops.write(EC_LPC_ADDR_HOST_CMD, 1, &sum);
+	if (ret < 0)
+		goto done;
 
-	if (ec_response_timed_out()) {
+	ret = ec_response_timed_out();
+	if (ret < 0)
+		goto done;
+	if (ret) {
 		dev_warn(ec->dev, "EC response timed out\n");
 		ret = -EIO;
 		goto done;
 	}
 
 	/* Check result */
-	msg->result = cros_ec_lpc_ops.read(EC_LPC_ADDR_HOST_DATA, 1, &sum);
+	ret = cros_ec_lpc_ops.read(EC_LPC_ADDR_HOST_DATA, 1, &sum);
+	if (ret < 0)
+		goto done;
+	msg->result = ret;
 	ret = cros_ec_check_result(ec, msg);
 	if (ret)
 		goto done;
 
 	/* Read back response */
 	dout = (u8 *)&response;
-	sum = cros_ec_lpc_ops.read(EC_LPC_ADDR_HOST_PACKET, sizeof(response),
+	ret = cros_ec_lpc_ops.read(EC_LPC_ADDR_HOST_PACKET, sizeof(response),
 				   dout);
+	if (ret < 0)
+		goto done;
+	sum = ret;
 
 	msg->result = response.result;
 
@@ -213,9 +231,12 @@ static int cros_ec_pkt_xfer_lpc(struct cros_ec_device *ec,
 	}
 
 	/* Read response and process checksum */
-	sum += cros_ec_lpc_ops.read(EC_LPC_ADDR_HOST_PACKET +
-				    sizeof(response), response.data_len,
-				    msg->data);
+	ret = cros_ec_lpc_ops.read(EC_LPC_ADDR_HOST_PACKET +
+				   sizeof(response), response.data_len,
+				   msg->data);
+	if (ret < 0)
+		goto done;
+	sum += ret;
 
 	if (sum) {
 		dev_err(ec->dev,
@@ -255,32 +276,47 @@ static int cros_ec_cmd_xfer_lpc(struct cros_ec_device *ec,
 	sum = msg->command + args.flags + args.command_version + args.data_size;
 
 	/* Copy data and update checksum */
-	sum += cros_ec_lpc_ops.write(EC_LPC_ADDR_HOST_PARAM, msg->outsize,
-				     msg->data);
+	ret = cros_ec_lpc_ops.write(EC_LPC_ADDR_HOST_PARAM, msg->outsize,
+				    msg->data);
+	if (ret < 0)
+		goto done;
+	sum += ret;
 
 	/* Finalize checksum and write args */
 	args.checksum = sum;
-	cros_ec_lpc_ops.write(EC_LPC_ADDR_HOST_ARGS, sizeof(args),
-			      (u8 *)&args);
+	ret = cros_ec_lpc_ops.write(EC_LPC_ADDR_HOST_ARGS, sizeof(args),
+				    (u8 *)&args);
+	if (ret < 0)
+		goto done;
 
 	/* Here we go */
 	sum = msg->command;
-	cros_ec_lpc_ops.write(EC_LPC_ADDR_HOST_CMD, 1, &sum);
+	ret = cros_ec_lpc_ops.write(EC_LPC_ADDR_HOST_CMD, 1, &sum);
+	if (ret < 0)
+		goto done;
 
-	if (ec_response_timed_out()) {
+	ret = ec_response_timed_out();
+	if (ret < 0)
+		goto done;
+	if (ret) {
 		dev_warn(ec->dev, "EC response timed out\n");
 		ret = -EIO;
 		goto done;
 	}
 
 	/* Check result */
-	msg->result = cros_ec_lpc_ops.read(EC_LPC_ADDR_HOST_DATA, 1, &sum);
+	ret = cros_ec_lpc_ops.read(EC_LPC_ADDR_HOST_DATA, 1, &sum);
+	if (ret < 0)
+		goto done;
+	msg->result = ret;
 	ret = cros_ec_check_result(ec, msg);
 	if (ret)
 		goto done;
 
 	/* Read back args */
-	cros_ec_lpc_ops.read(EC_LPC_ADDR_HOST_ARGS, sizeof(args), (u8 *)&args);
+	ret = cros_ec_lpc_ops.read(EC_LPC_ADDR_HOST_ARGS, sizeof(args), (u8 *)&args);
+	if (ret < 0)
+		goto done;
 
 	if (args.data_size > msg->insize) {
 		dev_err(ec->dev,
@@ -294,8 +330,11 @@ static int cros_ec_cmd_xfer_lpc(struct cros_ec_device *ec,
 	sum = msg->command + args.flags + args.command_version + args.data_size;
 
 	/* Read response and update checksum */
-	sum += cros_ec_lpc_ops.read(EC_LPC_ADDR_HOST_PARAM, args.data_size,
-				    msg->data);
+	ret = cros_ec_lpc_ops.read(EC_LPC_ADDR_HOST_PARAM, args.data_size,
+				   msg->data);
+	if (ret < 0)
+		goto done;
+	sum += ret;
 
 	/* Verify checksum */
 	if (args.checksum != sum) {
@@ -320,19 +359,24 @@ static int cros_ec_lpc_readmem(struct cros_ec_device *ec, unsigned int offset,
 	int i = offset;
 	char *s = dest;
 	int cnt = 0;
+	int ret;
 
 	if (offset >= EC_MEMMAP_SIZE - bytes)
 		return -EINVAL;
 
 	/* fixed length */
 	if (bytes) {
-		cros_ec_lpc_ops.read(ec_lpc->mmio_memory_base + offset, bytes, s);
+		ret = cros_ec_lpc_ops.read(ec_lpc->mmio_memory_base + offset, bytes, s);
+		if (ret < 0)
+			return ret;
 		return bytes;
 	}
 
 	/* string */
 	for (; i < EC_MEMMAP_SIZE; i++, s++) {
-		cros_ec_lpc_ops.read(ec_lpc->mmio_memory_base + i, 1, s);
+		ret = cros_ec_lpc_ops.read(ec_lpc->mmio_memory_base + i, 1, s);
+		if (ret < 0)
+			return ret;
 		cnt++;
 		if (!*s)
 			break;
@@ -425,8 +469,8 @@ static int cros_ec_lpc_probe(struct platform_device *pdev)
 	 */
 	cros_ec_lpc_ops.read = cros_ec_lpc_mec_read_bytes;
 	cros_ec_lpc_ops.write = cros_ec_lpc_mec_write_bytes;
-	cros_ec_lpc_ops.read(EC_LPC_ADDR_MEMMAP + EC_MEMMAP_ID, 2, buf);
-	if (buf[0] != 'E' || buf[1] != 'C') {
+	ret = cros_ec_lpc_ops.read(EC_LPC_ADDR_MEMMAP + EC_MEMMAP_ID, 2, buf);
+	if (ret < 0 || buf[0] != 'E' || buf[1] != 'C') {
 		if (!devm_request_region(dev, ec_lpc->mmio_memory_base, EC_MEMMAP_SIZE,
 					 dev_name(dev))) {
 			dev_err(dev, "couldn't reserve memmap region\n");
@@ -436,9 +480,9 @@ static int cros_ec_lpc_probe(struct platform_device *pdev)
 		/* Re-assign read/write operations for the non MEC variant */
 		cros_ec_lpc_ops.read = cros_ec_lpc_read_bytes;
 		cros_ec_lpc_ops.write = cros_ec_lpc_write_bytes;
-		cros_ec_lpc_ops.read(ec_lpc->mmio_memory_base + EC_MEMMAP_ID, 2,
-				     buf);
-		if (buf[0] != 'E' || buf[1] != 'C') {
+		ret = cros_ec_lpc_ops.read(ec_lpc->mmio_memory_base + EC_MEMMAP_ID, 2,
+					   buf);
+		if (ret < 0 || buf[0] != 'E' || buf[1] != 'C') {
 			dev_err(dev, "EC ID not detected\n");
 			return -ENODEV;
 		}
diff --git a/drivers/platform/chrome/cros_ec_lpc_mec.c b/drivers/platform/chrome/cros_ec_lpc_mec.c
index 0d9c79b270ce..395dc3a6fb5e 100644
--- a/drivers/platform/chrome/cros_ec_lpc_mec.c
+++ b/drivers/platform/chrome/cros_ec_lpc_mec.c
@@ -67,11 +67,12 @@ int cros_ec_lpc_mec_in_range(unsigned int offset, unsigned int length)
  * @length:  Number of bytes to read / write
  * @buf:     Destination / source buffer
  *
- * Return: 8-bit checksum of all bytes read / written
+ * @return:  A negative error code on error, or 8-bit checksum of all
+ *           bytes read / written
  */
-u8 cros_ec_lpc_io_bytes_mec(enum cros_ec_lpc_mec_io_type io_type,
-			    unsigned int offset, unsigned int length,
-			    u8 *buf)
+int cros_ec_lpc_io_bytes_mec(enum cros_ec_lpc_mec_io_type io_type,
+			     unsigned int offset, unsigned int length,
+			     u8 *buf)
 {
 	int i = 0;
 	int io_addr;
diff --git a/drivers/platform/chrome/cros_ec_lpc_mec.h b/drivers/platform/chrome/cros_ec_lpc_mec.h
index 9d0521b23e8a..69670832f187 100644
--- a/drivers/platform/chrome/cros_ec_lpc_mec.h
+++ b/drivers/platform/chrome/cros_ec_lpc_mec.h
@@ -64,9 +64,10 @@ int cros_ec_lpc_mec_in_range(unsigned int offset, unsigned int length);
  * @length:  Number of bytes to read / write
  * @buf:     Destination / source buffer
  *
- * @return 8-bit checksum of all bytes read / written
+ * @return:  A negative error code on error, or 8-bit checksum of all
+ *           bytes read / written
  */
-u8 cros_ec_lpc_io_bytes_mec(enum cros_ec_lpc_mec_io_type io_type,
-			    unsigned int offset, unsigned int length, u8 *buf);
+int cros_ec_lpc_io_bytes_mec(enum cros_ec_lpc_mec_io_type io_type,
+			     unsigned int offset, unsigned int length, u8 *buf);
 
 #endif /* __CROS_EC_LPC_MEC_H */
diff --git a/drivers/platform/chrome/wilco_ec/mailbox.c b/drivers/platform/chrome/wilco_ec/mailbox.c
index 0f98358ea824..4d8273b47cde 100644
--- a/drivers/platform/chrome/wilco_ec/mailbox.c
+++ b/drivers/platform/chrome/wilco_ec/mailbox.c
@@ -117,13 +117,17 @@ static int wilco_ec_transfer(struct wilco_ec_device *ec,
 			     struct wilco_ec_request *rq)
 {
 	struct wilco_ec_response *rs;
-	u8 checksum;
+	int ret;
 	u8 flag;
 
 	/* Write request header, then data */
-	cros_ec_lpc_io_bytes_mec(MEC_IO_WRITE, 0, sizeof(*rq), (u8 *)rq);
-	cros_ec_lpc_io_bytes_mec(MEC_IO_WRITE, sizeof(*rq), msg->request_size,
-				 msg->request_data);
+	ret = cros_ec_lpc_io_bytes_mec(MEC_IO_WRITE, 0, sizeof(*rq), (u8 *)rq);
+	if (ret < 0)
+		return ret;
+	ret = cros_ec_lpc_io_bytes_mec(MEC_IO_WRITE, sizeof(*rq), msg->request_size,
+				       msg->request_data);
+	if (ret < 0)
+		return ret;
 
 	/* Start the command */
 	outb(EC_MAILBOX_START_COMMAND, ec->io_command->start);
@@ -149,10 +153,12 @@ static int wilco_ec_transfer(struct wilco_ec_device *ec,
 
 	/* Read back response */
 	rs = ec->data_buffer;
-	checksum = cros_ec_lpc_io_bytes_mec(MEC_IO_READ, 0,
-					    sizeof(*rs) + EC_MAILBOX_DATA_SIZE,
-					    (u8 *)rs);
-	if (checksum) {
+	ret = cros_ec_lpc_io_bytes_mec(MEC_IO_READ, 0,
+				       sizeof(*rs) + EC_MAILBOX_DATA_SIZE,
+				       (u8 *)rs);
+	if (ret < 0)
+		return ret;
+	if (ret) {
 		dev_dbg(ec->dev, "bad packet checksum 0x%02x\n", rs->checksum);
 		return -EBADMSG;
 	}
-- 
2.45.1

Re: [PATCH v2 1/5] platform/chrome: cros_ec_lpc: MEC access can return error code

[Tzung-Bi Shih]

On Mon, Jun 03, 2024 at 07:38:30AM +0100, Ben Walsh wrote:
> @@ -425,8 +469,8 @@ static int cros_ec_lpc_probe(struct platform_device *pdev)
>  	 */
>  	cros_ec_lpc_ops.read = cros_ec_lpc_mec_read_bytes;
>  	cros_ec_lpc_ops.write = cros_ec_lpc_mec_write_bytes;
> -	cros_ec_lpc_ops.read(EC_LPC_ADDR_MEMMAP + EC_MEMMAP_ID, 2, buf);
> -	if (buf[0] != 'E' || buf[1] != 'C') {
> +	ret = cros_ec_lpc_ops.read(EC_LPC_ADDR_MEMMAP + EC_MEMMAP_ID, 2, buf);
> +	if (ret < 0 || buf[0] != 'E' || buf[1] != 'C') {

Slight concern: if the read failed (-EBUSY, because of the lock contention
failed for example), does it still need to probe for non-MEC devices?

> @@ -436,9 +480,9 @@ static int cros_ec_lpc_probe(struct platform_device *pdev)
>  		/* Re-assign read/write operations for the non MEC variant */
>  		cros_ec_lpc_ops.read = cros_ec_lpc_read_bytes;
>  		cros_ec_lpc_ops.write = cros_ec_lpc_write_bytes;
> -		cros_ec_lpc_ops.read(ec_lpc->mmio_memory_base + EC_MEMMAP_ID, 2,
> -				     buf);
> -		if (buf[0] != 'E' || buf[1] != 'C') {
> +		ret = cros_ec_lpc_ops.read(ec_lpc->mmio_memory_base + EC_MEMMAP_ID, 2,
> +					   buf);
> +		if (ret < 0 || buf[0] != 'E' || buf[1] != 'C') {
>  			dev_err(dev, "EC ID not detected\n");
>  			return -ENODEV;

Similar concern here: should `ret < 0` see as a -ENODEV?

Re: [PATCH v2 1/5] platform/chrome: cros_ec_lpc: MEC access can return error code

[Ben Walsh]

Tzung-Bi Shih <tzungbi@kernel.org> writes:

> On Mon, Jun 03, 2024 at 07:38:30AM +0100, Ben Walsh wrote:
>> @@ -425,8 +469,8 @@ static int cros_ec_lpc_probe(struct platform_device *pdev)
>>  	 */
>>  	cros_ec_lpc_ops.read = cros_ec_lpc_mec_read_bytes;
>>  	cros_ec_lpc_ops.write = cros_ec_lpc_mec_write_bytes;
>> -	cros_ec_lpc_ops.read(EC_LPC_ADDR_MEMMAP + EC_MEMMAP_ID, 2, buf);
>> -	if (buf[0] != 'E' || buf[1] != 'C') {
>> +	ret = cros_ec_lpc_ops.read(EC_LPC_ADDR_MEMMAP + EC_MEMMAP_ID, 2, buf);
>> +	if (ret < 0 || buf[0] != 'E' || buf[1] != 'C') {
>
> Slight concern: if the read failed (-EBUSY, because of the lock contention
> failed for example), does it still need to probe for non-MEC devices?

That's a very good point! Negative ret here means there's really an
error, not just "no MEC".

I think it's better to return early with the return code (not -ENODEV)
in these cases.

>> @@ -436,9 +480,9 @@ static int cros_ec_lpc_probe(struct platform_device *pdev)
>>  		/* Re-assign read/write operations for the non MEC variant */
>>  		cros_ec_lpc_ops.read = cros_ec_lpc_read_bytes;
>>  		cros_ec_lpc_ops.write = cros_ec_lpc_write_bytes;
>> -		cros_ec_lpc_ops.read(ec_lpc->mmio_memory_base + EC_MEMMAP_ID, 2,
>> -				     buf);
>> -		if (buf[0] != 'E' || buf[1] != 'C') {
>> +		ret = cros_ec_lpc_ops.read(ec_lpc->mmio_memory_base + EC_MEMMAP_ID, 2,
>> +					   buf);
>> +		if (ret < 0 || buf[0] != 'E' || buf[1] != 'C') {
>>  			dev_err(dev, "EC ID not detected\n");
>>  			return -ENODEV;
>
> Similar concern here: should `ret < 0` see as a -ENODEV?

As above, I think it should "return ret".

[PATCH v2 2/5] platform/chrome: cros_ec_lpc: MEC access can use an AML mutex

[Ben Walsh]

Framework Laptops have ACPI code which accesses the MEC memory. It
uses an AML mutex to prevent concurrent access. But the cros_ec_lpc
driver was not aware of this mutex. The ACPI code and LPC driver both
attempted to talk to the EC at the same time, messing up communication
with the EC.

Allow the LPC driver MEC code to find and use the AML mutex.

Signed-off-by: Ben Walsh <ben@jubnut.com>
---
 drivers/platform/chrome/cros_ec_lpc_mec.c | 78 ++++++++++++++++++++++-
 drivers/platform/chrome/cros_ec_lpc_mec.h | 11 ++++
 2 files changed, 87 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/chrome/cros_ec_lpc_mec.c b/drivers/platform/chrome/cros_ec_lpc_mec.c
index 395dc3a6fb5e..fce1bdfb0a64 100644
--- a/drivers/platform/chrome/cros_ec_lpc_mec.c
+++ b/drivers/platform/chrome/cros_ec_lpc_mec.c
@@ -10,13 +10,67 @@
 
 #include "cros_ec_lpc_mec.h"
 
+#define ACPI_LOCK_DELAY_MS 500
+
 /*
  * This mutex must be held while accessing the EMI unit. We can't rely on the
  * EC mutex because memmap data may be accessed without it being held.
  */
 static DEFINE_MUTEX(io_mutex);
+/*
+ * An alternative mutex to be used when the ACPI AML code may also
+ * access memmap data.  When set, this mutex is used in preference to
+ * io_mutex.
+ */
+static acpi_handle aml_mutex;
+
 static u16 mec_emi_base, mec_emi_end;
 
+/**
+ * cros_ec_lpc_mec_lock() - Acquire mutex for EMI
+ *
+ * @return: Negative error code, or zero for success
+ */
+static int cros_ec_lpc_mec_lock(void)
+{
+	bool success;
+
+	if (!aml_mutex) {
+		mutex_lock(&io_mutex);
+		return 0;
+	}
+
+	success = ACPI_SUCCESS(acpi_acquire_mutex(aml_mutex,
+						  NULL, ACPI_LOCK_DELAY_MS));
+
+	if (!success)
+		return -EBUSY;
+
+	return 0;
+}
+
+/**
+ * cros_ec_lpc_mec_unlock() - Release mutex for EMI
+ *
+ * @return: Negative error code, or zero for success
+ */
+static int cros_ec_lpc_mec_unlock(void)
+{
+	bool success;
+
+	if (!aml_mutex) {
+		mutex_unlock(&io_mutex);
+		return 0;
+	}
+
+	success = ACPI_SUCCESS(acpi_release_mutex(aml_mutex, NULL));
+
+	if (!success)
+		return -EBUSY;
+
+	return 0;
+}
+
 /**
  * cros_ec_lpc_mec_emi_write_address() - Initialize EMI at a given address.
  *
@@ -78,6 +132,7 @@ int cros_ec_lpc_io_bytes_mec(enum cros_ec_lpc_mec_io_type io_type,
 	int io_addr;
 	u8 sum = 0;
 	enum cros_ec_lpc_mec_emi_access_mode access, new_access;
+	int ret;
 
 	/* Return checksum of 0 if window is not initialized */
 	WARN_ON(mec_emi_base == 0 || mec_emi_end == 0);
@@ -93,7 +148,9 @@ int cros_ec_lpc_io_bytes_mec(enum cros_ec_lpc_mec_io_type io_type,
 	else
 		access = ACCESS_TYPE_LONG_AUTO_INCREMENT;
 
-	mutex_lock(&io_mutex);
+	ret = cros_ec_lpc_mec_lock();
+	if (ret)
+		return ret;
 
 	/* Initialize I/O at desired address */
 	cros_ec_lpc_mec_emi_write_address(offset, access);
@@ -135,7 +192,9 @@ int cros_ec_lpc_io_bytes_mec(enum cros_ec_lpc_mec_io_type io_type,
 	}
 
 done:
-	mutex_unlock(&io_mutex);
+	ret = cros_ec_lpc_mec_unlock();
+	if (ret < 0)
+		return ret;
 
 	return sum;
 }
@@ -147,3 +206,18 @@ void cros_ec_lpc_mec_init(unsigned int base, unsigned int end)
 	mec_emi_end = end;
 }
 EXPORT_SYMBOL(cros_ec_lpc_mec_init);
+
+int cros_ec_lpc_mec_acpi_mutex(struct acpi_device *adev, const char *pathname)
+{
+	int status;
+
+	if (!adev)
+		return -ENOENT;
+
+	status = acpi_get_handle(adev->handle, pathname, &aml_mutex);
+	if (ACPI_FAILURE(status))
+		return -ENOENT;
+
+	return 0;
+}
+EXPORT_SYMBOL(cros_ec_lpc_mec_acpi_mutex);
diff --git a/drivers/platform/chrome/cros_ec_lpc_mec.h b/drivers/platform/chrome/cros_ec_lpc_mec.h
index 69670832f187..69f9d8786f61 100644
--- a/drivers/platform/chrome/cros_ec_lpc_mec.h
+++ b/drivers/platform/chrome/cros_ec_lpc_mec.h
@@ -8,6 +8,8 @@
 #ifndef __CROS_EC_LPC_MEC_H
 #define __CROS_EC_LPC_MEC_H
 
+#include <linux/acpi.h>
+
 enum cros_ec_lpc_mec_emi_access_mode {
 	/* 8-bit access */
 	ACCESS_TYPE_BYTE = 0x0,
@@ -45,6 +47,15 @@ enum cros_ec_lpc_mec_io_type {
  */
 void cros_ec_lpc_mec_init(unsigned int base, unsigned int end);
 
+/**
+ * cros_ec_lpc_mec_acpi_mutex() - Find and set ACPI mutex for MEC
+ *
+ * @adev:     Parent ACPI device
+ * @pathname: Name of AML mutex
+ * @return:   Negative error code, or zero for success
+ */
+int cros_ec_lpc_mec_acpi_mutex(struct acpi_device *adev, const char *pathname);
+
 /**
  * cros_ec_lpc_mec_in_range() - Determine if addresses are in MEC EMI range.
  *
-- 
2.45.1

Re: [PATCH v2 2/5] platform/chrome: cros_ec_lpc: MEC access can use an AML mutex

[Tzung-Bi Shih]

On Mon, Jun 03, 2024 at 07:38:31AM +0100, Ben Walsh wrote:
> +static int cros_ec_lpc_mec_lock(void)
> +{
> +	bool success;
> +
> +	if (!aml_mutex) {
> +		mutex_lock(&io_mutex);
> +		return 0;
> +	}
> +
> +	success = ACPI_SUCCESS(acpi_acquire_mutex(aml_mutex,
> +						  NULL, ACPI_LOCK_DELAY_MS));
> +

The blank line can be dropped.  It's up to you.

> +static int cros_ec_lpc_mec_unlock(void)
> +{
> +	bool success;
> +
> +	if (!aml_mutex) {
> +		mutex_unlock(&io_mutex);
> +		return 0;
> +	}
> +
> +	success = ACPI_SUCCESS(acpi_release_mutex(aml_mutex, NULL));
> +

Same here.

[PATCH v2 3/5] platform/chrome: cros_ec_lpc: Add a new quirk for ACPI id

[Ben Walsh]

Framework Laptops' ACPI exposes the EC with id "PNP0C09". But
"PNP0C09" is part of the ACPI standard; there are lots of computers
with EC chips with this id, and most of them don't support the cros_ec
protocol.

The driver could find the ACPI device by having "PNP0C09" in the
acpi_match_table, but this would match devices which don't support the
cros_ec protocol. Instead, add a new quirk "CROS_EC_LPC_QUIRK_ACPI_ID"
which allows the id to be specified. This quirk is applied after the
DMI check shows that the device is supported.

Signed-off-by: Ben Walsh <ben@jubnut.com>
---
 drivers/platform/chrome/cros_ec_lpc.c | 53 ++++++++++++++++++++-------
 1 file changed, 40 insertions(+), 13 deletions(-)

diff --git a/drivers/platform/chrome/cros_ec_lpc.c b/drivers/platform/chrome/cros_ec_lpc.c
index 6663141a07d4..c1c26a7ba6d4 100644
--- a/drivers/platform/chrome/cros_ec_lpc.c
+++ b/drivers/platform/chrome/cros_ec_lpc.c
@@ -39,6 +39,11 @@ static bool cros_ec_lpc_acpi_device_found;
  * be used as the base port for EC mapped memory.
  */
 #define CROS_EC_LPC_QUIRK_REMAP_MEMORY              BIT(0)
+/*
+ * Indicates that lpc_driver_data.quirk_acpi_id should be used to find
+ * the ACPI device.
+ */
+#define CROS_EC_LPC_QUIRK_ACPI_ID                   BIT(1)
 
 /**
  * struct lpc_driver_data - driver data attached to a DMI device ID to indicate
@@ -46,10 +51,12 @@ static bool cros_ec_lpc_acpi_device_found;
  * @quirks: a bitfield composed of quirks from CROS_EC_LPC_QUIRK_*
  * @quirk_mmio_memory_base: The first I/O port addressing EC mapped memory (used
  *                          when quirk ...REMAP_MEMORY is set.)
+ * @quirk_acpi_id: An ACPI HID to be used to find the ACPI device.
  */
 struct lpc_driver_data {
 	u32 quirks;
 	u16 quirk_mmio_memory_base;
+	const char *quirk_acpi_id;
 };
 
 /**
@@ -418,6 +425,26 @@ static void cros_ec_lpc_acpi_notify(acpi_handle device, u32 value, void *data)
 		pm_system_wakeup();
 }
 
+static acpi_status cros_ec_lpc_parse_device(acpi_handle handle, u32 level,
+					    void *context, void **retval)
+{
+	*(struct acpi_device **)context = acpi_fetch_acpi_dev(handle);
+	return AE_CTRL_TERMINATE;
+}
+
+static struct acpi_device *cros_ec_lpc_get_device(const char *id)
+{
+	struct acpi_device *adev = NULL;
+	acpi_status status = acpi_get_devices(id, cros_ec_lpc_parse_device,
+					      &adev, NULL);
+	if (ACPI_FAILURE(status)) {
+		pr_warn(DRV_NAME ": Looking for %s failed\n", id);
+		return NULL;
+	}
+
+	return adev;
+}
+
 static int cros_ec_lpc_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -436,6 +463,8 @@ static int cros_ec_lpc_probe(struct platform_device *pdev)
 
 	ec_lpc->mmio_memory_base = EC_LPC_ADDR_MEMMAP;
 
+	adev = ACPI_COMPANION(dev);
+
 	driver_data = platform_get_drvdata(pdev);
 	if (driver_data) {
 		quirks = driver_data->quirks;
@@ -445,6 +474,16 @@ static int cros_ec_lpc_probe(struct platform_device *pdev)
 
 		if (quirks & CROS_EC_LPC_QUIRK_REMAP_MEMORY)
 			ec_lpc->mmio_memory_base = driver_data->quirk_mmio_memory_base;
+
+		if (quirks & CROS_EC_LPC_QUIRK_ACPI_ID) {
+			adev = cros_ec_lpc_get_device(driver_data->quirk_acpi_id);
+			if (!adev) {
+				dev_err(dev, "failed to get ACPI device '%s'",
+					driver_data->quirk_acpi_id);
+				return -ENODEV;
+			}
+			ACPI_COMPANION_SET(dev, adev);
+		}
 	}
 
 	/*
@@ -538,7 +577,6 @@ static int cros_ec_lpc_probe(struct platform_device *pdev)
 	 * Connect a notify handler to process MKBP messages if we have a
 	 * companion ACPI device.
 	 */
-	adev = ACPI_COMPANION(dev);
 	if (adev) {
 		status = acpi_install_notify_handler(adev->handle,
 						     ACPI_ALL_NOTIFY,
@@ -705,23 +743,12 @@ static struct platform_device cros_ec_lpc_device = {
 	.name = DRV_NAME
 };
 
-static acpi_status cros_ec_lpc_parse_device(acpi_handle handle, u32 level,
-					    void *context, void **retval)
-{
-	*(bool *)context = true;
-	return AE_CTRL_TERMINATE;
-}
-
 static int __init cros_ec_lpc_init(void)
 {
 	int ret;
-	acpi_status status;
 	const struct dmi_system_id *dmi_match;
 
-	status = acpi_get_devices(ACPI_DRV_NAME, cros_ec_lpc_parse_device,
-				  &cros_ec_lpc_acpi_device_found, NULL);
-	if (ACPI_FAILURE(status))
-		pr_warn(DRV_NAME ": Looking for %s failed\n", ACPI_DRV_NAME);
+	cros_ec_lpc_acpi_device_found = !!cros_ec_lpc_get_device(ACPI_DRV_NAME);
 
 	dmi_match = dmi_first_match(cros_ec_lpc_dmi_table);
 
-- 
2.45.1

Re: [PATCH v2 3/5] platform/chrome: cros_ec_lpc: Add a new quirk for ACPI id

[Tzung-Bi Shih]

On Mon, Jun 03, 2024 at 07:38:32AM +0100, Ben Walsh wrote:
> @@ -436,6 +463,8 @@ static int cros_ec_lpc_probe(struct platform_device *pdev)
>  
>  	ec_lpc->mmio_memory_base = EC_LPC_ADDR_MEMMAP;
>  
> +	adev = ACPI_COMPANION(dev);
> +

The change is irrelevant to the patch.

> @@ -538,7 +577,6 @@ static int cros_ec_lpc_probe(struct platform_device *pdev)
>  	 * Connect a notify handler to process MKBP messages if we have a
>  	 * companion ACPI device.
>  	 */
> -	adev = ACPI_COMPANION(dev);
>  	if (adev) {
>  		status = acpi_install_notify_handler(adev->handle,
>  						     ACPI_ALL_NOTIFY,

See above comment.

Re: [PATCH v2 3/5] platform/chrome: cros_ec_lpc: Add a new quirk for ACPI id

[Mario Limonciello]

On 6/3/2024 04:30, Tzung-Bi Shih wrote:
> On Mon, Jun 03, 2024 at 07:38:32AM +0100, Ben Walsh wrote:
>> @@ -436,6 +463,8 @@ static int cros_ec_lpc_probe(struct platform_device *pdev)
>>   
>>   	ec_lpc->mmio_memory_base = EC_LPC_ADDR_MEMMAP;
>>   
>> +	adev = ACPI_COMPANION(dev);
>> +
> 
> The change is irrelevant to the patch.

It looks relevant to me.  The companion needs to get set before the 
quirk overwrites it.

> 
>> @@ -538,7 +577,6 @@ static int cros_ec_lpc_probe(struct platform_device *pdev)
>>   	 * Connect a notify handler to process MKBP messages if we have a
>>   	 * companion ACPI device.
>>   	 */
>> -	adev = ACPI_COMPANION(dev);
>>   	if (adev) {
>>   		status = acpi_install_notify_handler(adev->handle,
>>   						     ACPI_ALL_NOTIFY,
> 
> See above comment.

Re: [PATCH v2 3/5] platform/chrome: cros_ec_lpc: Add a new quirk for ACPI id

[Ben Walsh]

Mario Limonciello <mario.limonciello@amd.com> writes:

> On 6/3/2024 04:30, Tzung-Bi Shih wrote:
>> On Mon, Jun 03, 2024 at 07:38:32AM +0100, Ben Walsh wrote:
>>> @@ -436,6 +463,8 @@ static int cros_ec_lpc_probe(struct platform_device *pdev)
>>>   
>>>   	ec_lpc->mmio_memory_base = EC_LPC_ADDR_MEMMAP;
>>>   
>>> +	adev = ACPI_COMPANION(dev);
>>> +
>> 
>> The change is irrelevant to the patch.
>
> It looks relevant to me.  The companion needs to get set before the 
> quirk overwrites it.

That was my thinking. You can either update "adev" and use it to refer
to the ACPI companion device, or (as Tzung-Bi suggests) use
"ACPI_COMPANION" / "ACPI_COMPANION_SET" as needed.

I really don't mind either way. I'm happy to change it if necessary.

Re: [PATCH v2 3/5] platform/chrome: cros_ec_lpc: Add a new quirk for ACPI id

[Tzung-Bi Shih]

On Mon, Jun 03, 2024 at 11:00:22AM -0500, Mario Limonciello wrote:
> On 6/3/2024 04:30, Tzung-Bi Shih wrote:
> > On Mon, Jun 03, 2024 at 07:38:32AM +0100, Ben Walsh wrote:
> > > @@ -436,6 +463,8 @@ static int cros_ec_lpc_probe(struct platform_device *pdev)
> > >   	ec_lpc->mmio_memory_base = EC_LPC_ADDR_MEMMAP;
> > > +	adev = ACPI_COMPANION(dev);
> > > +
> > 
> > The change is irrelevant to the patch.
> 
> It looks relevant to me.  The companion needs to get set before the quirk
> overwrites it.

Please see code for CROS_EC_LPC_QUIRK_ACPI_ID in the patch, `adev` is going to
be overridden soon.  I don't see why the ACPI_COMPANION() needs to move here.

[PATCH v2 4/5] platform/chrome: cros_ec_lpc: Add a new quirk for AML mutex

[Ben Walsh]

Add a new quirk "CROS_EC_LPC_QUIRK_AML_MUTEX" which provides the name
of an AML mutex to protect MEC memory access.

Signed-off-by: Ben Walsh <ben@jubnut.com>
---
 drivers/platform/chrome/cros_ec_lpc.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/platform/chrome/cros_ec_lpc.c b/drivers/platform/chrome/cros_ec_lpc.c
index c1c26a7ba6d4..60627c5a596b 100644
--- a/drivers/platform/chrome/cros_ec_lpc.c
+++ b/drivers/platform/chrome/cros_ec_lpc.c
@@ -44,6 +44,11 @@ static bool cros_ec_lpc_acpi_device_found;
  * the ACPI device.
  */
 #define CROS_EC_LPC_QUIRK_ACPI_ID                   BIT(1)
+/*
+ * Indicates that lpc_driver_data.quirk_aml_mutex_name should be used
+ * to find an AML mutex to protect access to Microchip EC.
+ */
+#define CROS_EC_LPC_QUIRK_AML_MUTEX                 BIT(2)
 
 /**
  * struct lpc_driver_data - driver data attached to a DMI device ID to indicate
@@ -52,11 +57,14 @@ static bool cros_ec_lpc_acpi_device_found;
  * @quirk_mmio_memory_base: The first I/O port addressing EC mapped memory (used
  *                          when quirk ...REMAP_MEMORY is set.)
  * @quirk_acpi_id: An ACPI HID to be used to find the ACPI device.
+ * @quirk_aml_mutex_name: The name of an AML mutex to be used to protect access
+ *                        to Microchip EC.
  */
 struct lpc_driver_data {
 	u32 quirks;
 	u16 quirk_mmio_memory_base;
 	const char *quirk_acpi_id;
+	const char *quirk_aml_mutex_name;
 };
 
 /**
@@ -484,6 +492,18 @@ static int cros_ec_lpc_probe(struct platform_device *pdev)
 			}
 			ACPI_COMPANION_SET(dev, adev);
 		}
+
+		if (quirks & CROS_EC_LPC_QUIRK_AML_MUTEX) {
+			const char *name
+				= driver_data->quirk_aml_mutex_name;
+			ret = cros_ec_lpc_mec_acpi_mutex(adev, name);
+			if (ret) {
+				dev_err(dev, "failed to get AML mutex '%s'", name);
+				return ret;
+			}
+
+			dev_info(dev, "got AML mutex '%s'", name);
+		}
 	}
 
 	/*
-- 
2.45.1

Re: [PATCH v2 4/5] platform/chrome: cros_ec_lpc: Add a new quirk for AML mutex

[Tzung-Bi Shih]

On Mon, Jun 03, 2024 at 07:38:33AM +0100, Ben Walsh wrote:
> @@ -484,6 +492,18 @@ static int cros_ec_lpc_probe(struct platform_device *pdev)
>  			}
>  			ACPI_COMPANION_SET(dev, adev);
>  		}
> +
> +		if (quirks & CROS_EC_LPC_QUIRK_AML_MUTEX) {
> +			const char *name
> +				= driver_data->quirk_aml_mutex_name;
> +			ret = cros_ec_lpc_mec_acpi_mutex(adev, name);

* Assuming the `adev = ACPI_COMPANION(dev);` didn't move in previous patch,
  `adev` here could be uninitialized.

* Also, it shouldn't rely on CROS_EC_LPC_QUIRK_ACPI_ID to set the `adev`.

To keep it simple, I would suggest to use ACPI_COMPANION() instead.

[PATCH v2 5/5] platform/chrome: cros_ec_lpc: Add quirks for Framework Laptop

[Ben Walsh]

For Framework Laptops with Microchip EC (MEC), use the ACPI id
"PNP0C09" to find the ACPI device, and AML mutex "ECMT" to protect EC
memory access.

Signed-off-by: Ben Walsh <ben@jubnut.com>
---
 drivers/platform/chrome/cros_ec_lpc.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/platform/chrome/cros_ec_lpc.c b/drivers/platform/chrome/cros_ec_lpc.c
index 60627c5a596b..bebf76404512 100644
--- a/drivers/platform/chrome/cros_ec_lpc.c
+++ b/drivers/platform/chrome/cros_ec_lpc.c
@@ -634,6 +634,12 @@ static const struct lpc_driver_data framework_laptop_amd_lpc_driver_data __initc
 	.quirk_mmio_memory_base = 0xE00,
 };
 
+static const struct lpc_driver_data framework_laptop_11_lpc_driver_data __initconst = {
+	.quirks = CROS_EC_LPC_QUIRK_ACPI_ID|CROS_EC_LPC_QUIRK_AML_MUTEX,
+	.quirk_acpi_id = "PNP0C09",
+	.quirk_aml_mutex_name = "ECMT",
+};
+
 static const struct dmi_system_id cros_ec_lpc_dmi_table[] __initconst = {
 	{
 		/*
@@ -702,6 +708,7 @@ static const struct dmi_system_id cros_ec_lpc_dmi_table[] __initconst = {
 			DMI_MATCH(DMI_SYS_VENDOR, "Framework"),
 			DMI_MATCH(DMI_PRODUCT_NAME, "Laptop"),
 		},
+		.driver_data = (void *)&framework_laptop_11_lpc_driver_data,
 	},
 	{ /* sentinel */ }
 };
-- 
2.45.1

Re: [PATCH v2 0/5] platform/chrome: Fix MEC concurrency problems for Framework Laptop

[Dustin Howett]

On Mon, Jun 3, 2024 at 1:38 AM Ben Walsh <ben@jubnut.com> wrote:
>
> It has _not_ been tested on any Chromebook devices.

Good news!

I've tested this patch series on the following devices:

- Chromebook Pixel 2013 ("link")
- Framework Laptop 11th Gen Intel Core ("hx20"), firmware revision 3.19
- Framework Laptop AMD Ryzen 7040 Series ("azalea"), firmware revision 3.05

It works as expected.
There is no detectable difference at runtime on Link, nor does the
driver appear to improperly bind any ACPI devices.
I cannot reproduce bus contention on hx20, even performing operations
that would consistently fail without this patch (e.g. long flash
reads).

Therefore, for the series:

Tested-on: link, hx20, azalea
Tested-by: Dustin L. Howett <dustin@howett.net>

Re: [PATCH v2 0/5] platform/chrome: Fix MEC concurrency problems for Framework Laptop

[Ben Walsh]

Dustin Howett <dustin@howett.net> writes:

> I've tested this patch series on the following devices:
>
> - Chromebook Pixel 2013 ("link")
> - Framework Laptop 11th Gen Intel Core ("hx20"), firmware revision 3.19
> - Framework Laptop AMD Ryzen 7040 Series ("azalea"), firmware revision 3.05
>
> It works as expected.

Very cool! Thanks for looking at this, Dustin.