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

Heikki Krogerus
2024-09-11T14:09:04Z

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