Re: [RFC] rpmsg: virtio rpmsg: Add RPMsg char driver support

[Arnaud Pouliquen]

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@xxxxxxxxxx>
> ---
> 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,
>