Received-SPF: pass (google.com: domain of linux-kernel+bounces-131496-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Thu, 4 Apr 2024 16:20:30 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Pavan Holla <pholla@chromium.org>, 
	Heikki Krogerus <heikki.krogerus@linux.intel.com>, Benson Leung <bleung@chromium.org>, 
	Tzung-Bi Shih <tzungbi@kernel.org>, Guenter Roeck <groeck@chromium.org>, 
	linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org, 
	Abhishek Pandit-Subedi <abhishekpandit@chromium.org>, chrome-platform@lists.linux.dev
Subject: Re: [PATCH v3 2/2] usb: typec: ucsi: Implement ChromeOS UCSI driver
Message-ID: <oi3bwdyvyaezpmyxfdtsbiwwprxi2ufc3hlzoz23d5rxdkperl@cxpd7enatg7h>
References: <20240403-public-ucsi-h-v3-0-f848e18c8ed2@chromium.org>
 <20240403-public-ucsi-h-v3-2-f848e18c8ed2@chromium.org>
 <3ezjocthsigo3t746slmgzffnmpxw7wwf3s535basiaf2qy6io@7ocxva6ndsbt>
 <2024040449-average-foyer-defa@gregkh>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <2024040449-average-foyer-defa@gregkh>

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

-- 
With best wishes
Dmitry