| Message ID | 20240403-public-ucsi-h-v3-2-f848e18c8ed2@chromium.org |
|---|---|
| State | New |
| Headers | show |
| Series | usb: typec: Implement UCSI driver for ChromeOS | expand |
On Wed, Apr 03, 2024 at 06:05:22PM +0000, Pavan Holla wrote: > Implementation of a UCSI transport driver for ChromeOS. > This driver will be loaded if the ChromeOS EC implements a PPM. > > Signed-off-by: Pavan Holla <pholla@chromium.org> > --- > drivers/usb/typec/ucsi/Kconfig | 13 ++ > drivers/usb/typec/ucsi/Makefile | 1 + > drivers/usb/typec/ucsi/cros_ec_ucsi.c | 245 ++++++++++++++++++++++++++++++++++ > 3 files changed, 259 insertions(+) > > diff --git a/drivers/usb/typec/ucsi/Kconfig b/drivers/usb/typec/ucsi/Kconfig > index bdcb1764cfae..4dceb14a66ee 100644 > --- a/drivers/usb/typec/ucsi/Kconfig > +++ b/drivers/usb/typec/ucsi/Kconfig > @@ -69,4 +69,17 @@ config UCSI_PMIC_GLINK > To compile the driver as a module, choose M here: the module will be > called ucsi_glink. > > +config CROS_EC_UCSI > + tristate "UCSI Driver for ChromeOS EC" > + depends on MFD_CROS_EC_DEV > + depends on CROS_USBPD_NOTIFY > + depends on !EXTCON_TCSS_CROS_EC > + default MFD_CROS_EC_DEV > + help > + This driver enables UCSI support for a ChromeOS EC. The EC is > + expected to implement a PPM. > + > + To compile the driver as a module, choose M here: the module > + will be called cros_ec_ucsi. > + > endif > diff --git a/drivers/usb/typec/ucsi/Makefile b/drivers/usb/typec/ucsi/Makefile > index b4679f94696b..cb336eee055c 100644 > --- a/drivers/usb/typec/ucsi/Makefile > +++ b/drivers/usb/typec/ucsi/Makefile > @@ -21,3 +21,4 @@ obj-$(CONFIG_UCSI_ACPI) += ucsi_acpi.o > obj-$(CONFIG_UCSI_CCG) += ucsi_ccg.o > obj-$(CONFIG_UCSI_STM32G0) += ucsi_stm32g0.o > obj-$(CONFIG_UCSI_PMIC_GLINK) += ucsi_glink.o > +obj-$(CONFIG_CROS_EC_UCSI) += cros_ec_ucsi.o > diff --git a/drivers/usb/typec/ucsi/cros_ec_ucsi.c b/drivers/usb/typec/ucsi/cros_ec_ucsi.c > new file mode 100644 > index 000000000000..dd46b46d430f > --- /dev/null > +++ b/drivers/usb/typec/ucsi/cros_ec_ucsi.c > @@ -0,0 +1,245 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * UCSI driver for ChromeOS EC > + * > + * Copyright 2024 Google LLC. > + */ > + > +#include <linux/container_of.h> > +#include <linux/dev_printk.h> > +#include <linux/mod_devicetable.h> > +#include <linux/module.h> > +#include <linux/platform_data/cros_ec_commands.h> > +#include <linux/platform_data/cros_usbpd_notify.h> > +#include <linux/platform_data/cros_ec_proto.h> > +#include <linux/platform_device.h> > +#include <linux/slab.h> > +#include <linux/wait.h> > + > +#include "ucsi.h" > + > +#define DRV_NAME "cros-ec-ucsi" > + > +#define MAX_EC_DATA_SIZE 256 > +#define WRITE_TMO_MS 500 > + > +struct cros_ucsi_data { > + struct device *dev; > + struct ucsi *ucsi; > + > + struct cros_ec_device *ec; > + struct notifier_block nb; > + struct work_struct work; > + > + struct completion complete; > + unsigned long flags; > +}; > + > +static int cros_ucsi_read(struct ucsi *ucsi, unsigned int offset, void *val, > + size_t val_len) > +{ > + struct cros_ucsi_data *udata = ucsi_get_drvdata(ucsi); > + struct ec_params_ucsi_ppm_get req = { > + .offset = offset, > + .size = val_len, > + }; > + int ret; > + > + if (val_len > MAX_EC_DATA_SIZE) { > + dev_err(udata->dev, "Can't read %zu bytes. Too big.", val_len); > + return -EINVAL; > + } > + > + ret = cros_ec_cmd(udata->ec, 0, EC_CMD_UCSI_PPM_GET, > + &req, sizeof(req), val, val_len); > + if (ret < 0) { > + dev_warn(udata->dev, "Failed to send EC message UCSI_PPM_GET: error=%d", ret); > + return ret; > + } > + return 0; > +} > + > +static int cros_ucsi_async_write(struct ucsi *ucsi, unsigned int offset, > + const void *val, size_t val_len) > +{ > + struct cros_ucsi_data *udata = ucsi_get_drvdata(ucsi); > + uint8_t ec_buffer[MAX_EC_DATA_SIZE + sizeof(struct ec_params_ucsi_ppm_set)]; > + struct ec_params_ucsi_ppm_set *req = (struct ec_params_ucsi_ppm_set *)ec_buffer; > + int ret = 0; > + > + if (val_len > MAX_EC_DATA_SIZE) { > + dev_err(udata->dev, "Can't write %zu bytes. Too big.", val_len); I think it's better be written as if (WARN_ON_ONCE(val_len > MAX_EC_DATA_SIZE)) return -EINVAL; Same applies to reading. > + return -EINVAL; > + } > + > + memset(req, 0, sizeof(ec_buffer)); > + req->offset = offset; > + memcpy(req->data, val, val_len); > + ret = cros_ec_cmd(udata->ec, 0, EC_CMD_UCSI_PPM_SET, > + req, sizeof(struct ec_params_ucsi_ppm_set) + val_len, NULL, 0); > + > + if (ret < 0) { > + dev_warn(udata->dev, "Failed to send EC message UCSI_PPM_SET: error=%d", ret); > + return ret; > + } > + return 0; > +} > + > +static int cros_ucsi_sync_write(struct ucsi *ucsi, unsigned int offset, > + const void *val, size_t val_len) > +{ > + struct cros_ucsi_data *udata = ucsi_get_drvdata(ucsi); > + bool ack = UCSI_COMMAND(*(u64 *)val) == UCSI_ACK_CC_CI; > + int ret; > + > + if (ack) > + set_bit(ACK_PENDING, &udata->flags); > + else > + set_bit(COMMAND_PENDING, &udata->flags); > + > + ret = cros_ucsi_async_write(ucsi, offset, val, val_len); > + if (ret) > + goto out; > + > + if (!wait_for_completion_timeout(&udata->complete, WRITE_TMO_MS)) > + ret = -ETIMEDOUT; > + > +out: > + if (ack) > + clear_bit(ACK_PENDING, &udata->flags); > + else > + clear_bit(COMMAND_PENDING, &udata->flags); > + return ret; > +} > + > +struct ucsi_operations cros_ucsi_ops = { > + .read = cros_ucsi_read, > + .async_write = cros_ucsi_async_write, > + .sync_write = cros_ucsi_sync_write, > +}; > + > +static void cros_ucsi_work(struct work_struct *work) > +{ > + struct cros_ucsi_data *udata = container_of(work, struct cros_ucsi_data, work); > + u32 cci; > + int ret; > + > + ret = cros_ucsi_read(udata->ucsi, UCSI_CCI, &cci, sizeof(cci)); > + if (ret) > + return; > + > + if (UCSI_CCI_CONNECTOR(cci)) > + ucsi_connector_change(udata->ucsi, UCSI_CCI_CONNECTOR(cci)); > + > + if (cci & UCSI_CCI_ACK_COMPLETE && test_bit(ACK_PENDING, &udata->flags)) > + complete(&udata->complete); > + if (cci & UCSI_CCI_COMMAND_COMPLETE && > + test_bit(COMMAND_PENDING, &udata->flags)) > + complete(&udata->complete); > +} > + > +static int cros_ucsi_event(struct notifier_block *nb, > + unsigned long host_event, void *_notify) > +{ > + struct cros_ucsi_data *udata = container_of(nb, struct cros_ucsi_data, nb); > + > + if (!(host_event & PD_EVENT_PPM)) > + return NOTIFY_OK; > + > + dev_dbg(udata->dev, "UCSI notification received"); > + flush_work(&udata->work); > + schedule_work(&udata->work); > + > + return NOTIFY_OK; > +} > + > +static int cros_ucsi_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct cros_ec_dev *ec_data = dev_get_drvdata(dev->parent); > + struct cros_ucsi_data *udata; > + int ret; > + > + udata = devm_kzalloc(dev, sizeof(*udata), GFP_KERNEL); > + if (!udata) > + return -ENOMEM; > + > + udata->dev = dev; > + > + udata->ec = ec_data->ec_dev; > + if (!udata->ec) { > + dev_err(dev, "couldn't find parent EC device\n"); > + return -ENODEV; > + } > + > + platform_set_drvdata(pdev, udata); > + > + INIT_WORK(&udata->work, cros_ucsi_work); > + init_completion(&udata->complete); > + > + udata->ucsi = ucsi_create(udata->dev, &cros_ucsi_ops); > + if (IS_ERR(udata->ucsi)) { > + dev_err(dev, "failed to allocate UCSI instance\n"); > + return PTR_ERR(udata->ucsi); > + } > + > + ucsi_set_drvdata(udata->ucsi, udata); > + > + ret = ucsi_register(udata->ucsi); > + if (ret) { > + ucsi_destroy(udata->ucsi); > + return ret; > + } > + > + udata->nb.notifier_call = cros_ucsi_event; > + return cros_usbpd_register_notify(&udata->nb); I think you should register notifier before calling ucsi_register(). Otherwise you have a window when the UCSI can attempt to communitcate with the hardware, but it will not get its notifications. > +} > + > +static int cros_ucsi_remove(struct platform_device *dev) > +{ > + struct cros_ucsi_data *udata = platform_get_drvdata(dev); > + > + ucsi_unregister(udata->ucsi); > + ucsi_destroy(udata->ucsi); > + return 0; > +} > + > +static int __maybe_unused cros_ucsi_suspend(struct device *dev) > +{ > + struct cros_ucsi_data *udata = dev_get_drvdata(dev); > + > + cancel_work_sync(&udata->work); > + > + return 0; > +} > + > +static int __maybe_unused cros_ucsi_resume(struct device *dev) > +{ > + struct cros_ucsi_data *udata = dev_get_drvdata(dev); > + > + return ucsi_resume(udata->ucsi); > +} > + > +static SIMPLE_DEV_PM_OPS(cros_ucsi_pm_ops, cros_ucsi_suspend, > + cros_ucsi_resume); > + > +static const struct platform_device_id cros_ec_ucsi_id[] = { > + { "cros-ec-ucsi"}, > + {} > +}; > +MODULE_DEVICE_TABLE(platform, cros_ec_ucsi_id); > + > +static struct platform_driver cros_ucsi_driver = { > + .driver = { > + .name = DRV_NAME, > + .pm = &cros_ucsi_pm_ops, > + }, > + .id_table = cros_ec_ucsi_id, > + .probe = cros_ucsi_probe, > + .remove = cros_ucsi_remove, > +}; > + > +module_platform_driver(cros_ucsi_driver); > + > +MODULE_LICENSE("GPL"); > +MODULE_DESCRIPTION("UCSI driver for ChromeOS EC"); > > -- > 2.44.0.478.gd926399ef9-goog >
On Wed, Apr 03, 2024 at 09:58:33PM +0300, Dmitry Baryshkov wrote: > On Wed, Apr 03, 2024 at 06:05:22PM +0000, Pavan Holla wrote: > > Implementation of a UCSI transport driver for ChromeOS. > > This driver will be loaded if the ChromeOS EC implements a PPM. > > > > Signed-off-by: Pavan Holla <pholla@chromium.org> > > --- > > drivers/usb/typec/ucsi/Kconfig | 13 ++ > > drivers/usb/typec/ucsi/Makefile | 1 + > > drivers/usb/typec/ucsi/cros_ec_ucsi.c | 245 ++++++++++++++++++++++++++++++++++ > > 3 files changed, 259 insertions(+) > > > > diff --git a/drivers/usb/typec/ucsi/Kconfig b/drivers/usb/typec/ucsi/Kconfig > > index bdcb1764cfae..4dceb14a66ee 100644 > > --- a/drivers/usb/typec/ucsi/Kconfig > > +++ b/drivers/usb/typec/ucsi/Kconfig > > @@ -69,4 +69,17 @@ config UCSI_PMIC_GLINK > > To compile the driver as a module, choose M here: the module will be > > called ucsi_glink. > > > > +config CROS_EC_UCSI > > + tristate "UCSI Driver for ChromeOS EC" > > + depends on MFD_CROS_EC_DEV > > + depends on CROS_USBPD_NOTIFY > > + depends on !EXTCON_TCSS_CROS_EC > > + default MFD_CROS_EC_DEV > > + help > > + This driver enables UCSI support for a ChromeOS EC. The EC is > > + expected to implement a PPM. > > + > > + To compile the driver as a module, choose M here: the module > > + will be called cros_ec_ucsi. > > + > > endif > > diff --git a/drivers/usb/typec/ucsi/Makefile b/drivers/usb/typec/ucsi/Makefile > > index b4679f94696b..cb336eee055c 100644 > > --- a/drivers/usb/typec/ucsi/Makefile > > +++ b/drivers/usb/typec/ucsi/Makefile > > @@ -21,3 +21,4 @@ obj-$(CONFIG_UCSI_ACPI) += ucsi_acpi.o > > obj-$(CONFIG_UCSI_CCG) += ucsi_ccg.o > > obj-$(CONFIG_UCSI_STM32G0) += ucsi_stm32g0.o > > obj-$(CONFIG_UCSI_PMIC_GLINK) += ucsi_glink.o > > +obj-$(CONFIG_CROS_EC_UCSI) += cros_ec_ucsi.o > > diff --git a/drivers/usb/typec/ucsi/cros_ec_ucsi.c b/drivers/usb/typec/ucsi/cros_ec_ucsi.c > > new file mode 100644 > > index 000000000000..dd46b46d430f > > --- /dev/null > > +++ b/drivers/usb/typec/ucsi/cros_ec_ucsi.c > > @@ -0,0 +1,245 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * UCSI driver for ChromeOS EC > > + * > > + * Copyright 2024 Google LLC. > > + */ > > + > > +#include <linux/container_of.h> > > +#include <linux/dev_printk.h> > > +#include <linux/mod_devicetable.h> > > +#include <linux/module.h> > > +#include <linux/platform_data/cros_ec_commands.h> > > +#include <linux/platform_data/cros_usbpd_notify.h> > > +#include <linux/platform_data/cros_ec_proto.h> > > +#include <linux/platform_device.h> > > +#include <linux/slab.h> > > +#include <linux/wait.h> > > + > > +#include "ucsi.h" > > + > > +#define DRV_NAME "cros-ec-ucsi" > > + > > +#define MAX_EC_DATA_SIZE 256 > > +#define WRITE_TMO_MS 500 > > + > > +struct cros_ucsi_data { > > + struct device *dev; > > + struct ucsi *ucsi; > > + > > + struct cros_ec_device *ec; > > + struct notifier_block nb; > > + struct work_struct work; > > + > > + struct completion complete; > > + unsigned long flags; > > +}; > > + > > +static int cros_ucsi_read(struct ucsi *ucsi, unsigned int offset, void *val, > > + size_t val_len) > > +{ > > + struct cros_ucsi_data *udata = ucsi_get_drvdata(ucsi); > > + struct ec_params_ucsi_ppm_get req = { > > + .offset = offset, > > + .size = val_len, > > + }; > > + int ret; > > + > > + if (val_len > MAX_EC_DATA_SIZE) { > > + dev_err(udata->dev, "Can't read %zu bytes. Too big.", val_len); > > + return -EINVAL; > > + } > > + > > + ret = cros_ec_cmd(udata->ec, 0, EC_CMD_UCSI_PPM_GET, > > + &req, sizeof(req), val, val_len); > > + if (ret < 0) { > > + dev_warn(udata->dev, "Failed to send EC message UCSI_PPM_GET: error=%d", ret); > > + return ret; > > + } > > + return 0; > > +} > > + > > +static int cros_ucsi_async_write(struct ucsi *ucsi, unsigned int offset, > > + const void *val, size_t val_len) > > +{ > > + struct cros_ucsi_data *udata = ucsi_get_drvdata(ucsi); > > + uint8_t ec_buffer[MAX_EC_DATA_SIZE + sizeof(struct ec_params_ucsi_ppm_set)]; > > + struct ec_params_ucsi_ppm_set *req = (struct ec_params_ucsi_ppm_set *)ec_buffer; > > + int ret = 0; > > + > > + if (val_len > MAX_EC_DATA_SIZE) { > > + dev_err(udata->dev, "Can't write %zu bytes. Too big.", val_len); > > I think it's better be written as > > if (WARN_ON_ONCE(val_len > MAX_EC_DATA_SIZE)) > return -EINVAL; So if you trigger this, you just rebooted all boxes that have panic-on-warn enabled (hint, the HUGE majority in quantity of Linux systems out there.) So don't do that, just handle it like this. BUT, if this can be triggered by userspace, do NOT use dev_err() as that will just allow userspace to flood the kernel log. Pavan, who calls this? If userspace, this needs to be fixed. If it's only a kernel driver, it's fine as-is. thanks, greg k-h
On Thu, Apr 04, 2024 at 03:07:15PM +0200, Greg Kroah-Hartman wrote: > On Wed, Apr 03, 2024 at 09:58:33PM +0300, Dmitry Baryshkov wrote: > > On Wed, Apr 03, 2024 at 06:05:22PM +0000, Pavan Holla wrote: > > > Implementation of a UCSI transport driver for ChromeOS. > > > This driver will be loaded if the ChromeOS EC implements a PPM. > > > > > > Signed-off-by: Pavan Holla <pholla@chromium.org> > > > --- > > > drivers/usb/typec/ucsi/Kconfig | 13 ++ > > > drivers/usb/typec/ucsi/Makefile | 1 + > > > drivers/usb/typec/ucsi/cros_ec_ucsi.c | 245 ++++++++++++++++++++++++++++++++++ > > > 3 files changed, 259 insertions(+) > > > > > > diff --git a/drivers/usb/typec/ucsi/Kconfig b/drivers/usb/typec/ucsi/Kconfig > > > index bdcb1764cfae..4dceb14a66ee 100644 > > > --- a/drivers/usb/typec/ucsi/Kconfig > > > +++ b/drivers/usb/typec/ucsi/Kconfig > > > @@ -69,4 +69,17 @@ config UCSI_PMIC_GLINK > > > To compile the driver as a module, choose M here: the module will be > > > called ucsi_glink. > > > > > > +config CROS_EC_UCSI > > > + tristate "UCSI Driver for ChromeOS EC" > > > + depends on MFD_CROS_EC_DEV > > > + depends on CROS_USBPD_NOTIFY > > > + depends on !EXTCON_TCSS_CROS_EC > > > + default MFD_CROS_EC_DEV > > > + help > > > + This driver enables UCSI support for a ChromeOS EC. The EC is > > > + expected to implement a PPM. > > > + > > > + To compile the driver as a module, choose M here: the module > > > + will be called cros_ec_ucsi. > > > + > > > endif > > > diff --git a/drivers/usb/typec/ucsi/Makefile b/drivers/usb/typec/ucsi/Makefile > > > index b4679f94696b..cb336eee055c 100644 > > > --- a/drivers/usb/typec/ucsi/Makefile > > > +++ b/drivers/usb/typec/ucsi/Makefile > > > @@ -21,3 +21,4 @@ obj-$(CONFIG_UCSI_ACPI) += ucsi_acpi.o > > > obj-$(CONFIG_UCSI_CCG) += ucsi_ccg.o > > > obj-$(CONFIG_UCSI_STM32G0) += ucsi_stm32g0.o > > > obj-$(CONFIG_UCSI_PMIC_GLINK) += ucsi_glink.o > > > +obj-$(CONFIG_CROS_EC_UCSI) += cros_ec_ucsi.o > > > diff --git a/drivers/usb/typec/ucsi/cros_ec_ucsi.c b/drivers/usb/typec/ucsi/cros_ec_ucsi.c > > > new file mode 100644 > > > index 000000000000..dd46b46d430f > > > --- /dev/null > > > +++ b/drivers/usb/typec/ucsi/cros_ec_ucsi.c > > > @@ -0,0 +1,245 @@ > > > +// SPDX-License-Identifier: GPL-2.0 > > > +/* > > > + * UCSI driver for ChromeOS EC > > > + * > > > + * Copyright 2024 Google LLC. > > > + */ > > > + > > > +#include <linux/container_of.h> > > > +#include <linux/dev_printk.h> > > > +#include <linux/mod_devicetable.h> > > > +#include <linux/module.h> > > > +#include <linux/platform_data/cros_ec_commands.h> > > > +#include <linux/platform_data/cros_usbpd_notify.h> > > > +#include <linux/platform_data/cros_ec_proto.h> > > > +#include <linux/platform_device.h> > > > +#include <linux/slab.h> > > > +#include <linux/wait.h> > > > + > > > +#include "ucsi.h" > > > + > > > +#define DRV_NAME "cros-ec-ucsi" > > > + > > > +#define MAX_EC_DATA_SIZE 256 > > > +#define WRITE_TMO_MS 500 > > > + > > > +struct cros_ucsi_data { > > > + struct device *dev; > > > + struct ucsi *ucsi; > > > + > > > + struct cros_ec_device *ec; > > > + struct notifier_block nb; > > > + struct work_struct work; > > > + > > > + struct completion complete; > > > + unsigned long flags; > > > +}; > > > + > > > +static int cros_ucsi_read(struct ucsi *ucsi, unsigned int offset, void *val, > > > + size_t val_len) > > > +{ > > > + struct cros_ucsi_data *udata = ucsi_get_drvdata(ucsi); > > > + struct ec_params_ucsi_ppm_get req = { > > > + .offset = offset, > > > + .size = val_len, > > > + }; > > > + int ret; > > > + > > > + if (val_len > MAX_EC_DATA_SIZE) { > > > + dev_err(udata->dev, "Can't read %zu bytes. Too big.", val_len); > > > + return -EINVAL; > > > + } > > > + > > > + ret = cros_ec_cmd(udata->ec, 0, EC_CMD_UCSI_PPM_GET, > > > + &req, sizeof(req), val, val_len); > > > + if (ret < 0) { > > > + dev_warn(udata->dev, "Failed to send EC message UCSI_PPM_GET: error=%d", ret); > > > + return ret; > > > + } > > > + return 0; > > > +} > > > + > > > +static int cros_ucsi_async_write(struct ucsi *ucsi, unsigned int offset, > > > + const void *val, size_t val_len) > > > +{ > > > + struct cros_ucsi_data *udata = ucsi_get_drvdata(ucsi); > > > + uint8_t ec_buffer[MAX_EC_DATA_SIZE + sizeof(struct ec_params_ucsi_ppm_set)]; > > > + struct ec_params_ucsi_ppm_set *req = (struct ec_params_ucsi_ppm_set *)ec_buffer; > > > + int ret = 0; > > > + > > > + if (val_len > MAX_EC_DATA_SIZE) { > > > + dev_err(udata->dev, "Can't write %zu bytes. Too big.", val_len); > > > > I think it's better be written as > > > > if (WARN_ON_ONCE(val_len > MAX_EC_DATA_SIZE)) > > return -EINVAL; > > So if you trigger this, you just rebooted all boxes that have > panic-on-warn enabled (hint, the HUGE majority in quantity of Linux > systems out there.) > > So don't do that, just handle it like this. Does that mean that we should not use WARN at all? What is the best current practice for WARN usage? I'm asking because for me this looks like a perfect usecase. If I were at the positiion of the driver developer, I'd like to know the whole path leading to the bad call, not just the fact that the function was called with the buffer being too big. > BUT, if this can be triggered by userspace, do NOT use dev_err() as that > will just allow userspace to flood the kernel log. > > Pavan, who calls this? If userspace, this needs to be fixed. If it's > only a kernel driver, it's fine as-is. > > thanks, > > greg k-h
On Thu, Apr 04, 2024 at 04:20:30PM +0300, Dmitry Baryshkov wrote: > On Thu, Apr 04, 2024 at 03:07:15PM +0200, Greg Kroah-Hartman wrote: > > On Wed, Apr 03, 2024 at 09:58:33PM +0300, Dmitry Baryshkov wrote: > > > On Wed, Apr 03, 2024 at 06:05:22PM +0000, Pavan Holla wrote: > > > > Implementation of a UCSI transport driver for ChromeOS. > > > > This driver will be loaded if the ChromeOS EC implements a PPM. > > > > > > > > Signed-off-by: Pavan Holla <pholla@chromium.org> > > > > --- > > > > drivers/usb/typec/ucsi/Kconfig | 13 ++ > > > > drivers/usb/typec/ucsi/Makefile | 1 + > > > > drivers/usb/typec/ucsi/cros_ec_ucsi.c | 245 ++++++++++++++++++++++++++++++++++ > > > > 3 files changed, 259 insertions(+) > > > > > > > > diff --git a/drivers/usb/typec/ucsi/Kconfig b/drivers/usb/typec/ucsi/Kconfig > > > > index bdcb1764cfae..4dceb14a66ee 100644 > > > > --- a/drivers/usb/typec/ucsi/Kconfig > > > > +++ b/drivers/usb/typec/ucsi/Kconfig > > > > @@ -69,4 +69,17 @@ config UCSI_PMIC_GLINK > > > > To compile the driver as a module, choose M here: the module will be > > > > called ucsi_glink. > > > > > > > > +config CROS_EC_UCSI > > > > + tristate "UCSI Driver for ChromeOS EC" > > > > + depends on MFD_CROS_EC_DEV > > > > + depends on CROS_USBPD_NOTIFY > > > > + depends on !EXTCON_TCSS_CROS_EC > > > > + default MFD_CROS_EC_DEV > > > > + help > > > > + This driver enables UCSI support for a ChromeOS EC. The EC is > > > > + expected to implement a PPM. > > > > + > > > > + To compile the driver as a module, choose M here: the module > > > > + will be called cros_ec_ucsi. > > > > + > > > > endif > > > > diff --git a/drivers/usb/typec/ucsi/Makefile b/drivers/usb/typec/ucsi/Makefile > > > > index b4679f94696b..cb336eee055c 100644 > > > > --- a/drivers/usb/typec/ucsi/Makefile > > > > +++ b/drivers/usb/typec/ucsi/Makefile > > > > @@ -21,3 +21,4 @@ obj-$(CONFIG_UCSI_ACPI) += ucsi_acpi.o > > > > obj-$(CONFIG_UCSI_CCG) += ucsi_ccg.o > > > > obj-$(CONFIG_UCSI_STM32G0) += ucsi_stm32g0.o > > > > obj-$(CONFIG_UCSI_PMIC_GLINK) += ucsi_glink.o > > > > +obj-$(CONFIG_CROS_EC_UCSI) += cros_ec_ucsi.o > > > > diff --git a/drivers/usb/typec/ucsi/cros_ec_ucsi.c b/drivers/usb/typec/ucsi/cros_ec_ucsi.c > > > > new file mode 100644 > > > > index 000000000000..dd46b46d430f > > > > --- /dev/null > > > > +++ b/drivers/usb/typec/ucsi/cros_ec_ucsi.c > > > > @@ -0,0 +1,245 @@ > > > > +// SPDX-License-Identifier: GPL-2.0 > > > > +/* > > > > + * UCSI driver for ChromeOS EC > > > > + * > > > > + * Copyright 2024 Google LLC. > > > > + */ > > > > + > > > > +#include <linux/container_of.h> > > > > +#include <linux/dev_printk.h> > > > > +#include <linux/mod_devicetable.h> > > > > +#include <linux/module.h> > > > > +#include <linux/platform_data/cros_ec_commands.h> > > > > +#include <linux/platform_data/cros_usbpd_notify.h> > > > > +#include <linux/platform_data/cros_ec_proto.h> > > > > +#include <linux/platform_device.h> > > > > +#include <linux/slab.h> > > > > +#include <linux/wait.h> > > > > + > > > > +#include "ucsi.h" > > > > + > > > > +#define DRV_NAME "cros-ec-ucsi" > > > > + > > > > +#define MAX_EC_DATA_SIZE 256 > > > > +#define WRITE_TMO_MS 500 > > > > + > > > > +struct cros_ucsi_data { > > > > + struct device *dev; > > > > + struct ucsi *ucsi; > > > > + > > > > + struct cros_ec_device *ec; > > > > + struct notifier_block nb; > > > > + struct work_struct work; > > > > + > > > > + struct completion complete; > > > > + unsigned long flags; > > > > +}; > > > > + > > > > +static int cros_ucsi_read(struct ucsi *ucsi, unsigned int offset, void *val, > > > > + size_t val_len) > > > > +{ > > > > + struct cros_ucsi_data *udata = ucsi_get_drvdata(ucsi); > > > > + struct ec_params_ucsi_ppm_get req = { > > > > + .offset = offset, > > > > + .size = val_len, > > > > + }; > > > > + int ret; > > > > + > > > > + if (val_len > MAX_EC_DATA_SIZE) { > > > > + dev_err(udata->dev, "Can't read %zu bytes. Too big.", val_len); > > > > + return -EINVAL; > > > > + } > > > > + > > > > + ret = cros_ec_cmd(udata->ec, 0, EC_CMD_UCSI_PPM_GET, > > > > + &req, sizeof(req), val, val_len); > > > > + if (ret < 0) { > > > > + dev_warn(udata->dev, "Failed to send EC message UCSI_PPM_GET: error=%d", ret); > > > > + return ret; > > > > + } > > > > + return 0; > > > > +} > > > > + > > > > +static int cros_ucsi_async_write(struct ucsi *ucsi, unsigned int offset, > > > > + const void *val, size_t val_len) > > > > +{ > > > > + struct cros_ucsi_data *udata = ucsi_get_drvdata(ucsi); > > > > + uint8_t ec_buffer[MAX_EC_DATA_SIZE + sizeof(struct ec_params_ucsi_ppm_set)]; > > > > + struct ec_params_ucsi_ppm_set *req = (struct ec_params_ucsi_ppm_set *)ec_buffer; > > > > + int ret = 0; > > > > + > > > > + if (val_len > MAX_EC_DATA_SIZE) { > > > > + dev_err(udata->dev, "Can't write %zu bytes. Too big.", val_len); > > > > > > I think it's better be written as > > > > > > if (WARN_ON_ONCE(val_len > MAX_EC_DATA_SIZE)) > > > return -EINVAL; > > > > So if you trigger this, you just rebooted all boxes that have > > panic-on-warn enabled (hint, the HUGE majority in quantity of Linux > > systems out there.) > > > > So don't do that, just handle it like this. > > Does that mean that we should not use WARN at all? What is the best > current practice for WARN usage? To never use it. Handle the issue and recover properly. > I'm asking because for me this looks like a perfect usecase. If I were > at the positiion of the driver developer, I'd like to know the whole > path leading to the bad call, not just the fact that the function was > called with the buffer being too big. Then use ftrace if you are a driver developer, don't crash users boxes please. If you REALLY need a traceback, then provide that, but do NOT use WARN() for just normal debugging calls that you want to leave around in the system for users to trip over. thanks, greg k-h
On Thu, Apr 4, 2024 at 6:07 AM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Wed, Apr 03, 2024 at 09:58:33PM +0300, Dmitry Baryshkov wrote: > > I think it's better be written as > > > > if (WARN_ON_ONCE(val_len > MAX_EC_DATA_SIZE)) > > return -EINVAL; > > So if you trigger this, you just rebooted all boxes that have > panic-on-warn enabled (hint, the HUGE majority in quantity of Linux > systems out there.) > > So don't do that, just handle it like this. > > BUT, if this can be triggered by userspace, do NOT use dev_err() as that > will just allow userspace to flood the kernel log. > > Pavan, who calls this? If userspace, this needs to be fixed. If it's > only a kernel driver, it's fine as-is. This code is only called by a kernel driver. Thanks, Pavan
On Wed, Apr 03, 2024 at 06:05:22PM +0000, Pavan Holla wrote: > Implementation of a UCSI transport driver for ChromeOS. > This driver will be loaded if the ChromeOS EC implements a PPM. How this driver get probed? From drivers/mfd/cros_ec_dev.c? If so, there is no "cros-ec-ucsi" in the MFD driver yet. > diff --git a/drivers/usb/typec/ucsi/cros_ec_ucsi.c b/drivers/usb/typec/ucsi/cros_ec_ucsi.c > [...] > +static int cros_ucsi_async_write(struct ucsi *ucsi, unsigned int offset, > + const void *val, size_t val_len) > +{ > + struct cros_ucsi_data *udata = ucsi_get_drvdata(ucsi); > + uint8_t ec_buffer[MAX_EC_DATA_SIZE + sizeof(struct ec_params_ucsi_ppm_set)]; > + struct ec_params_ucsi_ppm_set *req = (struct ec_params_ucsi_ppm_set *)ec_buffer; > + int ret = 0; The initialization is redundant. `ret` will be overridden soon. > + if (val_len > MAX_EC_DATA_SIZE) { > + dev_err(udata->dev, "Can't write %zu bytes. Too big.", val_len); > + return -EINVAL; > + } > + > + memset(req, 0, sizeof(ec_buffer)); The `memset` is redundant. > + req->offset = offset; > + memcpy(req->data, val, val_len); > + ret = cros_ec_cmd(udata->ec, 0, EC_CMD_UCSI_PPM_SET, > + req, sizeof(struct ec_params_ucsi_ppm_set) + val_len, NULL, 0); `sizeof(*req)`. > +static int cros_ucsi_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > [...] > + udata->ucsi = ucsi_create(udata->dev, &cros_ucsi_ops); `dev`. > [...] > +static const struct platform_device_id cros_ec_ucsi_id[] = { To be consistent with other symbols, consider either: - s/cros_ec_/cros_/ for the symbol. or - s/cros_ucsi_/cros_ec_ucsi_/g for echoing the file name. > + { "cros-ec-ucsi"}, The driver has declared DRV_NAME, use it. `{ DRV_NAME, 0 },`. > + {} > +}; > +MODULE_DEVICE_TABLE(platform, cros_ec_ucsi_id); Ditto. > +static struct platform_driver cros_ucsi_driver = { > + .driver = { > + .name = DRV_NAME, > + .pm = &cros_ucsi_pm_ops, > + }, > + .id_table = cros_ec_ucsi_id, Ditto.
On Thu, Apr 4, 2024 at 6:30 AM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: [ ... ] > > > > if (WARN_ON_ONCE(val_len > MAX_EC_DATA_SIZE)) > > > > return -EINVAL; > > > > > > So if you trigger this, you just rebooted all boxes that have > > > panic-on-warn enabled (hint, the HUGE majority in quantity of Linux > > > systems out there.) > > > > > > So don't do that, just handle it like this. > > > > Does that mean that we should not use WARN at all? What is the best > > current practice for WARN usage? > > To never use it. Handle the issue and recover properly. > > > I'm asking because for me this looks like a perfect usecase. If I were > > at the positiion of the driver developer, I'd like to know the whole > > path leading to the bad call, not just the fact that the function was > > called with the buffer being too big. > > Then use ftrace if you are a driver developer, don't crash users boxes > please. > > If you REALLY need a traceback, then provide that, but do NOT use WARN() > for just normal debugging calls that you want to leave around in the > system for users to trip over. > That is not common practice. $ git grep WARN_ON drivers/gpu | wc 3004 11999 246545 $ git grep WARN_ON drivers/net/ | wc 3679 14564 308230 $ git grep WARN_ON drivers/net/wireless | wc 1985 8112 166081 We get hundreds of thousands of reports with warning backtraces from Chromebooks in the field _every single day_. Most of those are from drm and wireless subsystems. We even had to scale back the percentage of reported warning backtraces because the large volume overwhelmed the reporting system. When approached about it, developers usually respond with "this backtrace is absolutely necessary", but nothing ever happens to fix the reported problems. In practice, they are just ignored. This means that any system using drm or wireless interfaces just can not really enable panic-on-warn because that would crash the system all the time. Guenter
On Mon, Apr 08, 2024 at 06:04:22AM -0700, Guenter Roeck wrote: > On Thu, Apr 4, 2024 at 6:30 AM Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > [ ... ] > > > > > > if (WARN_ON_ONCE(val_len > MAX_EC_DATA_SIZE)) > > > > > return -EINVAL; > > > > > > > > So if you trigger this, you just rebooted all boxes that have > > > > panic-on-warn enabled (hint, the HUGE majority in quantity of Linux > > > > systems out there.) > > > > > > > > So don't do that, just handle it like this. > > > > > > Does that mean that we should not use WARN at all? What is the best > > > current practice for WARN usage? > > > > To never use it. Handle the issue and recover properly. > > > > > I'm asking because for me this looks like a perfect usecase. If I were > > > at the positiion of the driver developer, I'd like to know the whole > > > path leading to the bad call, not just the fact that the function was > > > called with the buffer being too big. > > > > Then use ftrace if you are a driver developer, don't crash users boxes > > please. > > > > If you REALLY need a traceback, then provide that, but do NOT use WARN() > > for just normal debugging calls that you want to leave around in the > > system for users to trip over. > > > > That is not common practice. > > $ git grep WARN_ON drivers/gpu | wc > 3004 11999 246545 > $ git grep WARN_ON drivers/net/ | wc > 3679 14564 308230 > $ git grep WARN_ON drivers/net/wireless | wc > 1985 8112 166081 > > We get hundreds of thousands of reports with warning backtraces from > Chromebooks in the field _every single day_. Most of those are from > drm and wireless subsystems. We even had to scale back the percentage > of reported warning backtraces because the large volume overwhelmed > the reporting system. When approached about it, developers usually > respond with "this backtrace is absolutely necessary", but nothing > ever happens to fix the reported problems. In practice, they are just > ignored. Then push back on the developers please, this isn't ok. WARN_ON triggers so many automated systems it's not funny. And if a trace back is really needed, there is a function for that, but really, just fix the issue and handle it properly. > This means that any system using drm or wireless interfaces just can > not really enable panic-on-warn because that would crash the system > all the time. I guess Android doesn't use wireless or drm :) Again, billions of systems in the world has this enabled, let's learn to live with it and fix up our coding practices to not be lazy. thanks, greg k-h
On Mon, Apr 08, 2024 at 06:04:22AM -0700, Guenter Roeck wrote: > On Thu, Apr 4, 2024 at 6:30 AM Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > [ ... ] > > > > > > if (WARN_ON_ONCE(val_len > MAX_EC_DATA_SIZE)) > > > > > return -EINVAL; > > > > > > > > So if you trigger this, you just rebooted all boxes that have > > > > panic-on-warn enabled (hint, the HUGE majority in quantity of Linux > > > > systems out there.) > > > > > > > > So don't do that, just handle it like this. > > > > > > Does that mean that we should not use WARN at all? What is the best > > > current practice for WARN usage? > > > > To never use it. Handle the issue and recover properly. > > > > > I'm asking because for me this looks like a perfect usecase. If I were > > > at the positiion of the driver developer, I'd like to know the whole > > > path leading to the bad call, not just the fact that the function was > > > called with the buffer being too big. > > > > Then use ftrace if you are a driver developer, don't crash users boxes > > please. > > > > If you REALLY need a traceback, then provide that, but do NOT use WARN() > > for just normal debugging calls that you want to leave around in the > > system for users to trip over. > > > > That is not common practice. > > $ git grep WARN_ON drivers/gpu | wc > 3004 11999 246545 > $ git grep WARN_ON drivers/net/ | wc > 3679 14564 308230 > $ git grep WARN_ON drivers/net/wireless | wc > 1985 8112 166081 > > We get hundreds of thousands of reports with warning backtraces from > Chromebooks in the field _every single day_. Most of those are from > drm and wireless subsystems. We even had to scale back the percentage > of reported warning backtraces because the large volume overwhelmed > the reporting system. When approached about it, developers usually > respond with "this backtrace is absolutely necessary", but nothing > ever happens to fix the reported problems. In practice, they are just > ignored. That's sad. > > This means that any system using drm or wireless interfaces just can > not really enable panic-on-warn because that would crash the system > all the time. And this is good from my point of view. If I remember correctly, initially panic-on-warn was added to simplify debugging of the warnings rather than to disallow using WARN_ON(). The system is not supposed to continue running after BUG(), so panic/reset on BUG is a safe approach. But the WARN is different. It means that the system was able to cope with it. And as such there is no need to panic. Whoever enabled panic-on-warn is doing a strange thing from my POV.
I've uploaded v4. PTAL. On Mon, Apr 8, 2024 at 1:13 AM Tzung-Bi Shih <tzungbi@kernel.org> wrote: > > How this driver get probed? From drivers/mfd/cros_ec_dev.c? If so, there is > no "cros-ec-ucsi" in the MFD driver yet. Yes, this should get probed from drivers/mfd/cros_ec_dev.c. However, the corresponding change in the EC is still under review. I was planning to send it out once the EC change lands. Please let me know if you think that this review should wait until then. > > > diff --git a/drivers/usb/typec/ucsi/cros_ec_ucsi.c b/drivers/usb/typec/ucsi/cros_ec_ucsi.c > > [...] > > +static int cros_ucsi_async_write(struct ucsi *ucsi, unsigned int offset, > > + const void *val, size_t val_len) > > +{ > > + struct cros_ucsi_data *udata = ucsi_get_drvdata(ucsi); > > + uint8_t ec_buffer[MAX_EC_DATA_SIZE + sizeof(struct ec_params_ucsi_ppm_set)]; > > + struct ec_params_ucsi_ppm_set *req = (struct ec_params_ucsi_ppm_set *)ec_buffer; > > + int ret = 0; > > The initialization is redundant. `ret` will be overridden soon. Removed. > > > + if (val_len > MAX_EC_DATA_SIZE) { > > + dev_err(udata->dev, "Can't write %zu bytes. Too big.", val_len); > > + return -EINVAL; > > + } > > + > > + memset(req, 0, sizeof(ec_buffer)); > > The `memset` is redundant. Removed. > > > + req->offset = offset; > > + memcpy(req->data, val, val_len); > > + ret = cros_ec_cmd(udata->ec, 0, EC_CMD_UCSI_PPM_SET, > > + req, sizeof(struct ec_params_ucsi_ppm_set) + val_len, NULL, 0); > > `sizeof(*req)`. Done. > > > +static int cros_ucsi_probe(struct platform_device *pdev) > > +{ > > + struct device *dev = &pdev->dev; > > [...] > > + udata->ucsi = ucsi_create(udata->dev, &cros_ucsi_ops); > > `dev`. > > > [...] > > +static const struct platform_device_id cros_ec_ucsi_id[] = { > > To be consistent with other symbols, consider either: > - s/cros_ec_/cros_/ for the symbol. > or > - s/cros_ucsi_/cros_ec_ucsi_/g for echoing the file name. Replaced cros_ec_ucsi_id with cros_ucsi_id. > > + { "cros-ec-ucsi"}, > > The driver has declared DRV_NAME, use it. `{ DRV_NAME, 0 },`. > Used DRV_NAME. > > + {} > > +}; > > +MODULE_DEVICE_TABLE(platform, cros_ec_ucsi_id); > > Ditto. Replaced cros_ec_ucsi_id with cros_ucsi_id. > > +static struct platform_driver cros_ucsi_driver = { > > + .driver = { > > + .name = DRV_NAME, > > + .pm = &cros_ucsi_pm_ops, > > + }, > > + .id_table = cros_ec_ucsi_id, > > Ditto. Replaced cros_ec_ucsi_id with cros_ucsi_id. Thanks, Pavan
On Mon, Apr 08, 2024 at 07:47:35PM -0700, Pavan Holla wrote: > On Mon, Apr 8, 2024 at 1:13 AM Tzung-Bi Shih <tzungbi@kernel.org> wrote: > > > > How this driver get probed? From drivers/mfd/cros_ec_dev.c? If so, there is > > no "cros-ec-ucsi" in the MFD driver yet. > > Yes, this should get probed from drivers/mfd/cros_ec_dev.c. However, the > corresponding change in the EC is still under review. I was planning to send > it out once the EC change lands. Please let me know if you think that this > review should wait until then. I see. The mfd patch is independent and can be sent later. I was asking because the patch (and the series) don't reflect the message: "This driver will be loaded if the ChromeOS EC implements a PPM." However, include/linux/platform_data/cros_ec_commands.h (the previous patch in the series) usually follows include/ec_commands.h[1]. We should wait until the corresponding EC patches land. [1]: https://chromium.googlesource.com/chromiumos/platform/ec/+/refs/heads/main/include/ec_commands.h
diff --git a/drivers/usb/typec/ucsi/Kconfig b/drivers/usb/typec/ucsi/Kconfig index bdcb1764cfae..4dceb14a66ee 100644 --- a/drivers/usb/typec/ucsi/Kconfig +++ b/drivers/usb/typec/ucsi/Kconfig @@ -69,4 +69,17 @@ config UCSI_PMIC_GLINK To compile the driver as a module, choose M here: the module will be called ucsi_glink. +config CROS_EC_UCSI + tristate "UCSI Driver for ChromeOS EC" + depends on MFD_CROS_EC_DEV + depends on CROS_USBPD_NOTIFY + depends on !EXTCON_TCSS_CROS_EC + default MFD_CROS_EC_DEV + help + This driver enables UCSI support for a ChromeOS EC. The EC is + expected to implement a PPM. + + To compile the driver as a module, choose M here: the module + will be called cros_ec_ucsi. + endif diff --git a/drivers/usb/typec/ucsi/Makefile b/drivers/usb/typec/ucsi/Makefile index b4679f94696b..cb336eee055c 100644 --- a/drivers/usb/typec/ucsi/Makefile +++ b/drivers/usb/typec/ucsi/Makefile @@ -21,3 +21,4 @@ obj-$(CONFIG_UCSI_ACPI) += ucsi_acpi.o obj-$(CONFIG_UCSI_CCG) += ucsi_ccg.o obj-$(CONFIG_UCSI_STM32G0) += ucsi_stm32g0.o obj-$(CONFIG_UCSI_PMIC_GLINK) += ucsi_glink.o +obj-$(CONFIG_CROS_EC_UCSI) += cros_ec_ucsi.o diff --git a/drivers/usb/typec/ucsi/cros_ec_ucsi.c b/drivers/usb/typec/ucsi/cros_ec_ucsi.c new file mode 100644 index 000000000000..dd46b46d430f --- /dev/null +++ b/drivers/usb/typec/ucsi/cros_ec_ucsi.c @@ -0,0 +1,245 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * UCSI driver for ChromeOS EC + * + * Copyright 2024 Google LLC. + */ + +#include <linux/container_of.h> +#include <linux/dev_printk.h> +#include <linux/mod_devicetable.h> +#include <linux/module.h> +#include <linux/platform_data/cros_ec_commands.h> +#include <linux/platform_data/cros_usbpd_notify.h> +#include <linux/platform_data/cros_ec_proto.h> +#include <linux/platform_device.h> +#include <linux/slab.h> +#include <linux/wait.h> + +#include "ucsi.h" + +#define DRV_NAME "cros-ec-ucsi" + +#define MAX_EC_DATA_SIZE 256 +#define WRITE_TMO_MS 500 + +struct cros_ucsi_data { + struct device *dev; + struct ucsi *ucsi; + + struct cros_ec_device *ec; + struct notifier_block nb; + struct work_struct work; + + struct completion complete; + unsigned long flags; +}; + +static int cros_ucsi_read(struct ucsi *ucsi, unsigned int offset, void *val, + size_t val_len) +{ + struct cros_ucsi_data *udata = ucsi_get_drvdata(ucsi); + struct ec_params_ucsi_ppm_get req = { + .offset = offset, + .size = val_len, + }; + int ret; + + if (val_len > MAX_EC_DATA_SIZE) { + dev_err(udata->dev, "Can't read %zu bytes. Too big.", val_len); + return -EINVAL; + } + + ret = cros_ec_cmd(udata->ec, 0, EC_CMD_UCSI_PPM_GET, + &req, sizeof(req), val, val_len); + if (ret < 0) { + dev_warn(udata->dev, "Failed to send EC message UCSI_PPM_GET: error=%d", ret); + return ret; + } + return 0; +} + +static int cros_ucsi_async_write(struct ucsi *ucsi, unsigned int offset, + const void *val, size_t val_len) +{ + struct cros_ucsi_data *udata = ucsi_get_drvdata(ucsi); + uint8_t ec_buffer[MAX_EC_DATA_SIZE + sizeof(struct ec_params_ucsi_ppm_set)]; + struct ec_params_ucsi_ppm_set *req = (struct ec_params_ucsi_ppm_set *)ec_buffer; + int ret = 0; + + if (val_len > MAX_EC_DATA_SIZE) { + dev_err(udata->dev, "Can't write %zu bytes. Too big.", val_len); + return -EINVAL; + } + + memset(req, 0, sizeof(ec_buffer)); + req->offset = offset; + memcpy(req->data, val, val_len); + ret = cros_ec_cmd(udata->ec, 0, EC_CMD_UCSI_PPM_SET, + req, sizeof(struct ec_params_ucsi_ppm_set) + val_len, NULL, 0); + + if (ret < 0) { + dev_warn(udata->dev, "Failed to send EC message UCSI_PPM_SET: error=%d", ret); + return ret; + } + return 0; +} + +static int cros_ucsi_sync_write(struct ucsi *ucsi, unsigned int offset, + const void *val, size_t val_len) +{ + struct cros_ucsi_data *udata = ucsi_get_drvdata(ucsi); + bool ack = UCSI_COMMAND(*(u64 *)val) == UCSI_ACK_CC_CI; + int ret; + + if (ack) + set_bit(ACK_PENDING, &udata->flags); + else + set_bit(COMMAND_PENDING, &udata->flags); + + ret = cros_ucsi_async_write(ucsi, offset, val, val_len); + if (ret) + goto out; + + if (!wait_for_completion_timeout(&udata->complete, WRITE_TMO_MS)) + ret = -ETIMEDOUT; + +out: + if (ack) + clear_bit(ACK_PENDING, &udata->flags); + else + clear_bit(COMMAND_PENDING, &udata->flags); + return ret; +} + +struct ucsi_operations cros_ucsi_ops = { + .read = cros_ucsi_read, + .async_write = cros_ucsi_async_write, + .sync_write = cros_ucsi_sync_write, +}; + +static void cros_ucsi_work(struct work_struct *work) +{ + struct cros_ucsi_data *udata = container_of(work, struct cros_ucsi_data, work); + u32 cci; + int ret; + + ret = cros_ucsi_read(udata->ucsi, UCSI_CCI, &cci, sizeof(cci)); + if (ret) + return; + + if (UCSI_CCI_CONNECTOR(cci)) + ucsi_connector_change(udata->ucsi, UCSI_CCI_CONNECTOR(cci)); + + if (cci & UCSI_CCI_ACK_COMPLETE && test_bit(ACK_PENDING, &udata->flags)) + complete(&udata->complete); + if (cci & UCSI_CCI_COMMAND_COMPLETE && + test_bit(COMMAND_PENDING, &udata->flags)) + complete(&udata->complete); +} + +static int cros_ucsi_event(struct notifier_block *nb, + unsigned long host_event, void *_notify) +{ + struct cros_ucsi_data *udata = container_of(nb, struct cros_ucsi_data, nb); + + if (!(host_event & PD_EVENT_PPM)) + return NOTIFY_OK; + + dev_dbg(udata->dev, "UCSI notification received"); + flush_work(&udata->work); + schedule_work(&udata->work); + + return NOTIFY_OK; +} + +static int cros_ucsi_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct cros_ec_dev *ec_data = dev_get_drvdata(dev->parent); + struct cros_ucsi_data *udata; + int ret; + + udata = devm_kzalloc(dev, sizeof(*udata), GFP_KERNEL); + if (!udata) + return -ENOMEM; + + udata->dev = dev; + + udata->ec = ec_data->ec_dev; + if (!udata->ec) { + dev_err(dev, "couldn't find parent EC device\n"); + return -ENODEV; + } + + platform_set_drvdata(pdev, udata); + + INIT_WORK(&udata->work, cros_ucsi_work); + init_completion(&udata->complete); + + udata->ucsi = ucsi_create(udata->dev, &cros_ucsi_ops); + if (IS_ERR(udata->ucsi)) { + dev_err(dev, "failed to allocate UCSI instance\n"); + return PTR_ERR(udata->ucsi); + } + + ucsi_set_drvdata(udata->ucsi, udata); + + ret = ucsi_register(udata->ucsi); + if (ret) { + ucsi_destroy(udata->ucsi); + return ret; + } + + udata->nb.notifier_call = cros_ucsi_event; + return cros_usbpd_register_notify(&udata->nb); +} + +static int cros_ucsi_remove(struct platform_device *dev) +{ + struct cros_ucsi_data *udata = platform_get_drvdata(dev); + + ucsi_unregister(udata->ucsi); + ucsi_destroy(udata->ucsi); + return 0; +} + +static int __maybe_unused cros_ucsi_suspend(struct device *dev) +{ + struct cros_ucsi_data *udata = dev_get_drvdata(dev); + + cancel_work_sync(&udata->work); + + return 0; +} + +static int __maybe_unused cros_ucsi_resume(struct device *dev) +{ + struct cros_ucsi_data *udata = dev_get_drvdata(dev); + + return ucsi_resume(udata->ucsi); +} + +static SIMPLE_DEV_PM_OPS(cros_ucsi_pm_ops, cros_ucsi_suspend, + cros_ucsi_resume); + +static const struct platform_device_id cros_ec_ucsi_id[] = { + { "cros-ec-ucsi"}, + {} +}; +MODULE_DEVICE_TABLE(platform, cros_ec_ucsi_id); + +static struct platform_driver cros_ucsi_driver = { + .driver = { + .name = DRV_NAME, + .pm = &cros_ucsi_pm_ops, + }, + .id_table = cros_ec_ucsi_id, + .probe = cros_ucsi_probe, + .remove = cros_ucsi_remove, +}; + +module_platform_driver(cros_ucsi_driver); + +MODULE_LICENSE("GPL"); +MODULE_DESCRIPTION("UCSI driver for ChromeOS EC");
Implementation of a UCSI transport driver for ChromeOS. This driver will be loaded if the ChromeOS EC implements a PPM. Signed-off-by: Pavan Holla <pholla@chromium.org> --- drivers/usb/typec/ucsi/Kconfig | 13 ++ drivers/usb/typec/ucsi/Makefile | 1 + drivers/usb/typec/ucsi/cros_ec_ucsi.c | 245 ++++++++++++++++++++++++++++++++++ 3 files changed, 259 insertions(+)