[PATCH] platform/chrome: Add ChromeOS EC USB driver

[PATCH] platform/chrome: Add ChromeOS EC USB driver

[Dawid Niedzwiecki]

This uses USB to talk to the ChromeOS EC. The protocol
is defined by the EC and is fairly simple, with a length byte,
checksum, command byte and version byte in the header.

The driver uses vendor defined usb interface with in/out
endpoints to transfer requests and responses. The driver also
uses one interrupt in endpoint which signals readiness of response
and pending events on the EC side.

Signed-off-by: Dawid Niedzwiecki <dawidn@google.com>
---
 drivers/platform/chrome/Kconfig       |  11 +
 drivers/platform/chrome/Makefile      |   1 +
 drivers/platform/chrome/cros_ec_usb.c | 621 ++++++++++++++++++++++++++
 3 files changed, 633 insertions(+)
 create mode 100644 drivers/platform/chrome/cros_ec_usb.c

diff --git a/drivers/platform/chrome/Kconfig b/drivers/platform/chrome/Kconfig
index 10941ac37305..07f50816e333 100644
--- a/drivers/platform/chrome/Kconfig
+++ b/drivers/platform/chrome/Kconfig
@@ -316,6 +316,17 @@ config CROS_TYPEC_SWITCH
 	  To compile this driver as a module, choose M here: the module will be
 	  called cros_typec_switch.
 
+config CROS_EC_USB
+	tristate "ChromeOS Embedded Controller (USB)"
+	depends on CROS_EC && USB
+	help
+	  If you say Y here, you get support for talking to the ChromeOS EC
+	  through a USB. The driver uses vendor defined interface and is capable
+	  of signaling events from EC.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called cros_ec_usb.
+
 source "drivers/platform/chrome/wilco_ec/Kconfig"
 
 # Kunit test cases
diff --git a/drivers/platform/chrome/Makefile b/drivers/platform/chrome/Makefile
index b981a1bb5bd8..444383e8912d 100644
--- a/drivers/platform/chrome/Makefile
+++ b/drivers/platform/chrome/Makefile
@@ -38,6 +38,7 @@ obj-$(CONFIG_CROS_EC_SYSFS)		+= cros_ec_sysfs.o
 obj-$(CONFIG_CROS_HPS_I2C)		+= cros_hps_i2c.o
 obj-$(CONFIG_CROS_USBPD_LOGGER)		+= cros_usbpd_logger.o
 obj-$(CONFIG_CROS_USBPD_NOTIFY)		+= cros_usbpd_notify.o
+obj-$(CONFIG_CROS_EC_USB)		+= cros_ec_usb.o
 
 obj-$(CONFIG_WILCO_EC)			+= wilco_ec/
 
diff --git a/drivers/platform/chrome/cros_ec_usb.c b/drivers/platform/chrome/cros_ec_usb.c
new file mode 100644
index 000000000000..1f77c0c94936
--- /dev/null
+++ b/drivers/platform/chrome/cros_ec_usb.c
@@ -0,0 +1,621 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * USB interface for ChromeOS Embedded Controller
+ *
+ * Copyright (C) 2025 Google LLC.
+ */
+
+#include <linux/errno.h>
+#include <linux/kernel.h>
+#include <linux/kref.h>
+#include <linux/list.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/uaccess.h>
+#include <linux/usb.h>
+
+#include <linux/platform_data/cros_ec_commands.h>
+#include <linux/platform_data/cros_ec_proto.h>
+#include <linux/platform_device.h>
+
+#include "cros_ec.h"
+
+/* Google */
+#define USB_VENDOR_ID_GOOGLE 0x18d1
+
+#define USB_SUBCLASS_GOOGLE_EC_HOST_CMD 0x5a
+#define USB_PROTOCOL_GOOGLE_EC_HOST_CMD 0x00
+
+#define RESPONSE_TIMEOUT_MS 200
+#define BULK_TRANSFER_TIMEOUT_MS 100
+
+enum cros_ec_usb_int_type {
+	INT_TYPE_EVENT_OCCURED = 0,
+	INT_TYPE_RESPONSE_READY = 1,
+};
+
+/* table of devices that work with this driver */
+static const struct usb_device_id cros_ec_usb_table[] = {
+	{ USB_VENDOR_AND_INTERFACE_INFO(USB_VENDOR_ID_GOOGLE,
+					USB_CLASS_VENDOR_SPEC,
+					USB_SUBCLASS_GOOGLE_EC_HOST_CMD,
+					USB_PROTOCOL_GOOGLE_EC_HOST_CMD) },
+	{} /* Terminating entry */
+};
+MODULE_DEVICE_TABLE(usb, cros_ec_usb_table);
+
+/* Structure to hold all of our device specific stuff */
+struct cros_ec_usb {
+	/* the usb device for this device */
+	struct usb_device *udev;
+	/* the interface for this device */
+	struct usb_interface *interface;
+	/* Cros EC device structure */
+	struct cros_ec_device *ec_dev;
+	/* the buffer to receive data from bulk ep */
+	u8 *bulk_in_buffer;
+	/* the buffer to receive data from int ep */
+	u8 *int_in_buffer;
+	/* the urb to receive data from int ep */
+	struct urb *int_in_urb;
+	/* the size of the receive buffer from bulk ep */
+	size_t bulk_in_size;
+	/* the size of the receive buffer from int ep */
+	size_t int_in_size;
+	/* the pipe of the bulk in ep */
+	unsigned int bulk_in_pipe;
+	/* the pipe of the bulk out ep */
+	unsigned int bulk_out_pipe;
+	/* the pipe of the int in ep */
+	unsigned int int_in_pipe;
+	/* the interval of the int in ep */
+	uint8_t int_in_interval;
+	/* Response ready on EC side */
+	bool resp_ready;
+	/* EC has been registered */
+	bool registered;
+	/* EC is disconnected */
+	bool disconnected;
+	/* synchronize I/O with disconnect */
+	struct mutex io_mutex;
+	/* Work to handle EC events */
+	struct work_struct work_ec_evt;
+	/* Wait queue to signal the response is ready on EC side */
+	wait_queue_head_t resp_ready_wait;
+};
+
+struct int_msg {
+	uint8_t int_type;
+} __packed;
+
+struct registered_ec {
+	struct list_head node;
+	u16 idProduct;
+	struct cros_ec_usb *ec_usb;
+};
+
+static LIST_HEAD(registered_list);
+static DEFINE_MUTEX(registered_list_mutex);
+static void cros_ec_int_callback(struct urb *urb);
+
+static int expected_response_size(const struct ec_host_response *host_response)
+{
+	/* Check host request version */
+	if (host_response->struct_version != 3)
+		return 0;
+
+	/* Reserved byte should be 0 */
+	if (host_response->reserved)
+		return 0;
+
+	return sizeof(*host_response) + host_response->data_len;
+}
+
+static int cros_ec_usb_register(u16 idProduct, struct cros_ec_usb *ec_usb)
+{
+	struct registered_ec *ec;
+
+	ec = kzalloc(sizeof(*ec), GFP_KERNEL);
+	if (!ec)
+		return -ENOMEM;
+
+	ec->ec_usb = ec_usb;
+	ec->idProduct = idProduct;
+	mutex_lock(&registered_list_mutex);
+	list_add(&ec->node, &registered_list);
+	mutex_unlock(&registered_list_mutex);
+
+	return 0;
+}
+
+static struct cros_ec_usb *cros_ec_usb_get_registered(u16 idProduct)
+{
+	struct registered_ec *ec;
+	struct cros_ec_usb *ret = NULL;
+
+	mutex_lock(&registered_list_mutex);
+	list_for_each_entry(ec, &registered_list, node) {
+		if (ec->idProduct == idProduct) {
+			ret = ec->ec_usb;
+			break;
+		}
+	}
+	mutex_unlock(&registered_list_mutex);
+	return ret;
+}
+
+static int submit_int_urb(struct cros_ec_device *ec_dev)
+{
+	struct cros_ec_usb *ec_usb = ec_dev->priv;
+	struct usb_device *usb_dev = interface_to_usbdev(ec_usb->interface);
+	int ret;
+
+	/* Submit the INT URB. */
+	usb_fill_int_urb(ec_usb->int_in_urb, usb_dev, ec_usb->int_in_pipe,
+			 ec_usb->int_in_buffer, ec_usb->int_in_size,
+			 cros_ec_int_callback, ec_usb, ec_usb->int_in_interval);
+	ret = usb_submit_urb(ec_usb->int_in_urb, GFP_KERNEL);
+
+	return ret;
+}
+
+static void cros_ec_int_callback(struct urb *urb)
+{
+	struct cros_ec_usb *ec_usb = urb->context;
+	struct cros_ec_device *ec_dev = ec_usb->ec_dev;
+	int ret;
+
+	switch (urb->status) {
+	case 0:
+		break;
+	case -ECONNRESET:
+	case -ENOENT:
+	case -ESHUTDOWN:
+		/* Expected errors. */
+		return;
+	default:
+		dev_dbg(ec_dev->dev, "Unexpected int urb error: %d\n",
+			urb->status);
+		goto resubmit;
+	}
+
+	if (urb->actual_length >= sizeof(struct int_msg)) {
+		struct int_msg *int_msg =
+			(struct int_msg *)ec_usb->int_in_buffer;
+		enum cros_ec_usb_int_type int_type =
+			(enum cros_ec_usb_int_type)int_msg->int_type;
+		switch (int_type) {
+		case INT_TYPE_EVENT_OCCURED:
+			if (ec_usb->registered) {
+				ec_dev->last_event_time = cros_ec_get_time_ns();
+				schedule_work(&ec_usb->work_ec_evt);
+			}
+			break;
+		case INT_TYPE_RESPONSE_READY:
+			ec_usb->resp_ready = true;
+			wake_up(&ec_usb->resp_ready_wait);
+			break;
+		default:
+			dev_err(ec_dev->dev, "Unrecognized event: %d\n",
+				int_type);
+		}
+	} else {
+		dev_err(ec_dev->dev, "Incorrect int transfer len: %d\n",
+			urb->actual_length);
+	}
+
+resubmit:
+	/* Resubmit the INT URB. */
+	ret = submit_int_urb(ec_dev);
+	if (ret)
+		dev_err(ec_dev->dev, "Failed to resumbit int urb: %d", ret);
+}
+
+static int do_cros_ec_pkt_xfer_usb(struct cros_ec_device *ec_dev,
+				   struct cros_ec_command *ec_msg)
+{
+	struct cros_ec_usb *ec_usb = ec_dev->priv;
+	struct ec_host_response *host_response;
+	int req_size, ret, actual_length, expected_resp_size, resp_size;
+	const int header_size = sizeof(*host_response);
+	const int max_resp_size = header_size + ec_msg->insize;
+	const int bulk_in_size = umin(ec_usb->bulk_in_size, ec_dev->din_size);
+	uint8_t sum = 0;
+
+	mutex_lock(&ec_usb->io_mutex);
+	if (ec_usb->disconnected) {
+		mutex_unlock(&ec_usb->io_mutex);
+		ret = -ENODEV;
+		return ret;
+	}
+
+	if (max_resp_size > ec_dev->din_size) {
+		dev_err(ec_dev->dev, "Potential response too big: %d\n", max_resp_size);
+		ret = -EINVAL;
+		goto exit;
+	}
+
+	req_size = cros_ec_prepare_tx(ec_dev, ec_msg);
+	if (req_size < 0) {
+		dev_err(ec_dev->dev, "Failed to prepare msg %d\n", req_size);
+		ret = req_size;
+		goto exit;
+	}
+	dev_dbg(ec_dev->dev, "Prepared len=%d\n", req_size);
+
+	ec_usb->resp_ready = false;
+	/*
+	 * Buffers dout and din are allocated with devm_kzalloc which means it is suitable
+	 * for DMA and we can use by usb functions.
+	 */
+	ret = usb_bulk_msg(ec_usb->udev, ec_usb->bulk_out_pipe, ec_dev->dout,
+			   req_size, NULL, BULK_TRANSFER_TIMEOUT_MS);
+	if (ret) {
+		dev_err(ec_dev->dev, "Failed to send request: %d\n", ret);
+		goto exit;
+	}
+
+	/*
+	 * Wait till EC signals response ready event via INT endpoint,
+	 * before polling a response with a bulk transfer.
+	 */
+	if (!wait_event_timeout(ec_usb->resp_ready_wait, ec_usb->resp_ready,
+				msecs_to_jiffies(RESPONSE_TIMEOUT_MS))) {
+		dev_err(ec_dev->dev, "Timed out waiting for response\n");
+		ret = -ETIMEDOUT;
+		goto exit;
+	}
+
+	/* Get first part of response that contains a header. */
+	ret = usb_bulk_msg(ec_usb->udev, ec_usb->bulk_in_pipe, ec_dev->din,
+			   bulk_in_size, &actual_length,
+			   BULK_TRANSFER_TIMEOUT_MS);
+	if (ret) {
+		dev_err(ec_dev->dev, "Failed to get response: %d\n", ret);
+		goto exit;
+	}
+
+	/* Verify number of received bytes. */
+	if (actual_length < header_size) {
+		dev_err(ec_dev->dev, "Received too little bytes: %d\n",
+			actual_length);
+		ret = -ENOSPC;
+		goto exit;
+	}
+	expected_resp_size =
+		expected_response_size((struct ec_host_response *)ec_dev->din);
+	if ((expected_resp_size > max_resp_size) || (expected_resp_size == 0) ||
+	    (actual_length > expected_resp_size)) {
+		dev_err(ec_dev->dev, "Incorrect number of expected bytes: %d\n",
+			expected_resp_size);
+		ret = -ENOSPC;
+		goto exit;
+	}
+	resp_size = actual_length;
+	/* Get the rest of the response if needed. */
+	if (resp_size < expected_resp_size) {
+		ret = usb_bulk_msg(ec_usb->udev, ec_usb->bulk_in_pipe,
+				   ec_dev->din + resp_size,
+				   expected_resp_size - resp_size,
+				   &actual_length, BULK_TRANSFER_TIMEOUT_MS);
+		if (ret) {
+			dev_err(ec_dev->dev,
+				"Failed to get second part of response: %d\n",
+				ret);
+			goto exit;
+		}
+		resp_size += actual_length;
+	}
+
+	/* Check if number of received of bytes is correct. */
+	if (resp_size != expected_resp_size) {
+		dev_err(ec_dev->dev,
+			"Received incorrect number of bytes: %d, expected: %d\n",
+			resp_size, expected_resp_size);
+		ret = -ENOSPC;
+		goto exit;
+	}
+
+	/* Validate checksum */
+	host_response = (struct ec_host_response *)ec_dev->din;
+	for (int i = 0; i < sizeof(*host_response) + host_response->data_len;
+	     i++) {
+		sum += ec_dev->din[i];
+	}
+	if (sum) {
+		dev_err(ec_dev->dev, "Bad packet checksum calculated %x\n",
+			sum);
+		ret = -EBADMSG;
+		goto exit;
+	}
+
+	ec_msg->result = host_response->result;
+	memcpy(ec_msg->data, ec_dev->din + sizeof(*host_response),
+	       host_response->data_len);
+	ret = host_response->data_len;
+
+	if (ec_msg->command == EC_CMD_REBOOT_EC)
+		msleep(EC_REBOOT_DELAY_MS);
+exit:
+	mutex_unlock(&ec_usb->io_mutex);
+	if (ret < 0) {
+		/* Try to reset EC in case of error to restore default state. */
+		usb_reset_device(ec_usb->udev);
+	}
+
+	return ret;
+}
+
+/**
+ * usb_evt_handler - USB to AP event handler
+ * @work: Work struct
+ */
+static void usb_evt_handler(struct work_struct *work)
+{
+	struct cros_ec_usb *ec_usb =
+		container_of(work, struct cros_ec_usb, work_ec_evt);
+
+	cros_ec_irq_thread(0, ec_usb->ec_dev);
+}
+
+static void cros_ec_usb_delete(struct cros_ec_usb *ec_usb)
+{
+	usb_kill_urb(ec_usb->int_in_urb);
+	cancel_work_sync(&ec_usb->work_ec_evt);
+
+	usb_free_urb(ec_usb->int_in_urb);
+	usb_put_intf(ec_usb->interface);
+	usb_put_dev(ec_usb->udev);
+	kfree(ec_usb->int_in_buffer);
+	kfree(ec_usb->bulk_in_buffer);
+}
+
+static int cros_ec_usb_probe(struct usb_interface *intf,
+			     const struct usb_device_id *id)
+{
+	struct usb_device *usb_dev = interface_to_usbdev(intf);
+	struct usb_endpoint_descriptor *bulk_in, *bulk_out, *int_in;
+	struct device *if_dev = &intf->dev;
+	struct cros_ec_device *ec_dev;
+	const u16 idProduct = usb_dev->descriptor.idProduct;
+	struct cros_ec_usb *ec_usb = cros_ec_usb_get_registered(idProduct);
+	const bool is_registered = ec_usb != NULL;
+	int retval;
+
+	/*
+	 * Do not register the same EC device twice. The probing is performed every
+	 * reboot, sysjump, crash etc. Recreating the /dev/cros_X file every time
+	 * would force all application to reopen the file, which is not a case for
+	 * other cros_ec_x divers. Instead, keep the cros_ec_device and cros_ec_usb
+	 * structures constant and replace USB related structures for the same EC
+	 * that is reprobed.
+	 *
+	 * The driver doesn't support handling two devices with the same idProduct,
+	 * but it will never be a real usecase.
+	 */
+	if (!is_registered)
+		ec_usb = kzalloc(sizeof(*ec_usb), GFP_KERNEL);
+
+	if (!ec_usb)
+		return -ENOMEM;
+
+	if (!is_registered) {
+		mutex_init(&ec_usb->io_mutex);
+		ec_dev = kzalloc(sizeof(*ec_dev), GFP_KERNEL);
+		if (!ec_dev) {
+			retval = -ENOMEM;
+			goto error;
+		}
+		ec_usb->ec_dev = ec_dev;
+	} else {
+		ec_dev = ec_usb->ec_dev;
+	}
+
+	ec_usb->udev = usb_get_dev(usb_dev);
+	ec_usb->interface = usb_get_intf(intf);
+	/* Use first bulk-in/out endpoints + int-in endpoint */
+	retval = usb_find_common_endpoints(intf->cur_altsetting, &bulk_in,
+					   &bulk_out, &int_in, NULL);
+	if (retval) {
+		dev_err(if_dev,
+			"Could not find bulk-in, bulk-out or int-in endpoint\n");
+		goto error;
+	}
+	/* Bulk endpoints have to be capable of sending headers in one transfer. */
+	if ((usb_endpoint_maxp(bulk_out) < sizeof(struct ec_host_request)) ||
+	    (usb_endpoint_maxp(bulk_in) < sizeof(struct ec_host_response)) ||
+	    (usb_endpoint_maxp(int_in)) < sizeof(struct int_msg)) {
+		retval = -ENOSPC;
+		dev_err(if_dev, "Incorrect max packet size\n");
+		goto error;
+	}
+	ec_usb->bulk_out_pipe =
+		usb_sndbulkpipe(ec_usb->udev, bulk_out->bEndpointAddress);
+	ec_usb->bulk_in_size = usb_endpoint_maxp(bulk_in);
+	ec_usb->bulk_in_pipe =
+		usb_rcvbulkpipe(ec_usb->udev, bulk_in->bEndpointAddress);
+	ec_usb->bulk_in_buffer = kmalloc(ec_usb->bulk_in_size, GFP_KERNEL);
+	if (!ec_usb->bulk_in_buffer) {
+		dev_err(if_dev, "Failed to allocate bulk in buffer\n");
+		retval = -ENOMEM;
+		goto error;
+	}
+	ec_usb->int_in_size = usb_endpoint_maxp(int_in);
+	ec_usb->int_in_pipe =
+		usb_rcvintpipe(ec_usb->udev, int_in->bEndpointAddress);
+	ec_usb->int_in_interval = int_in->bInterval;
+	ec_usb->int_in_buffer = kmalloc(ec_usb->int_in_size, GFP_KERNEL);
+	if (!ec_usb->int_in_buffer) {
+		dev_err(if_dev, "Failed to allocate int in buffer\n");
+		retval = -ENOMEM;
+		goto error;
+	}
+	ec_usb->int_in_urb = usb_alloc_urb(0, GFP_KERNEL);
+	if (!ec_usb->int_in_urb) {
+		dev_err(if_dev, "Failed to allocate int in urb\n");
+		retval = -ENOMEM;
+		goto error;
+	}
+
+	ec_dev->dev = if_dev;
+	ec_dev->phys_name = dev_name(if_dev);
+	if (!is_registered) {
+		ec_dev->priv = ec_usb;
+		/* EC uses int endpoint to signal events. */
+		ec_dev->irq = 0;
+		ec_dev->cmd_xfer = NULL;
+		ec_dev->pkt_xfer = do_cros_ec_pkt_xfer_usb;
+		ec_dev->din_size = sizeof(struct ec_host_response) +
+				sizeof(struct ec_response_get_protocol_info);
+		ec_dev->dout_size = sizeof(struct ec_host_request) +
+				sizeof(struct ec_params_rwsig_action);
+		INIT_WORK(&ec_usb->work_ec_evt, usb_evt_handler);
+		init_waitqueue_head(&ec_usb->resp_ready_wait);
+	} else {
+		/*
+		 * We need to allocate dout and din buffers, because cros_ec_register
+		 * won't be called. These buffers were freed once previous usb device was
+		 * disconnected. Use buffer sizes from the last query.
+		 * The EC_HOST_EVENT_INTERFACE_READY event will be triggered at the end
+		 * of a boot, which calls cros_ec_query_all function, that reallocates
+		 * buffers.
+		 */
+		ec_dev->din = devm_kzalloc(ec_dev->dev, ec_dev->din_size, GFP_KERNEL);
+		if (!ec_dev->din) {
+			retval = -ENOMEM;
+			dev_err(if_dev, "Failed to allocate din buffer\n");
+			goto error;
+		}
+		ec_dev->dout = devm_kzalloc(ec_dev->dev, ec_dev->dout_size, GFP_KERNEL);
+		if (!ec_dev->dout) {
+			retval = -ENOMEM;
+			dev_err(if_dev, "Failed to allocate dout buffer\n");
+			goto error;
+		}
+	}
+
+	/* Needed by ec register function */
+	usb_set_intfdata(intf, ec_dev);
+
+	mutex_lock(&ec_usb->io_mutex);
+	ec_usb->disconnected = false;
+	mutex_unlock(&ec_usb->io_mutex);
+
+	/* Use URB for the int endpoint. */
+	retval = submit_int_urb(ec_dev);
+	if (retval) {
+		dev_err(if_dev, "Failed to sumbit int urb: %d\n", retval);
+		goto error;
+	}
+
+	if (!is_registered) {
+		retval = cros_ec_register(ec_dev);
+		if (retval) {
+			dev_err(if_dev, "Cannot register EC\n");
+			goto error;
+		}
+		retval = cros_ec_usb_register(idProduct, ec_usb);
+		if (retval) {
+			cros_ec_unregister(ec_dev);
+			goto error;
+		}
+		ec_usb->registered = true;
+	}
+
+	/* Handle potential events that haven't been handled before registration */
+	schedule_work(&ec_usb->work_ec_evt);
+
+	/*
+	 * Allows EC to do remote wake-up -
+	 * host sends SET_FEATURE(remote wake-up) before suspend.
+	 */
+	device_init_wakeup(&usb_dev->dev, true);
+
+	return 0;
+error:
+	/* Free allocated memory */
+	cros_ec_usb_delete(ec_usb);
+
+	return retval;
+}
+
+static void cros_ec_usb_disconnect(struct usb_interface *intf)
+{
+	struct cros_ec_device *ec_dev = usb_get_intfdata(intf);
+	struct cros_ec_usb *ec_usb = ec_dev->priv;
+
+	/* prevent more I/O from starting */
+	mutex_lock(&ec_usb->io_mutex);
+	ec_usb->disconnected = true;
+	mutex_unlock(&ec_usb->io_mutex);
+
+	cros_ec_usb_delete(ec_usb);
+
+	dev_info(&intf->dev, "Disconnected\n");
+}
+
+static int cros_ec_usb_suspend(struct usb_interface *intf, pm_message_t message)
+{
+	return 0;
+}
+
+static int cros_ec_usb_resume(struct usb_interface *intf)
+{
+	struct cros_ec_device *ec_dev = usb_get_intfdata(intf);
+	int err;
+
+	/* URB is killed during suspend. */
+	err = submit_int_urb(ec_dev);
+	if (err) {
+		dev_err(ec_dev->dev,
+			"Failed to sumbit int urb after resume: %d\n", err);
+	}
+
+	return 0;
+}
+
+static int cros_ec_usb_pre_reset(struct usb_interface *intf)
+{
+	struct cros_ec_device *ec_dev = usb_get_intfdata(intf);
+	struct cros_ec_usb *ec_usb = ec_dev->priv;
+
+	/* Do not start any new operations. */
+	mutex_lock(&ec_usb->io_mutex);
+
+	usb_kill_urb(ec_usb->int_in_urb);
+
+	return 0;
+}
+
+static int cros_ec_usb_post_reset(struct usb_interface *intf)
+{
+	struct cros_ec_device *ec_dev = usb_get_intfdata(intf);
+	struct cros_ec_usb *ec_usb = ec_dev->priv;
+	int err;
+
+	err = submit_int_urb(ec_dev);
+	if (err) {
+		dev_err(ec_dev->dev,
+			"Failed to sumbit int urb after reset: %d\n", err);
+	}
+	mutex_unlock(&ec_usb->io_mutex);
+
+	return 0;
+}
+
+static struct usb_driver cros_ec_usb = {
+	.name = "cros-ec-usb",
+	.probe = cros_ec_usb_probe,
+	.disconnect = cros_ec_usb_disconnect,
+	.suspend = cros_ec_usb_suspend,
+	.resume = cros_ec_usb_resume,
+	.pre_reset = cros_ec_usb_pre_reset,
+	.post_reset = cros_ec_usb_post_reset,
+	.id_table = cros_ec_usb_table,
+	/* Do not autosuspend EC */
+	.supports_autosuspend = 0,
+};
+module_usb_driver(cros_ec_usb);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("ChromeOS EC USB HC driver");
-- 
2.50.0.727.gbf7dc18ff4-goog

