| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CACeCKaerjLdGR_h_H=7+55mN31PUGJHAujfWXG2A3ELJTcnBMw@mail.gmail.com>
Date: Mon, 5 Dec 2022 11:56:24 -0800
From: Prashant Malani <pmalani@...omium.org>
To: Mark Hasemeyer <markhas@...omium.org>
Cc: LKML <linux-kernel@...r.kernel.org>,
Raul Rangel <rrangel@...omium.org>,
Bhanu Prakash Maiya <bhanumaiya@...omium.org>,
Benson Leung <bleung@...omium.org>,
Guenter Roeck <groeck@...omium.org>,
Tzung-Bi Shih <tzungbi@...nel.org>,
chrome-platform@...ts.linux.dev
Subject: Re: [PATCH v9 1/3] platform/chrome: cros_ec_uart: Add cros-ec-uart
transport layer
Hi Mark and Bhanu,
On Dec 05 00:19, Mark Hasemeyer wrote:
> From: Bhanu Prakash Maiya <bhanumaiya@...omium.org>
>
> obj-$(CONFIG_CROS_EC_TYPEC) += cros_ec_typec.o
> obj-$(CONFIG_CROS_EC_LPC) += cros_ec_lpcs.o
> diff --git a/drivers/platform/chrome/cros_ec_uart.c b/drivers/platform/chrome/cros_ec_uart.c
> new file mode 100644
> index 0000000000000..4b416441f973c
> --- /dev/null
> +++ b/drivers/platform/chrome/cros_ec_uart.c
> @@ -0,0 +1,373 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * UART interface for ChromeOS Embedded Controller
> + *
> + * Copyright 2020-2022 Google LLC.
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/delay.h>
> +#include <linux/errno.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_data/cros_ec_commands.h>
cros_ec_proto.h includes cros_ec_commmands.h [1], so explicitly
including it shouldn't be needed.
> + * @wait_queue: Wait queue EC response where the cros_ec sends request
> + * to EC and waits
> + */
> +struct response_info {
> + void *data;
> + size_t max_size;
> + size_t size;
> + size_t exp_len;
> + int error;
> + bool received;
I suspect "received" and "error" can be combined into a single
variable. (The condition
in wait_event_*() can be a compound statement, not just a boolean variable).
> +static int cros_ec_uart_rx_bytes(struct serdev_device *serdev,
> + const u8 *data,
> + size_t count)
> +{
> + struct ec_host_response *response;
> + struct cros_ec_device *ec_dev = serdev_device_get_drvdata(serdev);
> + struct cros_ec_uart *ec_uart = ec_dev->priv;
> +
> + /* Check if bytes were sent out of band */
This sounds like something that should generate a warning message...
> + if (!ec_uart->response.data)
> + /* Discard all bytes */
> + return count;
> +
> + /*
> + * Check if incoming bytes + response.size is greater than allocated
> + * buffer in din by cros_ec. This will ensure that if EC sends more
> + * bytes than max_size, waiting process will be notified with an error.
> + */
> + if (ec_uart->response.size + count > ec_uart->response.max_size) {
> + ec_uart->response.error = -EMSGSIZE;
> + ec_uart->response.received = true;
> + wake_up_interruptible(&ec_uart->response.wait_queue);
> + return count;
> + }
> +
> + memcpy((void *)ec_uart->response.data + ec_uart->response.size,
> + (void *)data, count);
Should be able to fit on 1 line (100 chars). Also, I don't believe the casts are
necessary.
> +
> + ec_uart->response.size += count;
> +
> + /*
> + * Read data_len if we received response header and if exp_len
> + * was not read before.
> + */
> + if (ec_uart->response.size >= sizeof(*response) &&
> + ec_uart->response.exp_len == 0) {
Please align clauses within the if condition parentheses.
> + response = (struct ec_host_response *)
> + ec_uart->response.data;
Should be able to fit on 1 line (100 chars).
> +
> + ec_uart->response.exp_len = response->data_len +
> + sizeof(*response);
Should be able to fit on 1 line (100 chars).
> + }
> +
> + /*
> + * If driver received response header and payload from EC,
> + * Wake up the wait queue.
> + */
> + if (ec_uart->response.size >= sizeof(*response) &&
> + ec_uart->response.size == ec_uart->response.exp_len) {
Please align clauses within the if condition parentheses.
> + ec_uart->response.received = true;
> + wake_up_interruptible(&ec_uart->response.wait_queue);
> + }
> +
> + return count;
> +}
> +
> +static int cros_ec_uart_pkt_xfer(struct cros_ec_device *ec_dev,
> + struct cros_ec_command *ec_msg)
> +{
> + struct cros_ec_uart *ec_uart = ec_dev->priv;
> + struct serdev_device *serdev = ec_uart->serdev;
> + struct ec_host_response *response;
> + unsigned int len;
> + int ret, i;
> + u8 sum = 0;
> +
> + len = cros_ec_prepare_tx(ec_dev, ec_msg);
> + dev_dbg(ec_dev->dev, "Prepared len=%d\n", len);
> +
> + /* Setup for incoming response */
> + ec_uart->response.data = ec_dev->din;
> + ec_uart->response.max_size = ec_dev->din_size;
> + ec_uart->response.size = 0;
> + ec_uart->response.error = 0;
> + ec_uart->response.exp_len = 0;
> + ec_uart->response.received = false;
> +
> + ret = serdev_device_write_buf(serdev, ec_dev->dout, len);
> + if (ret < len) {
> + dev_err(ec_dev->dev, "Unable to write data");
> + ret = -EIO;
> + goto exit;
> + }
> +
> + /* Once request is successfully sent to EC, wait to wait_queue */
Superfluous comment.
> + ret = wait_event_interruptible_timeout(ec_uart->response.wait_queue,
> + ec_uart->response.received,
> + msecs_to_jiffies(EC_MSG_DEADLINE_MS));
Is there any reason wait_event_timeout() is not sufficient? Which
signals/interrupt are you
expecting to receive that isn't handled by the rest of the code?
> + if (ret < 0) {
> + dev_warn(ec_dev->dev, "Interrupted waiting for response.\n");
> + goto exit;
> + } else if (ret == 0) {
> + dev_warn(ec_dev->dev, "Timed out waiting for response.\n");
> + ret = -ETIMEDOUT;
> + goto exit;
> + }
> +
> + if (ec_uart->response.error < 0) {
> + dev_warn(ec_dev->dev, "Response error detected.\n");
> + ret = ec_uart->response.error;
> + goto exit;
> + }
> +
> + response = (struct ec_host_response *)ec_dev->din;
> + ec_msg->result = response->result;
> +
> + if (response->data_len > ec_msg->insize) {
> + dev_err(ec_dev->dev, "Resp too long (%d bytes, expected %d)",
> + response->data_len,
> + ec_msg->insize);
> + ret = -ENOSPC;
> + goto exit;
> + }
> +
> + /* Validate checksum */
> + for (i = 0; i < sizeof(*response) + 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;
> + }
> +
> + memcpy(ec_msg->data,
> + ec_dev->din + sizeof(*response),
> + response->data_len);
Should be able to fit on 1 line (100 chars).
> +
> + ret = response->data_len;
> +
> +exit:
> + /* Invalidate response buffer to guard against out of band rx data */
> + ec_uart->response.data = NULL;
> +
> + if (ec_msg->command == EC_CMD_REBOOT_EC)
> + msleep(EC_REBOOT_DELAY_MS);
> +
> + return ret;
> +}
> +
> +static int cros_ec_uart_resource(struct acpi_resource *ares, void *data)
> +{
> + struct cros_ec_uart *ec_uart = data;
> + struct acpi_resource_uart_serialbus *sb = &ares->data.uart_serial_bus;
> +
> + if (ares->type == ACPI_RESOURCE_TYPE_SERIAL_BUS &&
> + sb->type == ACPI_RESOURCE_SERIAL_TYPE_UART) {
> + ec_uart->baudrate = sb->default_baud_rate;
> + dev_dbg(&ec_uart->serdev->dev, "Baudrate %d\n",
> + ec_uart->baudrate);
Should be able to fit on 1 line (100 chars).
> +
> + ec_uart->flowcontrol = sb->flow_control;
> + dev_dbg(&ec_uart->serdev->dev, "Flow control %d\n",
> + ec_uart->flowcontrol);
Should be able to fit on 1 line (100 chars).
> + }
> +
> + return 0;
> +}
> +
> +static int cros_ec_uart_acpi_probe(struct cros_ec_uart *ec_uart)
> +{
> + int ret;
> + LIST_HEAD(resources);
> + struct acpi_device *adev = ACPI_COMPANION(&ec_uart->serdev->dev);
> +
> + ret = acpi_dev_get_resources(adev, &resources,
> + cros_ec_uart_resource, ec_uart);
Should be able to fit on 1 line (100 chars).
> + if (ret < 0)
> + return ret;
> +
> + acpi_dev_free_resource_list(&resources);
> +
> + /* Retrieve GpioInt and translate it to Linux IRQ number */
> + ret = acpi_dev_gpio_irq_get(adev, 0);
> + if (ret < 0)
> + return ret;
> +
> + ec_uart->irq = ret;
> + dev_dbg(&ec_uart->serdev->dev, "IRQ number %d\n", ec_uart->irq);
> +
> + return 0;
> +}
> +
> +static const struct serdev_device_ops cros_ec_uart_client_ops = {
> + .receive_buf = cros_ec_uart_rx_bytes,
> +};
> +
> +static int cros_ec_uart_probe(struct serdev_device *serdev)
> +{
> + struct device *dev = &serdev->dev;
> + struct cros_ec_device *ec_dev;
> + struct cros_ec_uart *ec_uart;
> + int ret;
> +
> + ec_uart = devm_kzalloc(dev, sizeof(*ec_uart), GFP_KERNEL);
> + if (!ec_uart)
> + return -ENOMEM;
> +
> + ec_dev = devm_kzalloc(dev, sizeof(*ec_dev), GFP_KERNEL);
> + if (!ec_dev)
> + return -ENOMEM;
> +
> + ec_uart->serdev = serdev;
> +
> + ret = devm_serdev_device_open(dev, ec_uart->serdev);
> + if (ret) {
> + dev_err(dev, "Unable to open UART device");
> + return ret;
> + }
> +
> + serdev_device_set_drvdata(serdev, ec_dev);
> + serdev_device_set_client_ops(serdev, &cros_ec_uart_client_ops);
> + init_waitqueue_head(&ec_uart->response.wait_queue);
> +
> + ret = cros_ec_uart_acpi_probe(ec_uart);
> + if (ret < 0) {
> + dev_err(dev, "Failed to get ACPI info (%d)", ret);
> + return ret;
> + }
> +
> + ret = serdev_device_set_baudrate(serdev, ec_uart->baudrate);
> + if (ret < 0) {
> + dev_err(dev, "Failed to set up host baud rate (%d)", ret);
> + return ret;
> + }
> +
> + serdev_device_set_flow_control(serdev, ec_uart->flowcontrol);
> +
> + /* Initialize ec_dev for cros_ec */
> + ec_dev->phys_name = dev_name(dev);
> + ec_dev->dev = dev;
> + ec_dev->priv = ec_uart;
> + ec_dev->irq = ec_uart->irq;
> + ec_dev->cmd_xfer = NULL;
> + ec_dev->pkt_xfer = cros_ec_uart_pkt_xfer;
> + 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);
> +
> + return cros_ec_register(ec_dev);
> +}
> +
> +static void cros_ec_uart_remove(struct serdev_device *serdev)
> +{
> + struct cros_ec_device *ec_dev = serdev_device_get_drvdata(serdev);
> +
> + cros_ec_unregister(ec_dev);
> +};
> +
> +static int __maybe_unused cros_ec_uart_suspend(struct device *dev)
> +{
> + struct cros_ec_device *ec_dev = dev_get_drvdata(dev);
> +
> + return cros_ec_suspend(ec_dev);
> +}
> +
> +static int __maybe_unused cros_ec_uart_resume(struct device *dev)
> +{
> + struct cros_ec_device *ec_dev = dev_get_drvdata(dev);
> +
> + return cros_ec_resume(ec_dev);
> +}
> +
> +static SIMPLE_DEV_PM_OPS(cros_ec_uart_pm_ops, cros_ec_uart_suspend,
> + cros_ec_uart_resume);
> +
> +#ifdef CONFIG_ACPI
> +static const struct acpi_device_id cros_ec_uart_acpi_id[] = {
> + { "GOOG0019", 0 },
> + { /* sentinel */ }
nit: Other ACPI drivers in this directory don't use the word "sentinel",
so it seems like just "{}" should be sufficient.
Best regards,
-Prashant
[1] https://elixir.bootlin.com/linux/v6.1-rc8/source/include/linux/platform_data/cros_ec_proto.h#L15
Powered by blists - more mailing lists