Message-ID: <aF5N5jrRUlj3SUGS@google.com>
Date: Fri, 27 Jun 2025 07:53:10 +0000
From: Tzung-Bi Shih <tzungbi@...nel.org>
To: Dawid Niedzwiecki <dawidn@...gle.com>
Cc: Benson Leung <bleung@...omium.org>, chrome-platform@...ts.linux.dev,
	linux-kernel@...r.kernel.org, chromeos-krk-upstreaming@...gle.com,
	Łukasz Bartosik <ukaszb@...omium.org>
Subject: Re: [PATCH] platform/chrome: Add ChromeOS EC USB driver

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`.

Powered by blists - more mailing lists