| Message ID | 20240910101527.603452-7-ukaszb@chromium.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v6,1/8] platform/chrome: Update ChromeOS EC header for UCSI | expand |
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,
On Wed, Sep 11, 2024 at 4:09 PM Heikki Krogerus <heikki.krogerus@linux.intel.com> wrote: > > 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? > You're right. Netlink does not have to be cros_ec_ucsi specific. Would you like to have netlink in typec_ucsi ? > > 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. > I will document the new 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. > I will merge this change to the"usb: typec: ucsi: Implement ChromeOS UCSI driver". Thanks, Lukasz > > + 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
On Sun, Sep 15, 2024 at 12:08:45AM +0200, Łukasz Bartosik wrote: > On Wed, Sep 11, 2024 at 4:09 PM Heikki Krogerus > <heikki.krogerus@linux.intel.com> wrote: > > > > 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? > > > > You're right. Netlink does not have to be cros_ec_ucsi specific. > Would you like to have netlink in typec_ucsi ? Does it need to be netlink? We would then have tracepoints, the custom debugfs interface, and this netlink interface. I think this information could be exposed via trancepoints (unless I'm missing something). Br,
On Thu, Sep 19, 2024 at 11:38 AM Heikki Krogerus <heikki.krogerus@linux.intel.com> wrote: > > On Sun, Sep 15, 2024 at 12:08:45AM +0200, Łukasz Bartosik wrote: > > On Wed, Sep 11, 2024 at 4:09 PM Heikki Krogerus > > <heikki.krogerus@linux.intel.com> wrote: > > > > > > 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? > > > > > > > You're right. Netlink does not have to be cros_ec_ucsi specific. > > Would you like to have netlink in typec_ucsi ? > > Does it need to be netlink? We would then have tracepoints, the > custom debugfs interface, and this netlink interface. > > I think this information could be exposed via trancepoints (unless I'm > missing something). > Hi Heikki, I agree that there is a common area which is covered by both trace events and netlink. However netlink also has advantages which IMHO trace events lack. One of our cases is that from userspace it is easy to forward the UCSI messages to a Wireshark with a plugin which can decode it. Another case is to use UCSI forwarded messages through netlink for testing and validation of chromebooks. How about leaving netlink specific to cros_ec_ucsi driver ? Would you consent to that ? Thanks, Lukasz > Br, > > -- > heikki
On Thu, Sep 19, 2024 at 08:03:37PM GMT, Łukasz Bartosik wrote: > On Thu, Sep 19, 2024 at 11:38 AM Heikki Krogerus > <heikki.krogerus@linux.intel.com> wrote: > > > > On Sun, Sep 15, 2024 at 12:08:45AM +0200, Łukasz Bartosik wrote: > > > On Wed, Sep 11, 2024 at 4:09 PM Heikki Krogerus > > > <heikki.krogerus@linux.intel.com> wrote: > > > > > > > > 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? > > > > > > > > > > You're right. Netlink does not have to be cros_ec_ucsi specific. > > > Would you like to have netlink in typec_ucsi ? > > > > Does it need to be netlink? We would then have tracepoints, the > > custom debugfs interface, and this netlink interface. > > > > I think this information could be exposed via trancepoints (unless I'm > > missing something). > > > > Hi Heikki, > > I agree that there is a common area which is covered by both trace > events and netlink. > However netlink also has advantages which IMHO trace events lack. One > of our cases is that > from userspace it is easy to forward the UCSI messages to a Wireshark > with a plugin > which can decode it. Another case is to use UCSI forwarded messages > through netlink > for testing and validation of chromebooks. > > How about leaving netlink specific to cros_ec_ucsi driver ? Would you > consent to that ? I think having it specific to cros_ec_ucsi is the worst option out of three. It should either be generified to work with all UCSI drivers or go away and be replaced by tracepoints (against, generic to all UCSI drivers) or some other mechanism (e.g. TCPM has rolling log of messages).
Hi Łukasz, kernel test robot noticed the following build errors: [auto build test ERROR on usb/usb-testing] [also build test ERROR on usb/usb-next usb/usb-linus lee-mfd/for-mfd-next driver-core/driver-core-testing driver-core/driver-core-next driver-core/driver-core-linus linus/master v6.11 next-20240920] [cannot apply to lee-mfd/for-mfd-fixes] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/ukasz-Bartosik/platform-chrome-Update-ChromeOS-EC-header-for-UCSI/20240910-182729 base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing patch link: https://lore.kernel.org/r/20240910101527.603452-7-ukaszb%40chromium.org patch subject: [PATCH v6 6/8] usb: typec: cros_ec_ucsi: Add netlink config: x86_64-buildonly-randconfig-005-20240921 (https://download.01.org/0day-ci/archive/20240921/202409211431.LpvxyX25-lkp@intel.com/config) compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240921/202409211431.LpvxyX25-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202409211431.LpvxyX25-lkp@intel.com/ All errors (new ones prefixed by >>, old ones prefixed by <<): WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/fpga/tests/fpga-mgr-test.o WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/fpga/tests/fpga-bridge-test.o WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/fpga/tests/fpga-region-test.o >> ERROR: modpost: "genl_register_family" [drivers/usb/typec/ucsi/cros_ec_ucsi.ko] undefined! >> ERROR: modpost: "genl_unregister_family" [drivers/usb/typec/ucsi/cros_ec_ucsi.ko] undefined! >> ERROR: modpost: "__alloc_skb" [drivers/usb/typec/ucsi/cros_ec_ucsi.ko] undefined! >> ERROR: modpost: "genlmsg_put" [drivers/usb/typec/ucsi/cros_ec_ucsi.ko] undefined! >> ERROR: modpost: "nla_put" [drivers/usb/typec/ucsi/cros_ec_ucsi.ko] undefined! >> ERROR: modpost: "skb_trim" [drivers/usb/typec/ucsi/cros_ec_ucsi.ko] undefined! >> ERROR: modpost: "sk_skb_reason_drop" [drivers/usb/typec/ucsi/cros_ec_ucsi.ko] undefined! >> ERROR: modpost: "init_net" [drivers/usb/typec/ucsi/cros_ec_ucsi.ko] undefined! >> ERROR: modpost: "netlink_broadcast_filtered" [drivers/usb/typec/ucsi/cros_ec_ucsi.ko] undefined!
On Thu, Sep 19, 2024 at 10:00 PM Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > On Thu, Sep 19, 2024 at 08:03:37PM GMT, Łukasz Bartosik wrote: > > On Thu, Sep 19, 2024 at 11:38 AM Heikki Krogerus > > <heikki.krogerus@linux.intel.com> wrote: > > > > > > On Sun, Sep 15, 2024 at 12:08:45AM +0200, Łukasz Bartosik wrote: > > > > On Wed, Sep 11, 2024 at 4:09 PM Heikki Krogerus > > > > <heikki.krogerus@linux.intel.com> wrote: > > > > > > > > > > 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? > > > > > > > > > > > > > You're right. Netlink does not have to be cros_ec_ucsi specific. > > > > Would you like to have netlink in typec_ucsi ? > > > > > > Does it need to be netlink? We would then have tracepoints, the > > > custom debugfs interface, and this netlink interface. > > > > > > I think this information could be exposed via trancepoints (unless I'm > > > missing something). > > > > > > > Hi Heikki, > > > > I agree that there is a common area which is covered by both trace > > events and netlink. > > However netlink also has advantages which IMHO trace events lack. One > > of our cases is that > > from userspace it is easy to forward the UCSI messages to a Wireshark > > with a plugin > > which can decode it. Another case is to use UCSI forwarded messages > > through netlink > > for testing and validation of chromebooks. > > > > How about leaving netlink specific to cros_ec_ucsi driver ? Would you > > consent to that ? > > I think having it specific to cros_ec_ucsi is the worst option out of > three. It should either be generified to work with all UCSI drivers or > go away and be replaced by tracepoints (against, generic to all UCSI > drivers) or some other mechanism (e.g. TCPM has rolling log of > messages). > I will come up with a proposal to make netlink generic to all UCSI drivers. Thanks, Lukasz > -- > With best wishes > Dmitry
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, +}; + 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); + sysfs_remove_group(&pdev->dev.kobj, &cros_ec_ucsi_attrs_grp); + nl_cros_ec_unregister(); ucsi_unregister(udata->ucsi); cros_ucsi_destroy(udata); } diff --git a/drivers/usb/typec/ucsi/cros_ec_ucsi_nl.c b/drivers/usb/typec/ucsi/cros_ec_ucsi_nl.c new file mode 100644 index 000000000000..360568044891 --- /dev/null +++ b/drivers/usb/typec/ucsi/cros_ec_ucsi_nl.c @@ -0,0 +1,87 @@ +// SPDX-License-Identifier: GPL-2.0 + +#include <net/genetlink.h> +#include "cros_ec_ucsi_nl.h" + +static const struct genl_multicast_group nl_mc_grps[] = { + { .name = NL_CROS_EC_MC_GRP_NAME }, +}; + +static struct genl_family genl_fam = { + .name = NL_CROS_EC_NAME, + .version = NL_CROS_EC_VER, + .maxattr = NL_CROS_EC_A_MAX, + .mcgrps = nl_mc_grps, + .n_mcgrps = ARRAY_SIZE(nl_mc_grps), +}; + +int nl_cros_ec_register(void) +{ + return genl_register_family(&genl_fam); +} + +void nl_cros_ec_unregister(void) +{ + genl_unregister_family(&genl_fam); +} + +int nl_cros_ec_bcast_msg(enum nl_cros_ec_msg_dir dir, + enum nl_cros_ec_cmd_type cmd_type, + u16 offset, const u8 *payload, size_t msg_size) +{ + struct timespec64 ts; + struct sk_buff *skb; + int ret = -ENOMEM; + void *hdr; + + skb = genlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL); + if (!skb) + return -ENOMEM; + + hdr = genlmsg_put(skb, 0, 0, &genl_fam, 0, NL_CROS_EC_C_UCSI); + if (!hdr) + goto free_mem; + + ret = nla_put_u8(skb, NL_CROS_EC_A_SRC, NL_CROS_EC_AP); + if (ret) + goto cancel; + + ret = nla_put_u8(skb, NL_CROS_EC_A_DIR, dir); + if (ret) + goto cancel; + + ret = nla_put_u16(skb, NL_CROS_EC_A_OFFSET, offset); + if (ret) + goto cancel; + + ret = nla_put_u8(skb, NL_CROS_EC_A_CMD_TYPE, cmd_type); + if (ret) + goto cancel; + + ktime_get_ts64(&ts); + ret = nla_put_u32(skb, NL_CROS_EC_A_TSTAMP_SEC, (u32)ts.tv_sec); + if (ret) + goto cancel; + + ret = nla_put_u32(skb, NL_CROS_EC_A_TSTAMP_USEC, + (u32)(ts.tv_nsec/1000)); + if (ret) + goto cancel; + + ret = nla_put(skb, NL_CROS_EC_A_PAYLOAD, msg_size, payload); + if (ret) + goto cancel; + + genlmsg_end(skb, hdr); + + ret = genlmsg_multicast(&genl_fam, skb, 0, 0, GFP_KERNEL); + if (ret && ret != -ESRCH) + goto free_mem; + + return 0; +cancel: + genlmsg_cancel(skb, hdr); +free_mem: + nlmsg_free(skb); + return ret; +} diff --git a/drivers/usb/typec/ucsi/cros_ec_ucsi_nl.h b/drivers/usb/typec/ucsi/cros_ec_ucsi_nl.h new file mode 100644 index 000000000000..c6192d8ace56 --- /dev/null +++ b/drivers/usb/typec/ucsi/cros_ec_ucsi_nl.h @@ -0,0 +1,52 @@ +/* SPDX-License-Identifier: GPL-2.0 */ + +#ifndef __DRIVER_USB_TYPEC_CROS_EC_UCSI_NL_H +#define __DRIVER_USB_TYPEC_CROS_EC_UCSI_NL_H + +#define NL_CROS_EC_NAME "cros_ec_ucsi" +#define NL_CROS_EC_VER 1 +#define NL_CROS_EC_MC_GRP_NAME "cros_ec_ucsi_mc" + +/* attributes */ +enum nl_cros_ec_attrs { + NL_CROS_EC_A_SRC, + NL_CROS_EC_A_DIR, + NL_CROS_EC_A_OFFSET, + NL_CROS_EC_A_CMD_TYPE, + NL_CROS_EC_A_TSTAMP_SEC, + NL_CROS_EC_A_TSTAMP_USEC, + NL_CROS_EC_A_PAYLOAD, + NL_CROS_EC_A_MAX +}; + +enum nl_cros_ec_cmds { + NL_CROS_EC_C_UCSI, + NL_CROS_EC_C_MAX +}; + +/* where message was captured - EC or AP */ +enum nl_cros_ec_src { + NL_CROS_EC_AP, + NL_CROS_EC_EC +}; + +/* message destination */ +enum nl_cros_ec_msg_dir { + NL_CROS_EC_TO_PPM, + NL_CROS_EC_TO_OPM, + NL_CROS_EC_TO_LPM +}; + +/* command type - read or write */ +enum nl_cros_ec_cmd_type { + NL_CROS_EC_RD, + NL_CROS_EC_WR +}; + +int nl_cros_ec_register(void); +void nl_cros_ec_unregister(void); +int nl_cros_ec_bcast_msg(enum nl_cros_ec_msg_dir dir, + enum nl_cros_ec_cmd_type cmd_type, + u16 offset, const u8 *payload, size_t msg_size); + +#endif /* __DRIVER_USB_TYPEC_CROS_EC_UCSI_NL_H */
Add netlink to ChromeOS UCSI driver to allow forwarding of UCSI messages to userspace for debugging and testing purposes. 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