| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <76870793-8f08-1efc-512e-364ecbf0f6df@st.com>
Date: Tue, 9 Jan 2018 13:55:55 +0100
From: Arnaud Pouliquen <arnaud.pouliquen@...com>
To: Jiaying Liang <jliang@...inx.com>,
"ohad@...ery.com" <ohad@...ery.com>,
"bjorn.andersson@...aro.org" <bjorn.andersson@...aro.org>
CC: "linux-remoteproc@...r.kernel.org" <linux-remoteproc@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.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@...com]
>> Sent: Friday, January 05, 2018 6:48 AM
>> To: Jiaying Liang <jliang@...inx.com>; ohad@...ery.com;
>> bjorn.andersson@...aro.org
>> Cc: linux-remoteproc@...r.kernel.org; linux-kernel@...r.kernel.org; Jiaying
>> Liang <jliang@...inx.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@...inx.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,
>>>
Powered by blists - more mailing lists