| Message ID | 20200731114732.12815-6-arnaud.pouliquen@st.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | introduce name service announcement rpmsg driver | expand |
Hi Arnaud, I love your patch! Perhaps something to improve: [auto build test WARNING on next-20200730] [also build test WARNING on v5.8] [cannot apply to linux/master linus/master rpmsg/for-next v5.8-rc7 v5.8-rc6 v5.8-rc5] [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] url: https://github.com/0day-ci/linux/commits/Arnaud-Pouliquen/introduce-name-service-announcement-rpmsg-driver/20200731-195014 base: 7b287a5c6ac518c415a258f2aa7b1ebb25c263d2 compiler: nds32le-linux-gcc (GCC) 9.3.0 If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> cppcheck warnings: (new ones prefixed by >>) >> drivers/rpmsg/rpmsg_ns.c:68:34: warning: Clarify calculation precedence for '&' and '?'. [clarifyCalculation] msg->flags & RPMSG_NS_DESTROY ? "destroy" : "creat", ^ vim +68 drivers/rpmsg/rpmsg_ns.c 54 55 if (len != sizeof(*msg)) { 56 dev_err(dev, "malformed ns msg (%d)\n", len); 57 return -EINVAL; 58 } 59 60 /* don't trust the remote processor for null terminating the name */ 61 msg->name[RPMSG_NAME_SIZE - 1] = '\0'; 62 63 strncpy(chinfo.name, msg->name, sizeof(chinfo.name)); 64 chinfo.src = RPMSG_ADDR_ANY; 65 chinfo.dst = msg->addr; 66 67 dev_info(dev, "%sing channel %s addr 0x%x\n", > 68 msg->flags & RPMSG_NS_DESTROY ? "destroy" : "creat", 69 msg->name, msg->addr); 70 71 if (msg->flags & RPMSG_NS_DESTROY) { 72 ret = rpmsg_release_channel(rpdev, &chinfo); 73 if (ret) 74 dev_err(dev, "rpmsg_destroy_channel failed: %d\n", ret); 75 } else { 76 newch = rpmsg_create_channel(rpdev, &chinfo); 77 if (!newch) 78 dev_err(dev, "rpmsg_create_channel failed\n"); 79 } 80 81 return 0; 82 } 83 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On Fri, Jul 31, 2020 at 01:47:28PM +0200, Arnaud Pouliquen wrote: > The name service announcement should not be linked to the RPMsg virtio bus > but to the RPMsg protocol itself. > > This patch proposes to break the dependency with the RPmsg virtio bus by > the introduction of the reserved RPMsg name service driver which will be in > charge of managing the RPMsg name service announcement. > > This first patch only implements the probe and the RPMsg endpoint to > manage create and release channels remote requests. > > Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com> > --- > drivers/rpmsg/Kconfig | 8 ++ > drivers/rpmsg/Makefile | 1 + > drivers/rpmsg/rpmsg_internal.h | 17 +++++ > drivers/rpmsg/rpmsg_ns.c | 135 +++++++++++++++++++++++++++++++++ > 4 files changed, 161 insertions(+) > create mode 100644 drivers/rpmsg/rpmsg_ns.c > > diff --git a/drivers/rpmsg/Kconfig b/drivers/rpmsg/Kconfig > index f96716893c2a..140faa975ea1 100644 > --- a/drivers/rpmsg/Kconfig > +++ b/drivers/rpmsg/Kconfig > @@ -15,6 +15,14 @@ config RPMSG_CHAR > in /dev. They make it possible for user-space programs to send and > receive rpmsg packets. > > +config RPMSG_NS > + tristate "RPMSG name service announcement" > + depends on RPMSG > + help > + Say Y here to enable the support of the name service announcement > + channel that probes the associate RPMsg device on remote endpoint s/associate/associated > + service announcement. > + > config RPMSG_MTK_SCP > tristate "MediaTek SCP" > depends on MTK_SCP > diff --git a/drivers/rpmsg/Makefile b/drivers/rpmsg/Makefile > index ffe932ef6050..8d452656f0ee 100644 > --- a/drivers/rpmsg/Makefile > +++ b/drivers/rpmsg/Makefile > @@ -1,6 +1,7 @@ > # SPDX-License-Identifier: GPL-2.0 > obj-$(CONFIG_RPMSG) += rpmsg_core.o > obj-$(CONFIG_RPMSG_CHAR) += rpmsg_char.o > +obj-$(CONFIG_RPMSG_NS) += rpmsg_ns.o > obj-$(CONFIG_RPMSG_MTK_SCP) += mtk_rpmsg.o > qcom_glink-objs := qcom_glink_native.o qcom_glink_ssr.o > obj-$(CONFIG_RPMSG_QCOM_GLINK) += qcom_glink.o > diff --git a/drivers/rpmsg/rpmsg_internal.h b/drivers/rpmsg/rpmsg_internal.h > index d5ab286d0e5e..641b48f6bf2a 100644 > --- a/drivers/rpmsg/rpmsg_internal.h > +++ b/drivers/rpmsg/rpmsg_internal.h > @@ -102,4 +102,21 @@ static inline int rpmsg_chrdev_register_device(struct rpmsg_device *rpdev) > return rpmsg_register_device(rpdev); > } > > +/** > + * rpmsg_ns_register_device() - register name service device based on rpdev > + * @rpdev: prepared rpdev to be used for creating endpoints > + * > + * This function wraps rpmsg_register_device() preparing the rpdev for use as > + * basis for the rpmsg name service device. > + */ > +static inline int rpmsg_ns_register_device(struct rpmsg_device *rpdev) > +{ > + strcpy(rpdev->id.name, "rpmsg_ns"); > + rpdev->driver_override = "rpmsg_ns"; > + rpdev->src = RPMSG_NS_ADDR; > + rpdev->dst = RPMSG_NS_ADDR; > + > + return rpmsg_register_device(rpdev); > +} > + > #endif > diff --git a/drivers/rpmsg/rpmsg_ns.c b/drivers/rpmsg/rpmsg_ns.c > new file mode 100644 > index 000000000000..fe7713e737c2 > --- /dev/null > +++ b/drivers/rpmsg/rpmsg_ns.c > @@ -0,0 +1,135 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (C) STMicroelectronics 2020 - All Rights Reserved > + */ > +#include <linux/device.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/slab.h> > +#include "rpmsg_internal.h" > + > +/** > + * enum rpmsg_ns_flags - dynamic name service announcement flags > + * > + * @RPMSG_NS_CREATE: a new remote service was just created > + * @RPMSG_NS_DESTROY: a known remote service was just destroyed > + */ > +enum rpmsg_ns_flags { > + RPMSG_NS_CREATE = 0, > + RPMSG_NS_DESTROY = 1, > +}; > + > +/** > + * struct rpmsg_ns_msg - dynamic name service announcement message > + * @name: name of remote service that is published > + * @addr: address of remote service that is published > + * @flags: indicates whether service is created or destroyed > + * > + * This message is sent across to publish a new service, or announce > + * about its removal. When we receive these messages, an appropriate > + * rpmsg channel (i.e device) is created/destroyed. In turn, the ->probe() > + * or ->remove() handler of the appropriate rpmsg driver will be invoked > + * (if/as-soon-as one is registered). > + */ > +struct rpmsg_ns_msg { > + char name[RPMSG_NAME_SIZE]; > + u32 addr; > + u32 flags; > +} __packed; > + > +/* invoked when a name service announcement arrives */ > +static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void *data, int len, > + void *priv, u32 src) > +{ > + struct rpmsg_ns_msg *msg = data; > + struct rpmsg_device *newch; > + struct rpmsg_channel_info chinfo; > + struct device *dev = &rpdev->dev; > + int ret; > + > +#if defined(CONFIG_DYNAMIC_DEBUG) > + dynamic_hex_dump("NS announcement: ", DUMP_PREFIX_NONE, 16, 1, > + data, len, true); > +#endif > + > + if (len != sizeof(*msg)) { > + dev_err(dev, "malformed ns msg (%d)\n", len); > + return -EINVAL; > + } > + > + /* don't trust the remote processor for null terminating the name */ > + msg->name[RPMSG_NAME_SIZE - 1] = '\0'; > + > + strncpy(chinfo.name, msg->name, sizeof(chinfo.name)); > + chinfo.src = RPMSG_ADDR_ANY; > + chinfo.dst = msg->addr; > + > + dev_info(dev, "%sing channel %s addr 0x%x\n", > + msg->flags & RPMSG_NS_DESTROY ? "destroy" : "creat", > + msg->name, msg->addr); > + > + if (msg->flags & RPMSG_NS_DESTROY) { > + ret = rpmsg_release_channel(rpdev, &chinfo); > + if (ret) > + dev_err(dev, "rpmsg_destroy_channel failed: %d\n", ret); > + } else { > + newch = rpmsg_create_channel(rpdev, &chinfo); > + if (!newch) > + dev_err(dev, "rpmsg_create_channel failed\n"); > + } > + > + return 0; > +} > + > +static int rpmsg_ns_probe(struct rpmsg_device *rpdev) > +{ > + struct rpmsg_channel_info ns_chinfo; > + struct rpmsg_endpoint *ns_ept; > + > + ns_chinfo.src = RPMSG_NS_ADDR; > + ns_chinfo.dst = RPMSG_NS_ADDR; > + strcpy(ns_chinfo.name, "name_service"); > + > + /* > + * create and attach the endpoint to the rpmsg device that it would be > + * destroy when the rpmsg device will be deleted > + */ This comment doesn't work, please revise. > + ns_ept = rpmsg_create_ept(rpdev, rpmsg_ns_cb, NULL, ns_chinfo); > + if (!ns_ept) { > + dev_err(&rpdev->dev, "failed to create the ns ept\n"); > + return -ENOMEM; > + } > + rpdev->ept = ns_ept; > + > + rpdev->src = RPMSG_NS_ADDR; I think this is already done in rpmsg_ns_register_device(). > + > + return 0; > +} > + > +static struct rpmsg_driver rpmsg_ns_driver = { > + .drv.name = "rpmsg_ns", > + .probe = rpmsg_ns_probe, > +}; > + > +static int rpmsg_ns_init(void) > +{ > + int ret; > + > + ret = register_rpmsg_driver(&rpmsg_ns_driver); > + if (ret < 0) > + pr_err("%s: Failed to register rpmsg driver\n", __func__); > + > + return ret; > +} > +postcore_initcall(rpmsg_ns_init); > + > +static void rpmsg_ns_exit(void) > +{ > + unregister_rpmsg_driver(&rpmsg_ns_driver); > +} > +module_exit(rpmsg_ns_exit); > + > +MODULE_DESCRIPTION("Name service announcement rpmsg Driver"); > +MODULE_AUTHOR("Arnaud Pouliquen <arnaud.pouliquen@st.com>"); > +MODULE_ALIAS("rpmsg_ns"); > +MODULE_LICENSE("GPL v2"); > -- > 2.17.1 >
Hi Mathieu Thanks for the review! please find few comments below. On 8/25/20 12:47 AM, Mathieu Poirier wrote: > On Fri, Jul 31, 2020 at 01:47:28PM +0200, Arnaud Pouliquen wrote: >> The name service announcement should not be linked to the RPMsg virtio bus >> but to the RPMsg protocol itself. >> >> This patch proposes to break the dependency with the RPmsg virtio bus by >> the introduction of the reserved RPMsg name service driver which will be in >> charge of managing the RPMsg name service announcement. >> >> This first patch only implements the probe and the RPMsg endpoint to >> manage create and release channels remote requests. >> >> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com> >> --- >> drivers/rpmsg/Kconfig | 8 ++ >> drivers/rpmsg/Makefile | 1 + >> drivers/rpmsg/rpmsg_internal.h | 17 +++++ >> drivers/rpmsg/rpmsg_ns.c | 135 +++++++++++++++++++++++++++++++++ >> 4 files changed, 161 insertions(+) >> create mode 100644 drivers/rpmsg/rpmsg_ns.c >> >> diff --git a/drivers/rpmsg/Kconfig b/drivers/rpmsg/Kconfig >> index f96716893c2a..140faa975ea1 100644 >> --- a/drivers/rpmsg/Kconfig >> +++ b/drivers/rpmsg/Kconfig >> @@ -15,6 +15,14 @@ config RPMSG_CHAR >> in /dev. They make it possible for user-space programs to send and >> receive rpmsg packets. >> >> +config RPMSG_NS >> + tristate "RPMSG name service announcement" >> + depends on RPMSG >> + help >> + Say Y here to enable the support of the name service announcement >> + channel that probes the associate RPMsg device on remote endpoint > > s/associate/associated > >> + service announcement. >> + >> config RPMSG_MTK_SCP >> tristate "MediaTek SCP" >> depends on MTK_SCP >> diff --git a/drivers/rpmsg/Makefile b/drivers/rpmsg/Makefile >> index ffe932ef6050..8d452656f0ee 100644 >> --- a/drivers/rpmsg/Makefile >> +++ b/drivers/rpmsg/Makefile >> @@ -1,6 +1,7 @@ >> # SPDX-License-Identifier: GPL-2.0 >> obj-$(CONFIG_RPMSG) += rpmsg_core.o >> obj-$(CONFIG_RPMSG_CHAR) += rpmsg_char.o >> +obj-$(CONFIG_RPMSG_NS) += rpmsg_ns.o >> obj-$(CONFIG_RPMSG_MTK_SCP) += mtk_rpmsg.o >> qcom_glink-objs := qcom_glink_native.o qcom_glink_ssr.o >> obj-$(CONFIG_RPMSG_QCOM_GLINK) += qcom_glink.o >> diff --git a/drivers/rpmsg/rpmsg_internal.h b/drivers/rpmsg/rpmsg_internal.h >> index d5ab286d0e5e..641b48f6bf2a 100644 >> --- a/drivers/rpmsg/rpmsg_internal.h >> +++ b/drivers/rpmsg/rpmsg_internal.h >> @@ -102,4 +102,21 @@ static inline int rpmsg_chrdev_register_device(struct rpmsg_device *rpdev) >> return rpmsg_register_device(rpdev); >> } >> >> +/** >> + * rpmsg_ns_register_device() - register name service device based on rpdev >> + * @rpdev: prepared rpdev to be used for creating endpoints >> + * >> + * This function wraps rpmsg_register_device() preparing the rpdev for use as >> + * basis for the rpmsg name service device. >> + */ >> +static inline int rpmsg_ns_register_device(struct rpmsg_device *rpdev) >> +{ >> + strcpy(rpdev->id.name, "rpmsg_ns"); >> + rpdev->driver_override = "rpmsg_ns"; >> + rpdev->src = RPMSG_NS_ADDR; >> + rpdev->dst = RPMSG_NS_ADDR; >> + >> + return rpmsg_register_device(rpdev); >> +} >> + >> #endif >> diff --git a/drivers/rpmsg/rpmsg_ns.c b/drivers/rpmsg/rpmsg_ns.c >> new file mode 100644 >> index 000000000000..fe7713e737c2 >> --- /dev/null >> +++ b/drivers/rpmsg/rpmsg_ns.c >> @@ -0,0 +1,135 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Copyright (C) STMicroelectronics 2020 - All Rights Reserved >> + */ >> +#include <linux/device.h> >> +#include <linux/kernel.h> >> +#include <linux/module.h> >> +#include <linux/slab.h> >> +#include "rpmsg_internal.h" >> + >> +/** >> + * enum rpmsg_ns_flags - dynamic name service announcement flags >> + * >> + * @RPMSG_NS_CREATE: a new remote service was just created >> + * @RPMSG_NS_DESTROY: a known remote service was just destroyed >> + */ >> +enum rpmsg_ns_flags { >> + RPMSG_NS_CREATE = 0, >> + RPMSG_NS_DESTROY = 1, >> +}; >> + >> +/** >> + * struct rpmsg_ns_msg - dynamic name service announcement message >> + * @name: name of remote service that is published >> + * @addr: address of remote service that is published >> + * @flags: indicates whether service is created or destroyed >> + * >> + * This message is sent across to publish a new service, or announce >> + * about its removal. When we receive these messages, an appropriate >> + * rpmsg channel (i.e device) is created/destroyed. In turn, the ->probe() >> + * or ->remove() handler of the appropriate rpmsg driver will be invoked >> + * (if/as-soon-as one is registered). >> + */ >> +struct rpmsg_ns_msg { >> + char name[RPMSG_NAME_SIZE]; >> + u32 addr; >> + u32 flags; >> +} __packed; >> + >> +/* invoked when a name service announcement arrives */ >> +static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void *data, int len, >> + void *priv, u32 src) >> +{ >> + struct rpmsg_ns_msg *msg = data; >> + struct rpmsg_device *newch; >> + struct rpmsg_channel_info chinfo; >> + struct device *dev = &rpdev->dev; >> + int ret; >> + >> +#if defined(CONFIG_DYNAMIC_DEBUG) >> + dynamic_hex_dump("NS announcement: ", DUMP_PREFIX_NONE, 16, 1, >> + data, len, true); >> +#endif >> + >> + if (len != sizeof(*msg)) { >> + dev_err(dev, "malformed ns msg (%d)\n", len); >> + return -EINVAL; >> + } >> + >> + /* don't trust the remote processor for null terminating the name */ >> + msg->name[RPMSG_NAME_SIZE - 1] = '\0'; >> + >> + strncpy(chinfo.name, msg->name, sizeof(chinfo.name)); >> + chinfo.src = RPMSG_ADDR_ANY; >> + chinfo.dst = msg->addr; >> + >> + dev_info(dev, "%sing channel %s addr 0x%x\n", >> + msg->flags & RPMSG_NS_DESTROY ? "destroy" : "creat", >> + msg->name, msg->addr); >> + >> + if (msg->flags & RPMSG_NS_DESTROY) { >> + ret = rpmsg_release_channel(rpdev, &chinfo); >> + if (ret) >> + dev_err(dev, "rpmsg_destroy_channel failed: %d\n", ret); >> + } else { >> + newch = rpmsg_create_channel(rpdev, &chinfo); >> + if (!newch) >> + dev_err(dev, "rpmsg_create_channel failed\n"); >> + } >> + >> + return 0; >> +} >> + >> +static int rpmsg_ns_probe(struct rpmsg_device *rpdev) >> +{ >> + struct rpmsg_channel_info ns_chinfo; >> + struct rpmsg_endpoint *ns_ept; >> + >> + ns_chinfo.src = RPMSG_NS_ADDR; >> + ns_chinfo.dst = RPMSG_NS_ADDR; >> + strcpy(ns_chinfo.name, "name_service"); >> + >> + /* >> + * create and attach the endpoint to the rpmsg device that it would be >> + * destroy when the rpmsg device will be deleted >> + */ > > This comment doesn't work, please revise. Could you clarify what does not work, from your POV? in view of your comment, it seems I should at least rephrase it... proposal: /* * Create the NS service endpoint associated to the rpmsg device. * The endpoint will be automatically destroyed when the rpmsg device * will be deleted. */ > >> + ns_ept = rpmsg_create_ept(rpdev, rpmsg_ns_cb, NULL, ns_chinfo); >> + if (!ns_ept) { >> + dev_err(&rpdev->dev, "failed to create the ns ept\n"); >> + return -ENOMEM; >> + } >> + rpdev->ept = ns_ept; >> + >> + rpdev->src = RPMSG_NS_ADDR; > > I think this is already done in rpmsg_ns_register_device(). You are right! thanks, Arnaud > >> + >> + return 0; >> +} >> + >> +static struct rpmsg_driver rpmsg_ns_driver = { >> + .drv.name = "rpmsg_ns", >> + .probe = rpmsg_ns_probe, >> +}; >> + >> +static int rpmsg_ns_init(void) >> +{ >> + int ret; >> + >> + ret = register_rpmsg_driver(&rpmsg_ns_driver); >> + if (ret < 0) >> + pr_err("%s: Failed to register rpmsg driver\n", __func__); >> + >> + return ret; >> +} >> +postcore_initcall(rpmsg_ns_init); >> + >> +static void rpmsg_ns_exit(void) >> +{ >> + unregister_rpmsg_driver(&rpmsg_ns_driver); >> +} >> +module_exit(rpmsg_ns_exit); >> + >> +MODULE_DESCRIPTION("Name service announcement rpmsg Driver"); >> +MODULE_AUTHOR("Arnaud Pouliquen <arnaud.pouliquen@st.com>"); >> +MODULE_ALIAS("rpmsg_ns"); >> +MODULE_LICENSE("GPL v2"); >> -- >> 2.17.1 >>
On Tue, 25 Aug 2020 at 05:57, Arnaud POULIQUEN <arnaud.pouliquen@st.com> wrote: > > Hi Mathieu > > Thanks for the review! please find few comments below. > > On 8/25/20 12:47 AM, Mathieu Poirier wrote: > > On Fri, Jul 31, 2020 at 01:47:28PM +0200, Arnaud Pouliquen wrote: > >> The name service announcement should not be linked to the RPMsg virtio bus > >> but to the RPMsg protocol itself. > >> > >> This patch proposes to break the dependency with the RPmsg virtio bus by > >> the introduction of the reserved RPMsg name service driver which will be in > >> charge of managing the RPMsg name service announcement. > >> > >> This first patch only implements the probe and the RPMsg endpoint to > >> manage create and release channels remote requests. > >> > >> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com> > >> --- > >> drivers/rpmsg/Kconfig | 8 ++ > >> drivers/rpmsg/Makefile | 1 + > >> drivers/rpmsg/rpmsg_internal.h | 17 +++++ > >> drivers/rpmsg/rpmsg_ns.c | 135 +++++++++++++++++++++++++++++++++ > >> 4 files changed, 161 insertions(+) > >> create mode 100644 drivers/rpmsg/rpmsg_ns.c > >> > >> diff --git a/drivers/rpmsg/Kconfig b/drivers/rpmsg/Kconfig > >> index f96716893c2a..140faa975ea1 100644 > >> --- a/drivers/rpmsg/Kconfig > >> +++ b/drivers/rpmsg/Kconfig > >> @@ -15,6 +15,14 @@ config RPMSG_CHAR > >> in /dev. They make it possible for user-space programs to send and > >> receive rpmsg packets. > >> > >> +config RPMSG_NS > >> + tristate "RPMSG name service announcement" > >> + depends on RPMSG > >> + help > >> + Say Y here to enable the support of the name service announcement > >> + channel that probes the associate RPMsg device on remote endpoint > > > > s/associate/associated > > > >> + service announcement. > >> + > >> config RPMSG_MTK_SCP > >> tristate "MediaTek SCP" > >> depends on MTK_SCP > >> diff --git a/drivers/rpmsg/Makefile b/drivers/rpmsg/Makefile > >> index ffe932ef6050..8d452656f0ee 100644 > >> --- a/drivers/rpmsg/Makefile > >> +++ b/drivers/rpmsg/Makefile > >> @@ -1,6 +1,7 @@ > >> # SPDX-License-Identifier: GPL-2.0 > >> obj-$(CONFIG_RPMSG) += rpmsg_core.o > >> obj-$(CONFIG_RPMSG_CHAR) += rpmsg_char.o > >> +obj-$(CONFIG_RPMSG_NS) += rpmsg_ns.o > >> obj-$(CONFIG_RPMSG_MTK_SCP) += mtk_rpmsg.o > >> qcom_glink-objs := qcom_glink_native.o qcom_glink_ssr.o > >> obj-$(CONFIG_RPMSG_QCOM_GLINK) += qcom_glink.o > >> diff --git a/drivers/rpmsg/rpmsg_internal.h b/drivers/rpmsg/rpmsg_internal.h > >> index d5ab286d0e5e..641b48f6bf2a 100644 > >> --- a/drivers/rpmsg/rpmsg_internal.h > >> +++ b/drivers/rpmsg/rpmsg_internal.h > >> @@ -102,4 +102,21 @@ static inline int rpmsg_chrdev_register_device(struct rpmsg_device *rpdev) > >> return rpmsg_register_device(rpdev); > >> } > >> > >> +/** > >> + * rpmsg_ns_register_device() - register name service device based on rpdev > >> + * @rpdev: prepared rpdev to be used for creating endpoints > >> + * > >> + * This function wraps rpmsg_register_device() preparing the rpdev for use as > >> + * basis for the rpmsg name service device. > >> + */ > >> +static inline int rpmsg_ns_register_device(struct rpmsg_device *rpdev) > >> +{ > >> + strcpy(rpdev->id.name, "rpmsg_ns"); > >> + rpdev->driver_override = "rpmsg_ns"; > >> + rpdev->src = RPMSG_NS_ADDR; > >> + rpdev->dst = RPMSG_NS_ADDR; > >> + > >> + return rpmsg_register_device(rpdev); > >> +} > >> + > >> #endif > >> diff --git a/drivers/rpmsg/rpmsg_ns.c b/drivers/rpmsg/rpmsg_ns.c > >> new file mode 100644 > >> index 000000000000..fe7713e737c2 > >> --- /dev/null > >> +++ b/drivers/rpmsg/rpmsg_ns.c > >> @@ -0,0 +1,135 @@ > >> +// SPDX-License-Identifier: GPL-2.0 > >> +/* > >> + * Copyright (C) STMicroelectronics 2020 - All Rights Reserved > >> + */ > >> +#include <linux/device.h> > >> +#include <linux/kernel.h> > >> +#include <linux/module.h> > >> +#include <linux/slab.h> > >> +#include "rpmsg_internal.h" > >> + > >> +/** > >> + * enum rpmsg_ns_flags - dynamic name service announcement flags > >> + * > >> + * @RPMSG_NS_CREATE: a new remote service was just created > >> + * @RPMSG_NS_DESTROY: a known remote service was just destroyed > >> + */ > >> +enum rpmsg_ns_flags { > >> + RPMSG_NS_CREATE = 0, > >> + RPMSG_NS_DESTROY = 1, > >> +}; > >> + > >> +/** > >> + * struct rpmsg_ns_msg - dynamic name service announcement message > >> + * @name: name of remote service that is published > >> + * @addr: address of remote service that is published > >> + * @flags: indicates whether service is created or destroyed > >> + * > >> + * This message is sent across to publish a new service, or announce > >> + * about its removal. When we receive these messages, an appropriate > >> + * rpmsg channel (i.e device) is created/destroyed. In turn, the ->probe() > >> + * or ->remove() handler of the appropriate rpmsg driver will be invoked > >> + * (if/as-soon-as one is registered). > >> + */ > >> +struct rpmsg_ns_msg { > >> + char name[RPMSG_NAME_SIZE]; > >> + u32 addr; > >> + u32 flags; > >> +} __packed; > >> + > >> +/* invoked when a name service announcement arrives */ > >> +static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void *data, int len, > >> + void *priv, u32 src) > >> +{ > >> + struct rpmsg_ns_msg *msg = data; > >> + struct rpmsg_device *newch; > >> + struct rpmsg_channel_info chinfo; > >> + struct device *dev = &rpdev->dev; > >> + int ret; > >> + > >> +#if defined(CONFIG_DYNAMIC_DEBUG) > >> + dynamic_hex_dump("NS announcement: ", DUMP_PREFIX_NONE, 16, 1, > >> + data, len, true); > >> +#endif > >> + > >> + if (len != sizeof(*msg)) { > >> + dev_err(dev, "malformed ns msg (%d)\n", len); > >> + return -EINVAL; > >> + } > >> + > >> + /* don't trust the remote processor for null terminating the name */ > >> + msg->name[RPMSG_NAME_SIZE - 1] = '\0'; > >> + > >> + strncpy(chinfo.name, msg->name, sizeof(chinfo.name)); > >> + chinfo.src = RPMSG_ADDR_ANY; > >> + chinfo.dst = msg->addr; > >> + > >> + dev_info(dev, "%sing channel %s addr 0x%x\n", > >> + msg->flags & RPMSG_NS_DESTROY ? "destroy" : "creat", > >> + msg->name, msg->addr); > >> + > >> + if (msg->flags & RPMSG_NS_DESTROY) { > >> + ret = rpmsg_release_channel(rpdev, &chinfo); > >> + if (ret) > >> + dev_err(dev, "rpmsg_destroy_channel failed: %d\n", ret); > >> + } else { > >> + newch = rpmsg_create_channel(rpdev, &chinfo); > >> + if (!newch) > >> + dev_err(dev, "rpmsg_create_channel failed\n"); > >> + } > >> + > >> + return 0; > >> +} > >> + > >> +static int rpmsg_ns_probe(struct rpmsg_device *rpdev) > >> +{ > >> + struct rpmsg_channel_info ns_chinfo; > >> + struct rpmsg_endpoint *ns_ept; > >> + > >> + ns_chinfo.src = RPMSG_NS_ADDR; > >> + ns_chinfo.dst = RPMSG_NS_ADDR; > >> + strcpy(ns_chinfo.name, "name_service"); > >> + > >> + /* > >> + * create and attach the endpoint to the rpmsg device that it would be > >> + * destroy when the rpmsg device will be deleted > >> + */ > > > > This comment doesn't work, please revise. > > Could you clarify what does not work, from your POV? > in view of your comment, it seems I should at least rephrase it... > proposal: > /* > * Create the NS service endpoint associated to the rpmsg device. > * The endpoint will be automatically destroyed when the rpmsg device > * will be deleted. > */ Yes, now it is perfectly clear - thanks for revisiting. > > > > >> + ns_ept = rpmsg_create_ept(rpdev, rpmsg_ns_cb, NULL, ns_chinfo); > >> + if (!ns_ept) { > >> + dev_err(&rpdev->dev, "failed to create the ns ept\n"); > >> + return -ENOMEM; > >> + } > >> + rpdev->ept = ns_ept; > >> + > >> + rpdev->src = RPMSG_NS_ADDR; > > > > I think this is already done in rpmsg_ns_register_device(). > > You are right! > > thanks, > Arnaud > > > >> + > >> + return 0; > >> +} > >> + > >> +static struct rpmsg_driver rpmsg_ns_driver = { > >> + .drv.name = "rpmsg_ns", > >> + .probe = rpmsg_ns_probe, > >> +}; > >> + > >> +static int rpmsg_ns_init(void) > >> +{ > >> + int ret; > >> + > >> + ret = register_rpmsg_driver(&rpmsg_ns_driver); > >> + if (ret < 0) > >> + pr_err("%s: Failed to register rpmsg driver\n", __func__); > >> + > >> + return ret; > >> +} > >> +postcore_initcall(rpmsg_ns_init); > >> + > >> +static void rpmsg_ns_exit(void) > >> +{ > >> + unregister_rpmsg_driver(&rpmsg_ns_driver); > >> +} > >> +module_exit(rpmsg_ns_exit); > >> + > >> +MODULE_DESCRIPTION("Name service announcement rpmsg Driver"); > >> +MODULE_AUTHOR("Arnaud Pouliquen <arnaud.pouliquen@st.com>"); > >> +MODULE_ALIAS("rpmsg_ns"); > >> +MODULE_LICENSE("GPL v2"); > >> -- > >> 2.17.1 > >>
diff --git a/drivers/rpmsg/Kconfig b/drivers/rpmsg/Kconfig index f96716893c2a..140faa975ea1 100644 --- a/drivers/rpmsg/Kconfig +++ b/drivers/rpmsg/Kconfig @@ -15,6 +15,14 @@ config RPMSG_CHAR in /dev. They make it possible for user-space programs to send and receive rpmsg packets. +config RPMSG_NS + tristate "RPMSG name service announcement" + depends on RPMSG + help + Say Y here to enable the support of the name service announcement + channel that probes the associate RPMsg device on remote endpoint + service announcement. + config RPMSG_MTK_SCP tristate "MediaTek SCP" depends on MTK_SCP diff --git a/drivers/rpmsg/Makefile b/drivers/rpmsg/Makefile index ffe932ef6050..8d452656f0ee 100644 --- a/drivers/rpmsg/Makefile +++ b/drivers/rpmsg/Makefile @@ -1,6 +1,7 @@ # SPDX-License-Identifier: GPL-2.0 obj-$(CONFIG_RPMSG) += rpmsg_core.o obj-$(CONFIG_RPMSG_CHAR) += rpmsg_char.o +obj-$(CONFIG_RPMSG_NS) += rpmsg_ns.o obj-$(CONFIG_RPMSG_MTK_SCP) += mtk_rpmsg.o qcom_glink-objs := qcom_glink_native.o qcom_glink_ssr.o obj-$(CONFIG_RPMSG_QCOM_GLINK) += qcom_glink.o diff --git a/drivers/rpmsg/rpmsg_internal.h b/drivers/rpmsg/rpmsg_internal.h index d5ab286d0e5e..641b48f6bf2a 100644 --- a/drivers/rpmsg/rpmsg_internal.h +++ b/drivers/rpmsg/rpmsg_internal.h @@ -102,4 +102,21 @@ static inline int rpmsg_chrdev_register_device(struct rpmsg_device *rpdev) return rpmsg_register_device(rpdev); } +/** + * rpmsg_ns_register_device() - register name service device based on rpdev + * @rpdev: prepared rpdev to be used for creating endpoints + * + * This function wraps rpmsg_register_device() preparing the rpdev for use as + * basis for the rpmsg name service device. + */ +static inline int rpmsg_ns_register_device(struct rpmsg_device *rpdev) +{ + strcpy(rpdev->id.name, "rpmsg_ns"); + rpdev->driver_override = "rpmsg_ns"; + rpdev->src = RPMSG_NS_ADDR; + rpdev->dst = RPMSG_NS_ADDR; + + return rpmsg_register_device(rpdev); +} + #endif diff --git a/drivers/rpmsg/rpmsg_ns.c b/drivers/rpmsg/rpmsg_ns.c new file mode 100644 index 000000000000..fe7713e737c2 --- /dev/null +++ b/drivers/rpmsg/rpmsg_ns.c @@ -0,0 +1,135 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (C) STMicroelectronics 2020 - All Rights Reserved + */ +#include <linux/device.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/slab.h> +#include "rpmsg_internal.h" + +/** + * enum rpmsg_ns_flags - dynamic name service announcement flags + * + * @RPMSG_NS_CREATE: a new remote service was just created + * @RPMSG_NS_DESTROY: a known remote service was just destroyed + */ +enum rpmsg_ns_flags { + RPMSG_NS_CREATE = 0, + RPMSG_NS_DESTROY = 1, +}; + +/** + * struct rpmsg_ns_msg - dynamic name service announcement message + * @name: name of remote service that is published + * @addr: address of remote service that is published + * @flags: indicates whether service is created or destroyed + * + * This message is sent across to publish a new service, or announce + * about its removal. When we receive these messages, an appropriate + * rpmsg channel (i.e device) is created/destroyed. In turn, the ->probe() + * or ->remove() handler of the appropriate rpmsg driver will be invoked + * (if/as-soon-as one is registered). + */ +struct rpmsg_ns_msg { + char name[RPMSG_NAME_SIZE]; + u32 addr; + u32 flags; +} __packed; + +/* invoked when a name service announcement arrives */ +static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void *data, int len, + void *priv, u32 src) +{ + struct rpmsg_ns_msg *msg = data; + struct rpmsg_device *newch; + struct rpmsg_channel_info chinfo; + struct device *dev = &rpdev->dev; + int ret; + +#if defined(CONFIG_DYNAMIC_DEBUG) + dynamic_hex_dump("NS announcement: ", DUMP_PREFIX_NONE, 16, 1, + data, len, true); +#endif + + if (len != sizeof(*msg)) { + dev_err(dev, "malformed ns msg (%d)\n", len); + return -EINVAL; + } + + /* don't trust the remote processor for null terminating the name */ + msg->name[RPMSG_NAME_SIZE - 1] = '\0'; + + strncpy(chinfo.name, msg->name, sizeof(chinfo.name)); + chinfo.src = RPMSG_ADDR_ANY; + chinfo.dst = msg->addr; + + dev_info(dev, "%sing channel %s addr 0x%x\n", + msg->flags & RPMSG_NS_DESTROY ? "destroy" : "creat", + msg->name, msg->addr); + + if (msg->flags & RPMSG_NS_DESTROY) { + ret = rpmsg_release_channel(rpdev, &chinfo); + if (ret) + dev_err(dev, "rpmsg_destroy_channel failed: %d\n", ret); + } else { + newch = rpmsg_create_channel(rpdev, &chinfo); + if (!newch) + dev_err(dev, "rpmsg_create_channel failed\n"); + } + + return 0; +} + +static int rpmsg_ns_probe(struct rpmsg_device *rpdev) +{ + struct rpmsg_channel_info ns_chinfo; + struct rpmsg_endpoint *ns_ept; + + ns_chinfo.src = RPMSG_NS_ADDR; + ns_chinfo.dst = RPMSG_NS_ADDR; + strcpy(ns_chinfo.name, "name_service"); + + /* + * create and attach the endpoint to the rpmsg device that it would be + * destroy when the rpmsg device will be deleted + */ + ns_ept = rpmsg_create_ept(rpdev, rpmsg_ns_cb, NULL, ns_chinfo); + if (!ns_ept) { + dev_err(&rpdev->dev, "failed to create the ns ept\n"); + return -ENOMEM; + } + rpdev->ept = ns_ept; + + rpdev->src = RPMSG_NS_ADDR; + + return 0; +} + +static struct rpmsg_driver rpmsg_ns_driver = { + .drv.name = "rpmsg_ns", + .probe = rpmsg_ns_probe, +}; + +static int rpmsg_ns_init(void) +{ + int ret; + + ret = register_rpmsg_driver(&rpmsg_ns_driver); + if (ret < 0) + pr_err("%s: Failed to register rpmsg driver\n", __func__); + + return ret; +} +postcore_initcall(rpmsg_ns_init); + +static void rpmsg_ns_exit(void) +{ + unregister_rpmsg_driver(&rpmsg_ns_driver); +} +module_exit(rpmsg_ns_exit); + +MODULE_DESCRIPTION("Name service announcement rpmsg Driver"); +MODULE_AUTHOR("Arnaud Pouliquen <arnaud.pouliquen@st.com>"); +MODULE_ALIAS("rpmsg_ns"); +MODULE_LICENSE("GPL v2");
The name service announcement should not be linked to the RPMsg virtio bus but to the RPMsg protocol itself. This patch proposes to break the dependency with the RPmsg virtio bus by the introduction of the reserved RPMsg name service driver which will be in charge of managing the RPMsg name service announcement. This first patch only implements the probe and the RPMsg endpoint to manage create and release channels remote requests. Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com> --- drivers/rpmsg/Kconfig | 8 ++ drivers/rpmsg/Makefile | 1 + drivers/rpmsg/rpmsg_internal.h | 17 +++++ drivers/rpmsg/rpmsg_ns.c | 135 +++++++++++++++++++++++++++++++++ 4 files changed, 161 insertions(+) create mode 100644 drivers/rpmsg/rpmsg_ns.c