Re: [PATCH] platform/chrome: Add ChromeOS EC USB driver

[Tzung-Bi Shih]

On Tue, Jun 24, 2025 at 11:00:28AM +0000, Dawid Niedzwiecki wrote:
> This uses USB to talk to the ChromeOS EC. The protocol
> is defined by the EC and is fairly simple, with a length byte,
> checksum, command byte and version byte in the header.
> 
> The driver uses vendor defined usb interface with in/out
> endpoints to transfer requests and responses. The driver also
> uses one interrupt in endpoint which signals readiness of response
> and pending events on the EC side.

s/This//;s/The driver// and modify the rest of sentences accordingly.
Not a hard requirement but [1]("imperative mood").

[1]: https://www.kernel.org/doc/html/latest/process/submitting-patches.html

Some part of code is not easy to read to me. I'd suggest to consider to:
- Use shorter local variable names.
- Don't wrap lines whenever it's still under 100-cols.
- Put more relevant pieces of code closer.
- Insert blank lines for separating logic blocks.
- Drop redundant comments if the code is clear.
It doesn't need to be overkill as long as that makes sense.

> diff --git a/drivers/platform/chrome/cros_ec_usb.c b/drivers/platform/chrome/cros_ec_usb.c
> [...]
> +#include "cros_ec.h"
> +
> +/* Google */

Drop this comment which makes less sense.

> [...]
> +/* table of devices that work with this driver */

Drop the comment.

> +static const struct usb_device_id cros_ec_usb_table[] = {
> +	{ USB_VENDOR_AND_INTERFACE_INFO(USB_VENDOR_ID_GOOGLE,
> +					USB_CLASS_VENDOR_SPEC,
> +					USB_SUBCLASS_GOOGLE_EC_HOST_CMD,
> +					USB_PROTOCOL_GOOGLE_EC_HOST_CMD) },
> +	{} /* Terminating entry */

Drop the comment which is a very clear intent.

> +};
> +MODULE_DEVICE_TABLE(usb, cros_ec_usb_table);

I'd prefer to move the device_id definition just right before the struct
usb_driver.

> +/* Structure to hold all of our device specific stuff */

Drop the comment.

