[PATCH v3 0/2] usb: typec: Implement UCSI driver for ChromeOS

[PATCH v3 0/2] usb: typec: Implement UCSI driver for ChromeOS

[Pavan Holla]

This series implements a UCSI ChromeOS EC transport driver.
The ChromeOS EC is expected to implement a UCSI PPM.

Signed-off-by: Pavan Holla <pholla@chromium.org>
---
Changes in v3:
- Moved driver from platform/chrome to usb/typec/ucsi.
- Used id_table instead of MODULE_ALIAS.
- Split EC header changes into seperate commit.
- Fixes from additional internal reviews and kernel bot warnings.
- Link to v2: https://lore.kernel.org/r/20240325-public-ucsi-h-v2-0-a6d716968bb1@chromium.org

Changes in v2:
- No code or commit message changes.
- Added drivers/platform/chrome maintainers for review.
- Link to v1: https://lore.kernel.org/r/20240325-public-ucsi-h-v1-0-7c7e888edc0a@chromium.org

---
Pavan Holla (2):
      platform/chrome: Update ChromeOS EC header for UCSI
      usb: typec: ucsi: Implement ChromeOS UCSI driver

 drivers/usb/typec/ucsi/Kconfig                 |  13 ++
 drivers/usb/typec/ucsi/Makefile                |   1 +
 drivers/usb/typec/ucsi/cros_ec_ucsi.c          | 245 +++++++++++++++++++++++++
 include/linux/platform_data/cros_ec_commands.h |  20 ++
 4 files changed, 279 insertions(+)
---
base-commit: 4cece764965020c22cff7665b18a012006359095
change-id: 20240325-public-ucsi-h-3ecee4106a58

Best regards,
-- 
Pavan Holla <pholla@chromium.org>

[PATCH v3 1/2] platform/chrome: Update ChromeOS EC header for UCSI

[Pavan Holla]

Add EC host commands for reading and writing UCSI structures
in the EC. The corresponding kernel driver is cros-ec-ucsi.

Also update PD events supported by the EC.

Signed-off-by: Pavan Holla <pholla@chromium.org>
---
 include/linux/platform_data/cros_ec_commands.h | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/include/linux/platform_data/cros_ec_commands.h b/include/linux/platform_data/cros_ec_commands.h
index ecc47d5fe239..c0f6d054a566 100644
--- a/include/linux/platform_data/cros_ec_commands.h
+++ b/include/linux/platform_data/cros_ec_commands.h
@@ -4933,6 +4933,8 @@ struct ec_response_pd_status {
 #define PD_EVENT_POWER_CHANGE      BIT(1)
 #define PD_EVENT_IDENTITY_RECEIVED BIT(2)
 #define PD_EVENT_DATA_SWAP         BIT(3)
+#define PD_EVENT_TYPEC             BIT(4)
+#define PD_EVENT_PPM               BIT(5)
 struct ec_response_host_event_status {
 	uint32_t status;      /* PD MCU host event status */
 } __ec_align4;
@@ -5994,6 +5996,24 @@ struct ec_response_typec_vdm_response {
 
 #undef VDO_MAX_SIZE
 
+/*
+ * Read/write interface for UCSI OPM <-> PPM communication.
+ */
+#define EC_CMD_UCSI_PPM_SET 0x0140
+
+/* The data size is stored in the host command protocol header. */
+struct ec_params_ucsi_ppm_set {
+	uint16_t offset;
+	uint8_t data[];
+} __ec_align2;
+
+#define EC_CMD_UCSI_PPM_GET 0x0141
+
+struct ec_params_ucsi_ppm_get {
+	uint16_t offset;
+	uint8_t size;
+} __ec_align2;
+
 /*****************************************************************************/
 /* The command range 0x200-0x2FF is reserved for Rotor. */
 

-- 
2.44.0.478.gd926399ef9-goog

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

[Pavan Holla]

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);
+		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");

-- 
2.44.0.478.gd926399ef9-goog

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

[Dmitry Baryshkov]

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
> 

-- 
With best wishes
Dmitry

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

[Greg Kroah-Hartman]

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

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

[Dmitry Baryshkov]

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

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

[Greg Kroah-Hartman]

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

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

[Pavan Holla]

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

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

[Tzung-Bi Shih]

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.

Re: [PATCH v3 1/2] platform/chrome: Update ChromeOS EC header for UCSI

[Tzung-Bi Shih]

On Wed, Apr 03, 2024 at 06:05:21PM +0000, Pavan Holla wrote:
> Add EC host commands for reading and writing UCSI structures
> in the EC. The corresponding kernel driver is cros-ec-ucsi.

FWIW, the new host commands haven't been in [1] yet.

[1]: https://chromium.googlesource.com/chromiumos/platform/ec/+/refs/heads/main/include/ec_commands.h

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

[Guenter Roeck]

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

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

[Greg Kroah-Hartman]

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

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

[Dmitry Baryshkov]

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.

-- 
With best wishes
Dmitry

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

[Pavan Holla]

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

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

[Tzung-Bi Shih]

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