Re: [PATCH v3 2/2] usb: typec: ucsi: Implement ChromeOS UCSI driver
From: Greg Kroah-Hartman
Date: Thu Apr 04 2024 - 09:30:57 EST
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@xxxxxxxxxxxx>
> > > > ---
> > > > 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