> +struct cros_ec_usb {

For readability, insert some blank lines in the struct.

> +	/* the usb device for this device */
> +	struct usb_device *udev;
> +	/* the interface for this device */
> +	struct usb_interface *interface;
> +	/* Cros EC device structure */
> +	struct cros_ec_device *ec_dev;

Maybe insert a blank line here.

> +	/* the buffer to receive data from bulk ep */
> +	u8 *bulk_in_buffer;
> +	/* the buffer to receive data from int ep */
> +	u8 *int_in_buffer;
> +	/* the urb to receive data from int ep */
> +	struct urb *int_in_urb;
> +	/* the size of the receive buffer from bulk ep */
> +	size_t bulk_in_size;
> +	/* the size of the receive buffer from int ep */
> +	size_t int_in_size;

Maybe insert a blank line here.

> +	/* the pipe of the bulk in ep */
> +	unsigned int bulk_in_pipe;
> +	/* the pipe of the bulk out ep */
> +	unsigned int bulk_out_pipe;
> +	/* the pipe of the int in ep */
> +	unsigned int int_in_pipe;
> +	/* the interval of the int in ep */
> +	uint8_t int_in_interval;

Maybe insert a blank line here.

`./scripts/checkpatch.pl --strict` complains about:
CHECK: Prefer kernel type 'u8' over 'uint8_t'

> [...]
> +struct int_msg {
> +	uint8_t int_type;
> +} __packed;

`./scripts/checkpatch.pl --strict` complains about:
CHECK: Prefer kernel type 'u8' over 'uint8_t'

> +static void cros_ec_int_callback(struct urb *urb);

Move just right before submit_int_urb() to make the intent more clear?

> +static int expected_response_size(const struct ec_host_response *host_response)
> +{
> +	/* Check host request version */
> +	if (host_response->struct_version != 3)
> +		return 0;
> +
> +	/* Reserved byte should be 0 */
> +	if (host_response->reserved)
> +		return 0;
> +
> +	return sizeof(*host_response) + host_response->data_len;
> +}

Wondering if the function really helps readability. Maybe just inline to
do_cros_ec_pkt_xfer_usb()?

> +static int cros_ec_usb_register(u16 idProduct, struct cros_ec_usb *ec_usb)
> +{
> +	struct registered_ec *ec;
> +
> +	ec = kzalloc(sizeof(*ec), GFP_KERNEL);

kmalloc() should be sufficient. The member fields are going to be overridden
anyway.

> +static int submit_int_urb(struct cros_ec_device *ec_dev)
> +{
> +	struct cros_ec_usb *ec_usb = ec_dev->priv;
> +	struct usb_device *usb_dev = interface_to_usbdev(ec_usb->interface);
> +	int ret;
> +
> +	/* Submit the INT URB. */
> +	usb_fill_int_urb(ec_usb->int_in_urb, usb_dev, ec_usb->int_in_pipe,
> +			 ec_usb->int_in_buffer, ec_usb->int_in_size,
> +			 cros_ec_int_callback, ec_usb, ec_usb->int_in_interval);
> +	ret = usb_submit_urb(ec_usb->int_in_urb, GFP_KERNEL);
> +
> +	return ret;

Eliminate the `ret`. Just return usb_submit_urb(...).

> +static void cros_ec_int_callback(struct urb *urb)
> +{
> +	struct cros_ec_usb *ec_usb = urb->context;
> +	struct cros_ec_device *ec_dev = ec_usb->ec_dev;
> +	int ret;
> +
> [...]
> +	if (urb->actual_length >= sizeof(struct int_msg)) {
> +		struct int_msg *int_msg =
> +			(struct int_msg *)ec_usb->int_in_buffer;
> +		enum cros_ec_usb_int_type int_type =
> +			(enum cros_ec_usb_int_type)int_msg->int_type;

Maybe insert a blank line here.

> +		switch (int_type) {
> +		case INT_TYPE_EVENT_OCCURED:
> +			if (ec_usb->registered) {
> +				ec_dev->last_event_time = cros_ec_get_time_ns();
> +				schedule_work(&ec_usb->work_ec_evt);
> +			}
> +			break;
> +		case INT_TYPE_RESPONSE_READY:
> +			ec_usb->resp_ready = true;
> +			wake_up(&ec_usb->resp_ready_wait);
> +			break;

I'm wondering who fills the `int_type` (i.e. 0 and 1) here? EC? If so, why
aren't they in cros_ec_command.h?

> +		default:
> +			dev_err(ec_dev->dev, "Unrecognized event: %d\n",
> +				int_type);
> +		}
> +	} else {
> +		dev_err(ec_dev->dev, "Incorrect int transfer len: %d\n",
> +			urb->actual_length);
> +	}

So in either cases, all of them need to resubmit the URB? Doesn't some of
them just need to return?

> +
> +resubmit:
> +	/* Resubmit the INT URB. */
> +	ret = submit_int_urb(ec_dev);
> +	if (ret)
> +		dev_err(ec_dev->dev, "Failed to resumbit int urb: %d", ret);
> +}
> +
> +static int do_cros_ec_pkt_xfer_usb(struct cros_ec_device *ec_dev,
> +				   struct cros_ec_command *ec_msg)
> +{
> +	struct cros_ec_usb *ec_usb = ec_dev->priv;
> +	struct ec_host_response *host_response;
> +	int req_size, ret, actual_length, expected_resp_size, resp_size;
> +	const int header_size = sizeof(*host_response);
> +	const int max_resp_size = header_size + ec_msg->insize;
> +	const int bulk_in_size = umin(ec_usb->bulk_in_size, ec_dev->din_size);
> +	uint8_t sum = 0;

`./scripts/checkpatch.pl --strict` complains about:
CHECK: Prefer kernel type 'u8' over 'uint8_t'

> [...]
> +	/* Get first part of response that contains a header. */
> +	ret = usb_bulk_msg(ec_usb->udev, ec_usb->bulk_in_pipe, ec_dev->din,
> +			   bulk_in_size, &actual_length,
> +			   BULK_TRANSFER_TIMEOUT_MS);
> +	if (ret) {
> +		dev_err(ec_dev->dev, "Failed to get response: %d\n", ret);
> +		goto exit;
> +	}
> +
> +	/* Verify number of received bytes. */
> +	if (actual_length < header_size) {
> +		dev_err(ec_dev->dev, "Received too little bytes: %d\n",
> +			actual_length);

Is it possible that the `actual_length < header_size` just because it needs
to further read? I.e. need a read loop for waiting EOF or
`actual_length >= header_size`?

> +		ret = -ENOSPC;
> +		goto exit;
> +	}
> +	expected_resp_size =
> +		expected_response_size((struct ec_host_response *)ec_dev->din);
> +	if ((expected_resp_size > max_resp_size) || (expected_resp_size == 0) ||
> +	    (actual_length > expected_resp_size)) {
> +		dev_err(ec_dev->dev, "Incorrect number of expected bytes: %d\n",
> +			expected_resp_size);
> +		ret = -ENOSPC;
> +		goto exit;
> +	}

Maybe insert a blank line here.

> +	resp_size = actual_length;

Move next to the following line of the comment.

> +	/* Get the rest of the response if needed. */
> +	if (resp_size < expected_resp_size) {
> +		ret = usb_bulk_msg(ec_usb->udev, ec_usb->bulk_in_pipe,
> +				   ec_dev->din + resp_size,
> +				   expected_resp_size - resp_size,
> +				   &actual_length, BULK_TRANSFER_TIMEOUT_MS);
> +		if (ret) {
> +			dev_err(ec_dev->dev,
> +				"Failed to get second part of response: %d\n",
> +				ret);
> +			goto exit;
> +		}
> +		resp_size += actual_length;

Same here: doesn't it need a read loop for waiting EOF or
`resp_size >= expected_resp_size`?

> +	}
> +
> +	/* Check if number of received of bytes is correct. */
> +	if (resp_size != expected_resp_size) {
> +		dev_err(ec_dev->dev,
> +			"Received incorrect number of bytes: %d, expected: %d\n",
> +			resp_size, expected_resp_size);
> +		ret = -ENOSPC;
> +		goto exit;
> +	}
> +
> +	/* Validate checksum */
> +	host_response = (struct ec_host_response *)ec_dev->din;
> +	for (int i = 0; i < sizeof(*host_response) + host_response->data_len;
                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
I.e. expected_resp_size.

> +	     i++) {
> +		sum += ec_dev->din[i];
> +	}

Drop the {} pair.

> +	if (sum) {
> +		dev_err(ec_dev->dev, "Bad packet checksum calculated %x\n",
> +			sum);
> +		ret = -EBADMSG;
> +		goto exit;
> +	}
> +
> +	ec_msg->result = host_response->result;
> +	memcpy(ec_msg->data, ec_dev->din + sizeof(*host_response),

header_size == sizeof(*host_response). Either drop `header_size` or use it.

> +/**
> + * usb_evt_handler - USB to AP event handler
> + * @work: Work struct
> + */

Maybe drop the comment?

> +static int cros_ec_usb_probe(struct usb_interface *intf,
> +			     const struct usb_device_id *id)
> +{
> +	struct usb_device *usb_dev = interface_to_usbdev(intf);
> +	struct usb_endpoint_descriptor *bulk_in, *bulk_out, *int_in;
> +	struct device *if_dev = &intf->dev;
> +	struct cros_ec_device *ec_dev;
> +	const u16 idProduct = usb_dev->descriptor.idProduct;
> +	struct cros_ec_usb *ec_usb = cros_ec_usb_get_registered(idProduct);
> +	const bool is_registered = ec_usb != NULL;

Or !!ec_usb.

> +	int retval;
> +
> +	/*
> +	 * Do not register the same EC device twice. The probing is performed every
> +	 * reboot, sysjump, crash etc. Recreating the /dev/cros_X file every time
> +	 * would force all application to reopen the file, which is not a case for
> +	 * other cros_ec_x divers. Instead, keep the cros_ec_device and cros_ec_usb
> +	 * structures constant and replace USB related structures for the same EC
> +	 * that is reprobed.
> +	 *
> +	 * The driver doesn't support handling two devices with the same idProduct,
> +	 * but it will never be a real usecase.
> +	 */

I don't quite understand why does it need to memorize the registered ECs.
Supposedly, the probe function is only called few times during booting, and
gets success once. Hot-plugs?

> +	if (!is_registered)
> +		ec_usb = kzalloc(sizeof(*ec_usb), GFP_KERNEL);
> +
> +	if (!ec_usb)
> +		return -ENOMEM;
> +
> +	if (!is_registered) {
> +		mutex_init(&ec_usb->io_mutex);
> +		ec_dev = kzalloc(sizeof(*ec_dev), GFP_KERNEL);
> +		if (!ec_dev) {
> +			retval = -ENOMEM;
> +			goto error;
> +		}
> +		ec_usb->ec_dev = ec_dev;
> +	} else {
> +		ec_dev = ec_usb->ec_dev;
> +	}

The `!ec_usb` check is only needed after kzalloc(). Thus, the code block
could be simplified to:

        if (!is_registered) {
                ec_usb = kzalloc(...);
                if (!ec_usb)
                        return -ENOMEM

                ec_dev = kzalloc(...);

                /* initialized ec_usb and ec_dev */
                mutex_init(...);
                ec_usb->...
        }
        ec_dev = ec_usb->ec_dev;

> +
> +	ec_usb->udev = usb_get_dev(usb_dev);
> +	ec_usb->interface = usb_get_intf(intf);

Maybe insert a blank line here.

> +	/* Use first bulk-in/out endpoints + int-in endpoint */
> +	retval = usb_find_common_endpoints(intf->cur_altsetting, &bulk_in,
> +					   &bulk_out, &int_in, NULL);
> +	if (retval) {
> +		dev_err(if_dev,
> +			"Could not find bulk-in, bulk-out or int-in endpoint\n");
> +		goto error;
> +	}

Maybe insert a blank line here.

> +	/* Bulk endpoints have to be capable of sending headers in one transfer. */
> +	if ((usb_endpoint_maxp(bulk_out) < sizeof(struct ec_host_request)) ||
> +	    (usb_endpoint_maxp(bulk_in) < sizeof(struct ec_host_response)) ||
> +	    (usb_endpoint_maxp(int_in)) < sizeof(struct int_msg)) {
> +		retval = -ENOSPC;
> +		dev_err(if_dev, "Incorrect max packet size\n");
> +		goto error;
> +	}

Maybe insert a blank line here.

> +	ec_usb->bulk_out_pipe =
> +		usb_sndbulkpipe(ec_usb->udev, bulk_out->bEndpointAddress);
> +	ec_usb->bulk_in_size = usb_endpoint_maxp(bulk_in);
> +	ec_usb->bulk_in_pipe =
> +		usb_rcvbulkpipe(ec_usb->udev, bulk_in->bEndpointAddress);
> +	ec_usb->bulk_in_buffer = kmalloc(ec_usb->bulk_in_size, GFP_KERNEL);
> +	if (!ec_usb->bulk_in_buffer) {
> +		dev_err(if_dev, "Failed to allocate bulk in buffer\n");
> +		retval = -ENOMEM;
> +		goto error;
> +	}

Maybe insert a blank line here.

> +	ec_usb->int_in_size = usb_endpoint_maxp(int_in);
> +	ec_usb->int_in_pipe =
> +		usb_rcvintpipe(ec_usb->udev, int_in->bEndpointAddress);
> +	ec_usb->int_in_interval = int_in->bInterval;
> +	ec_usb->int_in_buffer = kmalloc(ec_usb->int_in_size, GFP_KERNEL);
> +	if (!ec_usb->int_in_buffer) {
> +		dev_err(if_dev, "Failed to allocate int in buffer\n");
> +		retval = -ENOMEM;
> +		goto error;
> +	}
> +	ec_usb->int_in_urb = usb_alloc_urb(0, GFP_KERNEL);
> +	if (!ec_usb->int_in_urb) {
> +		dev_err(if_dev, "Failed to allocate int in urb\n");
> +		retval = -ENOMEM;
> +		goto error;
> +	}
> +
> +	ec_dev->dev = if_dev;
> +	ec_dev->phys_name = dev_name(if_dev);
> +	if (!is_registered) {
> +		ec_dev->priv = ec_usb;
> +		/* EC uses int endpoint to signal events. */
> +		ec_dev->irq = 0;
> +		ec_dev->cmd_xfer = NULL;
> +		ec_dev->pkt_xfer = do_cros_ec_pkt_xfer_usb;
> +		ec_dev->din_size = sizeof(struct ec_host_response) +
> +				sizeof(struct ec_response_get_protocol_info);
> +		ec_dev->dout_size = sizeof(struct ec_host_request) +
> +				sizeof(struct ec_params_rwsig_action);
> +		INIT_WORK(&ec_usb->work_ec_evt, usb_evt_handler);
> +		init_waitqueue_head(&ec_usb->resp_ready_wait);
> +	} else {
> +		/*
> +		 * We need to allocate dout and din buffers, because cros_ec_register
> +		 * won't be called. These buffers were freed once previous usb device was
> +		 * disconnected. Use buffer sizes from the last query.
> +		 * The EC_HOST_EVENT_INTERFACE_READY event will be triggered at the end
> +		 * of a boot, which calls cros_ec_query_all function, that reallocates
> +		 * buffers.
> +		 */
> +		ec_dev->din = devm_kzalloc(ec_dev->dev, ec_dev->din_size, GFP_KERNEL);
> +		if (!ec_dev->din) {
> +			retval = -ENOMEM;
> +			dev_err(if_dev, "Failed to allocate din buffer\n");
> +			goto error;
> +		}
> +		ec_dev->dout = devm_kzalloc(ec_dev->dev, ec_dev->dout_size, GFP_KERNEL);
> +		if (!ec_dev->dout) {
> +			retval = -ENOMEM;
> +			dev_err(if_dev, "Failed to allocate dout buffer\n");
> +			goto error;
> +		}
> +	}

This whole block for initializing `ec_dev` can be done earlier. See another
`!is_registered` above.

> +
> +	/* Needed by ec register function */

Drop the comment.

> +	usb_set_intfdata(intf, ec_dev);

This also can be done earlier when `ec_dev` is determined.

> +
> +	mutex_lock(&ec_usb->io_mutex);
> +	ec_usb->disconnected = false;
> +	mutex_unlock(&ec_usb->io_mutex);

Wondering if it really needs to acquire the lock? Probe functions usually
don't need to as there is no possible concurrent paths yet.

> [...]
> +error:
> +	/* Free allocated memory */
> +	cros_ec_usb_delete(ec_usb);

Be careful to make sure whether cancel_work_sync() works even if
`&ec_usb->work_ec_evt` is uninitialized.

> +static void cros_ec_usb_disconnect(struct usb_interface *intf)
> +{
> +	struct cros_ec_device *ec_dev = usb_get_intfdata(intf);
> +	struct cros_ec_usb *ec_usb = ec_dev->priv;
> +
> +	/* prevent more I/O from starting */
> +	mutex_lock(&ec_usb->io_mutex);
> +	ec_usb->disconnected = true;
> +	mutex_unlock(&ec_usb->io_mutex);
> +
> +	cros_ec_usb_delete(ec_usb);
> +
> +	dev_info(&intf->dev, "Disconnected\n");

This is the only dev_info() in the various callbacks. Consider to drop
it if it might not very useful.

> +static int cros_ec_usb_resume(struct usb_interface *intf)
> +{
> +	struct cros_ec_device *ec_dev = usb_get_intfdata(intf);
> +	int err;
> +
> +	/* URB is killed during suspend. */
> +	err = submit_int_urb(ec_dev);
> +	if (err) {
> +		dev_err(ec_dev->dev,
> +			"Failed to sumbit int urb after resume: %d\n", err);
> +	}
> +
> +	return 0;

Doesn't it need to return `err`?

> +static int cros_ec_usb_post_reset(struct usb_interface *intf)
> +{
> +	struct cros_ec_device *ec_dev = usb_get_intfdata(intf);
> +	struct cros_ec_usb *ec_usb = ec_dev->priv;
> +	int err;
> +
> +	err = submit_int_urb(ec_dev);
> +	if (err) {
> +		dev_err(ec_dev->dev,
> +			"Failed to sumbit int urb after reset: %d\n", err);
> +	}
> +	mutex_unlock(&ec_usb->io_mutex);
> +
> +	return 0;

Doesn't it need to return `err`?

> +static struct usb_driver cros_ec_usb = {
> +	.name = "cros-ec-usb",
> +	.probe = cros_ec_usb_probe,
> +	.disconnect = cros_ec_usb_disconnect,
> +	.suspend = cros_ec_usb_suspend,
> +	.resume = cros_ec_usb_resume,
> +	.pre_reset = cros_ec_usb_pre_reset,
> +	.post_reset = cros_ec_usb_post_reset,
> +	.id_table = cros_ec_usb_table,
> +	/* Do not autosuspend EC */
> +	.supports_autosuspend = 0,
> +};

Most .X callbacks are named cros_ec_usb_X. Only the .id_table is different.
To be neat, I'd suggest to use `cros_ec_usb_id_table`.

Re: [PATCH] platform/chrome: Add ChromeOS EC USB driver

[Dawid Niedźwiecki]

Please see inline comments.

On Fri, Jun 27, 2025 at 9:53 AM Tzung-Bi Shih <tzungbi@kernel.org> wrote:
>
> On Tue, Jun 24, 2025 at 11:00:28AM +0000, Dawid Niedzwiecki wrote:
> > This uses USB to talk to the ChromeOS EC. The protocol
> > is defined by the EC and is fairly simple, with a length byte,
> > checksum, command byte and version byte in the header.
> >
> > The driver uses vendor defined usb interface with in/out
> > endpoints to transfer requests and responses. The driver also
> > uses one interrupt in endpoint which signals readiness of response
> > and pending events on the EC side.
>
> s/This//;s/The driver// and modify the rest of sentences accordingly.
> Not a hard requirement but [1]("imperative mood").
>
> [1]: https://www.kernel.org/doc/html/latest/process/submitting-patches.html
>

Will do.

> Some part of code is not easy to read to me. I'd suggest to consider to:
> - Use shorter local variable names.
> - Don't wrap lines whenever it's still under 100-cols.
> - Put more relevant pieces of code closer.
> - Insert blank lines for separating logic blocks.
> - Drop redundant comments if the code is clear.
> It doesn't need to be overkill as long as that makes sense.
>
> > diff --git a/drivers/platform/chrome/cros_ec_usb.c b/drivers/platform/chrome/cros_ec_usb.c
> > [...]
> > +#include "cros_ec.h"
> > +
> > +/* Google */
>
> Drop this comment which makes less sense.
>

Will do.

> > [...]
> > +/* table of devices that work with this driver */
>
> Drop the comment.
>

Will do.

> > +static const struct usb_device_id cros_ec_usb_table[] = {
> > +     { USB_VENDOR_AND_INTERFACE_INFO(USB_VENDOR_ID_GOOGLE,
> > +                                     USB_CLASS_VENDOR_SPEC,
> > +                                     USB_SUBCLASS_GOOGLE_EC_HOST_CMD,
> > +                                     USB_PROTOCOL_GOOGLE_EC_HOST_CMD) },
> > +     {} /* Terminating entry */
>
> Drop the comment which is a very clear intent.
>

Will do.

> > +};
> > +MODULE_DEVICE_TABLE(usb, cros_ec_usb_table);
>
> I'd prefer to move the device_id definition just right before the struct
> usb_driver.
>
> > +/* Structure to hold all of our device specific stuff */
>
> Drop the comment.
>

Will do.

> > +struct cros_ec_usb {
>
> For readability, insert some blank lines in the struct.
>
> > +     /* the usb device for this device */
> > +     struct usb_device *udev;
> > +     /* the interface for this device */
> > +     struct usb_interface *interface;
> > +     /* Cros EC device structure */
> > +     struct cros_ec_device *ec_dev;
>
> Maybe insert a blank line here.
>

Will do.

> > +     /* the buffer to receive data from bulk ep */
> > +     u8 *bulk_in_buffer;
> > +     /* the buffer to receive data from int ep */
> > +     u8 *int_in_buffer;
> > +     /* the urb to receive data from int ep */
> > +     struct urb *int_in_urb;
> > +     /* the size of the receive buffer from bulk ep */
> > +     size_t bulk_in_size;
> > +     /* the size of the receive buffer from int ep */
> > +     size_t int_in_size;
>
> Maybe insert a blank line here.
>

Will do.

> > +     /* the pipe of the bulk in ep */
> > +     unsigned int bulk_in_pipe;
> > +     /* the pipe of the bulk out ep */
> > +     unsigned int bulk_out_pipe;
> > +     /* the pipe of the int in ep */
> > +     unsigned int int_in_pipe;
> > +     /* the interval of the int in ep */
> > +     uint8_t int_in_interval;
>
> Maybe insert a blank line here.
>

Will do.

> `./scripts/checkpatch.pl --strict` complains about:
> CHECK: Prefer kernel type 'u8' over 'uint8_t'
>

I'll change that.

> > [...]
> > +struct int_msg {
> > +     uint8_t int_type;
> > +} __packed;
>
> `./scripts/checkpatch.pl --strict` complains about:
> CHECK: Prefer kernel type 'u8' over 'uint8_t'
>

I'll change that.

> > +static void cros_ec_int_callback(struct urb *urb);
>
> Move just right before submit_int_urb() to make the intent more clear?
>

Make sense.

> > +static int expected_response_size(const struct ec_host_response *host_response)
> > +{
> > +     /* Check host request version */
> > +     if (host_response->struct_version != 3)
> > +             return 0;
> > +
> > +     /* Reserved byte should be 0 */
> > +     if (host_response->reserved)
> > +             return 0;
> > +
> > +     return sizeof(*host_response) + host_response->data_len;
> > +}
>
> Wondering if the function really helps readability. Maybe just inline to
> do_cros_ec_pkt_xfer_usb()?
>

Make sense.

> > +static int cros_ec_usb_register(u16 idProduct, struct cros_ec_usb *ec_usb)
> > +{
> > +     struct registered_ec *ec;
> > +
> > +     ec = kzalloc(sizeof(*ec), GFP_KERNEL);
>
> kmalloc() should be sufficient. The member fields are going to be overridden
> anyway.
>

Make sense.

> > +static int submit_int_urb(struct cros_ec_device *ec_dev)
> > +{
> > +     struct cros_ec_usb *ec_usb = ec_dev->priv;
> > +     struct usb_device *usb_dev = interface_to_usbdev(ec_usb->interface);
> > +     int ret;
> > +
> > +     /* Submit the INT URB. */
> > +     usb_fill_int_urb(ec_usb->int_in_urb, usb_dev, ec_usb->int_in_pipe,
> > +                      ec_usb->int_in_buffer, ec_usb->int_in_size,
> > +                      cros_ec_int_callback, ec_usb, ec_usb->int_in_interval);
> > +     ret = usb_submit_urb(ec_usb->int_in_urb, GFP_KERNEL);
> > +
> > +     return ret;
>
> Eliminate the `ret`. Just return usb_submit_urb(...).
>

Will do.

> > +static void cros_ec_int_callback(struct urb *urb)
> > +{
> > +     struct cros_ec_usb *ec_usb = urb->context;
> > +     struct cros_ec_device *ec_dev = ec_usb->ec_dev;
> > +     int ret;
> > +
> > [...]
> > +     if (urb->actual_length >= sizeof(struct int_msg)) {
> > +             struct int_msg *int_msg =
> > +                     (struct int_msg *)ec_usb->int_in_buffer;
> > +             enum cros_ec_usb_int_type int_type =
> > +                     (enum cros_ec_usb_int_type)int_msg->int_type;
>
> Maybe insert a blank line here.
>

Will do.

> > +             switch (int_type) {
> > +             case INT_TYPE_EVENT_OCCURED:
> > +                     if (ec_usb->registered) {
> > +                             ec_dev->last_event_time = cros_ec_get_time_ns();
> > +                             schedule_work(&ec_usb->work_ec_evt);
> > +                     }
> > +                     break;
> > +             case INT_TYPE_RESPONSE_READY:
> > +                     ec_usb->resp_ready = true;
> > +                     wake_up(&ec_usb->resp_ready_wait);
> > +                     break;
>
> I'm wondering who fills the `int_type` (i.e. 0 and 1) here? EC? If so, why
> aren't they in cros_ec_command.h?
>

Yes, EC fills the int_type since it is the IN (from device to host)
interrupt transfer.
It is not in the cros_ec_command.h file, because it is a part of the
USB Host Command specification. Not related to EC itself or any
command.

> > +             default:
> > +                     dev_err(ec_dev->dev, "Unrecognized event: %d\n",
> > +                             int_type);
> > +             }
> > +     } else {
> > +             dev_err(ec_dev->dev, "Incorrect int transfer len: %d\n",
> > +                     urb->actual_length);
> > +     }
>
> So in either cases, all of them need to resubmit the URB? Doesn't some of
> them just need to return?
>

Yes, we do always need to resubmit URB, otherwise the interface will
be dysfunctional once any error happen

> > +
> > +resubmit:
> > +     /* Resubmit the INT URB. */
> > +     ret = submit_int_urb(ec_dev);
> > +     if (ret)
> > +             dev_err(ec_dev->dev, "Failed to resumbit int urb: %d", ret);
> > +}
> > +
> > +static int do_cros_ec_pkt_xfer_usb(struct cros_ec_device *ec_dev,
> > +                                struct cros_ec_command *ec_msg)
> > +{
> > +     struct cros_ec_usb *ec_usb = ec_dev->priv;
> > +     struct ec_host_response *host_response;
> > +     int req_size, ret, actual_length, expected_resp_size, resp_size;
> > +     const int header_size = sizeof(*host_response);
> > +     const int max_resp_size = header_size + ec_msg->insize;
> > +     const int bulk_in_size = umin(ec_usb->bulk_in_size, ec_dev->din_size);
> > +     uint8_t sum = 0;
>
> `./scripts/checkpatch.pl --strict` complains about:
> CHECK: Prefer kernel type 'u8' over 'uint8_t'
>

I'll change that

> > [...]
> > +     /* Get first part of response that contains a header. */
> > +     ret = usb_bulk_msg(ec_usb->udev, ec_usb->bulk_in_pipe, ec_dev->din,
> > +                        bulk_in_size, &actual_length,
> > +                        BULK_TRANSFER_TIMEOUT_MS);
> > +     if (ret) {
> > +             dev_err(ec_dev->dev, "Failed to get response: %d\n", ret);
> > +             goto exit;
> > +     }
> > +
> > +     /* Verify number of received bytes. */
> > +     if (actual_length < header_size) {
> > +             dev_err(ec_dev->dev, "Received too little bytes: %d\n",
> > +                     actual_length);
>
> Is it possible that the `actual_length < header_size` just because it needs
> to further read? I.e. need a read loop for waiting EOF or
> `actual_length >= header_size`?
>

No, we assume that the EP size is at least header size, so the entire
header has to be sent at once.

> > +             ret = -ENOSPC;
> > +             goto exit;
> > +     }
> > +     expected_resp_size =
> > +             expected_response_size((struct ec_host_response *)ec_dev->din);
> > +     if ((expected_resp_size > max_resp_size) || (expected_resp_size == 0) ||
> > +         (actual_length > expected_resp_size)) {
> > +             dev_err(ec_dev->dev, "Incorrect number of expected bytes: %d\n",
> > +                     expected_resp_size);
> > +             ret = -ENOSPC;
> > +             goto exit;
> > +     }
>
> Maybe insert a blank line here.
>

Will do.

> > +     resp_size = actual_length;
>
> Move next to the following line of the comment.
>

Will do.

> > +     /* Get the rest of the response if needed. */
> > +     if (resp_size < expected_resp_size) {
> > +             ret = usb_bulk_msg(ec_usb->udev, ec_usb->bulk_in_pipe,
> > +                                ec_dev->din + resp_size,
> > +                                expected_resp_size - resp_size,
> > +                                &actual_length, BULK_TRANSFER_TIMEOUT_MS);
> > +             if (ret) {
> > +                     dev_err(ec_dev->dev,
> > +                             "Failed to get second part of response: %d\n",
> > +                             ret);
> > +                     goto exit;
> > +             }
> > +             resp_size += actual_length;
>
> Same here: doesn't it need a read loop for waiting EOF or
> `resp_size >= expected_resp_size`?
>

No, we know exactly how many bytes are expected.
The usb_bulk_msg returns before receiving expected bytes only if there
was a not max size transfer detected.

> > +     }
> > +
> > +     /* Check if number of received of bytes is correct. */
> > +     if (resp_size != expected_resp_size) {
> > +             dev_err(ec_dev->dev,
> > +                     "Received incorrect number of bytes: %d, expected: %d\n",
> > +                     resp_size, expected_resp_size);
> > +             ret = -ENOSPC;
> > +             goto exit;
> > +     }
> > +
> > +     /* Validate checksum */
> > +     host_response = (struct ec_host_response *)ec_dev->din;
> > +     for (int i = 0; i < sizeof(*host_response) + host_response->data_len;
>                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> I.e. expected_resp_size.
>

I'll change that.

> > +          i++) {
> > +             sum += ec_dev->din[i];
> > +     }
>
> Drop the {} pair.
>

Will do.

> > +     if (sum) {
> > +             dev_err(ec_dev->dev, "Bad packet checksum calculated %x\n",
> > +                     sum);
> > +             ret = -EBADMSG;
> > +             goto exit;
> > +     }
> > +
> > +     ec_msg->result = host_response->result;
> > +     memcpy(ec_msg->data, ec_dev->din + sizeof(*host_response),
>
> header_size == sizeof(*host_response). Either drop `header_size` or use it.
>

Make sense.

> > +/**
> > + * usb_evt_handler - USB to AP event handler
> > + * @work: Work struct
> > + */
>
> Maybe drop the comment?
>

Will do.

> > +static int cros_ec_usb_probe(struct usb_interface *intf,
> > +                          const struct usb_device_id *id)
> > +{
> > +     struct usb_device *usb_dev = interface_to_usbdev(intf);
> > +     struct usb_endpoint_descriptor *bulk_in, *bulk_out, *int_in;
> > +     struct device *if_dev = &intf->dev;
> > +     struct cros_ec_device *ec_dev;
> > +     const u16 idProduct = usb_dev->descriptor.idProduct;
> > +     struct cros_ec_usb *ec_usb = cros_ec_usb_get_registered(idProduct);
> > +     const bool is_registered = ec_usb != NULL;
>
> Or !!ec_usb.
>

I'll change that.

> > +     int retval;
> > +
> > +     /*
> > +      * Do not register the same EC device twice. The probing is performed every
> > +      * reboot, sysjump, crash etc. Recreating the /dev/cros_X file every time
> > +      * would force all application to reopen the file, which is not a case for
> > +      * other cros_ec_x divers. Instead, keep the cros_ec_device and cros_ec_usb
> > +      * structures constant and replace USB related structures for the same EC
> > +      * that is reprobed.
> > +      *
> > +      * The driver doesn't support handling two devices with the same idProduct,
> > +      * but it will never be a real usecase.
> > +      */
>
> I don't quite understand why does it need to memorize the registered ECs.
> Supposedly, the probe function is only called few times during booting, and
> gets success once. Hot-plugs?
>

The probe is called every time the EC device boots from the beginning
- sysjumps, crashes, reboots etc. It succeeds the first time.
Once the /dev/cros_X file is created, we need the possibility to
access the same EC device, with the same, previously created file.
The only way to do that is to reused the already created
cros_ec_device structure.

> > +     if (!is_registered)
> > +             ec_usb = kzalloc(sizeof(*ec_usb), GFP_KERNEL);
> > +
> > +     if (!ec_usb)
> > +             return -ENOMEM;
> > +
> > +     if (!is_registered) {
> > +             mutex_init(&ec_usb->io_mutex);
> > +             ec_dev = kzalloc(sizeof(*ec_dev), GFP_KERNEL);
> > +             if (!ec_dev) {
> > +                     retval = -ENOMEM;
> > +                     goto error;
> > +             }
> > +             ec_usb->ec_dev = ec_dev;
> > +     } else {
> > +             ec_dev = ec_usb->ec_dev;
> > +     }
>
> The `!ec_usb` check is only needed after kzalloc(). Thus, the code block
> could be simplified to:
>
>         if (!is_registered) {
>                 ec_usb = kzalloc(...);
>                 if (!ec_usb)
>                         return -ENOMEM
>
>                 ec_dev = kzalloc(...);
>
>                 /* initialized ec_usb and ec_dev */
>                 mutex_init(...);
>                 ec_usb->...
>         }
>         ec_dev = ec_usb->ec_dev;
>

Make sense.

> > +
> > +     ec_usb->udev = usb_get_dev(usb_dev);
> > +     ec_usb->interface = usb_get_intf(intf);
>
> Maybe insert a blank line here.
>

Will do.

> > +     /* Use first bulk-in/out endpoints + int-in endpoint */
> > +     retval = usb_find_common_endpoints(intf->cur_altsetting, &bulk_in,
> > +                                        &bulk_out, &int_in, NULL);
> > +     if (retval) {
> > +             dev_err(if_dev,
> > +                     "Could not find bulk-in, bulk-out or int-in endpoint\n");
> > +             goto error;
> > +     }
>
> Maybe insert a blank line here.
>

Will do.

> > +     /* Bulk endpoints have to be capable of sending headers in one transfer. */
> > +     if ((usb_endpoint_maxp(bulk_out) < sizeof(struct ec_host_request)) ||
> > +         (usb_endpoint_maxp(bulk_in) < sizeof(struct ec_host_response)) ||
> > +         (usb_endpoint_maxp(int_in)) < sizeof(struct int_msg)) {
> > +             retval = -ENOSPC;
> > +             dev_err(if_dev, "Incorrect max packet size\n");
> > +             goto error;
> > +     }
>
> Maybe insert a blank line here.
>

Will do.

> > +     ec_usb->bulk_out_pipe =
> > +             usb_sndbulkpipe(ec_usb->udev, bulk_out->bEndpointAddress);
> > +     ec_usb->bulk_in_size = usb_endpoint_maxp(bulk_in);
> > +     ec_usb->bulk_in_pipe =
> > +             usb_rcvbulkpipe(ec_usb->udev, bulk_in->bEndpointAddress);
> > +     ec_usb->bulk_in_buffer = kmalloc(ec_usb->bulk_in_size, GFP_KERNEL);
> > +     if (!ec_usb->bulk_in_buffer) {
> > +             dev_err(if_dev, "Failed to allocate bulk in buffer\n");
> > +             retval = -ENOMEM;
> > +             goto error;
> > +     }
>
> Maybe insert a blank line here.
>

Wll do.

> > +     ec_usb->int_in_size = usb_endpoint_maxp(int_in);
> > +     ec_usb->int_in_pipe =
> > +             usb_rcvintpipe(ec_usb->udev, int_in->bEndpointAddress);
> > +     ec_usb->int_in_interval = int_in->bInterval;
> > +     ec_usb->int_in_buffer = kmalloc(ec_usb->int_in_size, GFP_KERNEL);
> > +     if (!ec_usb->int_in_buffer) {
> > +             dev_err(if_dev, "Failed to allocate int in buffer\n");
> > +             retval = -ENOMEM;
> > +             goto error;
> > +     }
> > +     ec_usb->int_in_urb = usb_alloc_urb(0, GFP_KERNEL);
> > +     if (!ec_usb->int_in_urb) {
> > +             dev_err(if_dev, "Failed to allocate int in urb\n");
> > +             retval = -ENOMEM;
> > +             goto error;
> > +     }
> > +
> > +     ec_dev->dev = if_dev;
> > +     ec_dev->phys_name = dev_name(if_dev);
> > +     if (!is_registered) {
> > +             ec_dev->priv = ec_usb;
> > +             /* EC uses int endpoint to signal events. */
> > +             ec_dev->irq = 0;
> > +             ec_dev->cmd_xfer = NULL;
> > +             ec_dev->pkt_xfer = do_cros_ec_pkt_xfer_usb;
> > +             ec_dev->din_size = sizeof(struct ec_host_response) +
> > +                             sizeof(struct ec_response_get_protocol_info);
> > +             ec_dev->dout_size = sizeof(struct ec_host_request) +
> > +                             sizeof(struct ec_params_rwsig_action);
> > +             INIT_WORK(&ec_usb->work_ec_evt, usb_evt_handler);
> > +             init_waitqueue_head(&ec_usb->resp_ready_wait);
> > +     } else {
> > +             /*
> > +              * We need to allocate dout and din buffers, because cros_ec_register
> > +              * won't be called. These buffers were freed once previous usb device was
> > +              * disconnected. Use buffer sizes from the last query.
> > +              * The EC_HOST_EVENT_INTERFACE_READY event will be triggered at the end
> > +              * of a boot, which calls cros_ec_query_all function, that reallocates
> > +              * buffers.
> > +              */
> > +             ec_dev->din = devm_kzalloc(ec_dev->dev, ec_dev->din_size, GFP_KERNEL);
> > +             if (!ec_dev->din) {
> > +                     retval = -ENOMEM;
> > +                     dev_err(if_dev, "Failed to allocate din buffer\n");
> > +                     goto error;
> > +             }
> > +             ec_dev->dout = devm_kzalloc(ec_dev->dev, ec_dev->dout_size, GFP_KERNEL);
> > +             if (!ec_dev->dout) {
> > +                     retval = -ENOMEM;
> > +                     dev_err(if_dev, "Failed to allocate dout buffer\n");
> > +                     goto error;
> > +             }
> > +     }
>
> This whole block for initializing `ec_dev` can be done earlier. See another
> `!is_registered` above.
>

Make sense to keep the code needed in !is_registered case together if
it is possible. I'll change that.

> > +
> > +     /* Needed by ec register function */
>
> Drop the comment.
>

Will do.

> > +     usb_set_intfdata(intf, ec_dev);
>
> This also can be done earlier when `ec_dev` is determined.
>

Will do.

> > +
> > +     mutex_lock(&ec_usb->io_mutex);
> > +     ec_usb->disconnected = false;
> > +     mutex_unlock(&ec_usb->io_mutex);
>
> Wondering if it really needs to acquire the lock? Probe functions usually
> don't need to as there is no possible concurrent paths yet.
>

That would be true for the first probing. Following probings can
happen when an ec device is already registered and
xfer function can be called during probing so I think it is more safe
to synchronize access to the disconnect variable.

> > [...]
> > +error:
> > +     /* Free allocated memory */
> > +     cros_ec_usb_delete(ec_usb);
>
> Be careful to make sure whether cancel_work_sync() works even if
> `&ec_usb->work_ec_evt` is uninitialized.
>

Good catch! cancel_work_sync() can not be used before the work_ec_evt init

> > +static void cros_ec_usb_disconnect(struct usb_interface *intf)
> > +{
> > +     struct cros_ec_device *ec_dev = usb_get_intfdata(intf);
> > +     struct cros_ec_usb *ec_usb = ec_dev->priv;
> > +
> > +     /* prevent more I/O from starting */
> > +     mutex_lock(&ec_usb->io_mutex);
> > +     ec_usb->disconnected = true;
> > +     mutex_unlock(&ec_usb->io_mutex);
> > +
> > +     cros_ec_usb_delete(ec_usb);
> > +
> > +     dev_info(&intf->dev, "Disconnected\n");
>
> This is the only dev_info() in the various callbacks. Consider to drop
> it if it might not very useful.
>

I'll remove that, kernel default logs are sufficient.

> > +static int cros_ec_usb_resume(struct usb_interface *intf)
> > +{
> > +     struct cros_ec_device *ec_dev = usb_get_intfdata(intf);
> > +     int err;
> > +
> > +     /* URB is killed during suspend. */
> > +     err = submit_int_urb(ec_dev);
> > +     if (err) {
> > +             dev_err(ec_dev->dev,
> > +                     "Failed to sumbit int urb after resume: %d\n", err);
> > +     }
> > +
> > +     return 0;
>
> Doesn't it need to return `err`?
>

It needs, I'll change that.

> > +static int cros_ec_usb_post_reset(struct usb_interface *intf)
> > +{
> > +     struct cros_ec_device *ec_dev = usb_get_intfdata(intf);
> > +     struct cros_ec_usb *ec_usb = ec_dev->priv;
> > +     int err;
> > +
> > +     err = submit_int_urb(ec_dev);
> > +     if (err) {
> > +             dev_err(ec_dev->dev,
> > +                     "Failed to sumbit int urb after reset: %d\n", err);
> > +     }
> > +     mutex_unlock(&ec_usb->io_mutex);
> > +
> > +     return 0;
>
> Doesn't it need to return `err`?
>

It needs, I'll change that.

> > +static struct usb_driver cros_ec_usb = {
> > +     .name = "cros-ec-usb",
> > +     .probe = cros_ec_usb_probe,
> > +     .disconnect = cros_ec_usb_disconnect,
> > +     .suspend = cros_ec_usb_suspend,
> > +     .resume = cros_ec_usb_resume,
> > +     .pre_reset = cros_ec_usb_pre_reset,
> > +     .post_reset = cros_ec_usb_post_reset,
> > +     .id_table = cros_ec_usb_table,
> > +     /* Do not autosuspend EC */
> > +     .supports_autosuspend = 0,
> > +};
>
> Most .X callbacks are named cros_ec_usb_X. Only the .id_table is different.
> To be neat, I'd suggest to use `cros_ec_usb_id_table`.

Make sense.

[PATCH v2] platform/chrome: Add ChromeOS EC USB driver

[Dawid Niedzwiecki]

Use USB to talk to the ChromeOS EC. The protocol is defined by the EC
and is fairly simple, with a length byte, checksum, command byte and
version byte in the header.

Use vendor defined usb interface with in/out endpoints to transfer
requests and responses. Also use one interrupt in endpoint which signals
readiness of response and pending events on the EC side.

Signed-off-by: Dawid Niedzwiecki <dawidn@google.com>
---
V1 -> V2:
- Initialize work before potential cancel_work_sync call
- Return errors in .post_reset and .resume callbacks
- Inline expected_response_size function
- Use "imperative mood" in the commit message
- Use u8 instead of uin8_t
- Use 100 columns
- Add some blank line
- Move some code and functions
- Adjust function and variables names
- Some additional formatting

 drivers/platform/chrome/Kconfig       |  11 +
 drivers/platform/chrome/Makefile      |   1 +
 drivers/platform/chrome/cros_ec_usb.c | 582 ++++++++++++++++++++++++++
 3 files changed, 594 insertions(+)
 create mode 100644 drivers/platform/chrome/cros_ec_usb.c

diff --git a/drivers/platform/chrome/Kconfig b/drivers/platform/chrome/Kconfig
index 10941ac37305..07f50816e333 100644
--- a/drivers/platform/chrome/Kconfig
+++ b/drivers/platform/chrome/Kconfig
@@ -316,6 +316,17 @@ config CROS_TYPEC_SWITCH
 	  To compile this driver as a module, choose M here: the module will be
 	  called cros_typec_switch.
 
+config CROS_EC_USB
+	tristate "ChromeOS Embedded Controller (USB)"
+	depends on CROS_EC && USB
+	help
+	  If you say Y here, you get support for talking to the ChromeOS EC
+	  through a USB. The driver uses vendor defined interface and is capable
+	  of signaling events from EC.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called cros_ec_usb.
+
 source "drivers/platform/chrome/wilco_ec/Kconfig"
 
 # Kunit test cases
diff --git a/drivers/platform/chrome/Makefile b/drivers/platform/chrome/Makefile
index b981a1bb5bd8..444383e8912d 100644
--- a/drivers/platform/chrome/Makefile
+++ b/drivers/platform/chrome/Makefile
@@ -38,6 +38,7 @@ obj-$(CONFIG_CROS_EC_SYSFS)		+= cros_ec_sysfs.o
 obj-$(CONFIG_CROS_HPS_I2C)		+= cros_hps_i2c.o
 obj-$(CONFIG_CROS_USBPD_LOGGER)		+= cros_usbpd_logger.o
 obj-$(CONFIG_CROS_USBPD_NOTIFY)		+= cros_usbpd_notify.o
+obj-$(CONFIG_CROS_EC_USB)		+= cros_ec_usb.o
 
 obj-$(CONFIG_WILCO_EC)			+= wilco_ec/
 
diff --git a/drivers/platform/chrome/cros_ec_usb.c b/drivers/platform/chrome/cros_ec_usb.c
new file mode 100644
index 000000000000..63e19eef5e16
--- /dev/null
+++ b/drivers/platform/chrome/cros_ec_usb.c
@@ -0,0 +1,582 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * USB interface for ChromeOS Embedded Controller
+ *
+ * Copyright (C) 2025 Google LLC.
+ */
+
+#include <linux/errno.h>
+#include <linux/kernel.h>
+#include <linux/kref.h>
+#include <linux/list.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/uaccess.h>
+#include <linux/usb.h>
+
+#include <linux/platform_data/cros_ec_commands.h>
+#include <linux/platform_data/cros_ec_proto.h>
+#include <linux/platform_device.h>
+
+#include "cros_ec.h"
+
+#define USB_VENDOR_ID_GOOGLE 0x18d1
+
+#define USB_SUBCLASS_GOOGLE_EC_HOST_CMD 0x5a
+#define USB_PROTOCOL_GOOGLE_EC_HOST_CMD 0x00
+
+#define RESPONSE_TIMEOUT_MS 200
+#define BULK_TRANSFER_TIMEOUT_MS 100
+
+enum cros_ec_usb_int_type {
+	INT_TYPE_EVENT_OCCURED = 0,
+	INT_TYPE_RESPONSE_READY = 1,
+};
+
+struct cros_ec_usb {
+	/* the usb device for this device */
+	struct usb_device *udev;
+	/* the interface for this device */
+	struct usb_interface *interface;
+	/* Cros EC device structure */
+	struct cros_ec_device *ec_dev;
+
+	/* the buffer to receive data from bulk ep */
+	u8 *bulk_in_buffer;
+	/* the buffer to receive data from int ep */
+	u8 *int_in_buffer;
+	/* the urb to receive data from int ep */
+	struct urb *int_in_urb;
+	/* the size of the receive buffer from bulk ep */
+	size_t bulk_in_size;
+	/* the size of the receive buffer from int ep */
+	size_t int_in_size;
+
+	/* the pipe of the bulk in ep */
+	unsigned int bulk_in_pipe;
+	/* the pipe of the bulk out ep */
+	unsigned int bulk_out_pipe;
+	/* the pipe of the int in ep */
+	unsigned int int_in_pipe;
+	/* the interval of the int in ep */
+	u8 int_in_interval;
+
+	/* Response ready on EC side */
+	bool resp_ready;
+	/* EC has been registered */
+	bool registered;
+	/* EC is disconnected */
+	bool disconnected;
+	/* synchronize I/O with disconnect */
+	struct mutex io_mutex;
+	/* Work to handle EC events */
+	struct work_struct work_ec_evt;
+	/* Wait queue to signal the response is ready on EC side */
+	wait_queue_head_t resp_ready_wait;
+};
+
+struct int_msg {
+	u8 int_type;
+} __packed;
+
+struct registered_ec {
+	struct list_head node;
+	u16 idProduct;
+	struct cros_ec_usb *ec_usb;
+};
+
+static LIST_HEAD(registered_list);
+static DEFINE_MUTEX(registered_list_mutex);
+
+static int cros_ec_usb_register(u16 idProduct, struct cros_ec_usb *ec_usb)
+{
+	struct registered_ec *ec;
+
+	ec = kmalloc(sizeof(*ec), GFP_KERNEL);
+	if (!ec)
+		return -ENOMEM;
+
+	ec->ec_usb = ec_usb;
+	ec->idProduct = idProduct;
+	mutex_lock(&registered_list_mutex);
+	list_add(&ec->node, &registered_list);
+	mutex_unlock(&registered_list_mutex);
+
+	return 0;
+}
+
+static struct cros_ec_usb *cros_ec_usb_get_registered(u16 idProduct)
+{
+	struct registered_ec *ec;
+	struct cros_ec_usb *ret = NULL;
+
+	mutex_lock(&registered_list_mutex);
+	list_for_each_entry(ec, &registered_list, node) {
+		if (ec->idProduct == idProduct) {
+			ret = ec->ec_usb;
+			break;
+		}
+	}
+	mutex_unlock(&registered_list_mutex);
+	return ret;
+}
+
+static void cros_ec_int_callback(struct urb *urb);
+
+static int submit_int_urb(struct cros_ec_device *ec_dev)
+{
+	struct cros_ec_usb *ec_usb = ec_dev->priv;
+	struct usb_device *usb_dev = interface_to_usbdev(ec_usb->interface);
+
+	/* Submit the INT URB. */
+	usb_fill_int_urb(ec_usb->int_in_urb, usb_dev, ec_usb->int_in_pipe, ec_usb->int_in_buffer,
+			 ec_usb->int_in_size, cros_ec_int_callback, ec_usb,
+			 ec_usb->int_in_interval);
+
+	return usb_submit_urb(ec_usb->int_in_urb, GFP_KERNEL);
+}
+
+static void cros_ec_int_callback(struct urb *urb)
+{
+	struct cros_ec_usb *ec_usb = urb->context;
+	struct cros_ec_device *ec_dev = ec_usb->ec_dev;
+	int ret;
+
+	switch (urb->status) {
+	case 0:
+		break;
+	case -ECONNRESET:
+	case -ENOENT:
+	case -ESHUTDOWN:
+		/* Expected errors. */
+		return;
+	default:
+		dev_dbg(ec_dev->dev, "Unexpected int urb error: %d\n", urb->status);
+		goto resubmit;
+	}
+
+	if (urb->actual_length >= sizeof(struct int_msg)) {
+		struct int_msg *int_msg = (struct int_msg *)ec_usb->int_in_buffer;
+		enum cros_ec_usb_int_type int_type = (enum cros_ec_usb_int_type)int_msg->int_type;
+
+		switch (int_type) {
+		case INT_TYPE_EVENT_OCCURED:
+			if (ec_usb->registered) {
+				ec_dev->last_event_time = cros_ec_get_time_ns();
+				schedule_work(&ec_usb->work_ec_evt);
+			}
+			break;
+		case INT_TYPE_RESPONSE_READY:
+			ec_usb->resp_ready = true;
+			wake_up(&ec_usb->resp_ready_wait);
+			break;
+		default:
+			dev_err(ec_dev->dev, "Unrecognized event: %d\n", int_type);
+		}
+	} else {
+		dev_err(ec_dev->dev, "Incorrect int transfer len: %d\n", urb->actual_length);
+	}
+
+resubmit:
+	/* Resubmit the INT URB. */
+	ret = submit_int_urb(ec_dev);
+	if (ret)
+		dev_err(ec_dev->dev, "Failed to resumbit int urb: %d", ret);
+}
+
+static int do_cros_ec_pkt_xfer_usb(struct cros_ec_device *ec_dev,
+				   struct cros_ec_command *ec_msg)
+{
+	struct cros_ec_usb *ec_usb = ec_dev->priv;
+	struct ec_host_response *host_response;
+	int req_size, ret, actual_length, expected_resp_size, resp_size;
+	const int header_size = sizeof(*host_response);
+	const int max_resp_size = header_size + ec_msg->insize;
+	const int bulk_in_size = umin(ec_usb->bulk_in_size, ec_dev->din_size);
+	u8 sum = 0;
+
+	mutex_lock(&ec_usb->io_mutex);
+	if (ec_usb->disconnected) {
+		mutex_unlock(&ec_usb->io_mutex);
+		ret = -ENODEV;
+		return ret;
+	}
+
+	if (max_resp_size > ec_dev->din_size) {
+		dev_err(ec_dev->dev, "Potential response too big: %d\n", max_resp_size);
+		ret = -EINVAL;
+		goto exit;
+	}
+
+	req_size = cros_ec_prepare_tx(ec_dev, ec_msg);
+	if (req_size < 0) {
+		dev_err(ec_dev->dev, "Failed to prepare msg %d\n", req_size);
+		ret = req_size;
+		goto exit;
+	}
+	dev_dbg(ec_dev->dev, "Prepared len=%d\n", req_size);
+
+	ec_usb->resp_ready = false;
+	/*
+	 * Buffers dout and din are allocated with devm_kzalloc which means it is suitable
+	 * for DMA and we can use by usb functions.
+	 */
+	ret = usb_bulk_msg(ec_usb->udev, ec_usb->bulk_out_pipe, ec_dev->dout, req_size, NULL,
+			   BULK_TRANSFER_TIMEOUT_MS);
+	if (ret) {
+		dev_err(ec_dev->dev, "Failed to send request: %d\n", ret);
+		goto exit;
+	}
+
+	/*
+	 * Wait till EC signals response ready event via INT endpoint,
+	 * before polling a response with a bulk transfer.
+	 */
+	if (!wait_event_timeout(ec_usb->resp_ready_wait, ec_usb->resp_ready,
+				msecs_to_jiffies(RESPONSE_TIMEOUT_MS))) {
+		dev_err(ec_dev->dev, "Timed out waiting for response\n");
+		ret = -ETIMEDOUT;
+		goto exit;
+	}
+
+	/* Get first part of response that contains a header. */
+	ret = usb_bulk_msg(ec_usb->udev, ec_usb->bulk_in_pipe, ec_dev->din, bulk_in_size,
+			   &actual_length, BULK_TRANSFER_TIMEOUT_MS);
+	if (ret) {
+		dev_err(ec_dev->dev, "Failed to get response: %d\n", ret);
+		goto exit;
+	}
+
+	/* Verify number of received bytes. */
+	if (actual_length < header_size) {
+		dev_err(ec_dev->dev, "Received too little bytes: %d\n", actual_length);
+		ret = -ENOSPC;
+		goto exit;
+	}
+
+	host_response = (struct ec_host_response *)ec_dev->din;
+	if (host_response->struct_version != 3 || host_response->reserved != 0) {
+		dev_err(ec_dev->dev, "Received invalid header\n");
+		ret = -ENOSPC;
+		goto exit;
+	}
+
+	expected_resp_size = header_size + host_response->data_len;
+	if (expected_resp_size > max_resp_size || actual_length > expected_resp_size) {
+		dev_err(ec_dev->dev, "Incorrect number of expected bytes: %d\n",
+			expected_resp_size);
+		ret = -ENOSPC;
+		goto exit;
+	}
+
+	/* Get the rest of the response if needed. */
+	resp_size = actual_length;
+	if (resp_size < expected_resp_size) {
+		ret = usb_bulk_msg(ec_usb->udev, ec_usb->bulk_in_pipe, ec_dev->din + resp_size,
+				   expected_resp_size - resp_size, &actual_length,
+				   BULK_TRANSFER_TIMEOUT_MS);
+		if (ret) {
+			dev_err(ec_dev->dev, "Failed to get second part of response: %d\n", ret);
+			goto exit;
+		}
+		resp_size += actual_length;
+	}
+
+	/* Check if number of received of bytes is correct. */
+	if (resp_size != expected_resp_size) {
+		dev_err(ec_dev->dev, "Received incorrect number of bytes: %d, expected: %d\n",
+			resp_size, expected_resp_size);
+		ret = -ENOSPC;
+		goto exit;
+	}
+
+	/* Validate checksum */
+	for (int i = 0; i < expected_resp_size; i++)
+		sum += ec_dev->din[i];
+
+	if (sum) {
+		dev_err(ec_dev->dev, "Bad packet checksum calculated %x\n", sum);
+		ret = -EBADMSG;
+		goto exit;
+	}
+
+	ec_msg->result = host_response->result;
+	memcpy(ec_msg->data, ec_dev->din + header_size, host_response->data_len);
+	ret = host_response->data_len;
+
+	if (ec_msg->command == EC_CMD_REBOOT_EC)
+		msleep(EC_REBOOT_DELAY_MS);
+
+exit:
+	mutex_unlock(&ec_usb->io_mutex);
+	if (ret < 0) {
+		/* Try to reset EC in case of error to restore default state. */
+		usb_reset_device(ec_usb->udev);
+	}
+
+	return ret;
+}
+
+static void usb_evt_handler(struct work_struct *work)
+{
+	struct cros_ec_usb *ec_usb = container_of(work, struct cros_ec_usb, work_ec_evt);
+
+	cros_ec_irq_thread(0, ec_usb->ec_dev);
+}
+
+static void cros_ec_usb_delete(struct cros_ec_usb *ec_usb)
+{
+	usb_kill_urb(ec_usb->int_in_urb);
+	cancel_work_sync(&ec_usb->work_ec_evt);
+
+	usb_free_urb(ec_usb->int_in_urb);
+	usb_put_intf(ec_usb->interface);
+	usb_put_dev(ec_usb->udev);
+	kfree(ec_usb->int_in_buffer);
+	kfree(ec_usb->bulk_in_buffer);
+}
+
+static int cros_ec_usb_probe(struct usb_interface *intf, const struct usb_device_id *id)
+{
+	struct usb_device *usb_dev = interface_to_usbdev(intf);
+	struct usb_endpoint_descriptor *bulk_in, *bulk_out, *int_in;
+	struct device *if_dev = &intf->dev;
+	struct cros_ec_device *ec_dev;
+	const u16 idProduct = usb_dev->descriptor.idProduct;
+	struct cros_ec_usb *ec_usb = cros_ec_usb_get_registered(idProduct);
+	const bool is_registered = !!ec_usb;
+	int ret;
+
+	/*
+	 * Do not register the same EC device twice. The probing is performed every reboot, sysjump,
+	 * crash etc. Recreating the /dev/cros_X file every time would force all application to
+	 * reopen the file, which is not a case for other cros_ec_x divers. Instead, keep
+	 * the cros_ec_device and cros_ec_usb structures constant and replace USB related structures
+	 * for the same EC that is reprobed.
+	 *
+	 * The driver doesn't support handling two devices with the same idProduct, but it will
+	 * never be a real usecase.
+	 */
+	if (!is_registered) {
+		ec_usb = kzalloc(sizeof(*ec_usb), GFP_KERNEL);
+		if (!ec_usb)
+			return -ENOMEM;
+
+		INIT_WORK(&ec_usb->work_ec_evt, usb_evt_handler);
+
+		ec_dev = kzalloc(sizeof(*ec_dev), GFP_KERNEL);
+		if (!ec_dev) {
+			ret = -ENOMEM;
+			goto error;
+		}
+
+		ec_usb->ec_dev = ec_dev;
+		mutex_init(&ec_usb->io_mutex);
+		init_waitqueue_head(&ec_usb->resp_ready_wait);
+
+		ec_dev->priv = ec_usb;
+		/* EC uses int endpoint to signal events. */
+		ec_dev->irq = 0;
+		ec_dev->cmd_xfer = NULL;
+		ec_dev->pkt_xfer = do_cros_ec_pkt_xfer_usb;
+		ec_dev->din_size = sizeof(struct ec_host_response) +
+				   sizeof(struct ec_response_get_protocol_info);
+		ec_dev->dout_size = sizeof(struct ec_host_request) +
+				    sizeof(struct ec_params_rwsig_action);
+	} else {
+		ec_dev = ec_usb->ec_dev;
+
+		/*
+		 * We need to allocate dout and din buffers, because cros_ec_register
+		 * won't be called. These buffers were freed once previous usb device was
+		 * disconnected. Use buffer sizes from the last query.
+		 * The EC_HOST_EVENT_INTERFACE_READY event will be triggered at the end
+		 * of a boot, which calls cros_ec_query_all function, that reallocates
+		 * buffers.
+		 */
+		ec_dev->din = devm_kzalloc(ec_dev->dev, ec_dev->din_size, GFP_KERNEL);
+		if (!ec_dev->din) {
+			ret = -ENOMEM;
+			dev_err(if_dev, "Failed to allocate din buffer\n");
+			goto error;
+		}
+		ec_dev->dout = devm_kzalloc(ec_dev->dev, ec_dev->dout_size, GFP_KERNEL);
+		if (!ec_dev->dout) {
+			ret = -ENOMEM;
+			dev_err(if_dev, "Failed to allocate dout buffer\n");
+			goto error;
+		}
+	}
+
+	ec_dev->dev = if_dev;
+	ec_dev->phys_name = dev_name(if_dev);
+	usb_set_intfdata(intf, ec_dev);
+	/* Allow EC to do remote wake-up - host sends SET_FEATURE(remote wake-up) before suspend. */
+	device_init_wakeup(&usb_dev->dev, true);
+
+	ec_usb->udev = usb_get_dev(usb_dev);
+	ec_usb->interface = usb_get_intf(intf);
+
+	/* Use first bulk-in/out endpoints + int-in endpoint */
+	ret = usb_find_common_endpoints(intf->cur_altsetting, &bulk_in, &bulk_out, &int_in, NULL);
+	if (ret) {
+		dev_err(if_dev,
+			"Could not find bulk-in, bulk-out or int-in endpoint\n");
+		goto error;
+	}
+	/* Bulk endpoints have to be capable of sending headers in one transfer. */
+	if ((usb_endpoint_maxp(bulk_out) < sizeof(struct ec_host_request)) ||
+	    (usb_endpoint_maxp(bulk_in) < sizeof(struct ec_host_response)) ||
+	    (usb_endpoint_maxp(int_in)) < sizeof(struct int_msg)) {
+		ret = -ENOSPC;
+		dev_err(if_dev, "Incorrect max packet size\n");
+		goto error;
+	}
+
+	ec_usb->bulk_out_pipe = usb_sndbulkpipe(ec_usb->udev, bulk_out->bEndpointAddress);
+	ec_usb->bulk_in_size = usb_endpoint_maxp(bulk_in);
+	ec_usb->bulk_in_pipe = usb_rcvbulkpipe(ec_usb->udev, bulk_in->bEndpointAddress);
+	ec_usb->bulk_in_buffer = kmalloc(ec_usb->bulk_in_size, GFP_KERNEL);
+	if (!ec_usb->bulk_in_buffer) {
+		dev_err(if_dev, "Failed to allocate bulk in buffer\n");
+		ret = -ENOMEM;
+		goto error;
+	}
+
+	ec_usb->int_in_size = usb_endpoint_maxp(int_in);
+	ec_usb->int_in_pipe = usb_rcvintpipe(ec_usb->udev, int_in->bEndpointAddress);
+	ec_usb->int_in_interval = int_in->bInterval;
+	ec_usb->int_in_buffer = kmalloc(ec_usb->int_in_size, GFP_KERNEL);
+	if (!ec_usb->int_in_buffer) {
+		dev_err(if_dev, "Failed to allocate int in buffer\n");
+		ret = -ENOMEM;
+		goto error;
+	}
+	ec_usb->int_in_urb = usb_alloc_urb(0, GFP_KERNEL);
+	if (!ec_usb->int_in_urb) {
+		dev_err(if_dev, "Failed to allocate int in urb\n");
+		ret = -ENOMEM;
+		goto error;
+	}
+
+	/* Use URB for the int endpoint. */
+	ret = submit_int_urb(ec_dev);
+	if (ret) {
+		dev_err(if_dev, "Failed to sumbit int urb: %d\n", ret);
+		goto error;
+	}
+
+	mutex_lock(&ec_usb->io_mutex);
+	ec_usb->disconnected = false;
+	mutex_unlock(&ec_usb->io_mutex);
+
+	if (!is_registered) {
+		ret = cros_ec_register(ec_dev);
+		if (ret) {
+			dev_err(if_dev, "Cannot register EC\n");
+			goto error;
+		}
+		ret = cros_ec_usb_register(idProduct, ec_usb);
+		if (ret) {
+			cros_ec_unregister(ec_dev);
+			goto error;
+		}
+		ec_usb->registered = true;
+	}
+
+	/* Handle potential events that haven't been handled before registration */
+	schedule_work(&ec_usb->work_ec_evt);
+
+	return 0;
+
+error:
+	/* Free allocated memory */
+	cros_ec_usb_delete(ec_usb);
+
+	return ret;
+}
+
+static void cros_ec_usb_disconnect(struct usb_interface *intf)
+{
+	struct cros_ec_device *ec_dev = usb_get_intfdata(intf);
+	struct cros_ec_usb *ec_usb = ec_dev->priv;
+
+	/* prevent more I/O from starting */
+	mutex_lock(&ec_usb->io_mutex);
+	ec_usb->disconnected = true;
+	mutex_unlock(&ec_usb->io_mutex);
+
+	cros_ec_usb_delete(ec_usb);
+}
+
+static int cros_ec_usb_suspend(struct usb_interface *intf, pm_message_t message)
+{
+	return 0;
+}
+
+static int cros_ec_usb_resume(struct usb_interface *intf)
+{
+	struct cros_ec_device *ec_dev = usb_get_intfdata(intf);
+	int err;
+
+	/* URB is killed during suspend. */
+	err = submit_int_urb(ec_dev);
+	if (err)
+		dev_err(ec_dev->dev, "Failed to sumbit int urb after resume: %d\n", err);
+
+	return err;
+}
+
+static int cros_ec_usb_pre_reset(struct usb_interface *intf)
+{
+	struct cros_ec_device *ec_dev = usb_get_intfdata(intf);
+	struct cros_ec_usb *ec_usb = ec_dev->priv;
+
+	/* Do not start any new operations. */
+	mutex_lock(&ec_usb->io_mutex);
+
+	usb_kill_urb(ec_usb->int_in_urb);
+
+	return 0;
+}
+
+static int cros_ec_usb_post_reset(struct usb_interface *intf)
+{
+	struct cros_ec_device *ec_dev = usb_get_intfdata(intf);
+	struct cros_ec_usb *ec_usb = ec_dev->priv;
+	int err;
+
+	err = submit_int_urb(ec_dev);
+	if (err)
+		dev_err(ec_dev->dev, "Failed to sumbit int urb after reset: %d\n", err);
+
+	mutex_unlock(&ec_usb->io_mutex);
+
+	return err;
+}
+
+static const struct usb_device_id cros_ec_usb_id_table[] = {
+	{ USB_VENDOR_AND_INTERFACE_INFO(USB_VENDOR_ID_GOOGLE,
+					USB_CLASS_VENDOR_SPEC,
+					USB_SUBCLASS_GOOGLE_EC_HOST_CMD,
+					USB_PROTOCOL_GOOGLE_EC_HOST_CMD) },
+	{} /* Terminating entry */
+};
+MODULE_DEVICE_TABLE(usb, cros_ec_usb_id_table);
+
+static struct usb_driver cros_ec_usb = {
+	.name = "cros-ec-usb",
+	.probe = cros_ec_usb_probe,
+	.disconnect = cros_ec_usb_disconnect,
+	.suspend = cros_ec_usb_suspend,
+	.resume = cros_ec_usb_resume,
+	.pre_reset = cros_ec_usb_pre_reset,
+	.post_reset = cros_ec_usb_post_reset,
+	.id_table = cros_ec_usb_id_table,
+	/* Do not autosuspend EC */
+	.supports_autosuspend = 0,
+};
+module_usb_driver(cros_ec_usb);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("ChromeOS EC USB HC driver");
-- 
2.50.0.727.gbf7dc18ff4-goog

Re: [PATCH] platform/chrome: Add ChromeOS EC USB driver

[Tzung-Bi Shih]

On Mon, Jun 30, 2025 at 01:59:39PM +0200, Dawid Niedźwiecki wrote:
> On Fri, Jun 27, 2025 at 9:53 AM Tzung-Bi Shih <tzungbi@kernel.org> wrote:
> > On Tue, Jun 24, 2025 at 11:00:28AM +0000, Dawid Niedzwiecki wrote:
> > > +     /*
> > > +      * Do not register the same EC device twice. The probing is performed every
> > > +      * reboot, sysjump, crash etc. Recreating the /dev/cros_X file every time
> > > +      * would force all application to reopen the file, which is not a case for
> > > +      * other cros_ec_x divers. Instead, keep the cros_ec_device and cros_ec_usb
> > > +      * structures constant and replace USB related structures for the same EC
> > > +      * that is reprobed.
> > > +      *
> > > +      * The driver doesn't support handling two devices with the same idProduct,
> > > +      * but it will never be a real usecase.
> > > +      */
> >
> > I don't quite understand why does it need to memorize the registered ECs.
> > Supposedly, the probe function is only called few times during booting, and
> > gets success once. Hot-plugs?
> >
> 
> The probe is called every time the EC device boots from the beginning
> - sysjumps, crashes, reboots etc. It succeeds the first time.
> Once the /dev/cros_X file is created, we need the possibility to
> access the same EC device, with the same, previously created file.
> The only way to do that is to reused the already created
> cros_ec_device structure.

What are the shortcomings if it re-creates /dev/cros_X everytime? Isn't it
also a way for userland programs to be aware of the EC device crashes?

Why other cros_ec_X drivers doesn't need the mechanism?

Re: [PATCH] platform/chrome: Add ChromeOS EC USB driver

[Dawid Niedźwiecki]

>What are the shortcomings if it re-creates /dev/cros_X everytime?

In that case, to avoid memory leakage, you would need to free the
cros_ec_device structure every disconnect.
An application with an opened cros_ec file would try to access the
freed resources (use-after-free) via the char driver [1], e.g.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/platform/chrome/cros_ec_chardev.c#n345

It would also require additional work from the userspace application
e.g. reopen the cros_ec file every time there was a communication
problem. I think recovering an already created file sounds like a
better idea.

> Isn't it also a way for userland programs to be aware of the EC device crashes?

No. First of all, we don't know a reason for the disconnect / reprobe,
so it is not always a crash. Also we signal that there is a problem
with the connection by returning -ENODEV when an EC device is
disconnected.
There are dedicated commands to check reboot reason, and detect
potential crashes.

> Why other cros_ec_X drivers doesn't need the mechanism?

It is a very USB specific problem. USB host/hub detects a new device
on the bus and starts probing procedure.
It is not a case for other drivers, e.g. SPI probes only once, at the
beginning of the boot, based on ACPI entry, there is no hardware
detection that a device is alive after reboot.
Additionally, the USB related structures are new every probe, so we
need to replace those.

On Tue, Jul 1, 2025 at 10:56 AM Tzung-Bi Shih <tzungbi@kernel.org> wrote:
>
> On Mon, Jun 30, 2025 at 01:59:39PM +0200, Dawid Niedźwiecki wrote:
> > On Fri, Jun 27, 2025 at 9:53 AM Tzung-Bi Shih <tzungbi@kernel.org> wrote:
> > > On Tue, Jun 24, 2025 at 11:00:28AM +0000, Dawid Niedzwiecki wrote:
> > > > +     /*
> > > > +      * Do not register the same EC device twice. The probing is performed every
> > > > +      * reboot, sysjump, crash etc. Recreating the /dev/cros_X file every time
> > > > +      * would force all application to reopen the file, which is not a case for
> > > > +      * other cros_ec_x divers. Instead, keep the cros_ec_device and cros_ec_usb
> > > > +      * structures constant and replace USB related structures for the same EC
> > > > +      * that is reprobed.
> > > > +      *
> > > > +      * The driver doesn't support handling two devices with the same idProduct,
> > > > +      * but it will never be a real usecase.
> > > > +      */
> > >
> > > I don't quite understand why does it need to memorize the registered ECs.
> > > Supposedly, the probe function is only called few times during booting, and
> > > gets success once. Hot-plugs?
> > >
> >
> > The probe is called every time the EC device boots from the beginning
> > - sysjumps, crashes, reboots etc. It succeeds the first time.
> > Once the /dev/cros_X file is created, we need the possibility to
> > access the same EC device, with the same, previously created file.
> > The only way to do that is to reused the already created
> > cros_ec_device structure.
>
> What are the shortcomings if it re-creates /dev/cros_X everytime? Isn't it
> also a way for userland programs to be aware of the EC device crashes?
>
> Why other cros_ec_X drivers doesn't need the mechanism?

Re: [PATCH] platform/chrome: Add ChromeOS EC USB driver

[Tzung-Bi Shih]

On Tue, Jul 01, 2025 at 12:29:51PM +0200, Dawid Niedźwiecki wrote:
> >What are the shortcomings if it re-creates /dev/cros_X everytime?
> 
> In that case, to avoid memory leakage, you would need to free the
> cros_ec_device structure every disconnect.
> An application with an opened cros_ec file would try to access the
> freed resources (use-after-free) via the char driver [1], e.g.
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/platform/chrome/cros_ec_chardev.c#n345
> 
> It would also require additional work from the userspace application
> e.g. reopen the cros_ec file every time there was a communication
> problem. I think recovering an already created file sounds like a
> better idea.
> 
> > Isn't it also a way for userland programs to be aware of the EC device crashes?
> 
> No. First of all, we don't know a reason for the disconnect / reprobe,
> so it is not always a crash. Also we signal that there is a problem
> with the connection by returning -ENODEV when an EC device is
> disconnected.
> There are dedicated commands to check reboot reason, and detect
> potential crashes.
> 
> > Why other cros_ec_X drivers doesn't need the mechanism?
> 
> It is a very USB specific problem. USB host/hub detects a new device
> on the bus and starts probing procedure.
> It is not a case for other drivers, e.g. SPI probes only once, at the
> beginning of the boot, based on ACPI entry, there is no hardware
> detection that a device is alive after reboot.
> Additionally, the USB related structures are new every probe, so we
> need to replace those.

Please don't top-posting [2]. The discussion thread is somehow messed.

[2]: https://docs.kernel.org/process/submitting-patches.html#use-trimmed-interleaved-replies-in-email-discussions

> On Tue, Jul 1, 2025 at 10:56 AM Tzung-Bi Shih <tzungbi@kernel.org> wrote:
> >
> > On Mon, Jun 30, 2025 at 01:59:39PM +0200, Dawid Niedźwiecki wrote:
> > > On Fri, Jun 27, 2025 at 9:53 AM Tzung-Bi Shih <tzungbi@kernel.org> wrote:
> > > > On Tue, Jun 24, 2025 at 11:00:28AM +0000, Dawid Niedzwiecki wrote:
> > > > > +     /*
> > > > > +      * Do not register the same EC device twice. The probing is performed every
> > > > > +      * reboot, sysjump, crash etc. Recreating the /dev/cros_X file every time
> > > > > +      * would force all application to reopen the file, which is not a case for
> > > > > +      * other cros_ec_x divers. Instead, keep the cros_ec_device and cros_ec_usb
> > > > > +      * structures constant and replace USB related structures for the same EC
> > > > > +      * that is reprobed.
> > > > > +      *
> > > > > +      * The driver doesn't support handling two devices with the same idProduct,
> > > > > +      * but it will never be a real usecase.
> > > > > +      */
> > > >
> > > > I don't quite understand why does it need to memorize the registered ECs.
> > > > Supposedly, the probe function is only called few times during booting, and
> > > > gets success once. Hot-plugs?
> > > >
> > >
> > > The probe is called every time the EC device boots from the beginning
> > > - sysjumps, crashes, reboots etc. It succeeds the first time.
> > > Once the /dev/cros_X file is created, we need the possibility to
> > > access the same EC device, with the same, previously created file.
> > > The only way to do that is to reused the already created
> > > cros_ec_device structure.
> >
> > What are the shortcomings if it re-creates /dev/cros_X everytime? Isn't it
> > also a way for userland programs to be aware of the EC device crashes?
> >
> > Why other cros_ec_X drivers doesn't need the mechanism?

Copied your reply here:

> >What are the shortcomings if it re-creates /dev/cros_X everytime?
> 
> In that case, to avoid memory leakage, you would need to free the
> cros_ec_device structure every disconnect.
> An application with an opened cros_ec file would try to access the
> freed resources (use-after-free) via the char driver [1], e.g.
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/platform/chrome/cros_ec_chardev.c#n345
> 
> It would also require additional work from the userspace application
> e.g. reopen the cros_ec file every time there was a communication
> problem. I think recovering an already created file sounds like a
> better idea.

They doesn't look like shortcomings to me. The corresponding destructor
callbacks have to be called when a device is removed anyway.

Instead, re-using the same inode for the userland interface however
*silently* swapping the underlying devices makes less sense to me.

> > Isn't it also a way for userland programs to be aware of the EC device crashes?
> 
> No. First of all, we don't know a reason for the disconnect / reprobe,
> so it is not always a crash. Also we signal that there is a problem
> with the connection by returning -ENODEV when an EC device is
> disconnected.
> There are dedicated commands to check reboot reason, and detect
> potential crashes.

My point here is: to let userland programs be aware of the underlying
device has been removed.

What are the reasons for disconnect and reprobe? From your previous message
and code comment, are they "reboot, sysjump, crash"? Would it happen
frequently after AP boot?

Note: we are talking about a EC-like device here. For a real EC either reboot
or crash, the AP will be reset.

> > Why other cros_ec_X drivers doesn't need the mechanism?
> 
> It is a very USB specific problem. USB host/hub detects a new device
> on the bus and starts probing procedure.
> It is not a case for other drivers, e.g. SPI probes only once, at the
> beginning of the boot, based on ACPI entry, there is no hardware
> detection that a device is alive after reboot.

No, I don't think so. I think all EC-like devices share the same concern
regardless of the transport medium (e.g. SCP over RPMSG, ISH over ISHTP).

Re: [PATCH] platform/chrome: Add ChromeOS EC USB driver

[Dawid Niedźwiecki]

> They doesn't look like shortcomings to me. The corresponding destructor
> callbacks have to be called when a device is removed anyway.
>

Yes, anything related to USB communication itself has to be freed, but
freeing the cros_ec_device structure and calling the cros_ec_unregister
function can crash the system if any of a userland application has the
original file cros_ec open and tries to send a command. The chardev driver
is not aware that the device has been removed and will try to access the
removed structures.

> Instead, re-using the same inode for the userland interface however
> *silently* swapping the underlying devices makes less sense to me.
>

We are sure it is the same EC device, so why do you think accessing the
same inode makes less sense? It is a case for specific interface, that reboot
causes reprobing, which is not a case for other interfaces. I believe
it should be
transparent for the cros_ec device user, what interface type is used
e.g. if it is
SPI, you can reboot the EC device and wait until it responds to a next command,
but if it is USB, you would need to reopen the cros_ec file after reboot.

> My point here is: to let userland programs be aware of the underlying
> device has been removed.
>

I got your point, but they are informed with a proper return code. The
host command
communication doesn't have context. Every command is independent, so userland
programs can not assume any state of the EC device e.g. reboot can
happen. I think
behaviour of the cros_ec file is not a good way to inform the userland
programs that
there were reboot, sysjump etc. There are special commands/events for that.

> What are the reasons for disconnect and reprobe? From your previous message
> and code comment, are they "reboot, sysjump, crash"? Would it happen
> frequently after AP boot?
>

Yes, every boot "from the beginning" , including sysjumps. It depends
on a EC device
type, but it can happen e.g. after AP reboot (RWSIG jumps to RW, it
depends on boot time),
during firmware update procedure, rollback region update etc.

> Note: we are talking about a EC-like device here. For a real EC either reboot
> or crash, the AP will be reset.

That's true. The problem doesn't exist for the real EC device.

> No, I don't think so. I think all EC-like devices share the same concern
> regardless of the transport medium (e.g. SCP over RPMSG, ISH over ISHTP).

Yes, you can run a userland program that opens a cros_ec file and constantly
sends commands e.g. ectool stress, and then manually unbind the ec
device, but I believe it can cause some crashes/memory leakage. I don't think
all drivers are adjusted to hot-plugging.

Re: [PATCH] platform/chrome: Add ChromeOS EC USB driver

[Tzung-Bi Shih]

On Wed, Jul 02, 2025 at 09:43:40AM +0200, Dawid Niedźwiecki wrote:
> > They doesn't look like shortcomings to me. The corresponding destructor
> > callbacks have to be called when a device is removed anyway.
> >
> 
> Yes, anything related to USB communication itself has to be freed, but
> freeing the cros_ec_device structure and calling the cros_ec_unregister
> function can crash the system if any of a userland application has the
> original file cros_ec open and tries to send a command. The chardev driver
> is not aware that the device has been removed and will try to access the
> removed structures.

We have seen a similar crash before but I didn't come out with a solution
at that time. Could you please try [3]?

[3]: https://patchwork.kernel.org/project/chrome-platform/cover/20250703113509.2511758-1-tzungbi@kernel.org/

> > Instead, re-using the same inode for the userland interface however
> > *silently* swapping the underlying devices makes less sense to me.
> >
> 
> We are sure it is the same EC device, so why do you think accessing the
> same inode makes less sense? It is a case for specific interface, that reboot
> causes reprobing, which is not a case for other interfaces. I believe
> it should be
> transparent for the cros_ec device user, what interface type is used
> e.g. if it is
> SPI, you can reboot the EC device and wait until it responds to a next command,
> but if it is USB, you would need to reopen the cros_ec file after reboot.

Did some experiments and I understand the difference now. It probably depends
on the bus implementation.

1) SCP over RPMSG
# ectool --name=cros_scp reboot_ec
# dmesg
cros-ec-rpmsg 10500000.scp.cros-ec-rpmsg.13.-1: rpmsg send timeout
mtk-scp 10500000.scp: SCP watchdog timeout! 0x0
remoteproc remoteproc0: crash detected in scp: type watchdog
...
cros-ec-rpmsg 10500000.scp.cros-ec-rpmsg.13.-1: Chrome EC device registered

2) FP over SPI
# ectool --name=cros_fp reboot_ec
# dmesg
cros-ec-spi spi-PRP0001:01: EC failed to respond in time

Anyway, I see your point about the EC over USB gets re-probe once it reboots,
sysjumps, crashes, firmware updates, and etc.

Given that:
- The crash you encountered is a common issue for all cros_ec_X drivers.
- I prefer to keep cros_ec_X drivers simple and similar rather than have
  something special (e.g. the memorized `struct cros_ec_device` in current
  cros_ec_usb) for fixing the crash.
Could you give [3] a try to see if it fixes the crash and also call
cros_ec_register()/cros_ec_unregister() everytime in the probe/disconnect?

> > No, I don't think so. I think all EC-like devices share the same concern
> > regardless of the transport medium (e.g. SCP over RPMSG, ISH over ISHTP).
> 
> Yes, you can run a userland program that opens a cros_ec file and constantly
> sends commands e.g. ectool stress, and then manually unbind the ec
> device, but I believe it can cause some crashes/memory leakage. I don't think
> all drivers are adjusted to hot-plugging.

Yes, we have seen a similar crash before. See comment above.

Re: [PATCH] platform/chrome: Add ChromeOS EC USB driver

[Dawid Niedźwiecki]

> We have seen a similar crash before but I didn't come out with a solution
> at that time. Could you please try [3]?
>
> [3]: https://patchwork.kernel.org/project/chrome-platform/cover/20250703113509.2511758-1-tzungbi@kernel.org/
>

Sure, I'll try the fix once it gets its final shape.

> Given that:
> - The crash you encountered is a common issue for all cros_ec_X drivers.
> - I prefer to keep cros_ec_X drivers simple and similar rather than have
>   something special (e.g. the memorized `struct cros_ec_device` in current
>   cros_ec_usb) for fixing the crash.
> Could you give [3] a try to see if it fixes the crash and also call
> cros_ec_register()/cros_ec_unregister() everytime in the probe/disconnect?

I agree that the drivers should be simple and similar as much as possible.
But to be precise, I think, it should behave in a similar way as much
as possible
(e.g. reboot EC device doesn't cause re-registering), not be implemented in the
same way. That's why I believe the current implementation of the drivers follows
the already present drivers in a better way.

The specification of a certain interface influences the driver
implementation a lot.
E.g. the SPI driver never frees the cros_ec_device struct [1], because
it is very rare
that the driver is removed and reprobed (only manual unbind/bind). I believe the
SPI device structure is constant in the system. On the other hand, the
USB device
structures are recreated every unplug/plug (reboot, sysjump etc. in
this case), so
this approach is unacceptable.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/platform/chrome/cros_ec_spi.c#n752

Re: [PATCH] platform/chrome: Add ChromeOS EC USB driver

[Tzung-Bi Shih]

On Fri, Jul 04, 2025 at 11:03:01AM +0200, Dawid Niedźwiecki wrote:
> > Given that:
> > - The crash you encountered is a common issue for all cros_ec_X drivers.
> > - I prefer to keep cros_ec_X drivers simple and similar rather than have
> >   something special (e.g. the memorized `struct cros_ec_device` in current
> >   cros_ec_usb) for fixing the crash.
> > Could you give [3] a try to see if it fixes the crash and also call
> > cros_ec_register()/cros_ec_unregister() everytime in the probe/disconnect?
> 
> I agree that the drivers should be simple and similar as much as possible.
> But to be precise, I think, it should behave in a similar way as much
> as possible
> (e.g. reboot EC device doesn't cause re-registering), not be implemented in the
> same way. That's why I believe the current implementation of the drivers follows
> the already present drivers in a better way.

FWIW: It depends on the bus details.  If you find my previous message, SCP
over RPMSG also re-registers everytime after the firmware reboot.

One challenge for current version: it makes the driver more complicated than
others.  E.g. what would be happening if some friend drivers try to access
`ec_dev` while the `cros_ec_usb_probe` is writing to `ec_usb` at a time?
It tries to manage the device's lifecycle one level upper than USB (don't
know what it should call, "session"?).

Another challenge: it doesn't call cros_ec_unregister() in its driver removal
entry.  What would be happening if someone re-inserts the module multiple
times?

Re: [PATCH] platform/chrome: Add ChromeOS EC USB driver

[Dawid Niedźwiecki]

Resending due to HTML incident

>FWIW: It depends on the bus details.  If you find my previous message, SCP
>over RPMSG also re-registers everytime after the firmware reboot.

You are right. But I'm not sure if it is a good approach. My understanding is
that the HAL/API should behave the same way, regardless of what interface
is behind.

>One challenge for current version: it makes the driver more complicated than
>others.  E.g. what would be happening if some friend drivers try to access
>`ec_dev` while the `cros_ec_usb_probe` is writing to `ec_usb` at a time?
>It tries to manage the device's lifecycle one level upper than USB (don't
>know what it should call, "session"?).

Yes, the more complicated driver is for sure not a good thing.
In that case, it shouldn't cause anything wrong. The disconnect field is set to
false once everything is ready. Before that, there are no transfers handled.

>Another challenge: it doesn't call cros_ec_unregister() in its driver removal
>entry.  What would be happening if someone re-inserts the module multiple
>times?

That's the same case as reboot/sysjump. We would get the probe callback,
the driver would notice (by Product ID) that this EC device has been already
registered and would replace the USB related structures in the cros_ec_usb
structure and the same cros_X  file would start working again.