Re: [PATCH v3 2/2] usb: typec: ucsi: Implement ChromeOS UCSI driver

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

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