Re: [PATCH v6 6/8] usb: typec: cros_ec_ucsi: Add netlink

[Heikki Krogerus <heikki.krogerus@linux.intel.com>]

Hi,

On Tue, Sep 10, 2024 at 10:15:25AM +0000, Łukasz Bartosik wrote:
> Add netlink to ChromeOS UCSI driver to allow forwarding
> of UCSI messages to userspace for debugging and testing
> purposes.

Why does this need to be cros_ec specific?

> Signed-off-by: Łukasz Bartosik <ukaszb@chromium.org>
> ---
>  MAINTAINERS                                   |  4 +-
>  drivers/usb/typec/ucsi/Makefile               |  4 +-
>  .../{cros_ec_ucsi.c => cros_ec_ucsi_main.c}   | 66 +++++++++++++-
>  drivers/usb/typec/ucsi/cros_ec_ucsi_nl.c      | 87 +++++++++++++++++++
>  drivers/usb/typec/ucsi/cros_ec_ucsi_nl.h      | 52 +++++++++++
>  5 files changed, 209 insertions(+), 4 deletions(-)
>  rename drivers/usb/typec/ucsi/{cros_ec_ucsi.c => cros_ec_ucsi_main.c} (79%)
>  create mode 100644 drivers/usb/typec/ucsi/cros_ec_ucsi_nl.c
>  create mode 100644 drivers/usb/typec/ucsi/cros_ec_ucsi_nl.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index d084f32208f0..2afb406a24ce 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -5305,7 +5305,9 @@ M:	Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
>  M:	Łukasz Bartosik <ukaszb@chromium.org>
>  L:	chrome-platform@lists.linux.dev
>  S:	Maintained
> -F:	drivers/usb/typec/ucsi/cros_ec_ucsi.c
> +F:	drivers/usb/typec/ucsi/cros_ec_ucsi_main.c
> +F:	drivers/usb/typec/ucsi/cros_ec_ucsi_nl.c
> +F:	drivers/usb/typec/ucsi/cros_ec_ucsi_nl.h
>  F:	drivers/usb/typec/ucsi/cros_ec_ucsi_trace.h
>  
>  CHRONTEL CH7322 CEC DRIVER
> diff --git a/drivers/usb/typec/ucsi/Makefile b/drivers/usb/typec/ucsi/Makefile
> index be98a879104d..82d960394c39 100644
> --- a/drivers/usb/typec/ucsi/Makefile
> +++ b/drivers/usb/typec/ucsi/Makefile
> @@ -21,5 +21,7 @@ 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
>  obj-$(CONFIG_UCSI_LENOVO_YOGA_C630)	+= ucsi_yoga_c630.o
> +
> +obj-$(CONFIG_CROS_EC_UCSI)		+= cros_ec_ucsi.o
> +cros_ec_ucsi-y				:= cros_ec_ucsi_main.o cros_ec_ucsi_nl.o
> diff --git a/drivers/usb/typec/ucsi/cros_ec_ucsi.c b/drivers/usb/typec/ucsi/cros_ec_ucsi_main.c
> similarity index 79%
> rename from drivers/usb/typec/ucsi/cros_ec_ucsi.c
> rename to drivers/usb/typec/ucsi/cros_ec_ucsi_main.c
> index 70185616ec86..008b61921278 100644
> --- a/drivers/usb/typec/ucsi/cros_ec_ucsi.c
> +++ b/drivers/usb/typec/ucsi/cros_ec_ucsi_main.c
> @@ -19,6 +19,7 @@
>  #define CREATE_TRACE_POINTS
>  #include "ucsi.h"
>  #include "cros_ec_ucsi_trace.h"
> +#include "cros_ec_ucsi_nl.h"
>  
>  /*
>   * Maximum size in bytes of a UCSI message between AP and EC
> @@ -43,6 +44,43 @@ struct cros_ucsi_data {
>  	unsigned long flags;
>  };
>  
> +/*
> + * When set to true the cros_ec_ucsi driver will forward all UCSI messages
> + * exchanged between OPM <-> PPM to userspace through netlink
> + */
> +static bool is_ap_sniffer_en;
> +
> +static ssize_t enable_ap_sniffer_show(struct device *dev,
> +				      struct device_attribute *attr,
> +				      char *buf)
> +{
> +	return sprintf(buf, "%d\n", is_ap_sniffer_en);
> +}
> +
> +static ssize_t enable_ap_sniffer_store(struct device *dev,
> +				       struct device_attribute *attr,
> +				       const char *buf, size_t count)
> +{
> +	u8 value;
> +
> +	if (kstrtou8(buf, 0, &value))
> +		return -EINVAL;
> +
> +	is_ap_sniffer_en = value ? 1 : 0;
> +	return count;
> +}
> +
> +static DEVICE_ATTR_RW(enable_ap_sniffer);
> +
> +static struct attribute *cros_ec_ucsi_attrs[] = {
> +	&dev_attr_enable_ap_sniffer.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group cros_ec_ucsi_attrs_grp = {
> +	.attrs = cros_ec_ucsi_attrs,
> +};

Undocumented sysfs entry.

>  static int cros_ucsi_read(struct ucsi *ucsi, unsigned int offset, void *val,
>  			  size_t val_len)
>  {
> @@ -65,6 +103,9 @@ static int cros_ucsi_read(struct ucsi *ucsi, unsigned int offset, void *val,
>  		return ret;
>  	}
>  
> +	if (is_ap_sniffer_en)
> +		nl_cros_ec_bcast_msg(NL_CROS_EC_TO_PPM, NL_CROS_EC_RD, offset,
> +				     val, val_len);
>  	trace_cros_ec_opm_to_ppm_rd(offset, val, val_len);
>  	return 0;
>  }
> @@ -101,6 +142,9 @@ static int cros_ucsi_async_control(struct ucsi *ucsi, u64 cmd)
>  		return ret;
>  	}
>  
> +	if (is_ap_sniffer_en)
> +		nl_cros_ec_bcast_msg(NL_CROS_EC_TO_PPM, NL_CROS_EC_WR,
> +				     req->offset, (u8 *) &cmd, sizeof(cmd));
>  	trace_cros_ec_opm_to_ppm_wr(req->offset, &cmd, sizeof(cmd));
>  	return 0;
>  }
> @@ -144,6 +188,8 @@ static void cros_ucsi_work(struct work_struct *work)
>  	struct cros_ucsi_data *udata = container_of(work, struct cros_ucsi_data, work);
>  	u32 cci;
>  
> +	if (is_ap_sniffer_en)
> +		nl_cros_ec_bcast_msg(NL_CROS_EC_TO_OPM, 0, 0, NULL, 0);
>  	trace_cros_ec_ppm_to_opm(0);
>  
>  	if (cros_ucsi_read_cci(udata->ucsi, &cci))
> @@ -229,13 +275,29 @@ static int cros_ucsi_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> +	ret = nl_cros_ec_register();
> +	if (ret) {
> +		dev_err(dev, "failed to register netlink: error=%d", ret);
> +		cros_ucsi_destroy(udata);
> +		return ret;
> +	}
> +
> +	ret = sysfs_create_group(&dev->kobj, &cros_ec_ucsi_attrs_grp);
> +	if (ret) {
> +		dev_err(dev, "failed to register sysfs group: error=%d", ret);
> +		cros_ucsi_destroy(udata);
> +		return ret;
> +	}
> +
>  	return 0;
>  }
>  
> -static void cros_ucsi_remove(struct platform_device *dev)
> +static void cros_ucsi_remove(struct platform_device *pdev)
>  {
> -	struct cros_ucsi_data *udata = platform_get_drvdata(dev);
> +	struct cros_ucsi_data *udata = platform_get_drvdata(pdev);

Please merge that change into the patch 3/8.

> +	sysfs_remove_group(&pdev->dev.kobj, &cros_ec_ucsi_attrs_grp);
> +	nl_cros_ec_unregister();
>  	ucsi_unregister(udata->ucsi);
>  	cros_ucsi_destroy(udata);
>  }

thanks,

-- 
heikki