* [RFC] rpmsg: virtio rpmsg: Add RPMsg char driver support
@ 2018-01-04 23:18 Wendy Liang
2018-01-05 14:48 ` Arnaud Pouliquen
2018-03-18 22:49 ` Bjorn Andersson
0 siblings, 2 replies; 6+ messages in thread
From: Wendy Liang @ 2018-01-04 23:18 UTC (permalink / raw)
To: ohad, bjorn.andersson; +Cc: linux-remoteproc, linux-kernel, Wendy Liang
virtio rpmsg was not implemented to use RPMsg char driver.
Each virtio ns announcement will create a new RPMsg device which
is supposed to bound to a RPMsg driver. It doesn't support
dynamic endpoints with name service per RPMsg device.
With RPMsg char driver, you can have multiple endpoints per
RPMsg device.
Here is the change from this patch:
* Introduce a macro to indicate if want to use RPMsg char driver
for virtio RPMsg. The RPMsg device can either be bounded to
a simple RPMsg driver or the RPMsg char driver.
* Create Virtio RPMsg char device when the virtio RPMsg driver is
probed.
* when there is a remote service announced, keep it in the virtio
proc remote services list.
* when there is an endpoint created, bind it to a remote service
from the remote services list. If the service doesn't exist yet,
create one and mark the service address as ANY.
Signed-off-by: Wendy Liang <jliang@xilinx.com>
---
We have different userspace applications to use RPMsg differently,
what we need is a RPMsg char driver which can supports multiple
endpoints per remote device.
The virtio rpmsg driver at the moment doesn't support the RPMsg
char driver.
Please advise if this is patch is the right direction. If not,
any suggestions? Thanks
---
drivers/rpmsg/Kconfig | 8 +
drivers/rpmsg/virtio_rpmsg_bus.c | 364 ++++++++++++++++++++++++++++++++++++++-
2 files changed, 365 insertions(+), 7 deletions(-)
diff --git a/drivers/rpmsg/Kconfig b/drivers/rpmsg/Kconfig
index 65a9f6b..746f07e 100644
--- a/drivers/rpmsg/Kconfig
+++ b/drivers/rpmsg/Kconfig
@@ -52,4 +52,12 @@ config RPMSG_VIRTIO
select RPMSG
select VIRTIO
+config RPMSG_VIRTIO_CHAR
+ bool "Enable Virtio RPMSG char device driver support"
+ default y
+ depends on RPMSG_VIRTIO
+ depends on RPMSG_CHAR
+ help
+ Say y here to enable to use RPMSG char device interface.
+
endmenu
diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
index 82b8300..6e30a3cc 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -56,6 +56,7 @@
* @sendq: wait queue of sending contexts waiting for a tx buffers
* @sleepers: number of senders that are waiting for a tx buffer
* @ns_ept: the bus's name service endpoint
+ * @rsvcs: remote services
*
* This structure stores the rpmsg state of a given virtio remote processor
* device (there might be several virtio proc devices for each physical
@@ -75,6 +76,9 @@ struct virtproc_info {
wait_queue_head_t sendq;
atomic_t sleepers;
struct rpmsg_endpoint *ns_ept;
+#ifdef CONFIG_RPMSG_VIRTIO_CHAR
+ struct list_head rsvcs;
+#endif
};
/* The feature bitmap for virtio rpmsg */
@@ -141,6 +145,36 @@ struct virtio_rpmsg_channel {
#define to_virtio_rpmsg_channel(_rpdev) \
container_of(_rpdev, struct virtio_rpmsg_channel, rpdev)
+#ifdef CONFIG_RPMSG_VIRTIO_CHAR
+/**
+ * struct virtio_rpmsg_rsvc - virtio RPMsg remote service
+ * @name: name of the RPMsg remote service
+ * @addr: RPMsg address of the remote service
+ * @ept: local endpoint bound to the remote service
+ * @node: list node
+ */
+struct virtio_rpmsg_rsvc {
+ char name[RPMSG_NAME_SIZE];
+ u32 addr;
+ struct rpmsg_endpoint *ept;
+ struct list_head node;
+};
+
+/**
+ * struct virtio_rpmsg_ept - virtio RPMsg endpoint
+ * @rsvc: RPMsg service
+ * @ept: RPMsg endpoint
+ *
+ */
+struct virtio_rpmsg_ept {
+ struct virtio_rpmsg_rsvc *rsvc;
+ struct rpmsg_endpoint ept;
+};
+
+#define to_virtio_rpmsg_ept(_ept) \
+ container_of(_ept, struct virtio_rpmsg_ept, ept)
+#endif
+
/*
* We're allocating buffers of 512 bytes each for communications. The
* number of buffers will be computed from the number of buffers supported
@@ -216,6 +250,199 @@ rpmsg_sg_init(struct scatterlist *sg, void *cpu_addr, unsigned int len)
}
}
+#ifdef CONFIG_RPMSG_VIRTIO_CHAR
+/**
+ * virtio_rpmsg_find_rsvc_by_name - find the announced remote service
+ * @vrp: virtio remote proc information
+ * @name: remote service name
+ *
+ * The caller is supposed to have mutex lock before calling this function
+ *
+ * return NULL if no remote service has been found, otherwise, return
+ * the remote service pointer.
+ */
+static struct virtio_rpmsg_rsvc *
+virtio_rpmsg_find_rsvc_by_name(struct virtproc_info *vrp, char *name)
+{
+ struct virtio_rpmsg_rsvc *rsvc;
+
+ list_for_each_entry(rsvc, &vrp->rsvcs, node) {
+ if (!strncmp(rsvc->name, name, RPMSG_NAME_SIZE))
+ /* remote service has found */
+ return rsvc;
+ }
+
+ return NULL;
+}
+
+/**
+ * virtio_rpmsg_create_rsvc_by_name - create remote service
+ * @vrp: virtio remote proc information
+ * @name: remote service name
+ *
+ * The caller is supposed to have mutex lock before calling this function
+ *
+ * return NULL if remote service creation failed. otherwise, return
+ * the remote service pointer.
+ */
+static struct virtio_rpmsg_rsvc *
+virtio_rpmsg_create_rsvc_by_name(struct virtproc_info *vrp, char *name)
+{
+ struct virtio_rpmsg_rsvc *rsvc;
+
+ rsvc = virtio_rpmsg_find_rsvc_by_name(vrp, name);
+ if (rsvc)
+ return rsvc;
+ rsvc = kzalloc(sizeof(*rsvc), GFP_KERNEL);
+ if (!rsvc)
+ return NULL;
+ strncpy(rsvc->name, name, RPMSG_NAME_SIZE);
+ list_add_tail(&rsvc->node, &vrp->rsvcs);
+ return rsvc;
+}
+
+/**
+ * virtio_rpmsg_remove_rsvc_by_name - remove remote service
+ * @vrp: virtio remote proc information
+ * @name: remote service name
+ *
+ */
+static void
+virtio_rpmsg_remove_rsvc_by_name(struct virtproc_info *vrp, char *name)
+{
+ struct virtio_rpmsg_rsvc *rsvc;
+ struct rpmsg_endpoint *ept;
+ struct virtio_rpmsg_ept *vept;
+
+ mutex_lock(&vrp->endpoints_lock);
+ list_for_each_entry(rsvc, &vrp->rsvcs, node) {
+ if (!strncmp(rsvc->name, name, RPMSG_NAME_SIZE)) {
+ /* remote service has found, no need to
+ * create
+ */
+ ept = rsvc->ept;
+ if (ept) {
+ vept = to_virtio_rpmsg_ept(ept);
+ vept->rsvc = NULL;
+ }
+ list_del(&rsvc->node);
+ kfree(rsvc);
+ break;
+ }
+ }
+ mutex_unlock(&vrp->endpoints_lock);
+}
+
+/**
+ * virtio_rpmsg_create_rsvc - create remote service with channel information
+ * @vrp: virtio remote proc information
+ * @chinfo: channel information
+ *
+ * return remote service pointer if it is successfully created; otherwise,
+ * return NULL.
+ */
+static struct virtio_rpmsg_rsvc *
+virtio_rpmsg_create_rsvc(struct virtproc_info *vrp,
+ struct rpmsg_channel_info *chinfo)
+{
+ struct virtio_rpmsg_rsvc *rsvc;
+
+ mutex_lock(&vrp->endpoints_lock);
+ rsvc = virtio_rpmsg_create_rsvc_by_name(vrp, chinfo->name);
+ if (rsvc) {
+ struct virtio_device *vdev = vrp->vdev;
+
+ rsvc->addr = chinfo->dst;
+ dev_info(&vdev->dev, "Remote has announced service %s, %d.\n",
+ chinfo->name, rsvc->addr);
+ }
+ mutex_unlock(&vrp->endpoints_lock);
+ return rsvc;
+}
+
+/**
+ * virtio_rpmsg_announce_ept_create - announce endpoint has been created
+ * @ept: RPMsg endpoint
+ *
+ * return 0 if succeeded, otherwise, return error code.
+ */
+static int virtio_rpmsg_announce_ept_create(struct rpmsg_endpoint *ept)
+{
+ struct virtio_rpmsg_ept *vept = to_virtio_rpmsg_ept(ept);
+ struct virtio_rpmsg_rsvc *rsvc = vept->rsvc;
+ struct rpmsg_device *rpdev = ept->rpdev;
+ struct virtio_rpmsg_channel *vch;
+ struct virtproc_info *vrp;
+ struct device *dev;
+ int err = 0;
+
+ if (!rpdev)
+ /* If the endpoint is not connected to a RPMsg device,
+ * no need to send the announcement.
+ */
+ return 0;
+ vch = to_virtio_rpmsg_channel(rpdev);
+ vrp = vch->vrp;
+ dev = &ept->rpdev->dev;
+ /* need to tell remote processor's name service about this channel ? */
+ if (virtio_has_feature(vrp->vdev, VIRTIO_RPMSG_F_NS)) {
+ struct rpmsg_ns_msg nsm;
+
+ strncpy(nsm.name, rsvc->name, RPMSG_NAME_SIZE);
+ nsm.addr = ept->addr;
+ nsm.flags = RPMSG_NS_CREATE;
+
+ err = rpmsg_sendto(ept, &nsm, sizeof(nsm),
+ RPMSG_NS_ADDR);
+ if (err)
+ dev_err(dev, "failed to announce service %d\n", err);
+ }
+
+ return err;
+}
+
+/**
+ * virtio_rpmsg_announce_ept_destroy - announce endpoint has been destroyed
+ * @ept: RPMsg endpoint
+ *
+ * return 0 if succeeded, otherwise, return error code.
+ */
+static int virtio_rpmsg_announce_ept_destroy(struct rpmsg_endpoint *ept)
+{
+ struct virtio_rpmsg_ept *vept = to_virtio_rpmsg_ept(ept);
+ struct virtio_rpmsg_rsvc *rsvc = vept->rsvc;
+ struct rpmsg_device *rpdev = ept->rpdev;
+ struct virtio_rpmsg_channel *vch;
+ struct virtproc_info *vrp;
+ struct device *dev;
+ int err = 0;
+
+ if (!rpdev)
+ /* If the endpoint is not connected to a RPMsg device,
+ * no need to send the announcement.
+ */
+ return 0;
+ vch = to_virtio_rpmsg_channel(rpdev);
+ vrp = vch->vrp;
+ dev = &ept->rpdev->dev;
+ /* tell remote processor's name service we're removing this channel */
+ if (virtio_has_feature(vrp->vdev, VIRTIO_RPMSG_F_NS)) {
+ struct rpmsg_ns_msg nsm;
+
+ strncpy(nsm.name, rsvc->name, RPMSG_NAME_SIZE);
+ nsm.addr = ept->addr;
+ nsm.flags = RPMSG_NS_DESTROY;
+
+ err = rpmsg_sendto(ept, &nsm, sizeof(nsm),
+ RPMSG_NS_ADDR);
+ if (err)
+ dev_err(dev, "failed to announce service %d\n", err);
+ }
+
+ return err;
+}
+#endif
+
/**
* __ept_release() - deallocate an rpmsg endpoint
* @kref: the ept's reference count
@@ -229,26 +456,53 @@ static void __ept_release(struct kref *kref)
{
struct rpmsg_endpoint *ept = container_of(kref, struct rpmsg_endpoint,
refcount);
+#ifdef CONFIG_RPMSG_VIRTIO_CHAR
+ struct virtio_rpmsg_ept *vept = to_virtio_rpmsg_ept(ept);
+ struct virtio_rpmsg_channel *vch;
+ struct virtproc_info *vrp;
+
+ if (ept->rpdev) {
+ vch = to_virtio_rpmsg_channel(ept->rpdev);
+ vrp = vch->vrp;
+ (void)virtio_rpmsg_announce_ept_destroy(ept);
+ mutex_lock(&vrp->endpoints_lock);
+ vept->rsvc->ept = NULL;
+ mutex_unlock(&vrp->endpoints_lock);
+ }
+ kfree(vept);
+#else
/*
* At this point no one holds a reference to ept anymore,
* so we can directly free it
*/
kfree(ept);
+#endif
}
/* for more info, see below documentation of rpmsg_create_ept() */
static struct rpmsg_endpoint *__rpmsg_create_ept(struct virtproc_info *vrp,
struct rpmsg_device *rpdev,
rpmsg_rx_cb_t cb,
- void *priv, u32 addr)
+ void *priv,
+ struct rpmsg_channel_info *ch)
{
int id_min, id_max, id;
struct rpmsg_endpoint *ept;
+ u32 addr = ch->src;
struct device *dev = rpdev ? &rpdev->dev : &vrp->vdev->dev;
+#ifdef CONFIG_RPMSG_VIRTIO_CHAR
+ struct virtio_rpmsg_ept *vept;
+ struct virtio_rpmsg_rsvc *rsvc;
+ vept = kzalloc(sizeof(*vept), GFP_KERNEL);
+ if (!vept)
+ return NULL;
+ ept = &vept->ept;
+#else
ept = kzalloc(sizeof(*ept), GFP_KERNEL);
if (!ept)
return NULL;
+#endif
kref_init(&ept->refcount);
mutex_init(&ept->cb_lock);
@@ -259,7 +513,7 @@ static struct rpmsg_endpoint *__rpmsg_create_ept(struct virtproc_info *vrp,
ept->ops = &virtio_endpoint_ops;
/* do we need to allocate a local address ? */
- if (addr == RPMSG_ADDR_ANY) {
+ if (ch->src == RPMSG_ADDR_ANY) {
id_min = RPMSG_RESERVED_ADDRESSES;
id_max = 0;
} else {
@@ -277,6 +531,19 @@ static struct rpmsg_endpoint *__rpmsg_create_ept(struct virtproc_info *vrp,
}
ept->addr = id;
+#ifdef CONFIG_RPMSG_VIRTIO_CHAR
+ if (ept->addr != RPMSG_NS_ADDR) {
+ rsvc = virtio_rpmsg_create_rsvc_by_name(vrp, ch->name);
+ if (!rsvc)
+ goto free_ept;
+ vept->rsvc = rsvc;
+ rsvc->ept = ept;
+ dev_info(&vrp->vdev->dev, "RPMsg ept created, %s:%d,%d.\n",
+ ch->name, ept->addr, rsvc->addr);
+ (void)virtio_rpmsg_announce_ept_create(ept);
+ }
+#endif
+
mutex_unlock(&vrp->endpoints_lock);
return ept;
@@ -294,7 +561,7 @@ static struct rpmsg_endpoint *virtio_rpmsg_create_ept(struct rpmsg_device *rpdev
{
struct virtio_rpmsg_channel *vch = to_virtio_rpmsg_channel(rpdev);
- return __rpmsg_create_ept(vch->vrp, rpdev, cb, priv, chinfo.src);
+ return __rpmsg_create_ept(vch->vrp, rpdev, cb, priv, &chinfo);
}
/**
@@ -392,6 +659,7 @@ static void virtio_rpmsg_release_device(struct device *dev)
kfree(vch);
}
+#ifndef CONFIG_RPMSG_VIRTIO_CHAR
/*
* create an rpmsg channel using its name and address info.
* this function will be used to create both static and dynamic
@@ -444,6 +712,7 @@ static struct rpmsg_device *rpmsg_create_channel(struct virtproc_info *vrp,
return rpdev;
}
+#endif
/* super simple buffer "allocator" that is just enough for now */
static void *get_a_tx_buf(struct virtproc_info *vrp)
@@ -660,8 +929,21 @@ static int rpmsg_send_offchannel_raw(struct rpmsg_device *rpdev,
static int virtio_rpmsg_send(struct rpmsg_endpoint *ept, void *data, int len)
{
struct rpmsg_device *rpdev = ept->rpdev;
- u32 src = ept->addr, dst = rpdev->dst;
+ u32 src = ept->addr;
+#ifdef CONFIG_RPMSG_VIRTIO_CHAR
+ struct virtio_rpmsg_ept *vept = to_virtio_rpmsg_ept(ept);
+ u32 dst = vept->rsvc->addr;
+
+ if (dst == RPMSG_ADDR_ANY)
+ return -EPIPE;
+#else
+ u32 dst = rpdev->dst;
+#endif
+ if (!rpdev) {
+ pr_err("%s, ept %p rpdev is NULL\n", __func__, ept);
+ return -EINVAL;
+ }
return rpmsg_send_offchannel_raw(rpdev, src, dst, data, len, true);
}
@@ -685,7 +967,16 @@ static int virtio_rpmsg_send_offchannel(struct rpmsg_endpoint *ept, u32 src,
static int virtio_rpmsg_trysend(struct rpmsg_endpoint *ept, void *data, int len)
{
struct rpmsg_device *rpdev = ept->rpdev;
- u32 src = ept->addr, dst = rpdev->dst;
+ u32 src = ept->addr;
+#ifdef CONFIG_RPMSG_VIRTIO_CHAR
+ struct virtio_rpmsg_ept *vept = to_virtio_rpmsg_ept(ept);
+ u32 dst = vept->rsvc->addr;
+
+ if (dst == RPMSG_ADDR_ANY)
+ return -EPIPE;
+#else
+ u32 dst = rpdev->dst;
+#endif
return rpmsg_send_offchannel_raw(rpdev, src, dst, data, len, false);
}
@@ -824,11 +1115,13 @@ 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 virtproc_info *vrp = priv;
struct device *dev = &vrp->vdev->dev;
+#ifndef CONFIG_RPMSG_VIRTIO_CHAR
+ struct rpmsg_device *newch;
int ret;
+#endif
#if defined(CONFIG_DYNAMIC_DEBUG)
dynamic_hex_dump("NS announcement: ", DUMP_PREFIX_NONE, 16, 1,
@@ -863,13 +1156,25 @@ static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void *data, int len,
chinfo.dst = msg->addr;
if (msg->flags & RPMSG_NS_DESTROY) {
+#ifdef CONFIG_RPMSG_VIRTIO_CHAR
+ virtio_rpmsg_remove_rsvc_by_name(vrp, chinfo.name);
+#else
ret = rpmsg_unregister_device(&vrp->vdev->dev, &chinfo);
if (ret)
dev_err(dev, "rpmsg_destroy_channel failed: %d\n", ret);
+#endif
} else {
+#ifdef CONFIG_RPMSG_VIRTIO_CHAR
+ struct virtio_rpmsg_rsvc *rsvc;
+
+ rsvc = virtio_rpmsg_create_rsvc(vrp, &chinfo);
+ if (!rsvc)
+ dev_err(dev, "virtio_rpmsg_create_rsvc failed\n");
+#else
newch = rpmsg_create_channel(vrp, &chinfo);
if (!newch)
dev_err(dev, "rpmsg_create_channel failed\n");
+#endif
}
return 0;
@@ -885,6 +1190,10 @@ static int rpmsg_probe(struct virtio_device *vdev)
int err = 0, i;
size_t total_buf_space;
bool notify;
+#ifdef CONFIG_RPMSG_VIRTIO_CHAR
+ struct virtio_rpmsg_channel *vch;
+ struct rpmsg_device *rp_char_dev;
+#endif
vrp = kzalloc(sizeof(*vrp), GFP_KERNEL);
if (!vrp)
@@ -956,9 +1265,14 @@ static int rpmsg_probe(struct virtio_device *vdev)
/* if supported by the remote processor, enable the name service */
if (virtio_has_feature(vdev, VIRTIO_RPMSG_F_NS)) {
+ struct rpmsg_channel_info ns_chinfo;
+
+ ns_chinfo.src = RPMSG_NS_ADDR;
+ ns_chinfo.dst = RPMSG_NS_ADDR;
+ strcpy(ns_chinfo.name, "name_service");
/* a dedicated endpoint handles the name service msgs */
vrp->ns_ept = __rpmsg_create_ept(vrp, NULL, rpmsg_ns_cb,
- vrp, RPMSG_NS_ADDR);
+ vrp, &ns_chinfo);
if (!vrp->ns_ept) {
dev_err(&vdev->dev, "failed to create the ns ept\n");
err = -ENOMEM;
@@ -966,6 +1280,32 @@ static int rpmsg_probe(struct virtio_device *vdev)
}
}
+#ifdef CONFIG_RPMSG_VIRTIO_CHAR
+ vch = kzalloc(sizeof(*vch), GFP_KERNEL);
+ if (!vch) {
+ err = -ENOMEM;
+ goto free_coherent;
+ }
+
+ /* Link the channel to our vrp */
+ vch->vrp = vrp;
+ /* Initialize remote services list */
+ INIT_LIST_HEAD(&vrp->rsvcs);
+
+ /* Assign public information to the rpmsg_device */
+ rp_char_dev = &vch->rpdev;
+ rp_char_dev->ops = &virtio_rpmsg_ops;
+
+ rp_char_dev->dev.parent = &vrp->vdev->dev;
+ rp_char_dev->dev.release = virtio_rpmsg_release_device;
+ err = rpmsg_chrdev_register_device(rp_char_dev);
+ if (err) {
+ kfree(vch);
+ goto free_coherent;
+ }
+ dev_info(&vdev->dev, "Registered as RPMsg char device.\n");
+#endif
+
/*
* Prepare to kick but don't notify yet - we can't do this before
* device is ready.
@@ -1009,6 +1349,9 @@ static void rpmsg_remove(struct virtio_device *vdev)
struct virtproc_info *vrp = vdev->priv;
size_t total_buf_space = vrp->num_bufs * vrp->buf_size;
int ret;
+#ifdef CONFIG_RPMSG_VIRTIO_CHAR
+ struct virtio_rpmsg_rsvc *rsvc, *tmp;
+#endif
vdev->config->reset(vdev);
@@ -1021,6 +1364,13 @@ static void rpmsg_remove(struct virtio_device *vdev)
idr_destroy(&vrp->endpoints);
+#ifdef CONFIG_RPMSG_VIRTIO_CHAR
+ list_for_each_entry_safe(rsvc, tmp, &vrp->rsvcs, node) {
+ list_del(&rsvc->node);
+ kfree(rsvc);
+ }
+#endif
+
vdev->config->del_vqs(vrp->vdev);
dma_free_coherent(vdev->dev.parent->parent, total_buf_space,
--
2.7.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RFC] rpmsg: virtio rpmsg: Add RPMsg char driver support
2018-01-04 23:18 [RFC] rpmsg: virtio rpmsg: Add RPMsg char driver support Wendy Liang
@ 2018-01-05 14:48 ` Arnaud Pouliquen
2018-01-05 22:10 ` Jiaying Liang
2018-03-18 22:49 ` Bjorn Andersson
1 sibling, 1 reply; 6+ messages in thread
From: Arnaud Pouliquen @ 2018-01-05 14:48 UTC (permalink / raw)
To: Wendy Liang, ohad, bjorn.andersson
Cc: linux-remoteproc, linux-kernel, Wendy Liang
Hi Wendy,
Few remarks on your patch.
On 01/05/2018 12:18 AM, Wendy Liang wrote:
> virtio rpmsg was not implemented to use RPMsg char driver.
> Each virtio ns announcement will create a new RPMsg device which
> is supposed to bound to a RPMsg driver. It doesn't support
> dynamic endpoints with name service per RPMsg device.
> With RPMsg char driver, you can have multiple endpoints per
> RPMsg device.
>
> Here is the change from this patch:
> * Introduce a macro to indicate if want to use RPMsg char driver
> for virtio RPMsg. The RPMsg device can either be bounded to
> a simple RPMsg driver or the RPMsg char driver.
> * Create Virtio RPMsg char device when the virtio RPMsg driver is
> probed.
> * when there is a remote service announced, keep it in the virtio
> proc remote services list.
> * when there is an endpoint created, bind it to a remote service
> from the remote services list. If the service doesn't exist yet,
> create one and mark the service address as ANY.
Would be nice to simplify the review if patch was split in several
patches (for instance per feature introduced).
>
> Signed-off-by: Wendy Liang <jliang@xilinx.com>
> ---
> We have different userspace applications to use RPMsg differently,
> what we need is a RPMsg char driver which can supports multiple
> endpoints per remote device.
> The virtio rpmsg driver at the moment doesn't support the RPMsg
> char driver.
> Please advise if this is patch is the right direction. If not,
> any suggestions? Thanks
> ---
> drivers/rpmsg/Kconfig | 8 +
> drivers/rpmsg/virtio_rpmsg_bus.c | 364 ++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 365 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/rpmsg/Kconfig b/drivers/rpmsg/Kconfig
> index 65a9f6b..746f07e 100644
> --- a/drivers/rpmsg/Kconfig
> +++ b/drivers/rpmsg/Kconfig
> @@ -52,4 +52,12 @@ config RPMSG_VIRTIO
> select RPMSG
> select VIRTIO
>
> +config RPMSG_VIRTIO_CHAR
> + bool "Enable Virtio RPMSG char device driver support"
> + default y
> + depends on RPMSG_VIRTIO
> + depends on RPMSG_CHAR
> + help
> + Say y here to enable to use RPMSG char device interface.
> +
> endmenu
> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> index 82b8300..6e30a3cc 100644
> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> @@ -56,6 +56,7 @@
> * @sendq: wait queue of sending contexts waiting for a tx buffers
> * @sleepers: number of senders that are waiting for a tx buffer
> * @ns_ept: the bus's name service endpoint
> + * @rsvcs: remote services
> *
> * This structure stores the rpmsg state of a given virtio remote processor
> * device (there might be several virtio proc devices for each physical
> @@ -75,6 +76,9 @@ struct virtproc_info {
> wait_queue_head_t sendq;
> atomic_t sleepers;
> struct rpmsg_endpoint *ns_ept;
> +#ifdef CONFIG_RPMSG_VIRTIO_CHAR
> + struct list_head rsvcs;
> +#endif
> };
>
> /* The feature bitmap for virtio rpmsg */
> @@ -141,6 +145,36 @@ struct virtio_rpmsg_channel {
> #define to_virtio_rpmsg_channel(_rpdev) \
> container_of(_rpdev, struct virtio_rpmsg_channel, rpdev)
>
> +#ifdef CONFIG_RPMSG_VIRTIO_CHAR
> +/**
> + * struct virtio_rpmsg_rsvc - virtio RPMsg remote service
> + * @name: name of the RPMsg remote service
> + * @addr: RPMsg address of the remote service
> + * @ept: local endpoint bound to the remote service
> + * @node: list node
> + */
> +struct virtio_rpmsg_rsvc {
> + char name[RPMSG_NAME_SIZE];
> + u32 addr;
> + struct rpmsg_endpoint *ept;
> + struct list_head node;
> +};
> +
> +/**
> + * struct virtio_rpmsg_ept - virtio RPMsg endpoint
> + * @rsvc: RPMsg service
> + * @ept: RPMsg endpoint
> + *
> + */
> +struct virtio_rpmsg_ept {
> + struct virtio_rpmsg_rsvc *rsvc;
> + struct rpmsg_endpoint ept;
> +};
> +
> +#define to_virtio_rpmsg_ept(_ept) \
> + container_of(_ept, struct virtio_rpmsg_ept, ept)
> +#endif
> +
> /*
> * We're allocating buffers of 512 bytes each for communications. The
> * number of buffers will be computed from the number of buffers supported
> @@ -216,6 +250,199 @@ rpmsg_sg_init(struct scatterlist *sg, void *cpu_addr, unsigned int len)
> }
> }
>
> +#ifdef CONFIG_RPMSG_VIRTIO_CHAR
> +/**
> + * virtio_rpmsg_find_rsvc_by_name - find the announced remote service
> + * @vrp: virtio remote proc information
> + * @name: remote service name
> + *
> + * The caller is supposed to have mutex lock before calling this function
> + *
> + * return NULL if no remote service has been found, otherwise, return
> + * the remote service pointer.
> + */
> +static struct virtio_rpmsg_rsvc *
> +virtio_rpmsg_find_rsvc_by_name(struct virtproc_info *vrp, char *name)
> +{
> + struct virtio_rpmsg_rsvc *rsvc;
> +
> + list_for_each_entry(rsvc, &vrp->rsvcs, node) {
> + if (!strncmp(rsvc->name, name, RPMSG_NAME_SIZE))
> + /* remote service has found */
> + return rsvc;
> + }
> +
> + return NULL;
> +}
> +
> +/**
> + * virtio_rpmsg_create_rsvc_by_name - create remote service
> + * @vrp: virtio remote proc information
> + * @name: remote service name
> + *
> + * The caller is supposed to have mutex lock before calling this function
> + *
> + * return NULL if remote service creation failed. otherwise, return
> + * the remote service pointer.
> + */
> +static struct virtio_rpmsg_rsvc *
> +virtio_rpmsg_create_rsvc_by_name(struct virtproc_info *vrp, char *name)
> +{
> + struct virtio_rpmsg_rsvc *rsvc;
> +
> + rsvc = virtio_rpmsg_find_rsvc_by_name(vrp, name);
> + if (rsvc)
> + return rsvc;
Here, I think you break the possibility to instantiate several times the
same service .
For instance, today if you have 2 communications in parallel:
Core1_Aplli1 <-- "foo" service --> core2_appli1
Core1_Aplli2 <-- "foo" service --> core2_appli2
2 ways to do it:
1) one service and 4 endpoints
=> issue it to know the destination address.
2) 2 instances of the same service with default endpoint.
> + rsvc = kzalloc(sizeof(*rsvc), GFP_KERNEL);
> + if (!rsvc)
> + return NULL;
> + strncpy(rsvc->name, name, RPMSG_NAME_SIZE);
> + list_add_tail(&rsvc->node, &vrp->rsvcs);
> + return rsvc;
> +}
> +
> +/**
> + * virtio_rpmsg_remove_rsvc_by_name - remove remote service
> + * @vrp: virtio remote proc information
> + * @name: remote service name
> + *
> + */
> +static void
> +virtio_rpmsg_remove_rsvc_by_name(struct virtproc_info *vrp, char *name)
> +{
> + struct virtio_rpmsg_rsvc *rsvc;
> + struct rpmsg_endpoint *ept;
> + struct virtio_rpmsg_ept *vept;
> +
> + mutex_lock(&vrp->endpoints_lock);
> + list_for_each_entry(rsvc, &vrp->rsvcs, node) {
> + if (!strncmp(rsvc->name, name, RPMSG_NAME_SIZE)) {
> + /* remote service has found, no need to
> + * create
> + */
> + ept = rsvc->ept;
> + if (ept) {
> + vept = to_virtio_rpmsg_ept(ept);
> + vept->rsvc = NULL;
> + }
> + list_del(&rsvc->node);
> + kfree(rsvc);
> + break;
> + }
> + }
> + mutex_unlock(&vrp->endpoints_lock);
> +}
> +
> +/**
> + * virtio_rpmsg_create_rsvc - create remote service with channel information
> + * @vrp: virtio remote proc information
> + * @chinfo: channel information
> + *
> + * return remote service pointer if it is successfully created; otherwise,
> + * return NULL.
> + */
> +static struct virtio_rpmsg_rsvc *
> +virtio_rpmsg_create_rsvc(struct virtproc_info *vrp,
> + struct rpmsg_channel_info *chinfo)
> +{
> + struct virtio_rpmsg_rsvc *rsvc;
> +
> + mutex_lock(&vrp->endpoints_lock);
> + rsvc = virtio_rpmsg_create_rsvc_by_name(vrp, chinfo->name);
> + if (rsvc) {
> + struct virtio_device *vdev = vrp->vdev;
> +
> + rsvc->addr = chinfo->dst;
> + dev_info(&vdev->dev, "Remote has announced service %s, %d.\n",
> + chinfo->name, rsvc->addr);
> + }
> + mutex_unlock(&vrp->endpoints_lock);
> + return rsvc;
> +}
> +
> +/**
> + * virtio_rpmsg_announce_ept_create - announce endpoint has been created
> + * @ept: RPMsg endpoint
> + *
> + * return 0 if succeeded, otherwise, return error code.
> + */
> +static int virtio_rpmsg_announce_ept_create(struct rpmsg_endpoint *ept)
> +{
> + struct virtio_rpmsg_ept *vept = to_virtio_rpmsg_ept(ept);
> + struct virtio_rpmsg_rsvc *rsvc = vept->rsvc;
> + struct rpmsg_device *rpdev = ept->rpdev;
> + struct virtio_rpmsg_channel *vch;
> + struct virtproc_info *vrp;
> + struct device *dev;
> + int err = 0;
> +
> + if (!rpdev)
> + /* If the endpoint is not connected to a RPMsg device,
> + * no need to send the announcement.
> + */
> + return 0;
> + vch = to_virtio_rpmsg_channel(rpdev);
> + vrp = vch->vrp;
> + dev = &ept->rpdev->dev;
> + /* need to tell remote processor's name service about this channel ? */
> + if (virtio_has_feature(vrp->vdev, VIRTIO_RPMSG_F_NS)) {
> + struct rpmsg_ns_msg nsm;
> +
> + strncpy(nsm.name, rsvc->name, RPMSG_NAME_SIZE);
> + nsm.addr = ept->addr;
> + nsm.flags = RPMSG_NS_CREATE;
> +
> + err = rpmsg_sendto(ept, &nsm, sizeof(nsm),
> + RPMSG_NS_ADDR);
> + if (err)
> + dev_err(dev, "failed to announce service %d\n", err);
> + }
> +
> + return err;
> +}
> +
> +/**
> + * virtio_rpmsg_announce_ept_destroy - announce endpoint has been destroyed
> + * @ept: RPMsg endpoint
> + *
> + * return 0 if succeeded, otherwise, return error code.
> + */
> +static int virtio_rpmsg_announce_ept_destroy(struct rpmsg_endpoint *ept)
> +{
> + struct virtio_rpmsg_ept *vept = to_virtio_rpmsg_ept(ept);
> + struct virtio_rpmsg_rsvc *rsvc = vept->rsvc;
> + struct rpmsg_device *rpdev = ept->rpdev;
> + struct virtio_rpmsg_channel *vch;
> + struct virtproc_info *vrp;
> + struct device *dev;
> + int err = 0;
> +
> + if (!rpdev)
> + /* If the endpoint is not connected to a RPMsg device,
> + * no need to send the announcement.
> + */
> + return 0;
> + vch = to_virtio_rpmsg_channel(rpdev);
> + vrp = vch->vrp;
> + dev = &ept->rpdev->dev;
> + /* tell remote processor's name service we're removing this channel */
> + if (virtio_has_feature(vrp->vdev, VIRTIO_RPMSG_F_NS)) {
> + struct rpmsg_ns_msg nsm;
> +
> + strncpy(nsm.name, rsvc->name, RPMSG_NAME_SIZE);
> + nsm.addr = ept->addr;
> + nsm.flags = RPMSG_NS_DESTROY;
> +
> + err = rpmsg_sendto(ept, &nsm, sizeof(nsm),
> + RPMSG_NS_ADDR);
> + if (err)
> + dev_err(dev, "failed to announce service %d\n", err);
> + }
> +
> + return err;
> +}
> +#endif
> +
> /**
> * __ept_release() - deallocate an rpmsg endpoint
> * @kref: the ept's reference count
> @@ -229,26 +456,53 @@ static void __ept_release(struct kref *kref)
> {
> struct rpmsg_endpoint *ept = container_of(kref, struct rpmsg_endpoint,
> refcount);
> +#ifdef CONFIG_RPMSG_VIRTIO_CHAR
> + struct virtio_rpmsg_ept *vept = to_virtio_rpmsg_ept(ept);
> + struct virtio_rpmsg_channel *vch;
> + struct virtproc_info *vrp;
> +
> + if (ept->rpdev) {
> + vch = to_virtio_rpmsg_channel(ept->rpdev);
> + vrp = vch->vrp;
> + (void)virtio_rpmsg_announce_ept_destroy(ept);
> + mutex_lock(&vrp->endpoints_lock);
> + vept->rsvc->ept = NULL;
> + mutex_unlock(&vrp->endpoints_lock);
> + }
> + kfree(vept);
> +#else
> /*
> * At this point no one holds a reference to ept anymore,
> * so we can directly free it
> */
> kfree(ept);
> +#endif
> }
>
> /* for more info, see below documentation of rpmsg_create_ept() */
> static struct rpmsg_endpoint *__rpmsg_create_ept(struct virtproc_info *vrp,
> struct rpmsg_device *rpdev,
> rpmsg_rx_cb_t cb,
> - void *priv, u32 addr)
> + void *priv,
> + struct rpmsg_channel_info *ch)
> {
> int id_min, id_max, id;
> struct rpmsg_endpoint *ept;
> + u32 addr = ch->src;
> struct device *dev = rpdev ? &rpdev->dev : &vrp->vdev->dev;
> +#ifdef CONFIG_RPMSG_VIRTIO_CHAR
> + struct virtio_rpmsg_ept *vept;
> + struct virtio_rpmsg_rsvc *rsvc;
>
> + vept = kzalloc(sizeof(*vept), GFP_KERNEL);
> + if (!vept)
> + return NULL;
> + ept = &vept->ept;
> +#else
> ept = kzalloc(sizeof(*ept), GFP_KERNEL);
> if (!ept)
> return NULL;
> +#endif
>
> kref_init(&ept->refcount);
> mutex_init(&ept->cb_lock);
> @@ -259,7 +513,7 @@ static struct rpmsg_endpoint *__rpmsg_create_ept(struct virtproc_info *vrp,
> ept->ops = &virtio_endpoint_ops;
>
> /* do we need to allocate a local address ? */
> - if (addr == RPMSG_ADDR_ANY) {
> + if (ch->src == RPMSG_ADDR_ANY) {
> id_min = RPMSG_RESERVED_ADDRESSES;
> id_max = 0;
> } else {
> @@ -277,6 +531,19 @@ static struct rpmsg_endpoint *__rpmsg_create_ept(struct virtproc_info *vrp,
> }
> ept->addr = id;
>
> +#ifdef CONFIG_RPMSG_VIRTIO_CHAR
> + if (ept->addr != RPMSG_NS_ADDR) {
> + rsvc = virtio_rpmsg_create_rsvc_by_name(vrp, ch->name);
> + if (!rsvc)
> + goto free_ept;
> + vept->rsvc = rsvc;
> + rsvc->ept = ept;
> + dev_info(&vrp->vdev->dev, "RPMsg ept created, %s:%d,%d.\n",
> + ch->name, ept->addr, rsvc->addr);
> + (void)virtio_rpmsg_announce_ept_create(ept);
> + }
> +#endif
> +
> mutex_unlock(&vrp->endpoints_lock);
>
> return ept;
> @@ -294,7 +561,7 @@ static struct rpmsg_endpoint *virtio_rpmsg_create_ept(struct rpmsg_device *rpdev
> {
> struct virtio_rpmsg_channel *vch = to_virtio_rpmsg_channel(rpdev);
>
> - return __rpmsg_create_ept(vch->vrp, rpdev, cb, priv, chinfo.src);
> + return __rpmsg_create_ept(vch->vrp, rpdev, cb, priv, &chinfo);
> }
>
> /**
> @@ -392,6 +659,7 @@ static void virtio_rpmsg_release_device(struct device *dev)
> kfree(vch);
> }
>
> +#ifndef CONFIG_RPMSG_VIRTIO_CHAR
> /*
> * create an rpmsg channel using its name and address info.
> * this function will be used to create both static and dynamic
> @@ -444,6 +712,7 @@ static struct rpmsg_device *rpmsg_create_channel(struct virtproc_info *vrp,
>
> return rpdev;
> }
> +#endif
>
> /* super simple buffer "allocator" that is just enough for now */
> static void *get_a_tx_buf(struct virtproc_info *vrp)
> @@ -660,8 +929,21 @@ static int rpmsg_send_offchannel_raw(struct rpmsg_device *rpdev,
> static int virtio_rpmsg_send(struct rpmsg_endpoint *ept, void *data, int len)
> {
> struct rpmsg_device *rpdev = ept->rpdev;
> - u32 src = ept->addr, dst = rpdev->dst;
> + u32 src = ept->addr;
> +#ifdef CONFIG_RPMSG_VIRTIO_CHAR
> + struct virtio_rpmsg_ept *vept = to_virtio_rpmsg_ept(ept);
> + u32 dst = vept->rsvc->addr;
> +
> + if (dst == RPMSG_ADDR_ANY)
> + return -EPIPE;
> +#else
> + u32 dst = rpdev->dst;
> +#endif
>
> + if (!rpdev) {
> + pr_err("%s, ept %p rpdev is NULL\n", __func__, ept);
> + return -EINVAL;
> + }
> return rpmsg_send_offchannel_raw(rpdev, src, dst, data, len, true);
> }
>
> @@ -685,7 +967,16 @@ static int virtio_rpmsg_send_offchannel(struct rpmsg_endpoint *ept, u32 src,
> static int virtio_rpmsg_trysend(struct rpmsg_endpoint *ept, void *data, int len)
> {
> struct rpmsg_device *rpdev = ept->rpdev;
> - u32 src = ept->addr, dst = rpdev->dst;
> + u32 src = ept->addr;
> +#ifdef CONFIG_RPMSG_VIRTIO_CHAR
> + struct virtio_rpmsg_ept *vept = to_virtio_rpmsg_ept(ept);
> + u32 dst = vept->rsvc->addr;
> +
> + if (dst == RPMSG_ADDR_ANY)
> + return -EPIPE;
> +#else
> + u32 dst = rpdev->dst;
> +#endif
>
> return rpmsg_send_offchannel_raw(rpdev, src, dst, data, len, false);
> }
> @@ -824,11 +1115,13 @@ 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 virtproc_info *vrp = priv;
> struct device *dev = &vrp->vdev->dev;
> +#ifndef CONFIG_RPMSG_VIRTIO_CHAR
> + struct rpmsg_device *newch;
> int ret;
> +#endif
>
> #if defined(CONFIG_DYNAMIC_DEBUG)
> dynamic_hex_dump("NS announcement: ", DUMP_PREFIX_NONE, 16, 1,
> @@ -863,13 +1156,25 @@ static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void *data, int len,
> chinfo.dst = msg->addr;
>
> if (msg->flags & RPMSG_NS_DESTROY) {
> +#ifdef CONFIG_RPMSG_VIRTIO_CHAR
> + virtio_rpmsg_remove_rsvc_by_name(vrp, chinfo.name);
> +#else
> ret = rpmsg_unregister_device(&vrp->vdev->dev, &chinfo);
> if (ret)
> dev_err(dev, "rpmsg_destroy_channel failed: %d\n", ret);
> +#endif
> } else {
> +#ifdef CONFIG_RPMSG_VIRTIO_CHAR
> + struct virtio_rpmsg_rsvc *rsvc;
> +
> + rsvc = virtio_rpmsg_create_rsvc(vrp, &chinfo);
> + if (!rsvc)
> + dev_err(dev, "virtio_rpmsg_create_rsvc failed\n");
> +#else
> newch = rpmsg_create_channel(vrp, &chinfo);
> if (!newch)
> dev_err(dev, "rpmsg_create_channel failed\n");
> +#endif
> }
>
> return 0;
> @@ -885,6 +1190,10 @@ static int rpmsg_probe(struct virtio_device *vdev)
> int err = 0, i;
> size_t total_buf_space;
> bool notify;
> +#ifdef CONFIG_RPMSG_VIRTIO_CHAR
> + struct virtio_rpmsg_channel *vch;
> + struct rpmsg_device *rp_char_dev;
> +#endif
>
> vrp = kzalloc(sizeof(*vrp), GFP_KERNEL);
> if (!vrp)
> @@ -956,9 +1265,14 @@ static int rpmsg_probe(struct virtio_device *vdev)
>
> /* if supported by the remote processor, enable the name service */
> if (virtio_has_feature(vdev, VIRTIO_RPMSG_F_NS)) {
> + struct rpmsg_channel_info ns_chinfo;
> +
> + ns_chinfo.src = RPMSG_NS_ADDR;
> + ns_chinfo.dst = RPMSG_NS_ADDR;
> + strcpy(ns_chinfo.name, "name_service");
> /* a dedicated endpoint handles the name service msgs */
> vrp->ns_ept = __rpmsg_create_ept(vrp, NULL, rpmsg_ns_cb,
> - vrp, RPMSG_NS_ADDR);
> + vrp, &ns_chinfo);
> if (!vrp->ns_ept) {
> dev_err(&vdev->dev, "failed to create the ns ept\n");
> err = -ENOMEM;
> @@ -966,6 +1280,32 @@ static int rpmsg_probe(struct virtio_device *vdev)
> }
> }
>
> +#ifdef CONFIG_RPMSG_VIRTIO_CHAR
> + vch = kzalloc(sizeof(*vch), GFP_KERNEL);
> + if (!vch) {
> + err = -ENOMEM;
> + goto free_coherent;
> + }
> +
> + /* Link the channel to our vrp */
> + vch->vrp = vrp;
> + /* Initialize remote services list */
> + INIT_LIST_HEAD(&vrp->rsvcs);
> +
> + /* Assign public information to the rpmsg_device */
> + rp_char_dev = &vch->rpdev;
> + rp_char_dev->ops = &virtio_rpmsg_ops;
> +
> + rp_char_dev->dev.parent = &vrp->vdev->dev;
> + rp_char_dev->dev.release = virtio_rpmsg_release_device;
> + err = rpmsg_chrdev_register_device(rp_char_dev);
I also tried to use this char device to expose rpmsg service for
application.
Issue with this device (from my point of view) is that it has to be
relied on a kernel rpmsg device.
I suppose that your need is to expose RMPSG to userland without creating
a platform driver.
In this case perhaps an alternative could be to move this part in
rmpsg_char to allow to probe it in standalone...
Regards
Arnaud
> + if (err) {
> + kfree(vch);
> + goto free_coherent;
> + }
> + dev_info(&vdev->dev, "Registered as RPMsg char device.\n");
> +#endif
> +
> /*
> * Prepare to kick but don't notify yet - we can't do this before
> * device is ready.
> @@ -1009,6 +1349,9 @@ static void rpmsg_remove(struct virtio_device *vdev)
> struct virtproc_info *vrp = vdev->priv;
> size_t total_buf_space = vrp->num_bufs * vrp->buf_size;
> int ret;
> +#ifdef CONFIG_RPMSG_VIRTIO_CHAR
> + struct virtio_rpmsg_rsvc *rsvc, *tmp;
> +#endif
>
> vdev->config->reset(vdev);
>
> @@ -1021,6 +1364,13 @@ static void rpmsg_remove(struct virtio_device *vdev)
>
> idr_destroy(&vrp->endpoints);
>
> +#ifdef CONFIG_RPMSG_VIRTIO_CHAR
> + list_for_each_entry_safe(rsvc, tmp, &vrp->rsvcs, node) {
> + list_del(&rsvc->node);
> + kfree(rsvc);
> + }
> +#endif
> +
> vdev->config->del_vqs(vrp->vdev);
>
> dma_free_coherent(vdev->dev.parent->parent, total_buf_space,
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [RFC] rpmsg: virtio rpmsg: Add RPMsg char driver support
2018-01-05 14:48 ` Arnaud Pouliquen
@ 2018-01-05 22:10 ` Jiaying Liang
2018-01-09 12:55 ` Arnaud Pouliquen
0 siblings, 1 reply; 6+ messages in thread
From: Jiaying Liang @ 2018-01-05 22:10 UTC (permalink / raw)
To: Arnaud Pouliquen, ohad, bjorn.andersson; +Cc: linux-remoteproc, linux-kernel
> -----Original Message-----
> From: Arnaud Pouliquen [mailto:arnaud.pouliquen@st.com]
> Sent: Friday, January 05, 2018 6:48 AM
> To: Jiaying Liang <jliang@xilinx.com>; ohad@wizery.com;
> bjorn.andersson@linaro.org
> Cc: linux-remoteproc@vger.kernel.org; linux-kernel@vger.kernel.org; Jiaying
> Liang <jliang@xilinx.com>
> Subject: Re: [RFC] rpmsg: virtio rpmsg: Add RPMsg char driver support
>
> Hi Wendy,
>
> Few remarks on your patch.
>
> On 01/05/2018 12:18 AM, Wendy Liang wrote:
> > virtio rpmsg was not implemented to use RPMsg char driver.
> > Each virtio ns announcement will create a new RPMsg device which is
> > supposed to bound to a RPMsg driver. It doesn't support dynamic
> > endpoints with name service per RPMsg device.
> > With RPMsg char driver, you can have multiple endpoints per RPMsg
> > device.
> >
> > Here is the change from this patch:
> > * Introduce a macro to indicate if want to use RPMsg char driver
> > for virtio RPMsg. The RPMsg device can either be bounded to
> > a simple RPMsg driver or the RPMsg char driver.
> > * Create Virtio RPMsg char device when the virtio RPMsg driver is
> > probed.
> > * when there is a remote service announced, keep it in the virtio
> > proc remote services list.
> > * when there is an endpoint created, bind it to a remote service
> > from the remote services list. If the service doesn't exist yet,
> > create one and mark the service address as ANY.
> Would be nice to simplify the review if patch was split in several patches (for
> instance per feature introduced).
[Wendy] These changes are made to use the RPMsg char driver.
Some items such when an endpoint is created while the remote hasn't announced
this service yet is "new feature", but at the moment I am not 100% sure if this change follows
the right direction.
>
> >
> > Signed-off-by: Wendy Liang <jliang@xilinx.com>
> > ---
> > We have different userspace applications to use RPMsg differently,
> > what we need is a RPMsg char driver which can supports multiple
> > endpoints per remote device.
> > The virtio rpmsg driver at the moment doesn't support the RPMsg char
> > driver.
> > Please advise if this is patch is the right direction. If not, any
> > suggestions? Thanks
> > ---
> > drivers/rpmsg/Kconfig | 8 +
> > drivers/rpmsg/virtio_rpmsg_bus.c | 364
> > ++++++++++++++++++++++++++++++++++++++-
> > 2 files changed, 365 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/rpmsg/Kconfig b/drivers/rpmsg/Kconfig index
> > 65a9f6b..746f07e 100644
> > --- a/drivers/rpmsg/Kconfig
> > +++ b/drivers/rpmsg/Kconfig
> > @@ -52,4 +52,12 @@ config RPMSG_VIRTIO
> > select RPMSG
> > select VIRTIO
> >
> > +config RPMSG_VIRTIO_CHAR
> > + bool "Enable Virtio RPMSG char device driver support"
> > + default y
> > + depends on RPMSG_VIRTIO
> > + depends on RPMSG_CHAR
> > + help
> > + Say y here to enable to use RPMSG char device interface.
> > +
> > endmenu
> > diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c
> > b/drivers/rpmsg/virtio_rpmsg_bus.c
> > index 82b8300..6e30a3cc 100644
> > --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> > +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> > @@ -56,6 +56,7 @@
> > * @sendq: wait queue of sending contexts waiting for a tx buffers
> > * @sleepers: number of senders that are waiting for a tx buffer
> > * @ns_ept: the bus's name service endpoint
> > + * @rsvcs: remote services
> > *
> > * This structure stores the rpmsg state of a given virtio remote processor
> > * device (there might be several virtio proc devices for each
> > physical @@ -75,6 +76,9 @@ struct virtproc_info {
> > wait_queue_head_t sendq;
> > atomic_t sleepers;
> > struct rpmsg_endpoint *ns_ept;
> > +#ifdef CONFIG_RPMSG_VIRTIO_CHAR
> > + struct list_head rsvcs;
> > +#endif
> > };
> >
> > /* The feature bitmap for virtio rpmsg */ @@ -141,6 +145,36 @@ struct
> > virtio_rpmsg_channel { #define to_virtio_rpmsg_channel(_rpdev) \
> > container_of(_rpdev, struct virtio_rpmsg_channel, rpdev)
> >
> > +#ifdef CONFIG_RPMSG_VIRTIO_CHAR
> > +/**
> > + * struct virtio_rpmsg_rsvc - virtio RPMsg remote service
> > + * @name: name of the RPMsg remote service
> > + * @addr: RPMsg address of the remote service
> > + * @ept: local endpoint bound to the remote service
> > + * @node: list node
> > + */
> > +struct virtio_rpmsg_rsvc {
> > + char name[RPMSG_NAME_SIZE];
> > + u32 addr;
> > + struct rpmsg_endpoint *ept;
> > + struct list_head node;
> > +};
> > +
> > +/**
> > + * struct virtio_rpmsg_ept - virtio RPMsg endpoint
> > + * @rsvc: RPMsg service
> > + * @ept: RPMsg endpoint
> > + *
> > + */
> > +struct virtio_rpmsg_ept {
> > + struct virtio_rpmsg_rsvc *rsvc;
> > + struct rpmsg_endpoint ept;
> > +};
> > +
> > +#define to_virtio_rpmsg_ept(_ept) \
> > + container_of(_ept, struct virtio_rpmsg_ept, ept) #endif
> > +
> > /*
> > * We're allocating buffers of 512 bytes each for communications. The
> > * number of buffers will be computed from the number of buffers
> > supported @@ -216,6 +250,199 @@ rpmsg_sg_init(struct scatterlist *sg,
> void *cpu_addr, unsigned int len)
> > }
> > }
> >
> > +#ifdef CONFIG_RPMSG_VIRTIO_CHAR
> > +/**
> > + * virtio_rpmsg_find_rsvc_by_name - find the announced remote service
> > + * @vrp: virtio remote proc information
> > + * @name: remote service name
> > + *
> > + * The caller is supposed to have mutex lock before calling this
> > +function
> > + *
> > + * return NULL if no remote service has been found, otherwise, return
> > + * the remote service pointer.
> > + */
> > +static struct virtio_rpmsg_rsvc *
> > +virtio_rpmsg_find_rsvc_by_name(struct virtproc_info *vrp, char *name)
> > +{
> > + struct virtio_rpmsg_rsvc *rsvc;
> > +
> > + list_for_each_entry(rsvc, &vrp->rsvcs, node) {
> > + if (!strncmp(rsvc->name, name, RPMSG_NAME_SIZE))
> > + /* remote service has found */
> > + return rsvc;
> > + }
> > +
> > + return NULL;
> > +}
> > +
> > +/**
> > + * virtio_rpmsg_create_rsvc_by_name - create remote service
> > + * @vrp: virtio remote proc information
> > + * @name: remote service name
> > + *
> > + * The caller is supposed to have mutex lock before calling this
> > +function
> > + *
> > + * return NULL if remote service creation failed. otherwise, return
> > + * the remote service pointer.
> > + */
> > +static struct virtio_rpmsg_rsvc *
> > +virtio_rpmsg_create_rsvc_by_name(struct virtproc_info *vrp, char
> > +*name) {
> > + struct virtio_rpmsg_rsvc *rsvc;
> > +
> > + rsvc = virtio_rpmsg_find_rsvc_by_name(vrp, name);
> > + if (rsvc)
> > + return rsvc;
> Here, I think you break the possibility to instantiate several times the same
> service .
> For instance, today if you have 2 communications in parallel:
> Core1_Aplli1 <-- "foo" service --> core2_appli1
> Core1_Aplli2 <-- "foo" service --> core2_appli2
[Wendy] I can add the remote service address to the input argument
That is to identify a remote service, it will be the "name" + service address.
>
> 2 ways to do it:
> 1) one service and 4 endpoints
> => issue it to know the destination address.
> 2) 2 instances of the same service with default endpoint.
>
> > + rsvc = kzalloc(sizeof(*rsvc), GFP_KERNEL);
> > + if (!rsvc)
> > + return NULL;
> > + strncpy(rsvc->name, name, RPMSG_NAME_SIZE);
> > + list_add_tail(&rsvc->node, &vrp->rsvcs);
> > + return rsvc;
> > +}
> > +
> > +/**
> > + * virtio_rpmsg_remove_rsvc_by_name - remove remote service
> > + * @vrp: virtio remote proc information
> > + * @name: remote service name
> > + *
> > + */
> > +static void
> > +virtio_rpmsg_remove_rsvc_by_name(struct virtproc_info *vrp, char
> > +*name) {
> > + struct virtio_rpmsg_rsvc *rsvc;
> > + struct rpmsg_endpoint *ept;
> > + struct virtio_rpmsg_ept *vept;
> > +
> > + mutex_lock(&vrp->endpoints_lock);
> > + list_for_each_entry(rsvc, &vrp->rsvcs, node) {
> > + if (!strncmp(rsvc->name, name, RPMSG_NAME_SIZE)) {
> > + /* remote service has found, no need to
> > + * create
> > + */
> > + ept = rsvc->ept;
> > + if (ept) {
> > + vept = to_virtio_rpmsg_ept(ept);
> > + vept->rsvc = NULL;
> > + }
> > + list_del(&rsvc->node);
> > + kfree(rsvc);
> > + break;
> > + }
> > + }
> > + mutex_unlock(&vrp->endpoints_lock);
> > +}
> > +
> > +/**
> > + * virtio_rpmsg_create_rsvc - create remote service with channel
> > +information
> > + * @vrp: virtio remote proc information
> > + * @chinfo: channel information
> > + *
> > + * return remote service pointer if it is successfully created;
> > +otherwise,
> > + * return NULL.
> > + */
> > +static struct virtio_rpmsg_rsvc *
> > +virtio_rpmsg_create_rsvc(struct virtproc_info *vrp,
> > + struct rpmsg_channel_info *chinfo) {
> > + struct virtio_rpmsg_rsvc *rsvc;
> > +
> > + mutex_lock(&vrp->endpoints_lock);
> > + rsvc = virtio_rpmsg_create_rsvc_by_name(vrp, chinfo->name);
> > + if (rsvc) {
> > + struct virtio_device *vdev = vrp->vdev;
> > +
> > + rsvc->addr = chinfo->dst;
> > + dev_info(&vdev->dev, "Remote has announced
> service %s, %d.\n",
> > + chinfo->name, rsvc->addr);
> > + }
> > + mutex_unlock(&vrp->endpoints_lock);
> > + return rsvc;
> > +}
> > +
> > +/**
> > + * virtio_rpmsg_announce_ept_create - announce endpoint has been
> > +created
> > + * @ept: RPMsg endpoint
> > + *
> > + * return 0 if succeeded, otherwise, return error code.
> > + */
> > +static int virtio_rpmsg_announce_ept_create(struct rpmsg_endpoint
> > +*ept) {
> > + struct virtio_rpmsg_ept *vept = to_virtio_rpmsg_ept(ept);
> > + struct virtio_rpmsg_rsvc *rsvc = vept->rsvc;
> > + struct rpmsg_device *rpdev = ept->rpdev;
> > + struct virtio_rpmsg_channel *vch;
> > + struct virtproc_info *vrp;
> > + struct device *dev;
> > + int err = 0;
> > +
> > + if (!rpdev)
> > + /* If the endpoint is not connected to a RPMsg device,
> > + * no need to send the announcement.
> > + */
> > + return 0;
> > + vch = to_virtio_rpmsg_channel(rpdev);
> > + vrp = vch->vrp;
> > + dev = &ept->rpdev->dev;
> > + /* need to tell remote processor's name service about this channel ?
> */
> > + if (virtio_has_feature(vrp->vdev, VIRTIO_RPMSG_F_NS)) {
> > + struct rpmsg_ns_msg nsm;
> > +
> > + strncpy(nsm.name, rsvc->name, RPMSG_NAME_SIZE);
> > + nsm.addr = ept->addr;
> > + nsm.flags = RPMSG_NS_CREATE;
> > +
> > + err = rpmsg_sendto(ept, &nsm, sizeof(nsm),
> > + RPMSG_NS_ADDR);
> > + if (err)
> > + dev_err(dev, "failed to announce service %d\n", err);
> > + }
> > +
> > + return err;
> > +}
> > +
> > +/**
> > + * virtio_rpmsg_announce_ept_destroy - announce endpoint has been
> > +destroyed
> > + * @ept: RPMsg endpoint
> > + *
> > + * return 0 if succeeded, otherwise, return error code.
> > + */
> > +static int virtio_rpmsg_announce_ept_destroy(struct rpmsg_endpoint
> > +*ept) {
> > + struct virtio_rpmsg_ept *vept = to_virtio_rpmsg_ept(ept);
> > + struct virtio_rpmsg_rsvc *rsvc = vept->rsvc;
> > + struct rpmsg_device *rpdev = ept->rpdev;
> > + struct virtio_rpmsg_channel *vch;
> > + struct virtproc_info *vrp;
> > + struct device *dev;
> > + int err = 0;
> > +
> > + if (!rpdev)
> > + /* If the endpoint is not connected to a RPMsg device,
> > + * no need to send the announcement.
> > + */
> > + return 0;
> > + vch = to_virtio_rpmsg_channel(rpdev);
> > + vrp = vch->vrp;
> > + dev = &ept->rpdev->dev;
> > + /* tell remote processor's name service we're removing this channel
> */
> > + if (virtio_has_feature(vrp->vdev, VIRTIO_RPMSG_F_NS)) {
> > + struct rpmsg_ns_msg nsm;
> > +
> > + strncpy(nsm.name, rsvc->name, RPMSG_NAME_SIZE);
> > + nsm.addr = ept->addr;
> > + nsm.flags = RPMSG_NS_DESTROY;
> > +
> > + err = rpmsg_sendto(ept, &nsm, sizeof(nsm),
> > + RPMSG_NS_ADDR);
> > + if (err)
> > + dev_err(dev, "failed to announce service %d\n", err);
> > + }
> > +
> > + return err;
> > +}
> > +#endif
> > +
> > /**
> > * __ept_release() - deallocate an rpmsg endpoint
> > * @kref: the ept's reference count
> > @@ -229,26 +456,53 @@ static void __ept_release(struct kref *kref) {
> > struct rpmsg_endpoint *ept = container_of(kref, struct
> rpmsg_endpoint,
> > refcount);
> > +#ifdef CONFIG_RPMSG_VIRTIO_CHAR
> > + struct virtio_rpmsg_ept *vept = to_virtio_rpmsg_ept(ept);
> > + struct virtio_rpmsg_channel *vch;
> > + struct virtproc_info *vrp;
> > +
> > + if (ept->rpdev) {
> > + vch = to_virtio_rpmsg_channel(ept->rpdev);
> > + vrp = vch->vrp;
> > + (void)virtio_rpmsg_announce_ept_destroy(ept);
> > + mutex_lock(&vrp->endpoints_lock);
> > + vept->rsvc->ept = NULL;
> > + mutex_unlock(&vrp->endpoints_lock);
> > + }
> > + kfree(vept);
> > +#else
> > /*
> > * At this point no one holds a reference to ept anymore,
> > * so we can directly free it
> > */
> > kfree(ept);
> > +#endif
> > }
> >
> > /* for more info, see below documentation of rpmsg_create_ept() */
> > static struct rpmsg_endpoint *__rpmsg_create_ept(struct virtproc_info
> *vrp,
> > struct rpmsg_device *rpdev,
> > rpmsg_rx_cb_t cb,
> > - void *priv, u32 addr)
> > + void *priv,
> > + struct rpmsg_channel_info
> *ch)
> > {
> > int id_min, id_max, id;
> > struct rpmsg_endpoint *ept;
> > + u32 addr = ch->src;
> > struct device *dev = rpdev ? &rpdev->dev : &vrp->vdev->dev;
> > +#ifdef CONFIG_RPMSG_VIRTIO_CHAR
> > + struct virtio_rpmsg_ept *vept;
> > + struct virtio_rpmsg_rsvc *rsvc;
> >
> > + vept = kzalloc(sizeof(*vept), GFP_KERNEL);
> > + if (!vept)
> > + return NULL;
> > + ept = &vept->ept;
> > +#else
> > ept = kzalloc(sizeof(*ept), GFP_KERNEL);
> > if (!ept)
> > return NULL;
> > +#endif
> >
> > kref_init(&ept->refcount);
> > mutex_init(&ept->cb_lock);
> > @@ -259,7 +513,7 @@ static struct rpmsg_endpoint
> *__rpmsg_create_ept(struct virtproc_info *vrp,
> > ept->ops = &virtio_endpoint_ops;
> >
> > /* do we need to allocate a local address ? */
> > - if (addr == RPMSG_ADDR_ANY) {
> > + if (ch->src == RPMSG_ADDR_ANY) {
> > id_min = RPMSG_RESERVED_ADDRESSES;
> > id_max = 0;
> > } else {
> > @@ -277,6 +531,19 @@ static struct rpmsg_endpoint
> *__rpmsg_create_ept(struct virtproc_info *vrp,
> > }
> > ept->addr = id;
> >
> > +#ifdef CONFIG_RPMSG_VIRTIO_CHAR
> > + if (ept->addr != RPMSG_NS_ADDR) {
> > + rsvc = virtio_rpmsg_create_rsvc_by_name(vrp, ch->name);
> > + if (!rsvc)
> > + goto free_ept;
> > + vept->rsvc = rsvc;
> > + rsvc->ept = ept;
> > + dev_info(&vrp->vdev->dev, "RPMsg ept
> created, %s:%d,%d.\n",
> > + ch->name, ept->addr, rsvc->addr);
> > + (void)virtio_rpmsg_announce_ept_create(ept);
> > + }
> > +#endif
> > +
> > mutex_unlock(&vrp->endpoints_lock);
> >
> > return ept;
> > @@ -294,7 +561,7 @@ static struct rpmsg_endpoint
> > *virtio_rpmsg_create_ept(struct rpmsg_device *rpdev {
> > struct virtio_rpmsg_channel *vch = to_virtio_rpmsg_channel(rpdev);
> >
> > - return __rpmsg_create_ept(vch->vrp, rpdev, cb, priv, chinfo.src);
> > + return __rpmsg_create_ept(vch->vrp, rpdev, cb, priv, &chinfo);
> > }
> >
> > /**
> > @@ -392,6 +659,7 @@ static void virtio_rpmsg_release_device(struct
> device *dev)
> > kfree(vch);
> > }
> >
> > +#ifndef CONFIG_RPMSG_VIRTIO_CHAR
> > /*
> > * create an rpmsg channel using its name and address info.
> > * this function will be used to create both static and dynamic @@
> > -444,6 +712,7 @@ static struct rpmsg_device
> > *rpmsg_create_channel(struct virtproc_info *vrp,
> >
> > return rpdev;
> > }
> > +#endif
> >
> > /* super simple buffer "allocator" that is just enough for now */
> > static void *get_a_tx_buf(struct virtproc_info *vrp) @@ -660,8 +929,21
> > @@ static int rpmsg_send_offchannel_raw(struct rpmsg_device *rpdev,
> > static int virtio_rpmsg_send(struct rpmsg_endpoint *ept, void *data,
> > int len) {
> > struct rpmsg_device *rpdev = ept->rpdev;
> > - u32 src = ept->addr, dst = rpdev->dst;
> > + u32 src = ept->addr;
> > +#ifdef CONFIG_RPMSG_VIRTIO_CHAR
> > + struct virtio_rpmsg_ept *vept = to_virtio_rpmsg_ept(ept);
> > + u32 dst = vept->rsvc->addr;
> > +
> > + if (dst == RPMSG_ADDR_ANY)
> > + return -EPIPE;
> > +#else
> > + u32 dst = rpdev->dst;
> > +#endif
> >
> > + if (!rpdev) {
> > + pr_err("%s, ept %p rpdev is NULL\n", __func__, ept);
> > + return -EINVAL;
> > + }
> > return rpmsg_send_offchannel_raw(rpdev, src, dst, data, len, true);
> > }
> >
> > @@ -685,7 +967,16 @@ static int virtio_rpmsg_send_offchannel(struct
> > rpmsg_endpoint *ept, u32 src, static int virtio_rpmsg_trysend(struct
> > rpmsg_endpoint *ept, void *data, int len) {
> > struct rpmsg_device *rpdev = ept->rpdev;
> > - u32 src = ept->addr, dst = rpdev->dst;
> > + u32 src = ept->addr;
> > +#ifdef CONFIG_RPMSG_VIRTIO_CHAR
> > + struct virtio_rpmsg_ept *vept = to_virtio_rpmsg_ept(ept);
> > + u32 dst = vept->rsvc->addr;
> > +
> > + if (dst == RPMSG_ADDR_ANY)
> > + return -EPIPE;
> > +#else
> > + u32 dst = rpdev->dst;
> > +#endif
> >
> > return rpmsg_send_offchannel_raw(rpdev, src, dst, data, len, false);
> > } @@ -824,11 +1115,13 @@ 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 virtproc_info *vrp = priv;
> > struct device *dev = &vrp->vdev->dev;
> > +#ifndef CONFIG_RPMSG_VIRTIO_CHAR
> > + struct rpmsg_device *newch;
> > int ret;
> > +#endif
> >
> > #if defined(CONFIG_DYNAMIC_DEBUG)
> > dynamic_hex_dump("NS announcement: ", DUMP_PREFIX_NONE, 16,
> 1, @@
> > -863,13 +1156,25 @@ static int rpmsg_ns_cb(struct rpmsg_device *rpdev,
> void *data, int len,
> > chinfo.dst = msg->addr;
> >
> > if (msg->flags & RPMSG_NS_DESTROY) {
> > +#ifdef CONFIG_RPMSG_VIRTIO_CHAR
> > + virtio_rpmsg_remove_rsvc_by_name(vrp, chinfo.name); #else
> > ret = rpmsg_unregister_device(&vrp->vdev->dev, &chinfo);
> > if (ret)
> > dev_err(dev, "rpmsg_destroy_channel failed: %d\n",
> ret);
> > +#endif
> > } else {
> > +#ifdef CONFIG_RPMSG_VIRTIO_CHAR
> > + struct virtio_rpmsg_rsvc *rsvc;
> > +
> > + rsvc = virtio_rpmsg_create_rsvc(vrp, &chinfo);
> > + if (!rsvc)
> > + dev_err(dev, "virtio_rpmsg_create_rsvc failed\n");
> #else
> > newch = rpmsg_create_channel(vrp, &chinfo);
> > if (!newch)
> > dev_err(dev, "rpmsg_create_channel failed\n");
> > +#endif
> > }
> >
> > return 0;
> > @@ -885,6 +1190,10 @@ static int rpmsg_probe(struct virtio_device *vdev)
> > int err = 0, i;
> > size_t total_buf_space;
> > bool notify;
> > +#ifdef CONFIG_RPMSG_VIRTIO_CHAR
> > + struct virtio_rpmsg_channel *vch;
> > + struct rpmsg_device *rp_char_dev;
> > +#endif
> >
> > vrp = kzalloc(sizeof(*vrp), GFP_KERNEL);
> > if (!vrp)
> > @@ -956,9 +1265,14 @@ static int rpmsg_probe(struct virtio_device
> > *vdev)
> >
> > /* if supported by the remote processor, enable the name service */
> > if (virtio_has_feature(vdev, VIRTIO_RPMSG_F_NS)) {
> > + struct rpmsg_channel_info ns_chinfo;
> > +
> > + ns_chinfo.src = RPMSG_NS_ADDR;
> > + ns_chinfo.dst = RPMSG_NS_ADDR;
> > + strcpy(ns_chinfo.name, "name_service");
> > /* a dedicated endpoint handles the name service msgs */
> > vrp->ns_ept = __rpmsg_create_ept(vrp, NULL, rpmsg_ns_cb,
> > - vrp, RPMSG_NS_ADDR);
> > + vrp, &ns_chinfo);
> > if (!vrp->ns_ept) {
> > dev_err(&vdev->dev, "failed to create the ns ept\n");
> > err = -ENOMEM;
> > @@ -966,6 +1280,32 @@ static int rpmsg_probe(struct virtio_device *vdev)
> > }
> > }
> >
> > +#ifdef CONFIG_RPMSG_VIRTIO_CHAR
> > + vch = kzalloc(sizeof(*vch), GFP_KERNEL);
> > + if (!vch) {
> > + err = -ENOMEM;
> > + goto free_coherent;
> > + }
> > +
> > + /* Link the channel to our vrp */
> > + vch->vrp = vrp;
> > + /* Initialize remote services list */
> > + INIT_LIST_HEAD(&vrp->rsvcs);
> > +
> > + /* Assign public information to the rpmsg_device */
> > + rp_char_dev = &vch->rpdev;
> > + rp_char_dev->ops = &virtio_rpmsg_ops;
> > +
> > + rp_char_dev->dev.parent = &vrp->vdev->dev;
> > + rp_char_dev->dev.release = virtio_rpmsg_release_device;
> > + err = rpmsg_chrdev_register_device(rp_char_dev);
>
> I also tried to use this char device to expose rpmsg service for application.
> Issue with this device (from my point of view) is that it has to be relied on a
> kernel rpmsg device.
> I suppose that your need is to expose RMPSG to userland without creating a
> platform driver.
> In this case perhaps an alternative could be to move this part in rmpsg_char
> to allow to probe it in standalone...
[Wendy] with the existing virtio rpmsg implementation, each service will be binded
to a rpmsg_device. Each rpmsg_device can have multiple static endpoints.
But this is different to how rpmsg_char is supposed to use, rpmsg_char looks to me
Each endpoint created by rpmsg_char, either dynamic (without knowing the remote
Address beforehand) or static(specifying the remote endpoint) should be binded
to a service.
rpmsg_char user APIs can meet what we need, however, we uses rpmsg over virtio.
But at the moment the virtio_rpmsg doesn't support rpmsg_char.
You talked about the allow rpmsg_char to be probed in standalone, I think that's good
Idea, however, we need to bind the rpmsg_char to the virtio rpmsg device.
How about to have "driver_override" of rpmsg_device exposed to userspace with channel
name information available (sysfs) and then we can dynamically bind it to rpmsg_char driver
from userspace? And then we can limit the rpmsg_char create endpoint feature to
only create static endpoints over the same channel in case of virtio_rpmsg usage?
Thanks,
Wendy
>
> Regards
> Arnaud
>
> > + if (err) {
> > + kfree(vch);
> > + goto free_coherent;
> > + }
> > + dev_info(&vdev->dev, "Registered as RPMsg char device.\n"); #endif
> > +
> > /*
> > * Prepare to kick but don't notify yet - we can't do this before
> > * device is ready.
> > @@ -1009,6 +1349,9 @@ static void rpmsg_remove(struct virtio_device
> *vdev)
> > struct virtproc_info *vrp = vdev->priv;
> > size_t total_buf_space = vrp->num_bufs * vrp->buf_size;
> > int ret;
> > +#ifdef CONFIG_RPMSG_VIRTIO_CHAR
> > + struct virtio_rpmsg_rsvc *rsvc, *tmp; #endif
> >
> > vdev->config->reset(vdev);
> >
> > @@ -1021,6 +1364,13 @@ static void rpmsg_remove(struct virtio_device
> > *vdev)
> >
> > idr_destroy(&vrp->endpoints);
> >
> > +#ifdef CONFIG_RPMSG_VIRTIO_CHAR
> > + list_for_each_entry_safe(rsvc, tmp, &vrp->rsvcs, node) {
> > + list_del(&rsvc->node);
> > + kfree(rsvc);
> > + }
> > +#endif
> > +
> > vdev->config->del_vqs(vrp->vdev);
> >
> > dma_free_coherent(vdev->dev.parent->parent, total_buf_space,
> >
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC] rpmsg: virtio rpmsg: Add RPMsg char driver support
2018-01-05 22:10 ` Jiaying Liang
@ 2018-01-09 12:55 ` Arnaud Pouliquen
2018-01-11 6:28 ` Jiaying Liang
0 siblings, 1 reply; 6+ messages in thread
From: Arnaud Pouliquen @ 2018-01-09 12:55 UTC (permalink / raw)
To: Jiaying Liang, ohad, bjorn.andersson; +Cc: linux-remoteproc, linux-kernel
On 01/05/2018 11:10 PM, Jiaying Liang wrote:
>
>
>> -----Original Message-----
>> From: Arnaud Pouliquen [mailto:arnaud.pouliquen@st.com]
>> Sent: Friday, January 05, 2018 6:48 AM
>> To: Jiaying Liang <jliang@xilinx.com>; ohad@wizery.com;
>> bjorn.andersson@linaro.org
>> Cc: linux-remoteproc@vger.kernel.org; linux-kernel@vger.kernel.org; Jiaying
>> Liang <jliang@xilinx.com>
>> Subject: Re: [RFC] rpmsg: virtio rpmsg: Add RPMsg char driver support
>>
>> Hi Wendy,
>>
>> Few remarks on your patch.
>>
>> On 01/05/2018 12:18 AM, Wendy Liang wrote:
>>> virtio rpmsg was not implemented to use RPMsg char driver.
>>> Each virtio ns announcement will create a new RPMsg device which is
>>> supposed to bound to a RPMsg driver. It doesn't support dynamic
>>> endpoints with name service per RPMsg device.
>>> With RPMsg char driver, you can have multiple endpoints per RPMsg
>>> device.
>>>
>>> Here is the change from this patch:
>>> * Introduce a macro to indicate if want to use RPMsg char driver
>>> for virtio RPMsg. The RPMsg device can either be bounded to
>>> a simple RPMsg driver or the RPMsg char driver.
>>> * Create Virtio RPMsg char device when the virtio RPMsg driver is
>>> probed.
>>> * when there is a remote service announced, keep it in the virtio
>>> proc remote services list.
>>> * when there is an endpoint created, bind it to a remote service
>>> from the remote services list. If the service doesn't exist yet,
>>> create one and mark the service address as ANY.
>> Would be nice to simplify the review if patch was split in several patches (for
>> instance per feature introduced).
> [Wendy] These changes are made to use the RPMsg char driver.
> Some items such when an endpoint is created while the remote hasn't announced
> this service yet is "new feature", but at the moment I am not 100% sure if this change follows
> the right direction.
>
>>
>>>
>>> Signed-off-by: Wendy Liang <jliang@xilinx.com>
>>> ---
>>> We have different userspace applications to use RPMsg differently,
>>> what we need is a RPMsg char driver which can supports multiple
>>> endpoints per remote device.
>>> The virtio rpmsg driver at the moment doesn't support the RPMsg char
>>> driver.
>>> Please advise if this is patch is the right direction. If not, any
>>> suggestions? Thanks
>>> ---
>>> drivers/rpmsg/Kconfig | 8 +
>>> drivers/rpmsg/virtio_rpmsg_bus.c | 364
>>> ++++++++++++++++++++++++++++++++++++++-
>>> 2 files changed, 365 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/rpmsg/Kconfig b/drivers/rpmsg/Kconfig index
>>> 65a9f6b..746f07e 100644
>>> --- a/drivers/rpmsg/Kconfig
>>> +++ b/drivers/rpmsg/Kconfig
>>> @@ -52,4 +52,12 @@ config RPMSG_VIRTIO
>>> select RPMSG
>>> select VIRTIO
>>>
>>> +config RPMSG_VIRTIO_CHAR
>>> + bool "Enable Virtio RPMSG char device driver support"
>>> + default y
>>> + depends on RPMSG_VIRTIO
>>> + depends on RPMSG_CHAR
>>> + help
>>> + Say y here to enable to use RPMSG char device interface.
>>> +
>>> endmenu
>>> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c
>>> b/drivers/rpmsg/virtio_rpmsg_bus.c
>>> index 82b8300..6e30a3cc 100644
>>> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
>>> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
>>> @@ -56,6 +56,7 @@
>>> * @sendq: wait queue of sending contexts waiting for a tx buffers
>>> * @sleepers: number of senders that are waiting for a tx buffer
>>> * @ns_ept: the bus's name service endpoint
>>> + * @rsvcs: remote services
>>> *
>>> * This structure stores the rpmsg state of a given virtio remote processor
>>> * device (there might be several virtio proc devices for each
>>> physical @@ -75,6 +76,9 @@ struct virtproc_info {
>>> wait_queue_head_t sendq;
>>> atomic_t sleepers;
>>> struct rpmsg_endpoint *ns_ept;
>>> +#ifdef CONFIG_RPMSG_VIRTIO_CHAR
>>> + struct list_head rsvcs;
>>> +#endif
>>> };
>>>
>>> /* The feature bitmap for virtio rpmsg */ @@ -141,6 +145,36 @@ struct
>>> virtio_rpmsg_channel { #define to_virtio_rpmsg_channel(_rpdev) \
>>> container_of(_rpdev, struct virtio_rpmsg_channel, rpdev)
>>>
>>> +#ifdef CONFIG_RPMSG_VIRTIO_CHAR
>>> +/**
>>> + * struct virtio_rpmsg_rsvc - virtio RPMsg remote service
>>> + * @name: name of the RPMsg remote service
>>> + * @addr: RPMsg address of the remote service
>>> + * @ept: local endpoint bound to the remote service
>>> + * @node: list node
>>> + */
>>> +struct virtio_rpmsg_rsvc {
>>> + char name[RPMSG_NAME_SIZE];
>>> + u32 addr;
>>> + struct rpmsg_endpoint *ept;
>>> + struct list_head node;
>>> +};
>>> +
>>> +/**
>>> + * struct virtio_rpmsg_ept - virtio RPMsg endpoint
>>> + * @rsvc: RPMsg service
>>> + * @ept: RPMsg endpoint
>>> + *
>>> + */
>>> +struct virtio_rpmsg_ept {
>>> + struct virtio_rpmsg_rsvc *rsvc;
>>> + struct rpmsg_endpoint ept;
>>> +};
>>> +
>>> +#define to_virtio_rpmsg_ept(_ept) \
>>> + container_of(_ept, struct virtio_rpmsg_ept, ept) #endif
>>> +
>>> /*
>>> * We're allocating buffers of 512 bytes each for communications. The
>>> * number of buffers will be computed from the number of buffers
>>> supported @@ -216,6 +250,199 @@ rpmsg_sg_init(struct scatterlist *sg,
>> void *cpu_addr, unsigned int len)
>>> }
>>> }
>>>
>>> +#ifdef CONFIG_RPMSG_VIRTIO_CHAR
>>> +/**
>>> + * virtio_rpmsg_find_rsvc_by_name - find the announced remote service
>>> + * @vrp: virtio remote proc information
>>> + * @name: remote service name
>>> + *
>>> + * The caller is supposed to have mutex lock before calling this
>>> +function
>>> + *
>>> + * return NULL if no remote service has been found, otherwise, return
>>> + * the remote service pointer.
>>> + */
>>> +static struct virtio_rpmsg_rsvc *
>>> +virtio_rpmsg_find_rsvc_by_name(struct virtproc_info *vrp, char *name)
>>> +{
>>> + struct virtio_rpmsg_rsvc *rsvc;
>>> +
>>> + list_for_each_entry(rsvc, &vrp->rsvcs, node) {
>>> + if (!strncmp(rsvc->name, name, RPMSG_NAME_SIZE))
>>> + /* remote service has found */
>>> + return rsvc;
>>> + }
>>> +
>>> + return NULL;
>>> +}
>>> +
>>> +/**
>>> + * virtio_rpmsg_create_rsvc_by_name - create remote service
>>> + * @vrp: virtio remote proc information
>>> + * @name: remote service name
>>> + *
>>> + * The caller is supposed to have mutex lock before calling this
>>> +function
>>> + *
>>> + * return NULL if remote service creation failed. otherwise, return
>>> + * the remote service pointer.
>>> + */
>>> +static struct virtio_rpmsg_rsvc *
>>> +virtio_rpmsg_create_rsvc_by_name(struct virtproc_info *vrp, char
>>> +*name) {
>>> + struct virtio_rpmsg_rsvc *rsvc;
>>> +
>>> + rsvc = virtio_rpmsg_find_rsvc_by_name(vrp, name);
>>> + if (rsvc)
>>> + return rsvc;
>> Here, I think you break the possibility to instantiate several times the same
>> service .
>> For instance, today if you have 2 communications in parallel:
>> Core1_Aplli1 <-- "foo" service --> core2_appli1
>> Core1_Aplli2 <-- "foo" service --> core2_appli2
> [Wendy] I can add the remote service address to the input argument
> That is to identify a remote service, it will be the "name" + service address.
>>
>> 2 ways to do it:
>> 1) one service and 4 endpoints
>> => issue it to know the destination address.
>> 2) 2 instances of the same service with default endpoint.
>>
>>> + rsvc = kzalloc(sizeof(*rsvc), GFP_KERNEL);
>>> + if (!rsvc)
>>> + return NULL;
>>> + strncpy(rsvc->name, name, RPMSG_NAME_SIZE);
>>> + list_add_tail(&rsvc->node, &vrp->rsvcs);
>>> + return rsvc;
>>> +}
>>> +
>>> +/**
>>> + * virtio_rpmsg_remove_rsvc_by_name - remove remote service
>>> + * @vrp: virtio remote proc information
>>> + * @name: remote service name
>>> + *
>>> + */
>>> +static void
>>> +virtio_rpmsg_remove_rsvc_by_name(struct virtproc_info *vrp, char
>>> +*name) {
>>> + struct virtio_rpmsg_rsvc *rsvc;
>>> + struct rpmsg_endpoint *ept;
>>> + struct virtio_rpmsg_ept *vept;
>>> +
>>> + mutex_lock(&vrp->endpoints_lock);
>>> + list_for_each_entry(rsvc, &vrp->rsvcs, node) {
>>> + if (!strncmp(rsvc->name, name, RPMSG_NAME_SIZE)) {
>>> + /* remote service has found, no need to
>>> + * create
>>> + */
>>> + ept = rsvc->ept;
>>> + if (ept) {
>>> + vept = to_virtio_rpmsg_ept(ept);
>>> + vept->rsvc = NULL;
>>> + }
>>> + list_del(&rsvc->node);
>>> + kfree(rsvc);
>>> + break;
>>> + }
>>> + }
>>> + mutex_unlock(&vrp->endpoints_lock);
>>> +}
>>> +
>>> +/**
>>> + * virtio_rpmsg_create_rsvc - create remote service with channel
>>> +information
>>> + * @vrp: virtio remote proc information
>>> + * @chinfo: channel information
>>> + *
>>> + * return remote service pointer if it is successfully created;
>>> +otherwise,
>>> + * return NULL.
>>> + */
>>> +static struct virtio_rpmsg_rsvc *
>>> +virtio_rpmsg_create_rsvc(struct virtproc_info *vrp,
>>> + struct rpmsg_channel_info *chinfo) {
>>> + struct virtio_rpmsg_rsvc *rsvc;
>>> +
>>> + mutex_lock(&vrp->endpoints_lock);
>>> + rsvc = virtio_rpmsg_create_rsvc_by_name(vrp, chinfo->name);
>>> + if (rsvc) {
>>> + struct virtio_device *vdev = vrp->vdev;
>>> +
>>> + rsvc->addr = chinfo->dst;
>>> + dev_info(&vdev->dev, "Remote has announced
>> service %s, %d.\n",
>>> + chinfo->name, rsvc->addr);
>>> + }
>>> + mutex_unlock(&vrp->endpoints_lock);
>>> + return rsvc;
>>> +}
>>> +
>>> +/**
>>> + * virtio_rpmsg_announce_ept_create - announce endpoint has been
>>> +created
>>> + * @ept: RPMsg endpoint
>>> + *
>>> + * return 0 if succeeded, otherwise, return error code.
>>> + */
>>> +static int virtio_rpmsg_announce_ept_create(struct rpmsg_endpoint
>>> +*ept) {
>>> + struct virtio_rpmsg_ept *vept = to_virtio_rpmsg_ept(ept);
>>> + struct virtio_rpmsg_rsvc *rsvc = vept->rsvc;
>>> + struct rpmsg_device *rpdev = ept->rpdev;
>>> + struct virtio_rpmsg_channel *vch;
>>> + struct virtproc_info *vrp;
>>> + struct device *dev;
>>> + int err = 0;
>>> +
>>> + if (!rpdev)
>>> + /* If the endpoint is not connected to a RPMsg device,
>>> + * no need to send the announcement.
>>> + */
>>> + return 0;
>>> + vch = to_virtio_rpmsg_channel(rpdev);
>>> + vrp = vch->vrp;
>>> + dev = &ept->rpdev->dev;
>>> + /* need to tell remote processor's name service about this channel ?
>> */
>>> + if (virtio_has_feature(vrp->vdev, VIRTIO_RPMSG_F_NS)) {
>>> + struct rpmsg_ns_msg nsm;
>>> +
>>> + strncpy(nsm.name, rsvc->name, RPMSG_NAME_SIZE);
>>> + nsm.addr = ept->addr;
>>> + nsm.flags = RPMSG_NS_CREATE;
>>> +
>>> + err = rpmsg_sendto(ept, &nsm, sizeof(nsm),
>>> + RPMSG_NS_ADDR);
>>> + if (err)
>>> + dev_err(dev, "failed to announce service %d\n", err);
>>> + }
>>> +
>>> + return err;
>>> +}
>>> +
>>> +/**
>>> + * virtio_rpmsg_announce_ept_destroy - announce endpoint has been
>>> +destroyed
>>> + * @ept: RPMsg endpoint
>>> + *
>>> + * return 0 if succeeded, otherwise, return error code.
>>> + */
>>> +static int virtio_rpmsg_announce_ept_destroy(struct rpmsg_endpoint
>>> +*ept) {
>>> + struct virtio_rpmsg_ept *vept = to_virtio_rpmsg_ept(ept);
>>> + struct virtio_rpmsg_rsvc *rsvc = vept->rsvc;
>>> + struct rpmsg_device *rpdev = ept->rpdev;
>>> + struct virtio_rpmsg_channel *vch;
>>> + struct virtproc_info *vrp;
>>> + struct device *dev;
>>> + int err = 0;
>>> +
>>> + if (!rpdev)
>>> + /* If the endpoint is not connected to a RPMsg device,
>>> + * no need to send the announcement.
>>> + */
>>> + return 0;
>>> + vch = to_virtio_rpmsg_channel(rpdev);
>>> + vrp = vch->vrp;
>>> + dev = &ept->rpdev->dev;
>>> + /* tell remote processor's name service we're removing this channel
>> */
>>> + if (virtio_has_feature(vrp->vdev, VIRTIO_RPMSG_F_NS)) {
>>> + struct rpmsg_ns_msg nsm;
>>> +
>>> + strncpy(nsm.name, rsvc->name, RPMSG_NAME_SIZE);
>>> + nsm.addr = ept->addr;
>>> + nsm.flags = RPMSG_NS_DESTROY;
>>> +
>>> + err = rpmsg_sendto(ept, &nsm, sizeof(nsm),
>>> + RPMSG_NS_ADDR);
>>> + if (err)
>>> + dev_err(dev, "failed to announce service %d\n", err);
>>> + }
>>> +
>>> + return err;
>>> +}
>>> +#endif
>>> +
>>> /**
>>> * __ept_release() - deallocate an rpmsg endpoint
>>> * @kref: the ept's reference count
>>> @@ -229,26 +456,53 @@ static void __ept_release(struct kref *kref) {
>>> struct rpmsg_endpoint *ept = container_of(kref, struct
>> rpmsg_endpoint,
>>> refcount);
>>> +#ifdef CONFIG_RPMSG_VIRTIO_CHAR
>>> + struct virtio_rpmsg_ept *vept = to_virtio_rpmsg_ept(ept);
>>> + struct virtio_rpmsg_channel *vch;
>>> + struct virtproc_info *vrp;
>>> +
>>> + if (ept->rpdev) {
>>> + vch = to_virtio_rpmsg_channel(ept->rpdev);
>>> + vrp = vch->vrp;
>>> + (void)virtio_rpmsg_announce_ept_destroy(ept);
>>> + mutex_lock(&vrp->endpoints_lock);
>>> + vept->rsvc->ept = NULL;
>>> + mutex_unlock(&vrp->endpoints_lock);
>>> + }
>>> + kfree(vept);
>>> +#else
>>> /*
>>> * At this point no one holds a reference to ept anymore,
>>> * so we can directly free it
>>> */
>>> kfree(ept);
>>> +#endif
>>> }
>>>
>>> /* for more info, see below documentation of rpmsg_create_ept() */
>>> static struct rpmsg_endpoint *__rpmsg_create_ept(struct virtproc_info
>> *vrp,
>>> struct rpmsg_device *rpdev,
>>> rpmsg_rx_cb_t cb,
>>> - void *priv, u32 addr)
>>> + void *priv,
>>> + struct rpmsg_channel_info
>> *ch)
>>> {
>>> int id_min, id_max, id;
>>> struct rpmsg_endpoint *ept;
>>> + u32 addr = ch->src;
>>> struct device *dev = rpdev ? &rpdev->dev : &vrp->vdev->dev;
>>> +#ifdef CONFIG_RPMSG_VIRTIO_CHAR
>>> + struct virtio_rpmsg_ept *vept;
>>> + struct virtio_rpmsg_rsvc *rsvc;
>>>
>>> + vept = kzalloc(sizeof(*vept), GFP_KERNEL);
>>> + if (!vept)
>>> + return NULL;
>>> + ept = &vept->ept;
>>> +#else
>>> ept = kzalloc(sizeof(*ept), GFP_KERNEL);
>>> if (!ept)
>>> return NULL;
>>> +#endif
>>>
>>> kref_init(&ept->refcount);
>>> mutex_init(&ept->cb_lock);
>>> @@ -259,7 +513,7 @@ static struct rpmsg_endpoint
>> *__rpmsg_create_ept(struct virtproc_info *vrp,
>>> ept->ops = &virtio_endpoint_ops;
>>>
>>> /* do we need to allocate a local address ? */
>>> - if (addr == RPMSG_ADDR_ANY) {
>>> + if (ch->src == RPMSG_ADDR_ANY) {
>>> id_min = RPMSG_RESERVED_ADDRESSES;
>>> id_max = 0;
>>> } else {
>>> @@ -277,6 +531,19 @@ static struct rpmsg_endpoint
>> *__rpmsg_create_ept(struct virtproc_info *vrp,
>>> }
>>> ept->addr = id;
>>>
>>> +#ifdef CONFIG_RPMSG_VIRTIO_CHAR
>>> + if (ept->addr != RPMSG_NS_ADDR) {
>>> + rsvc = virtio_rpmsg_create_rsvc_by_name(vrp, ch->name);
>>> + if (!rsvc)
>>> + goto free_ept;
>>> + vept->rsvc = rsvc;
>>> + rsvc->ept = ept;
>>> + dev_info(&vrp->vdev->dev, "RPMsg ept
>> created, %s:%d,%d.\n",
>>> + ch->name, ept->addr, rsvc->addr);
>>> + (void)virtio_rpmsg_announce_ept_create(ept);
>>> + }
>>> +#endif
>>> +
>>> mutex_unlock(&vrp->endpoints_lock);
>>>
>>> return ept;
>>> @@ -294,7 +561,7 @@ static struct rpmsg_endpoint
>>> *virtio_rpmsg_create_ept(struct rpmsg_device *rpdev {
>>> struct virtio_rpmsg_channel *vch = to_virtio_rpmsg_channel(rpdev);
>>>
>>> - return __rpmsg_create_ept(vch->vrp, rpdev, cb, priv, chinfo.src);
>>> + return __rpmsg_create_ept(vch->vrp, rpdev, cb, priv, &chinfo);
>>> }
>>>
>>> /**
>>> @@ -392,6 +659,7 @@ static void virtio_rpmsg_release_device(struct
>> device *dev)
>>> kfree(vch);
>>> }
>>>
>>> +#ifndef CONFIG_RPMSG_VIRTIO_CHAR
>>> /*
>>> * create an rpmsg channel using its name and address info.
>>> * this function will be used to create both static and dynamic @@
>>> -444,6 +712,7 @@ static struct rpmsg_device
>>> *rpmsg_create_channel(struct virtproc_info *vrp,
>>>
>>> return rpdev;
>>> }
>>> +#endif
>>>
>>> /* super simple buffer "allocator" that is just enough for now */
>>> static void *get_a_tx_buf(struct virtproc_info *vrp) @@ -660,8 +929,21
>>> @@ static int rpmsg_send_offchannel_raw(struct rpmsg_device *rpdev,
>>> static int virtio_rpmsg_send(struct rpmsg_endpoint *ept, void *data,
>>> int len) {
>>> struct rpmsg_device *rpdev = ept->rpdev;
>>> - u32 src = ept->addr, dst = rpdev->dst;
>>> + u32 src = ept->addr;
>>> +#ifdef CONFIG_RPMSG_VIRTIO_CHAR
>>> + struct virtio_rpmsg_ept *vept = to_virtio_rpmsg_ept(ept);
>>> + u32 dst = vept->rsvc->addr;
>>> +
>>> + if (dst == RPMSG_ADDR_ANY)
>>> + return -EPIPE;
>>> +#else
>>> + u32 dst = rpdev->dst;
>>> +#endif
>>>
>>> + if (!rpdev) {
>>> + pr_err("%s, ept %p rpdev is NULL\n", __func__, ept);
>>> + return -EINVAL;
>>> + }
>>> return rpmsg_send_offchannel_raw(rpdev, src, dst, data, len, true);
>>> }
>>>
>>> @@ -685,7 +967,16 @@ static int virtio_rpmsg_send_offchannel(struct
>>> rpmsg_endpoint *ept, u32 src, static int virtio_rpmsg_trysend(struct
>>> rpmsg_endpoint *ept, void *data, int len) {
>>> struct rpmsg_device *rpdev = ept->rpdev;
>>> - u32 src = ept->addr, dst = rpdev->dst;
>>> + u32 src = ept->addr;
>>> +#ifdef CONFIG_RPMSG_VIRTIO_CHAR
>>> + struct virtio_rpmsg_ept *vept = to_virtio_rpmsg_ept(ept);
>>> + u32 dst = vept->rsvc->addr;
>>> +
>>> + if (dst == RPMSG_ADDR_ANY)
>>> + return -EPIPE;
>>> +#else
>>> + u32 dst = rpdev->dst;
>>> +#endif
>>>
>>> return rpmsg_send_offchannel_raw(rpdev, src, dst, data, len, false);
>>> } @@ -824,11 +1115,13 @@ 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 virtproc_info *vrp = priv;
>>> struct device *dev = &vrp->vdev->dev;
>>> +#ifndef CONFIG_RPMSG_VIRTIO_CHAR
>>> + struct rpmsg_device *newch;
>>> int ret;
>>> +#endif
>>>
>>> #if defined(CONFIG_DYNAMIC_DEBUG)
>>> dynamic_hex_dump("NS announcement: ", DUMP_PREFIX_NONE, 16,
>> 1, @@
>>> -863,13 +1156,25 @@ static int rpmsg_ns_cb(struct rpmsg_device *rpdev,
>> void *data, int len,
>>> chinfo.dst = msg->addr;
>>>
>>> if (msg->flags & RPMSG_NS_DESTROY) {
>>> +#ifdef CONFIG_RPMSG_VIRTIO_CHAR
>>> + virtio_rpmsg_remove_rsvc_by_name(vrp, chinfo.name); #else
>>> ret = rpmsg_unregister_device(&vrp->vdev->dev, &chinfo);
>>> if (ret)
>>> dev_err(dev, "rpmsg_destroy_channel failed: %d\n",
>> ret);
>>> +#endif
>>> } else {
>>> +#ifdef CONFIG_RPMSG_VIRTIO_CHAR
>>> + struct virtio_rpmsg_rsvc *rsvc;
>>> +
>>> + rsvc = virtio_rpmsg_create_rsvc(vrp, &chinfo);
>>> + if (!rsvc)
>>> + dev_err(dev, "virtio_rpmsg_create_rsvc failed\n");
>> #else
>>> newch = rpmsg_create_channel(vrp, &chinfo);
>>> if (!newch)
>>> dev_err(dev, "rpmsg_create_channel failed\n");
>>> +#endif
>>> }
>>>
>>> return 0;
>>> @@ -885,6 +1190,10 @@ static int rpmsg_probe(struct virtio_device *vdev)
>>> int err = 0, i;
>>> size_t total_buf_space;
>>> bool notify;
>>> +#ifdef CONFIG_RPMSG_VIRTIO_CHAR
>>> + struct virtio_rpmsg_channel *vch;
>>> + struct rpmsg_device *rp_char_dev;
>>> +#endif
>>>
>>> vrp = kzalloc(sizeof(*vrp), GFP_KERNEL);
>>> if (!vrp)
>>> @@ -956,9 +1265,14 @@ static int rpmsg_probe(struct virtio_device
>>> *vdev)
>>>
>>> /* if supported by the remote processor, enable the name service */
>>> if (virtio_has_feature(vdev, VIRTIO_RPMSG_F_NS)) {
>>> + struct rpmsg_channel_info ns_chinfo;
>>> +
>>> + ns_chinfo.src = RPMSG_NS_ADDR;
>>> + ns_chinfo.dst = RPMSG_NS_ADDR;
>>> + strcpy(ns_chinfo.name, "name_service");
>>> /* a dedicated endpoint handles the name service msgs */
>>> vrp->ns_ept = __rpmsg_create_ept(vrp, NULL, rpmsg_ns_cb,
>>> - vrp, RPMSG_NS_ADDR);
>>> + vrp, &ns_chinfo);
>>> if (!vrp->ns_ept) {
>>> dev_err(&vdev->dev, "failed to create the ns ept\n");
>>> err = -ENOMEM;
>>> @@ -966,6 +1280,32 @@ static int rpmsg_probe(struct virtio_device *vdev)
>>> }
>>> }
>>>
>>> +#ifdef CONFIG_RPMSG_VIRTIO_CHAR
>>> + vch = kzalloc(sizeof(*vch), GFP_KERNEL);
>>> + if (!vch) {
>>> + err = -ENOMEM;
>>> + goto free_coherent;
>>> + }
>>> +
>>> + /* Link the channel to our vrp */
>>> + vch->vrp = vrp;
>>> + /* Initialize remote services list */
>>> + INIT_LIST_HEAD(&vrp->rsvcs);
>>> +
>>> + /* Assign public information to the rpmsg_device */
>>> + rp_char_dev = &vch->rpdev;
>>> + rp_char_dev->ops = &virtio_rpmsg_ops;
>>> +
>>> + rp_char_dev->dev.parent = &vrp->vdev->dev;
>>> + rp_char_dev->dev.release = virtio_rpmsg_release_device;
>>> + err = rpmsg_chrdev_register_device(rp_char_dev);
>>
>> I also tried to use this char device to expose rpmsg service for application.
>> Issue with this device (from my point of view) is that it has to be relied on a
>> kernel rpmsg device.
>> I suppose that your need is to expose RMPSG to userland without creating a
>> platform driver.
>> In this case perhaps an alternative could be to move this part in rmpsg_char
>> to allow to probe it in standalone...
> [Wendy] with the existing virtio rpmsg implementation, each service will be binded
> to a rpmsg_device. Each rpmsg_device can have multiple static endpoints.
> But this is different to how rpmsg_char is supposed to use, rpmsg_char looks to me
> Each endpoint created by rpmsg_char, either dynamic (without knowing the remote
> Address beforehand) or static(specifying the remote endpoint) should be binded
> to a service.
>
> rpmsg_char user APIs can meet what we need, however, we uses rpmsg over virtio.
> But at the moment the virtio_rpmsg doesn't support rpmsg_char.
> You talked about the allow rpmsg_char to be probed in standalone, I think that's good
> Idea, however, we need to bind the rpmsg_char to the virtio rpmsg device.
remoteproc binds rpmsg with virtio, so calling register_rpmsg_driver
should do the job.
>
> How about to have "driver_override" of rpmsg_device exposed to userspace with channel
> name information available (sysfs) and then we can dynamically bind it to rpmsg_char driver
> from userspace? And then we can limit the rpmsg_char create endpoint feature to
> only create static endpoints over the same channel in case of virtio_rpmsg usage?
Creating a specific device could be another solution. I have no opinion
on what could be the best strategy.
Bjorn, what is your feeling?
A constraint to add in design is that we should not expose all the
rpmsg device to userland. Except if i missed something, it seems that
activating RPMSG_VIRTIO_CHAR config exposes all the rpmsg devices.
Another alternative is to expose rpmsg to userland trough some generic
interfaces like proxy, tty, FS posix....
Thanks,
Arnaud
>
> Thanks,
> Wendy
>
>>
>> Regards
>> Arnaud
>>
>>> + if (err) {
>>> + kfree(vch);
>>> + goto free_coherent;
>>> + }
>>> + dev_info(&vdev->dev, "Registered as RPMsg char device.\n"); #endif
>>> +
>>> /*
>>> * Prepare to kick but don't notify yet - we can't do this before
>>> * device is ready.
>>> @@ -1009,6 +1349,9 @@ static void rpmsg_remove(struct virtio_device
>> *vdev)
>>> struct virtproc_info *vrp = vdev->priv;
>>> size_t total_buf_space = vrp->num_bufs * vrp->buf_size;
>>> int ret;
>>> +#ifdef CONFIG_RPMSG_VIRTIO_CHAR
>>> + struct virtio_rpmsg_rsvc *rsvc, *tmp; #endif
>>>
>>> vdev->config->reset(vdev);
>>>
>>> @@ -1021,6 +1364,13 @@ static void rpmsg_remove(struct virtio_device
>>> *vdev)
>>>
>>> idr_destroy(&vrp->endpoints);
>>>
>>> +#ifdef CONFIG_RPMSG_VIRTIO_CHAR
>>> + list_for_each_entry_safe(rsvc, tmp, &vrp->rsvcs, node) {
>>> + list_del(&rsvc->node);
>>> + kfree(rsvc);
>>> + }
>>> +#endif
>>> +
>>> vdev->config->del_vqs(vrp->vdev);
>>>
>>> dma_free_coherent(vdev->dev.parent->parent, total_buf_space,
>>>
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [RFC] rpmsg: virtio rpmsg: Add RPMsg char driver support
2018-01-09 12:55 ` Arnaud Pouliquen
@ 2018-01-11 6:28 ` Jiaying Liang
0 siblings, 0 replies; 6+ messages in thread
From: Jiaying Liang @ 2018-01-11 6:28 UTC (permalink / raw)
To: Arnaud Pouliquen, ohad, bjorn.andersson; +Cc: linux-remoteproc, linux-kernel
> -----Original Message-----
> From: Arnaud Pouliquen [mailto:arnaud.pouliquen@st.com]
> Sent: Tuesday, January 09, 2018 4:56 AM
> To: Jiaying Liang <jliang@xilinx.com>; ohad@wizery.com;
> bjorn.andersson@linaro.org
> Cc: linux-remoteproc@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [RFC] rpmsg: virtio rpmsg: Add RPMsg char driver support
>
>
>
> On 01/05/2018 11:10 PM, Jiaying Liang wrote:
> >
> >
> >> -----Original Message-----
> >> From: Arnaud Pouliquen [mailto:arnaud.pouliquen@st.com]
> >> Sent: Friday, January 05, 2018 6:48 AM
> >> To: Jiaying Liang <jliang@xilinx.com>; ohad@wizery.com;
> >> bjorn.andersson@linaro.org
> >> Cc: linux-remoteproc@vger.kernel.org; linux-kernel@vger.kernel.org;
> >> Jiaying Liang <jliang@xilinx.com>
> >> Subject: Re: [RFC] rpmsg: virtio rpmsg: Add RPMsg char driver support
> >>
> >> Hi Wendy,
> >>
> >> Few remarks on your patch.
> >>
> >> On 01/05/2018 12:18 AM, Wendy Liang wrote:
> >>> virtio rpmsg was not implemented to use RPMsg char driver.
> >>> Each virtio ns announcement will create a new RPMsg device which is
> >>> supposed to bound to a RPMsg driver. It doesn't support dynamic
> >>> endpoints with name service per RPMsg device.
> >>> With RPMsg char driver, you can have multiple endpoints per RPMsg
> >>> device.
> >>>
> >>> Here is the change from this patch:
> >>> * Introduce a macro to indicate if want to use RPMsg char driver
> >>> for virtio RPMsg. The RPMsg device can either be bounded to
> >>> a simple RPMsg driver or the RPMsg char driver.
> >>> * Create Virtio RPMsg char device when the virtio RPMsg driver is
> >>> probed.
> >>> * when there is a remote service announced, keep it in the virtio
> >>> proc remote services list.
> >>> * when there is an endpoint created, bind it to a remote service
> >>> from the remote services list. If the service doesn't exist yet,
> >>> create one and mark the service address as ANY.
> >> Would be nice to simplify the review if patch was split in several
> >> patches (for instance per feature introduced).
> > [Wendy] These changes are made to use the RPMsg char driver.
> > Some items such when an endpoint is created while the remote hasn't
> > announced this service yet is "new feature", but at the moment I am
> > not 100% sure if this change follows the right direction.
> >
> >>
> >>>
> >>> Signed-off-by: Wendy Liang <jliang@xilinx.com>
> >>> ---
> >>> We have different userspace applications to use RPMsg differently,
> >>> what we need is a RPMsg char driver which can supports multiple
> >>> endpoints per remote device.
> >>> The virtio rpmsg driver at the moment doesn't support the RPMsg char
> >>> driver.
> >>> Please advise if this is patch is the right direction. If not, any
> >>> suggestions? Thanks
> >>> ---
> >>> drivers/rpmsg/Kconfig | 8 +
> >>> drivers/rpmsg/virtio_rpmsg_bus.c | 364
> >>> ++++++++++++++++++++++++++++++++++++++-
> >>> 2 files changed, 365 insertions(+), 7 deletions(-)
> >>>
> >>> diff --git a/drivers/rpmsg/Kconfig b/drivers/rpmsg/Kconfig index
> >>> 65a9f6b..746f07e 100644
> >>> --- a/drivers/rpmsg/Kconfig
> >>> +++ b/drivers/rpmsg/Kconfig
> >>> @@ -52,4 +52,12 @@ config RPMSG_VIRTIO
> >>> select RPMSG
> >>> select VIRTIO
> >>>
> >>> +config RPMSG_VIRTIO_CHAR
> >>> + bool "Enable Virtio RPMSG char device driver support"
> >>> + default y
> >>> + depends on RPMSG_VIRTIO
> >>> + depends on RPMSG_CHAR
> >>> + help
> >>> + Say y here to enable to use RPMSG char device interface.
> >>> +
> >>> endmenu
> >>> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c
> >>> b/drivers/rpmsg/virtio_rpmsg_bus.c
> >>> index 82b8300..6e30a3cc 100644
> >>> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> >>> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> >>> @@ -56,6 +56,7 @@
> >>> * @sendq: wait queue of sending contexts waiting for a tx
> buffers
> >>> * @sleepers: number of senders that are waiting for a tx buffer
> >>> * @ns_ept: the bus's name service endpoint
> >>> + * @rsvcs: remote services
> >>> *
> >>> * This structure stores the rpmsg state of a given virtio remote
> processor
> >>> * device (there might be several virtio proc devices for each
> >>> physical @@ -75,6 +76,9 @@ struct virtproc_info {
> >>> wait_queue_head_t sendq;
> >>> atomic_t sleepers;
> >>> struct rpmsg_endpoint *ns_ept;
> >>> +#ifdef CONFIG_RPMSG_VIRTIO_CHAR
> >>> + struct list_head rsvcs;
> >>> +#endif
> >>> };
> >>>
> >>> /* The feature bitmap for virtio rpmsg */ @@ -141,6 +145,36 @@
> >>> struct virtio_rpmsg_channel { #define to_virtio_rpmsg_channel(_rpdev)
> \
> >>> container_of(_rpdev, struct virtio_rpmsg_channel, rpdev)
> >>>
> >>> +#ifdef CONFIG_RPMSG_VIRTIO_CHAR
> >>> +/**
> >>> + * struct virtio_rpmsg_rsvc - virtio RPMsg remote service
> >>> + * @name: name of the RPMsg remote service
> >>> + * @addr: RPMsg address of the remote service
> >>> + * @ept: local endpoint bound to the remote service
> >>> + * @node: list node
> >>> + */
> >>> +struct virtio_rpmsg_rsvc {
> >>> + char name[RPMSG_NAME_SIZE];
> >>> + u32 addr;
> >>> + struct rpmsg_endpoint *ept;
> >>> + struct list_head node;
> >>> +};
> >>> +
> >>> +/**
> >>> + * struct virtio_rpmsg_ept - virtio RPMsg endpoint
> >>> + * @rsvc: RPMsg service
> >>> + * @ept: RPMsg endpoint
> >>> + *
> >>> + */
> >>> +struct virtio_rpmsg_ept {
> >>> + struct virtio_rpmsg_rsvc *rsvc;
> >>> + struct rpmsg_endpoint ept;
> >>> +};
> >>> +
> >>> +#define to_virtio_rpmsg_ept(_ept) \
> >>> + container_of(_ept, struct virtio_rpmsg_ept, ept) #endif
> >>> +
> >>> /*
> >>> * We're allocating buffers of 512 bytes each for communications. The
> >>> * number of buffers will be computed from the number of buffers
> >>> supported @@ -216,6 +250,199 @@ rpmsg_sg_init(struct scatterlist
> >>> *sg,
> >> void *cpu_addr, unsigned int len)
> >>> }
> >>> }
> >>>
> >>> +#ifdef CONFIG_RPMSG_VIRTIO_CHAR
> >>> +/**
> >>> + * virtio_rpmsg_find_rsvc_by_name - find the announced remote
> >>> +service
> >>> + * @vrp: virtio remote proc information
> >>> + * @name: remote service name
> >>> + *
> >>> + * The caller is supposed to have mutex lock before calling this
> >>> +function
> >>> + *
> >>> + * return NULL if no remote service has been found, otherwise,
> >>> +return
> >>> + * the remote service pointer.
> >>> + */
> >>> +static struct virtio_rpmsg_rsvc *
> >>> +virtio_rpmsg_find_rsvc_by_name(struct virtproc_info *vrp, char
> >>> +*name) {
> >>> + struct virtio_rpmsg_rsvc *rsvc;
> >>> +
> >>> + list_for_each_entry(rsvc, &vrp->rsvcs, node) {
> >>> + if (!strncmp(rsvc->name, name, RPMSG_NAME_SIZE))
> >>> + /* remote service has found */
> >>> + return rsvc;
> >>> + }
> >>> +
> >>> + return NULL;
> >>> +}
> >>> +
> >>> +/**
> >>> + * virtio_rpmsg_create_rsvc_by_name - create remote service
> >>> + * @vrp: virtio remote proc information
> >>> + * @name: remote service name
> >>> + *
> >>> + * The caller is supposed to have mutex lock before calling this
> >>> +function
> >>> + *
> >>> + * return NULL if remote service creation failed. otherwise, return
> >>> + * the remote service pointer.
> >>> + */
> >>> +static struct virtio_rpmsg_rsvc *
> >>> +virtio_rpmsg_create_rsvc_by_name(struct virtproc_info *vrp, char
> >>> +*name) {
> >>> + struct virtio_rpmsg_rsvc *rsvc;
> >>> +
> >>> + rsvc = virtio_rpmsg_find_rsvc_by_name(vrp, name);
> >>> + if (rsvc)
> >>> + return rsvc;
> >> Here, I think you break the possibility to instantiate several times
> >> the same service .
> >> For instance, today if you have 2 communications in parallel:
> >> Core1_Aplli1 <-- "foo" service --> core2_appli1
> >> Core1_Aplli2 <-- "foo" service --> core2_appli2
> > [Wendy] I can add the remote service address to the input argument
> > That is to identify a remote service, it will be the "name" + service address.
> >>
> >> 2 ways to do it:
> >> 1) one service and 4 endpoints
> >> => issue it to know the destination address.
> >> 2) 2 instances of the same service with default endpoint.
> >>
> >>> + rsvc = kzalloc(sizeof(*rsvc), GFP_KERNEL);
> >>> + if (!rsvc)
> >>> + return NULL;
> >>> + strncpy(rsvc->name, name, RPMSG_NAME_SIZE);
> >>> + list_add_tail(&rsvc->node, &vrp->rsvcs);
> >>> + return rsvc;
> >>> +}
> >>> +
> >>> +/**
> >>> + * virtio_rpmsg_remove_rsvc_by_name - remove remote service
> >>> + * @vrp: virtio remote proc information
> >>> + * @name: remote service name
> >>> + *
> >>> + */
> >>> +static void
> >>> +virtio_rpmsg_remove_rsvc_by_name(struct virtproc_info *vrp, char
> >>> +*name) {
> >>> + struct virtio_rpmsg_rsvc *rsvc;
> >>> + struct rpmsg_endpoint *ept;
> >>> + struct virtio_rpmsg_ept *vept;
> >>> +
> >>> + mutex_lock(&vrp->endpoints_lock);
> >>> + list_for_each_entry(rsvc, &vrp->rsvcs, node) {
> >>> + if (!strncmp(rsvc->name, name, RPMSG_NAME_SIZE)) {
> >>> + /* remote service has found, no need to
> >>> + * create
> >>> + */
> >>> + ept = rsvc->ept;
> >>> + if (ept) {
> >>> + vept = to_virtio_rpmsg_ept(ept);
> >>> + vept->rsvc = NULL;
> >>> + }
> >>> + list_del(&rsvc->node);
> >>> + kfree(rsvc);
> >>> + break;
> >>> + }
> >>> + }
> >>> + mutex_unlock(&vrp->endpoints_lock);
> >>> +}
> >>> +
> >>> +/**
> >>> + * virtio_rpmsg_create_rsvc - create remote service with channel
> >>> +information
> >>> + * @vrp: virtio remote proc information
> >>> + * @chinfo: channel information
> >>> + *
> >>> + * return remote service pointer if it is successfully created;
> >>> +otherwise,
> >>> + * return NULL.
> >>> + */
> >>> +static struct virtio_rpmsg_rsvc *
> >>> +virtio_rpmsg_create_rsvc(struct virtproc_info *vrp,
> >>> + struct rpmsg_channel_info *chinfo) {
> >>> + struct virtio_rpmsg_rsvc *rsvc;
> >>> +
> >>> + mutex_lock(&vrp->endpoints_lock);
> >>> + rsvc = virtio_rpmsg_create_rsvc_by_name(vrp, chinfo->name);
> >>> + if (rsvc) {
> >>> + struct virtio_device *vdev = vrp->vdev;
> >>> +
> >>> + rsvc->addr = chinfo->dst;
> >>> + dev_info(&vdev->dev, "Remote has announced
> >> service %s, %d.\n",
> >>> + chinfo->name, rsvc->addr);
> >>> + }
> >>> + mutex_unlock(&vrp->endpoints_lock);
> >>> + return rsvc;
> >>> +}
> >>> +
> >>> +/**
> >>> + * virtio_rpmsg_announce_ept_create - announce endpoint has been
> >>> +created
> >>> + * @ept: RPMsg endpoint
> >>> + *
> >>> + * return 0 if succeeded, otherwise, return error code.
> >>> + */
> >>> +static int virtio_rpmsg_announce_ept_create(struct rpmsg_endpoint
> >>> +*ept) {
> >>> + struct virtio_rpmsg_ept *vept = to_virtio_rpmsg_ept(ept);
> >>> + struct virtio_rpmsg_rsvc *rsvc = vept->rsvc;
> >>> + struct rpmsg_device *rpdev = ept->rpdev;
> >>> + struct virtio_rpmsg_channel *vch;
> >>> + struct virtproc_info *vrp;
> >>> + struct device *dev;
> >>> + int err = 0;
> >>> +
> >>> + if (!rpdev)
> >>> + /* If the endpoint is not connected to a RPMsg device,
> >>> + * no need to send the announcement.
> >>> + */
> >>> + return 0;
> >>> + vch = to_virtio_rpmsg_channel(rpdev);
> >>> + vrp = vch->vrp;
> >>> + dev = &ept->rpdev->dev;
> >>> + /* need to tell remote processor's name service about this channel ?
> >> */
> >>> + if (virtio_has_feature(vrp->vdev, VIRTIO_RPMSG_F_NS)) {
> >>> + struct rpmsg_ns_msg nsm;
> >>> +
> >>> + strncpy(nsm.name, rsvc->name, RPMSG_NAME_SIZE);
> >>> + nsm.addr = ept->addr;
> >>> + nsm.flags = RPMSG_NS_CREATE;
> >>> +
> >>> + err = rpmsg_sendto(ept, &nsm, sizeof(nsm),
> >>> + RPMSG_NS_ADDR);
> >>> + if (err)
> >>> + dev_err(dev, "failed to announce service %d\n", err);
> >>> + }
> >>> +
> >>> + return err;
> >>> +}
> >>> +
> >>> +/**
> >>> + * virtio_rpmsg_announce_ept_destroy - announce endpoint has been
> >>> +destroyed
> >>> + * @ept: RPMsg endpoint
> >>> + *
> >>> + * return 0 if succeeded, otherwise, return error code.
> >>> + */
> >>> +static int virtio_rpmsg_announce_ept_destroy(struct rpmsg_endpoint
> >>> +*ept) {
> >>> + struct virtio_rpmsg_ept *vept = to_virtio_rpmsg_ept(ept);
> >>> + struct virtio_rpmsg_rsvc *rsvc = vept->rsvc;
> >>> + struct rpmsg_device *rpdev = ept->rpdev;
> >>> + struct virtio_rpmsg_channel *vch;
> >>> + struct virtproc_info *vrp;
> >>> + struct device *dev;
> >>> + int err = 0;
> >>> +
> >>> + if (!rpdev)
> >>> + /* If the endpoint is not connected to a RPMsg device,
> >>> + * no need to send the announcement.
> >>> + */
> >>> + return 0;
> >>> + vch = to_virtio_rpmsg_channel(rpdev);
> >>> + vrp = vch->vrp;
> >>> + dev = &ept->rpdev->dev;
> >>> + /* tell remote processor's name service we're removing this
> >>> +channel
> >> */
> >>> + if (virtio_has_feature(vrp->vdev, VIRTIO_RPMSG_F_NS)) {
> >>> + struct rpmsg_ns_msg nsm;
> >>> +
> >>> + strncpy(nsm.name, rsvc->name, RPMSG_NAME_SIZE);
> >>> + nsm.addr = ept->addr;
> >>> + nsm.flags = RPMSG_NS_DESTROY;
> >>> +
> >>> + err = rpmsg_sendto(ept, &nsm, sizeof(nsm),
> >>> + RPMSG_NS_ADDR);
> >>> + if (err)
> >>> + dev_err(dev, "failed to announce service %d\n", err);
> >>> + }
> >>> +
> >>> + return err;
> >>> +}
> >>> +#endif
> >>> +
> >>> /**
> >>> * __ept_release() - deallocate an rpmsg endpoint
> >>> * @kref: the ept's reference count @@ -229,26 +456,53 @@ static
> >>> void __ept_release(struct kref *kref) {
> >>> struct rpmsg_endpoint *ept = container_of(kref, struct
> >> rpmsg_endpoint,
> >>> refcount);
> >>> +#ifdef CONFIG_RPMSG_VIRTIO_CHAR
> >>> + struct virtio_rpmsg_ept *vept = to_virtio_rpmsg_ept(ept);
> >>> + struct virtio_rpmsg_channel *vch;
> >>> + struct virtproc_info *vrp;
> >>> +
> >>> + if (ept->rpdev) {
> >>> + vch = to_virtio_rpmsg_channel(ept->rpdev);
> >>> + vrp = vch->vrp;
> >>> + (void)virtio_rpmsg_announce_ept_destroy(ept);
> >>> + mutex_lock(&vrp->endpoints_lock);
> >>> + vept->rsvc->ept = NULL;
> >>> + mutex_unlock(&vrp->endpoints_lock);
> >>> + }
> >>> + kfree(vept);
> >>> +#else
> >>> /*
> >>> * At this point no one holds a reference to ept anymore,
> >>> * so we can directly free it
> >>> */
> >>> kfree(ept);
> >>> +#endif
> >>> }
> >>>
> >>> /* for more info, see below documentation of rpmsg_create_ept() */
> >>> static struct rpmsg_endpoint *__rpmsg_create_ept(struct
> >>> virtproc_info
> >> *vrp,
> >>> struct rpmsg_device *rpdev,
> >>> rpmsg_rx_cb_t cb,
> >>> - void *priv, u32 addr)
> >>> + void *priv,
> >>> + struct rpmsg_channel_info
> >> *ch)
> >>> {
> >>> int id_min, id_max, id;
> >>> struct rpmsg_endpoint *ept;
> >>> + u32 addr = ch->src;
> >>> struct device *dev = rpdev ? &rpdev->dev : &vrp->vdev->dev;
> >>> +#ifdef CONFIG_RPMSG_VIRTIO_CHAR
> >>> + struct virtio_rpmsg_ept *vept;
> >>> + struct virtio_rpmsg_rsvc *rsvc;
> >>>
> >>> + vept = kzalloc(sizeof(*vept), GFP_KERNEL);
> >>> + if (!vept)
> >>> + return NULL;
> >>> + ept = &vept->ept;
> >>> +#else
> >>> ept = kzalloc(sizeof(*ept), GFP_KERNEL);
> >>> if (!ept)
> >>> return NULL;
> >>> +#endif
> >>>
> >>> kref_init(&ept->refcount);
> >>> mutex_init(&ept->cb_lock);
> >>> @@ -259,7 +513,7 @@ static struct rpmsg_endpoint
> >> *__rpmsg_create_ept(struct virtproc_info *vrp,
> >>> ept->ops = &virtio_endpoint_ops;
> >>>
> >>> /* do we need to allocate a local address ? */
> >>> - if (addr == RPMSG_ADDR_ANY) {
> >>> + if (ch->src == RPMSG_ADDR_ANY) {
> >>> id_min = RPMSG_RESERVED_ADDRESSES;
> >>> id_max = 0;
> >>> } else {
> >>> @@ -277,6 +531,19 @@ static struct rpmsg_endpoint
> >> *__rpmsg_create_ept(struct virtproc_info *vrp,
> >>> }
> >>> ept->addr = id;
> >>>
> >>> +#ifdef CONFIG_RPMSG_VIRTIO_CHAR
> >>> + if (ept->addr != RPMSG_NS_ADDR) {
> >>> + rsvc = virtio_rpmsg_create_rsvc_by_name(vrp, ch->name);
> >>> + if (!rsvc)
> >>> + goto free_ept;
> >>> + vept->rsvc = rsvc;
> >>> + rsvc->ept = ept;
> >>> + dev_info(&vrp->vdev->dev, "RPMsg ept
> >> created, %s:%d,%d.\n",
> >>> + ch->name, ept->addr, rsvc->addr);
> >>> + (void)virtio_rpmsg_announce_ept_create(ept);
> >>> + }
> >>> +#endif
> >>> +
> >>> mutex_unlock(&vrp->endpoints_lock);
> >>>
> >>> return ept;
> >>> @@ -294,7 +561,7 @@ static struct rpmsg_endpoint
> >>> *virtio_rpmsg_create_ept(struct rpmsg_device *rpdev {
> >>> struct virtio_rpmsg_channel *vch = to_virtio_rpmsg_channel(rpdev);
> >>>
> >>> - return __rpmsg_create_ept(vch->vrp, rpdev, cb, priv, chinfo.src);
> >>> + return __rpmsg_create_ept(vch->vrp, rpdev, cb, priv, &chinfo);
> >>> }
> >>>
> >>> /**
> >>> @@ -392,6 +659,7 @@ static void virtio_rpmsg_release_device(struct
> >> device *dev)
> >>> kfree(vch);
> >>> }
> >>>
> >>> +#ifndef CONFIG_RPMSG_VIRTIO_CHAR
> >>> /*
> >>> * create an rpmsg channel using its name and address info.
> >>> * this function will be used to create both static and dynamic @@
> >>> -444,6 +712,7 @@ static struct rpmsg_device
> >>> *rpmsg_create_channel(struct virtproc_info *vrp,
> >>>
> >>> return rpdev;
> >>> }
> >>> +#endif
> >>>
> >>> /* super simple buffer "allocator" that is just enough for now */
> >>> static void *get_a_tx_buf(struct virtproc_info *vrp) @@ -660,8
> >>> +929,21 @@ static int rpmsg_send_offchannel_raw(struct rpmsg_device
> >>> *rpdev, static int virtio_rpmsg_send(struct rpmsg_endpoint *ept,
> >>> void *data, int len) {
> >>> struct rpmsg_device *rpdev = ept->rpdev;
> >>> - u32 src = ept->addr, dst = rpdev->dst;
> >>> + u32 src = ept->addr;
> >>> +#ifdef CONFIG_RPMSG_VIRTIO_CHAR
> >>> + struct virtio_rpmsg_ept *vept = to_virtio_rpmsg_ept(ept);
> >>> + u32 dst = vept->rsvc->addr;
> >>> +
> >>> + if (dst == RPMSG_ADDR_ANY)
> >>> + return -EPIPE;
> >>> +#else
> >>> + u32 dst = rpdev->dst;
> >>> +#endif
> >>>
> >>> + if (!rpdev) {
> >>> + pr_err("%s, ept %p rpdev is NULL\n", __func__, ept);
> >>> + return -EINVAL;
> >>> + }
> >>> return rpmsg_send_offchannel_raw(rpdev, src, dst, data, len,
> >>> true); }
> >>>
> >>> @@ -685,7 +967,16 @@ static int virtio_rpmsg_send_offchannel(struct
> >>> rpmsg_endpoint *ept, u32 src, static int
> >>> virtio_rpmsg_trysend(struct rpmsg_endpoint *ept, void *data, int len) {
> >>> struct rpmsg_device *rpdev = ept->rpdev;
> >>> - u32 src = ept->addr, dst = rpdev->dst;
> >>> + u32 src = ept->addr;
> >>> +#ifdef CONFIG_RPMSG_VIRTIO_CHAR
> >>> + struct virtio_rpmsg_ept *vept = to_virtio_rpmsg_ept(ept);
> >>> + u32 dst = vept->rsvc->addr;
> >>> +
> >>> + if (dst == RPMSG_ADDR_ANY)
> >>> + return -EPIPE;
> >>> +#else
> >>> + u32 dst = rpdev->dst;
> >>> +#endif
> >>>
> >>> return rpmsg_send_offchannel_raw(rpdev, src, dst, data, len,
> >>> false); } @@ -824,11 +1115,13 @@ 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 virtproc_info *vrp = priv;
> >>> struct device *dev = &vrp->vdev->dev;
> >>> +#ifndef CONFIG_RPMSG_VIRTIO_CHAR
> >>> + struct rpmsg_device *newch;
> >>> int ret;
> >>> +#endif
> >>>
> >>> #if defined(CONFIG_DYNAMIC_DEBUG)
> >>> dynamic_hex_dump("NS announcement: ", DUMP_PREFIX_NONE, 16,
> >> 1, @@
> >>> -863,13 +1156,25 @@ static int rpmsg_ns_cb(struct rpmsg_device
> >>> *rpdev,
> >> void *data, int len,
> >>> chinfo.dst = msg->addr;
> >>>
> >>> if (msg->flags & RPMSG_NS_DESTROY) {
> >>> +#ifdef CONFIG_RPMSG_VIRTIO_CHAR
> >>> + virtio_rpmsg_remove_rsvc_by_name(vrp, chinfo.name); #else
> >>> ret = rpmsg_unregister_device(&vrp->vdev->dev, &chinfo);
> >>> if (ret)
> >>> dev_err(dev, "rpmsg_destroy_channel failed: %d\n",
> >> ret);
> >>> +#endif
> >>> } else {
> >>> +#ifdef CONFIG_RPMSG_VIRTIO_CHAR
> >>> + struct virtio_rpmsg_rsvc *rsvc;
> >>> +
> >>> + rsvc = virtio_rpmsg_create_rsvc(vrp, &chinfo);
> >>> + if (!rsvc)
> >>> + dev_err(dev, "virtio_rpmsg_create_rsvc failed\n");
> >> #else
> >>> newch = rpmsg_create_channel(vrp, &chinfo);
> >>> if (!newch)
> >>> dev_err(dev, "rpmsg_create_channel failed\n");
> >>> +#endif
> >>> }
> >>>
> >>> return 0;
> >>> @@ -885,6 +1190,10 @@ static int rpmsg_probe(struct virtio_device
> *vdev)
> >>> int err = 0, i;
> >>> size_t total_buf_space;
> >>> bool notify;
> >>> +#ifdef CONFIG_RPMSG_VIRTIO_CHAR
> >>> + struct virtio_rpmsg_channel *vch;
> >>> + struct rpmsg_device *rp_char_dev;
> >>> +#endif
> >>>
> >>> vrp = kzalloc(sizeof(*vrp), GFP_KERNEL);
> >>> if (!vrp)
> >>> @@ -956,9 +1265,14 @@ static int rpmsg_probe(struct virtio_device
> >>> *vdev)
> >>>
> >>> /* if supported by the remote processor, enable the name service */
> >>> if (virtio_has_feature(vdev, VIRTIO_RPMSG_F_NS)) {
> >>> + struct rpmsg_channel_info ns_chinfo;
> >>> +
> >>> + ns_chinfo.src = RPMSG_NS_ADDR;
> >>> + ns_chinfo.dst = RPMSG_NS_ADDR;
> >>> + strcpy(ns_chinfo.name, "name_service");
> >>> /* a dedicated endpoint handles the name service msgs */
> >>> vrp->ns_ept = __rpmsg_create_ept(vrp, NULL, rpmsg_ns_cb,
> >>> - vrp, RPMSG_NS_ADDR);
> >>> + vrp, &ns_chinfo);
> >>> if (!vrp->ns_ept) {
> >>> dev_err(&vdev->dev, "failed to create the ns ept\n");
> >>> err = -ENOMEM;
> >>> @@ -966,6 +1280,32 @@ static int rpmsg_probe(struct virtio_device
> *vdev)
> >>> }
> >>> }
> >>>
> >>> +#ifdef CONFIG_RPMSG_VIRTIO_CHAR
> >>> + vch = kzalloc(sizeof(*vch), GFP_KERNEL);
> >>> + if (!vch) {
> >>> + err = -ENOMEM;
> >>> + goto free_coherent;
> >>> + }
> >>> +
> >>> + /* Link the channel to our vrp */
> >>> + vch->vrp = vrp;
> >>> + /* Initialize remote services list */
> >>> + INIT_LIST_HEAD(&vrp->rsvcs);
> >>> +
> >>> + /* Assign public information to the rpmsg_device */
> >>> + rp_char_dev = &vch->rpdev;
> >>> + rp_char_dev->ops = &virtio_rpmsg_ops;
> >>> +
> >>> + rp_char_dev->dev.parent = &vrp->vdev->dev;
> >>> + rp_char_dev->dev.release = virtio_rpmsg_release_device;
> >>> + err = rpmsg_chrdev_register_device(rp_char_dev);
> >>
> >> I also tried to use this char device to expose rpmsg service for application.
> >> Issue with this device (from my point of view) is that it has to be
> >> relied on a kernel rpmsg device.
> >> I suppose that your need is to expose RMPSG to userland without
> >> creating a platform driver.
> >> In this case perhaps an alternative could be to move this part in
> >> rmpsg_char to allow to probe it in standalone...
> > [Wendy] with the existing virtio rpmsg implementation, each service
> > will be binded to a rpmsg_device. Each rpmsg_device can have multiple
> static endpoints.
> > But this is different to how rpmsg_char is supposed to use, rpmsg_char
> > looks to me Each endpoint created by rpmsg_char, either dynamic
> > (without knowing the remote Address beforehand) or static(specifying
> > the remote endpoint) should be binded to a service.
> >
> > rpmsg_char user APIs can meet what we need, however, we uses rpmsg
> over virtio.
> > But at the moment the virtio_rpmsg doesn't support rpmsg_char.
> > You talked about the allow rpmsg_char to be probed in standalone, I
> > think that's good Idea, however, we need to bind the rpmsg_char to the
> virtio rpmsg device.
> remoteproc binds rpmsg with virtio, so calling register_rpmsg_driver should
> do the job.
>
> >
> > How about to have "driver_override" of rpmsg_device exposed to
> > userspace with channel name information available (sysfs) and then we
> > can dynamically bind it to rpmsg_char driver from userspace? And then
> > we can limit the rpmsg_char create endpoint feature to only create static
> endpoints over the same channel in case of virtio_rpmsg usage?
>
> Creating a specific device could be another solution. I have no opinion on
> what could be the best strategy.
> Bjorn, what is your feeling?
[Wendy] I just noticed that Anup has sent a patch to add "driver_override".
>
> A constraint to add in design is that we should not expose all the rpmsg
> device to userland. Except if i missed something, it seems that activating
> RPMSG_VIRTIO_CHAR config exposes all the rpmsg devices.
[Wendy] just wonder with "driver_override", can we limit to apply to those
RPMsg device which is not bound to any driver yet?
>
> Another alternative is to expose rpmsg to userland trough some generic
> interfaces like proxy, tty, FS posix....
[Wendy] Yes, but it needs to have RPMsg driver for each of them.
>
> Thanks,
> Arnaud
>
> >
> > Thanks,
> > Wendy
> >
> >>
> >> Regards
> >> Arnaud
> >>
> >>> + if (err) {
> >>> + kfree(vch);
> >>> + goto free_coherent;
> >>> + }
> >>> + dev_info(&vdev->dev, "Registered as RPMsg char device.\n"); #endif
> >>> +
> >>> /*
> >>> * Prepare to kick but don't notify yet - we can't do this before
> >>> * device is ready.
> >>> @@ -1009,6 +1349,9 @@ static void rpmsg_remove(struct virtio_device
> >> *vdev)
> >>> struct virtproc_info *vrp = vdev->priv;
> >>> size_t total_buf_space = vrp->num_bufs * vrp->buf_size;
> >>> int ret;
> >>> +#ifdef CONFIG_RPMSG_VIRTIO_CHAR
> >>> + struct virtio_rpmsg_rsvc *rsvc, *tmp; #endif
> >>>
> >>> vdev->config->reset(vdev);
> >>>
> >>> @@ -1021,6 +1364,13 @@ static void rpmsg_remove(struct virtio_device
> >>> *vdev)
> >>>
> >>> idr_destroy(&vrp->endpoints);
> >>>
> >>> +#ifdef CONFIG_RPMSG_VIRTIO_CHAR
> >>> + list_for_each_entry_safe(rsvc, tmp, &vrp->rsvcs, node) {
> >>> + list_del(&rsvc->node);
> >>> + kfree(rsvc);
> >>> + }
> >>> +#endif
> >>> +
> >>> vdev->config->del_vqs(vrp->vdev);
> >>>
> >>> dma_free_coherent(vdev->dev.parent->parent, total_buf_space,
> >>>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC] rpmsg: virtio rpmsg: Add RPMsg char driver support
2018-01-04 23:18 [RFC] rpmsg: virtio rpmsg: Add RPMsg char driver support Wendy Liang
2018-01-05 14:48 ` Arnaud Pouliquen
@ 2018-03-18 22:49 ` Bjorn Andersson
1 sibling, 0 replies; 6+ messages in thread
From: Bjorn Andersson @ 2018-03-18 22:49 UTC (permalink / raw)
To: Wendy Liang; +Cc: ohad, linux-remoteproc, linux-kernel, Wendy Liang
On Thu 04 Jan 15:18 PST 2018, Wendy Liang wrote:
> virtio rpmsg was not implemented to use RPMsg char driver.
> Each virtio ns announcement will create a new RPMsg device which
> is supposed to bound to a RPMsg driver. It doesn't support
> dynamic endpoints with name service per RPMsg device.
> With RPMsg char driver, you can have multiple endpoints per
> RPMsg device.
>
> Here is the change from this patch:
> * Introduce a macro to indicate if want to use RPMsg char driver
> for virtio RPMsg. The RPMsg device can either be bounded to
> a simple RPMsg driver or the RPMsg char driver.
> * Create Virtio RPMsg char device when the virtio RPMsg driver is
> probed.
> * when there is a remote service announced, keep it in the virtio
> proc remote services list.
> * when there is an endpoint created, bind it to a remote service
> from the remote services list. If the service doesn't exist yet,
> create one and mark the service address as ANY.
I think it would be preferable to split this patch up in one that adds
client support and then one that adds service support - in particular
since the latter isn't something we support in Linux today.
>
> Signed-off-by: Wendy Liang <jliang@xilinx.com>
> ---
> We have different userspace applications to use RPMsg differently,
> what we need is a RPMsg char driver which can supports multiple
> endpoints per remote device.
> The virtio rpmsg driver at the moment doesn't support the RPMsg
> char driver.
> Please advise if this is patch is the right direction. If not,
> any suggestions? Thanks
> ---
> drivers/rpmsg/Kconfig | 8 +
> drivers/rpmsg/virtio_rpmsg_bus.c | 364 ++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 365 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/rpmsg/Kconfig b/drivers/rpmsg/Kconfig
> index 65a9f6b..746f07e 100644
> --- a/drivers/rpmsg/Kconfig
> +++ b/drivers/rpmsg/Kconfig
> @@ -52,4 +52,12 @@ config RPMSG_VIRTIO
> select RPMSG
> select VIRTIO
>
> +config RPMSG_VIRTIO_CHAR
> + bool "Enable Virtio RPMSG char device driver support"
> + default y
Please don't "default" anything.
> + depends on RPMSG_VIRTIO
> + depends on RPMSG_CHAR
> + help
> + Say y here to enable to use RPMSG char device interface.
> +
Rather than sprinkling the entire implementation with conditionals on
RPMSG_VIRTIO_CHAR I think we should just add this code unconditionally
and rely on RPMSG_CHAR to make it functional or not.
> endmenu
> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
[..]
> +#ifdef CONFIG_RPMSG_VIRTIO_CHAR
> +/**
> + * virtio_rpmsg_find_rsvc_by_name - find the announced remote service
> + * @vrp: virtio remote proc information
> + * @name: remote service name
> + *
> + * The caller is supposed to have mutex lock before calling this function
> + *
> + * return NULL if no remote service has been found, otherwise, return
> + * the remote service pointer.
This should be "Return: NULL if no..."
And more succinct "Return: reference to found service, NULL otherwise"
> + */
> +static struct virtio_rpmsg_rsvc *
> +virtio_rpmsg_find_rsvc_by_name(struct virtproc_info *vrp, char *name)
> +{
> + struct virtio_rpmsg_rsvc *rsvc;
> +
> + list_for_each_entry(rsvc, &vrp->rsvcs, node) {
> + if (!strncmp(rsvc->name, name, RPMSG_NAME_SIZE))
> + /* remote service has found */
> + return rsvc;
> + }
> +
> + return NULL;
> +}
> +
> +/**
> + * virtio_rpmsg_create_rsvc_by_name - create remote service
> + * @vrp: virtio remote proc information
> + * @name: remote service name
> + *
> + * The caller is supposed to have mutex lock before calling this function
> + *
> + * return NULL if remote service creation failed. otherwise, return
> + * the remote service pointer.
> + */
> +static struct virtio_rpmsg_rsvc *
> +virtio_rpmsg_create_rsvc_by_name(struct virtproc_info *vrp, char *name)
> +{
> + struct virtio_rpmsg_rsvc *rsvc;
> +
> + rsvc = virtio_rpmsg_find_rsvc_by_name(vrp, name);
> + if (rsvc)
> + return rsvc;
> + rsvc = kzalloc(sizeof(*rsvc), GFP_KERNEL);
> + if (!rsvc)
> + return NULL;
> + strncpy(rsvc->name, name, RPMSG_NAME_SIZE);
> + list_add_tail(&rsvc->node, &vrp->rsvcs);
> + return rsvc;
> +}
> +
> +/**
> + * virtio_rpmsg_remove_rsvc_by_name - remove remote service
> + * @vrp: virtio remote proc information
> + * @name: remote service name
> + *
> + */
> +static void
> +virtio_rpmsg_remove_rsvc_by_name(struct virtproc_info *vrp, char *name)
> +{
> + struct virtio_rpmsg_rsvc *rsvc;
> + struct rpmsg_endpoint *ept;
> + struct virtio_rpmsg_ept *vept;
> +
> + mutex_lock(&vrp->endpoints_lock);
> + list_for_each_entry(rsvc, &vrp->rsvcs, node) {
> + if (!strncmp(rsvc->name, name, RPMSG_NAME_SIZE)) {
> + /* remote service has found, no need to
> + * create
> + */
> + ept = rsvc->ept;
> + if (ept) {
> + vept = to_virtio_rpmsg_ept(ept);
> + vept->rsvc = NULL;
> + }
> + list_del(&rsvc->node);
> + kfree(rsvc);
> + break;
> + }
> + }
> + mutex_unlock(&vrp->endpoints_lock);
> +}
> +
> +/**
> + * virtio_rpmsg_create_rsvc - create remote service with channel information
> + * @vrp: virtio remote proc information
> + * @chinfo: channel information
> + *
> + * return remote service pointer if it is successfully created; otherwise,
> + * return NULL.
> + */
> +static struct virtio_rpmsg_rsvc *
> +virtio_rpmsg_create_rsvc(struct virtproc_info *vrp,
> + struct rpmsg_channel_info *chinfo)
> +{
> + struct virtio_rpmsg_rsvc *rsvc;
> +
> + mutex_lock(&vrp->endpoints_lock);
> + rsvc = virtio_rpmsg_create_rsvc_by_name(vrp, chinfo->name);
> + if (rsvc) {
> + struct virtio_device *vdev = vrp->vdev;
> +
> + rsvc->addr = chinfo->dst;
> + dev_info(&vdev->dev, "Remote has announced service %s, %d.\n",
> + chinfo->name, rsvc->addr);
> + }
> + mutex_unlock(&vrp->endpoints_lock);
> + return rsvc;
> +}
> +
> +/**
> + * virtio_rpmsg_announce_ept_create - announce endpoint has been created
> + * @ept: RPMsg endpoint
> + *
> + * return 0 if succeeded, otherwise, return error code.
> + */
> +static int virtio_rpmsg_announce_ept_create(struct rpmsg_endpoint *ept)
As described above, move this to a separate patch - as this not only
adds rpmsg_char integration but support for local services.
> +{
> + struct virtio_rpmsg_ept *vept = to_virtio_rpmsg_ept(ept);
> + struct virtio_rpmsg_rsvc *rsvc = vept->rsvc;
> + struct rpmsg_device *rpdev = ept->rpdev;
> + struct virtio_rpmsg_channel *vch;
> + struct virtproc_info *vrp;
> + struct device *dev;
> + int err = 0;
> +
> + if (!rpdev)
Is it really possible to get here without ept->rpdev being assigned?
> + /* If the endpoint is not connected to a RPMsg device,
> + * no need to send the announcement.
> + */
> + return 0;
> + vch = to_virtio_rpmsg_channel(rpdev);
> + vrp = vch->vrp;
> + dev = &ept->rpdev->dev;
> + /* need to tell remote processor's name service about this channel ? */
> + if (virtio_has_feature(vrp->vdev, VIRTIO_RPMSG_F_NS)) {
Flip this around:
if (!has_feature(X))
return;
...
> + struct rpmsg_ns_msg nsm;
> +
> + strncpy(nsm.name, rsvc->name, RPMSG_NAME_SIZE);
> + nsm.addr = ept->addr;
> + nsm.flags = RPMSG_NS_CREATE;
> +
> + err = rpmsg_sendto(ept, &nsm, sizeof(nsm),
> + RPMSG_NS_ADDR);
> + if (err)
> + dev_err(dev, "failed to announce service %d\n", err);
> + }
> +
> + return err;
> +}
> +
> +/**
> + * virtio_rpmsg_announce_ept_destroy - announce endpoint has been destroyed
> + * @ept: RPMsg endpoint
> + *
> + * return 0 if succeeded, otherwise, return error code.
> + */
> +static int virtio_rpmsg_announce_ept_destroy(struct rpmsg_endpoint *ept)
> +{
> + struct virtio_rpmsg_ept *vept = to_virtio_rpmsg_ept(ept);
> + struct virtio_rpmsg_rsvc *rsvc = vept->rsvc;
> + struct rpmsg_device *rpdev = ept->rpdev;
> + struct virtio_rpmsg_channel *vch;
> + struct virtproc_info *vrp;
> + struct device *dev;
> + int err = 0;
> +
> + if (!rpdev)
This is only called inside a if (ept->rpdev) region, so no need to spend
5 lines on this...
> + /* If the endpoint is not connected to a RPMsg device,
> + * no need to send the announcement.
> + */
> + return 0;
> + vch = to_virtio_rpmsg_channel(rpdev);
> + vrp = vch->vrp;
> + dev = &ept->rpdev->dev;
> + /* tell remote processor's name service we're removing this channel */
> + if (virtio_has_feature(vrp->vdev, VIRTIO_RPMSG_F_NS)) {
> + struct rpmsg_ns_msg nsm;
> +
> + strncpy(nsm.name, rsvc->name, RPMSG_NAME_SIZE);
> + nsm.addr = ept->addr;
> + nsm.flags = RPMSG_NS_DESTROY;
> +
> + err = rpmsg_sendto(ept, &nsm, sizeof(nsm),
> + RPMSG_NS_ADDR);
> + if (err)
> + dev_err(dev, "failed to announce service %d\n", err);
> + }
> +
> + return err;
> +}
> +#endif
> +
> /**
> * __ept_release() - deallocate an rpmsg endpoint
> * @kref: the ept's reference count
> @@ -229,26 +456,53 @@ static void __ept_release(struct kref *kref)
> {
> struct rpmsg_endpoint *ept = container_of(kref, struct rpmsg_endpoint,
> refcount);
> +#ifdef CONFIG_RPMSG_VIRTIO_CHAR
> + struct virtio_rpmsg_ept *vept = to_virtio_rpmsg_ept(ept);
> + struct virtio_rpmsg_channel *vch;
> + struct virtproc_info *vrp;
> +
> + if (ept->rpdev) {
> + vch = to_virtio_rpmsg_channel(ept->rpdev);
> + vrp = vch->vrp;
> + (void)virtio_rpmsg_announce_ept_destroy(ept);
> + mutex_lock(&vrp->endpoints_lock);
> + vept->rsvc->ept = NULL;
> + mutex_unlock(&vrp->endpoints_lock);
> + }
> + kfree(vept);
> +#else
> /*
> * At this point no one holds a reference to ept anymore,
> * so we can directly free it
> */
> kfree(ept);
> +#endif
> }
>
> /* for more info, see below documentation of rpmsg_create_ept() */
> static struct rpmsg_endpoint *__rpmsg_create_ept(struct virtproc_info *vrp,
> struct rpmsg_device *rpdev,
> rpmsg_rx_cb_t cb,
> - void *priv, u32 addr)
> + void *priv,
> + struct rpmsg_channel_info *ch)
> {
> int id_min, id_max, id;
> struct rpmsg_endpoint *ept;
> + u32 addr = ch->src;
> struct device *dev = rpdev ? &rpdev->dev : &vrp->vdev->dev;
> +#ifdef CONFIG_RPMSG_VIRTIO_CHAR
> + struct virtio_rpmsg_ept *vept;
> + struct virtio_rpmsg_rsvc *rsvc;
>
> + vept = kzalloc(sizeof(*vept), GFP_KERNEL);
> + if (!vept)
> + return NULL;
> + ept = &vept->ept;
> +#else
> ept = kzalloc(sizeof(*ept), GFP_KERNEL);
> if (!ept)
> return NULL;
> +#endif
>
> kref_init(&ept->refcount);
> mutex_init(&ept->cb_lock);
> @@ -259,7 +513,7 @@ static struct rpmsg_endpoint *__rpmsg_create_ept(struct virtproc_info *vrp,
> ept->ops = &virtio_endpoint_ops;
>
> /* do we need to allocate a local address ? */
> - if (addr == RPMSG_ADDR_ANY) {
> + if (ch->src == RPMSG_ADDR_ANY) {
> id_min = RPMSG_RESERVED_ADDRESSES;
> id_max = 0;
> } else {
> @@ -277,6 +531,19 @@ static struct rpmsg_endpoint *__rpmsg_create_ept(struct virtproc_info *vrp,
> }
> ept->addr = id;
>
> +#ifdef CONFIG_RPMSG_VIRTIO_CHAR
> + if (ept->addr != RPMSG_NS_ADDR) {
> + rsvc = virtio_rpmsg_create_rsvc_by_name(vrp, ch->name);
Afaict for client endpoints this will never create a new service, just
return the old one so that the vept can be linked to the rsvc. But I
fail to see why the vept needs to be tied to the rsvc.
The only case where it seems to be used is in order to access the addr
of the rsvc because the ept value isn't trusted, but this seems wrong.
> + if (!rsvc)
> + goto free_ept;
> + vept->rsvc = rsvc;
> + rsvc->ept = ept;
> + dev_info(&vrp->vdev->dev, "RPMsg ept created, %s:%d,%d.\n",
> + ch->name, ept->addr, rsvc->addr);
> + (void)virtio_rpmsg_announce_ept_create(ept);
Won't this announce back channels that was announced to us by the
remote?
Also, if your only caller to a function has to explicitly discard the
returned value (void), then just make the function void.
> + }
> +#endif
> +
> mutex_unlock(&vrp->endpoints_lock);
>
> return ept;
> @@ -294,7 +561,7 @@ static struct rpmsg_endpoint *virtio_rpmsg_create_ept(struct rpmsg_device *rpdev
> {
> struct virtio_rpmsg_channel *vch = to_virtio_rpmsg_channel(rpdev);
>
> - return __rpmsg_create_ept(vch->vrp, rpdev, cb, priv, chinfo.src);
> + return __rpmsg_create_ept(vch->vrp, rpdev, cb, priv, &chinfo);
> }
>
> /**
> @@ -392,6 +659,7 @@ static void virtio_rpmsg_release_device(struct device *dev)
> kfree(vch);
> }
>
> +#ifndef CONFIG_RPMSG_VIRTIO_CHAR
> /*
> * create an rpmsg channel using its name and address info.
> * this function will be used to create both static and dynamic
> @@ -444,6 +712,7 @@ static struct rpmsg_device *rpmsg_create_channel(struct virtproc_info *vrp,
>
> return rpdev;
> }
> +#endif
>
> /* super simple buffer "allocator" that is just enough for now */
> static void *get_a_tx_buf(struct virtproc_info *vrp)
> @@ -660,8 +929,21 @@ static int rpmsg_send_offchannel_raw(struct rpmsg_device *rpdev,
> static int virtio_rpmsg_send(struct rpmsg_endpoint *ept, void *data, int len)
> {
> struct rpmsg_device *rpdev = ept->rpdev;
> - u32 src = ept->addr, dst = rpdev->dst;
> + u32 src = ept->addr;
> +#ifdef CONFIG_RPMSG_VIRTIO_CHAR
> + struct virtio_rpmsg_ept *vept = to_virtio_rpmsg_ept(ept);
> + u32 dst = vept->rsvc->addr;
Why will vept->rsvc->addr != rpdev->dst ?
> +
> + if (dst == RPMSG_ADDR_ANY)
> + return -EPIPE;
> +#else
> + u32 dst = rpdev->dst;
> +#endif
>
> + if (!rpdev) {
> + pr_err("%s, ept %p rpdev is NULL\n", __func__, ept);
> + return -EINVAL;
> + }
> return rpmsg_send_offchannel_raw(rpdev, src, dst, data, len, true);
> }
>
> @@ -685,7 +967,16 @@ static int virtio_rpmsg_send_offchannel(struct rpmsg_endpoint *ept, u32 src,
> static int virtio_rpmsg_trysend(struct rpmsg_endpoint *ept, void *data, int len)
> {
> struct rpmsg_device *rpdev = ept->rpdev;
> - u32 src = ept->addr, dst = rpdev->dst;
> + u32 src = ept->addr;
> +#ifdef CONFIG_RPMSG_VIRTIO_CHAR
> + struct virtio_rpmsg_ept *vept = to_virtio_rpmsg_ept(ept);
> + u32 dst = vept->rsvc->addr;
> +
> + if (dst == RPMSG_ADDR_ANY)
> + return -EPIPE;
> +#else
> + u32 dst = rpdev->dst;
> +#endif
>
> return rpmsg_send_offchannel_raw(rpdev, src, dst, data, len, false);
> }
> @@ -824,11 +1115,13 @@ 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 virtproc_info *vrp = priv;
> struct device *dev = &vrp->vdev->dev;
> +#ifndef CONFIG_RPMSG_VIRTIO_CHAR
> + struct rpmsg_device *newch;
> int ret;
> +#endif
>
> #if defined(CONFIG_DYNAMIC_DEBUG)
> dynamic_hex_dump("NS announcement: ", DUMP_PREFIX_NONE, 16, 1,
> @@ -863,13 +1156,25 @@ static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void *data, int len,
> chinfo.dst = msg->addr;
>
> if (msg->flags & RPMSG_NS_DESTROY) {
> +#ifdef CONFIG_RPMSG_VIRTIO_CHAR
> + virtio_rpmsg_remove_rsvc_by_name(vrp, chinfo.name);
> +#else
> ret = rpmsg_unregister_device(&vrp->vdev->dev, &chinfo);
> if (ret)
> dev_err(dev, "rpmsg_destroy_channel failed: %d\n", ret);
> +#endif
> } else {
> +#ifdef CONFIG_RPMSG_VIRTIO_CHAR
> + struct virtio_rpmsg_rsvc *rsvc;
> +
> + rsvc = virtio_rpmsg_create_rsvc(vrp, &chinfo);
> + if (!rsvc)
> + dev_err(dev, "virtio_rpmsg_create_rsvc failed\n");
> +#else
> newch = rpmsg_create_channel(vrp, &chinfo);
> if (!newch)
> dev_err(dev, "rpmsg_create_channel failed\n");
We most definitely want to rpmsg_create_channel() for newly announced
channels, otherwise you're disabling support for in-kernel rpmsg
devices.
The expectation is that you rpmsg_create_channel() which will attempt to
probe a kernel driver and if none is found it's left dangling and
rpmsg_char can open it. (or if the kernel driver is unbound)
[..]
> +#ifdef CONFIG_RPMSG_VIRTIO_CHAR
> + vch = kzalloc(sizeof(*vch), GFP_KERNEL);
> + if (!vch) {
> + err = -ENOMEM;
> + goto free_coherent;
> + }
> +
> + /* Link the channel to our vrp */
> + vch->vrp = vrp;
> + /* Initialize remote services list */
> + INIT_LIST_HEAD(&vrp->rsvcs);
> +
> + /* Assign public information to the rpmsg_device */
> + rp_char_dev = &vch->rpdev;
> + rp_char_dev->ops = &virtio_rpmsg_ops;
> +
> + rp_char_dev->dev.parent = &vrp->vdev->dev;
> + rp_char_dev->dev.release = virtio_rpmsg_release_device;
> + err = rpmsg_chrdev_register_device(rp_char_dev);
> + if (err) {
> + kfree(vch);
> + goto free_coherent;
> + }
Break this out into its own helper function.
> + dev_info(&vdev->dev, "Registered as RPMsg char device.\n");
Please don't spam the logs, use dev_dbg().
> +#endif
> +
Regards,
Bjorn
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-03-18 22:49 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-01-04 23:18 [RFC] rpmsg: virtio rpmsg: Add RPMsg char driver support Wendy Liang
2018-01-05 14:48 ` Arnaud Pouliquen
2018-01-05 22:10 ` Jiaying Liang
2018-01-09 12:55 ` Arnaud Pouliquen
2018-01-11 6:28 ` Jiaying Liang
2018-03-18 22:49 ` Bjorn Andersson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).