Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1463287 > unrolled thread

[PATCH 06/14] rpmsg: Indirect all virtio related function calls

Started byBjorn Andersson <bjorn.andersson@linaro.org>
First post2016-08-16 02:20 +0200
Last post2016-08-19 03:40 +0200
Articles 3 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 06/14] rpmsg: Indirect all virtio related function calls Bjorn Andersson <bjorn.andersson@linaro.org> - 2016-08-16 02:20 +0200
    RE: [PATCH 06/14] rpmsg: Indirect all virtio related function calls Loic PALLARDY <loic.pallardy@st.com> - 2016-08-18 14:20 +0200
      Re: [PATCH 06/14] rpmsg: Indirect all virtio related function calls Bjorn Andersson <bjorn.andersson@linaro.org> - 2016-08-19 03:40 +0200

#1463287 — [PATCH 06/14] rpmsg: Indirect all virtio related function calls

FromBjorn Andersson <bjorn.andersson@linaro.org>
Date2016-08-16 02:20 +0200
Subject[PATCH 06/14] rpmsg: Indirect all virtio related function calls
Message-ID<s6AgN-4eu-1@gated-at.bofh.it>
Create indirections for functions in prepration for splitting the rpmsg
core functionality from the virtio backend.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 drivers/rpmsg/virtio_rpmsg_bus.c | 131 +++++++++++++++++++++++++++++++++++----
 include/linux/rpmsg.h            |  23 ++++++-
 2 files changed, 139 insertions(+), 15 deletions(-)

diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
index a9f8cc8d968b..1dc7a1149c51 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -119,6 +119,17 @@ struct rpmsg_channel_info {
 /* Address 53 is reserved for advertising remote services */
 #define RPMSG_NS_ADDR			(53)
 
+static int virtio_rpmsg_send(struct rpmsg_endpoint *ept, void *data, int len);
+static int virtio_rpmsg_sendto(struct rpmsg_endpoint *ept, void *data, int len,
+			       u32 dst);
+static int virtio_rpmsg_send_offchannel(struct rpmsg_endpoint *ept, u32 src,
+					u32 dst, void *data, int len);
+static int virtio_rpmsg_trysend(struct rpmsg_endpoint *ept, void *data, int len);
+static int virtio_rpmsg_trysendto(struct rpmsg_endpoint *ept, void *data,
+				  int len, u32 dst);
+static int virtio_rpmsg_trysend_offchannel(struct rpmsg_endpoint *ept, u32 src,
+					   u32 dst, void *data, int len);
+
 /* sysfs show configuration fields */
 #define rpmsg_show_attr(field, path, format_string)			\
 static ssize_t								\
@@ -297,10 +308,17 @@ free_ept:
 struct rpmsg_endpoint *rpmsg_create_ept(struct rpmsg_device *rpdev,
 					rpmsg_rx_cb_t cb, void *priv, u32 addr)
 {
-	return __rpmsg_create_ept(rpdev->vrp, rpdev, cb, priv, addr);
+	return rpdev->create_ept(rpdev, cb, priv, addr);
 }
 EXPORT_SYMBOL(rpmsg_create_ept);
 
+static struct rpmsg_endpoint *virtio_rpmsg_create_ept(struct rpmsg_device *rpdev,
+						      rpmsg_rx_cb_t cb,
+						      void *priv, u32 addr)
+{
+	return __rpmsg_create_ept(rpdev->vrp, rpdev, cb, priv, addr);
+}
+
 /**
  * __rpmsg_destroy_ept() - destroy an existing rpmsg endpoint
  * @vrp: virtproc which owns this ept
@@ -336,10 +354,15 @@ __rpmsg_destroy_ept(struct virtproc_info *vrp, struct rpmsg_endpoint *ept)
  */
 void rpmsg_destroy_ept(struct rpmsg_endpoint *ept)
 {
-	__rpmsg_destroy_ept(ept->rpdev->vrp, ept);
+	ept->rpdev->destroy_ept(ept);
 }
 EXPORT_SYMBOL(rpmsg_destroy_ept);
 
+static void virtio_rpmsg_destroy_ept(struct rpmsg_endpoint *ept)
+{
+	__rpmsg_destroy_ept(ept->rpdev->vrp, ept);
+}
+
 /*
  * when an rpmsg driver is probed with a channel, we seamlessly create
  * it an endpoint, binding its rx callback to a unique local rpmsg
@@ -352,7 +375,6 @@ static int rpmsg_dev_probe(struct device *dev)
 {
 	struct rpmsg_device *rpdev = to_rpmsg_device(dev);
 	struct rpmsg_driver *rpdrv = to_rpmsg_driver(rpdev->dev.driver);
-	struct virtproc_info *vrp = rpdev->vrp;
 	struct rpmsg_endpoint *ept;
 	int err;
 
@@ -373,6 +395,18 @@ static int rpmsg_dev_probe(struct device *dev)
 		goto out;
 	}
 
+	if (rpdev->announce_create)
+		err = rpdev->announce_create(rpdev);
+out:
+	return err;
+}
+
+static int virtio_rpmsg_announce_create(struct rpmsg_device *rpdev)
+{
+	struct virtproc_info *vrp = rpdev->vrp;
+	struct device *dev = &rpdev->dev;
+	int err = 0;
+
 	/* need to tell remote processor's name service about this channel ? */
 	if (rpdev->announce &&
 	    virtio_has_feature(vrp->vdev, VIRTIO_RPMSG_F_NS)) {
@@ -387,15 +421,13 @@ static int rpmsg_dev_probe(struct device *dev)
 			dev_err(dev, "failed to announce service %d\n", err);
 	}
 
-out:
 	return err;
 }
 
-static int rpmsg_dev_remove(struct device *dev)
+static int virtio_rpmsg_announce_destroy(struct rpmsg_device *rpdev)
 {
-	struct rpmsg_device *rpdev = to_rpmsg_device(dev);
-	struct rpmsg_driver *rpdrv = to_rpmsg_driver(rpdev->dev.driver);
 	struct virtproc_info *vrp = rpdev->vrp;
+	struct device *dev = &rpdev->dev;
 	int err = 0;
 
 	/* tell remote processor's name service we're removing this channel */
@@ -412,6 +444,18 @@ static int rpmsg_dev_remove(struct device *dev)
 			dev_err(dev, "failed to announce service %d\n", err);
 	}
 
+	return err;
+}
+
+static int rpmsg_dev_remove(struct device *dev)
+{
+	struct rpmsg_device *rpdev = to_rpmsg_device(dev);
+	struct rpmsg_driver *rpdrv = to_rpmsg_driver(rpdev->dev.driver);
+	int err = 0;
+
+	if (rpdev->announce_destroy)
+		err = rpdev->announce_destroy(rpdev);
+
 	rpdrv->remove(rpdev);
 
 	rpmsg_destroy_ept(rpdev->ept);
@@ -485,6 +529,19 @@ static int rpmsg_device_match(struct device *dev, void *data)
 	return 1;
 }
 
+static const struct rpmsg_device virtio_rpmsg_ops = {
+	.create_ept = virtio_rpmsg_create_ept,
+	.destroy_ept = virtio_rpmsg_destroy_ept,
+	.send = virtio_rpmsg_send,
+	.sendto = virtio_rpmsg_sendto,
+	.send_offchannel = virtio_rpmsg_send_offchannel,
+	.trysend = virtio_rpmsg_trysend,
+	.trysendto = virtio_rpmsg_trysendto,
+	.trysend_offchannel = virtio_rpmsg_trysend_offchannel,
+	.announce_create = virtio_rpmsg_announce_create,
+	.announce_destroy = virtio_rpmsg_announce_destroy,
+};
+
 /*
  * create an rpmsg channel using its name and address info.
  * this function will be used to create both static and dynamic
@@ -511,6 +568,9 @@ static struct rpmsg_device *rpmsg_create_channel(struct virtproc_info *vrp,
 	if (!rpdev)
 		return NULL;
 
+	/* Assign callbacks for rpmsg_channel */
+	*rpdev = virtio_rpmsg_ops;
+
 	rpdev->vrp = vrp;
 	rpdev->src = chinfo->src;
 	rpdev->dst = chinfo->dst;
@@ -793,11 +853,17 @@ out:
 int rpmsg_send(struct rpmsg_endpoint *ept, void *data, int len)
 {
 	struct rpmsg_device *rpdev = ept->rpdev;
+
+	return rpdev->send(ept, data, len);
+}
+
+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;
 
 	return rpmsg_send_offchannel_raw(rpdev, src, dst, data, len, true);
 }
-EXPORT_SYMBOL(rpmsg_send);
 
 /**
  * rpmsg_sendto() - send a message across to the remote processor, specify dst
@@ -820,11 +886,19 @@ EXPORT_SYMBOL(rpmsg_send);
 int rpmsg_sendto(struct rpmsg_endpoint *ept, void *data, int len, u32 dst)
 {
 	struct rpmsg_device *rpdev = ept->rpdev;
+
+	return rpdev->sendto(ept, data, len, dst);
+}
+EXPORT_SYMBOL(rpmsg_sendto);
+
+static int virtio_rpmsg_sendto(struct rpmsg_endpoint *ept, void *data, int len,
+			       u32 dst)
+{
+	struct rpmsg_device *rpdev = ept->rpdev;
 	u32 src = ept->addr;
 
 	return rpmsg_send_offchannel_raw(rpdev, src, dst, data, len, true);
 }
-EXPORT_SYMBOL(rpmsg_sendto);
 
 /**
  * rpmsg_send_offchannel() - send a message using explicit src/dst addresses
@@ -851,10 +925,18 @@ int rpmsg_send_offchannel(struct rpmsg_endpoint *ept, u32 src, u32 dst,
 {
 	struct rpmsg_device *rpdev = ept->rpdev;
 
-	return rpmsg_send_offchannel_raw(rpdev, src, dst, data, len, true);
+	return rpdev->send_offchannel(ept, src, dst, data, len);
 }
 EXPORT_SYMBOL(rpmsg_send_offchannel);
 
+static int virtio_rpmsg_send_offchannel(struct rpmsg_endpoint *ept, u32 src,
+					u32 dst, void *data, int len)
+{
+	struct rpmsg_device *rpdev = ept->rpdev;
+
+	return rpmsg_send_offchannel_raw(rpdev, src, dst, data, len, true);
+}
+
 /**
  * rpmsg_send() - send a message across to the remote processor
  * @ept: the rpmsg endpoint
@@ -875,11 +957,18 @@ EXPORT_SYMBOL(rpmsg_send_offchannel);
 int rpmsg_trysend(struct rpmsg_endpoint *ept, void *data, int len)
 {
 	struct rpmsg_device *rpdev = ept->rpdev;
+
+	return rpdev->trysend(ept, data, len);
+}
+EXPORT_SYMBOL(rpmsg_trysend);
+
+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;
 
 	return rpmsg_send_offchannel_raw(rpdev, src, dst, data, len, false);
 }
-EXPORT_SYMBOL(rpmsg_trysend);
 
 /**
  * rpmsg_sendto() - send a message across to the remote processor, specify dst
@@ -901,11 +990,19 @@ EXPORT_SYMBOL(rpmsg_trysend);
 int rpmsg_trysendto(struct rpmsg_endpoint *ept, void *data, int len, u32 dst)
 {
 	struct rpmsg_device *rpdev = ept->rpdev;
+
+	return rpdev->trysendto(ept, data, len, dst);
+}
+EXPORT_SYMBOL(rpmsg_trysendto);
+
+static int virtio_rpmsg_trysendto(struct rpmsg_endpoint *ept, void *data,
+				  int len, u32 dst)
+{
+	struct rpmsg_device *rpdev = ept->rpdev;
 	u32 src = ept->addr;
 
 	return rpmsg_send_offchannel_raw(rpdev, src, dst, data, len, false);
 }
-EXPORT_SYMBOL(rpmsg_trysendto);
 
 /**
  * rpmsg_send_offchannel() - send a message using explicit src/dst addresses
@@ -931,10 +1028,18 @@ int rpmsg_trysend_offchannel(struct rpmsg_endpoint *ept, u32 src, u32 dst,
 {
 	struct rpmsg_device *rpdev = ept->rpdev;
 
-	return rpmsg_send_offchannel_raw(rpdev, src, dst, data, len, false);
+	return rpdev->trysend_offchannel(ept, src, dst, data, len);
 }
 EXPORT_SYMBOL(rpmsg_trysend_offchannel);
 
+static int virtio_rpmsg_trysend_offchannel(struct rpmsg_endpoint *ept, u32 src,
+					   u32 dst, void *data, int len)
+{
+	struct rpmsg_device *rpdev = ept->rpdev;
+
+	return rpmsg_send_offchannel_raw(rpdev, src, dst, data, len, false);
+}
+
 static int rpmsg_recv_single(struct virtproc_info *vrp, struct device *dev,
 			     struct rpmsg_hdr *msg, unsigned int len)
 {
diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h
index 3e3d5b1c154a..0b290ae18e70 100644
--- a/include/linux/rpmsg.h
+++ b/include/linux/rpmsg.h
@@ -96,6 +96,10 @@ enum rpmsg_ns_flags {
 #define RPMSG_ADDR_ANY		0xFFFFFFFF
 
 struct virtproc_info;
+struct rpmsg_device;
+struct rpmsg_endpoint;
+
+typedef void (*rpmsg_rx_cb_t)(struct rpmsg_device *, void *, int, void *, u32);
 
 /**
  * rpmsg_device - device that belong to the rpmsg bus
@@ -115,9 +119,24 @@ struct rpmsg_device {
 	u32 dst;
 	struct rpmsg_endpoint *ept;
 	bool announce;
-};
 
-typedef void (*rpmsg_rx_cb_t)(struct rpmsg_device *, void *, int, void *, u32);
+	struct rpmsg_endpoint *(*create_ept)(struct rpmsg_device *rpdev,
+					    rpmsg_rx_cb_t cb, void *priv, u32 addr);
+	void (*destroy_ept)(struct rpmsg_endpoint *ept);
+
+	int (*send)(struct rpmsg_endpoint *ept, void *data, int len);
+	int (*sendto)(struct rpmsg_endpoint *ept, void *data, int len, u32 dst);
+	int (*send_offchannel)(struct rpmsg_endpoint *ept, u32 src, u32 dst,
+				  void *data, int len);
+
+	int (*trysend)(struct rpmsg_endpoint *ept, void *data, int len);
+	int (*trysendto)(struct rpmsg_endpoint *ept, void *data, int len, u32 dst);
+	int (*trysend_offchannel)(struct rpmsg_endpoint *ept, u32 src, u32 dst,
+			     void *data, int len);
+
+	int (*announce_create)(struct rpmsg_device *ept);
+	int (*announce_destroy)(struct rpmsg_device *ept);
+};
 
 /**
  * struct rpmsg_endpoint - binds a local rpmsg address to its user
-- 
2.5.0

[toc] | [next] | [standalone]

#1465217

FromLoic PALLARDY <loic.pallardy@st.com>
Date2016-08-18 14:20 +0200
Message-ID<s7usG-86E-25@gated-at.bofh.it>
In reply to#1463287

> -----Original Message-----
> From: linux-remoteproc-owner@vger.kernel.org [mailto:linux-remoteproc-
> owner@vger.kernel.org] On Behalf Of Bjorn Andersson
> Sent: Tuesday, August 16, 2016 2:17 AM
> To: Ohad Ben-Cohen <ohad@wizery.com>; Bjorn Andersson
> <bjorn.andersson@linaro.org>
> Cc: linux-remoteproc@vger.kernel.org; linux-arm-msm@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> Subject: [PATCH 06/14] rpmsg: Indirect all virtio related function calls
> 
> Create indirections for functions in prepration for splitting the rpmsg
> core functionality from the virtio backend.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
>  drivers/rpmsg/virtio_rpmsg_bus.c | 131
> +++++++++++++++++++++++++++++++++++----
>  include/linux/rpmsg.h            |  23 ++++++-
>  2 files changed, 139 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c
> b/drivers/rpmsg/virtio_rpmsg_bus.c
> index a9f8cc8d968b..1dc7a1149c51 100644
> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> @@ -119,6 +119,17 @@ struct rpmsg_channel_info {
>  /* Address 53 is reserved for advertising remote services */
>  #define RPMSG_NS_ADDR			(53)
> 
> +static int virtio_rpmsg_send(struct rpmsg_endpoint *ept, void *data, int
> len);
> +static int virtio_rpmsg_sendto(struct rpmsg_endpoint *ept, void *data, int
> len,
> +			       u32 dst);
> +static int virtio_rpmsg_send_offchannel(struct rpmsg_endpoint *ept, u32
> src,
> +					u32 dst, void *data, int len);
> +static int virtio_rpmsg_trysend(struct rpmsg_endpoint *ept, void *data, int
> len);
> +static int virtio_rpmsg_trysendto(struct rpmsg_endpoint *ept, void *data,
> +				  int len, u32 dst);
> +static int virtio_rpmsg_trysend_offchannel(struct rpmsg_endpoint *ept,
> u32 src,
> +					   u32 dst, void *data, int len);
> +
>  /* sysfs show configuration fields */
>  #define rpmsg_show_attr(field, path, format_string)			\
>  static ssize_t								\
> @@ -297,10 +308,17 @@ free_ept:
>  struct rpmsg_endpoint *rpmsg_create_ept(struct rpmsg_device *rpdev,
>  					rpmsg_rx_cb_t cb, void *priv, u32
> addr)
>  {
> -	return __rpmsg_create_ept(rpdev->vrp, rpdev, cb, priv, addr);
> +	return rpdev->create_ept(rpdev, cb, priv, addr);
Hi Bjorn,

It will be good to test if pointer is valid before calling function.

>  }
>  EXPORT_SYMBOL(rpmsg_create_ept);
> 
> +static struct rpmsg_endpoint *virtio_rpmsg_create_ept(struct
> rpmsg_device *rpdev,
> +						      rpmsg_rx_cb_t cb,
> +						      void *priv, u32 addr)
> +{
> +	return __rpmsg_create_ept(rpdev->vrp, rpdev, cb, priv, addr);
> +}
> +
>  /**
>   * __rpmsg_destroy_ept() - destroy an existing rpmsg endpoint
>   * @vrp: virtproc which owns this ept
> @@ -336,10 +354,15 @@ __rpmsg_destroy_ept(struct virtproc_info *vrp,
> struct rpmsg_endpoint *ept)
>   */
>  void rpmsg_destroy_ept(struct rpmsg_endpoint *ept)
>  {
> -	__rpmsg_destroy_ept(ept->rpdev->vrp, ept);
> +	ept->rpdev->destroy_ept(ept);
Ditto

>  }
>  EXPORT_SYMBOL(rpmsg_destroy_ept);
> 
> +static void virtio_rpmsg_destroy_ept(struct rpmsg_endpoint *ept)
> +{
> +	__rpmsg_destroy_ept(ept->rpdev->vrp, ept);
> +}
> +
>  /*
>   * when an rpmsg driver is probed with a channel, we seamlessly create
>   * it an endpoint, binding its rx callback to a unique local rpmsg
> @@ -352,7 +375,6 @@ static int rpmsg_dev_probe(struct device *dev)
>  {
>  	struct rpmsg_device *rpdev = to_rpmsg_device(dev);
>  	struct rpmsg_driver *rpdrv = to_rpmsg_driver(rpdev->dev.driver);
> -	struct virtproc_info *vrp = rpdev->vrp;
>  	struct rpmsg_endpoint *ept;
>  	int err;
> 
> @@ -373,6 +395,18 @@ static int rpmsg_dev_probe(struct device *dev)
>  		goto out;
>  	}
> 
> +	if (rpdev->announce_create)
> +		err = rpdev->announce_create(rpdev);
> +out:
> +	return err;
> +}
> +
> +static int virtio_rpmsg_announce_create(struct rpmsg_device *rpdev)
> +{
> +	struct virtproc_info *vrp = rpdev->vrp;
> +	struct device *dev = &rpdev->dev;
> +	int err = 0;
> +
>  	/* need to tell remote processor's name service about this channel ?
> */
>  	if (rpdev->announce &&
>  	    virtio_has_feature(vrp->vdev, VIRTIO_RPMSG_F_NS)) {
> @@ -387,15 +421,13 @@ static int rpmsg_dev_probe(struct device *dev)
>  			dev_err(dev, "failed to announce service %d\n", err);
>  	}
> 
> -out:
>  	return err;
>  }
> 
> -static int rpmsg_dev_remove(struct device *dev)
> +static int virtio_rpmsg_announce_destroy(struct rpmsg_device *rpdev)
>  {
> -	struct rpmsg_device *rpdev = to_rpmsg_device(dev);
> -	struct rpmsg_driver *rpdrv = to_rpmsg_driver(rpdev->dev.driver);
>  	struct virtproc_info *vrp = rpdev->vrp;
> +	struct device *dev = &rpdev->dev;
>  	int err = 0;
> 
>  	/* tell remote processor's name service we're removing this channel
> */
> @@ -412,6 +444,18 @@ static int rpmsg_dev_remove(struct device *dev)
>  			dev_err(dev, "failed to announce service %d\n", err);
>  	}
> 
> +	return err;
> +}
> +
> +static int rpmsg_dev_remove(struct device *dev)
> +{
> +	struct rpmsg_device *rpdev = to_rpmsg_device(dev);
> +	struct rpmsg_driver *rpdrv = to_rpmsg_driver(rpdev->dev.driver);
> +	int err = 0;
> +
> +	if (rpdev->announce_destroy)
> +		err = rpdev->announce_destroy(rpdev);
> +
>  	rpdrv->remove(rpdev);
> 
>  	rpmsg_destroy_ept(rpdev->ept);
> @@ -485,6 +529,19 @@ static int rpmsg_device_match(struct device *dev,
> void *data)
>  	return 1;
>  }
> 
> +static const struct rpmsg_device virtio_rpmsg_ops = {
> +	.create_ept = virtio_rpmsg_create_ept,
> +	.destroy_ept = virtio_rpmsg_destroy_ept,
> +	.send = virtio_rpmsg_send,
> +	.sendto = virtio_rpmsg_sendto,
> +	.send_offchannel = virtio_rpmsg_send_offchannel,
> +	.trysend = virtio_rpmsg_trysend,
> +	.trysendto = virtio_rpmsg_trysendto,
> +	.trysend_offchannel = virtio_rpmsg_trysend_offchannel,
> +	.announce_create = virtio_rpmsg_announce_create,
> +	.announce_destroy = virtio_rpmsg_announce_destroy,
> +};
Why not creating a dedicated ops struct like other framework?
Here ops are mixed with other parameters.

> +
>  /*
>   * create an rpmsg channel using its name and address info.
>   * this function will be used to create both static and dynamic
> @@ -511,6 +568,9 @@ static struct rpmsg_device
> *rpmsg_create_channel(struct virtproc_info *vrp,
>  	if (!rpdev)
>  		return NULL;
> 
> +	/* Assign callbacks for rpmsg_channel */
> +	*rpdev = virtio_rpmsg_ops;
It is not a simple affectation behind this operation, more a memcopy of the complete struct.
Not easy to read from my pov.
> +
>  	rpdev->vrp = vrp;
>  	rpdev->src = chinfo->src;
>  	rpdev->dst = chinfo->dst;
> @@ -793,11 +853,17 @@ out:
>  int rpmsg_send(struct rpmsg_endpoint *ept, void *data, int len)
>  {
>  	struct rpmsg_device *rpdev = ept->rpdev;
> +
> +	return rpdev->send(ept, data, len);
Test pointer before using it

> +}
> +
> +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;
> 
>  	return rpmsg_send_offchannel_raw(rpdev, src, dst, data, len, true);
>  }
> -EXPORT_SYMBOL(rpmsg_send);
> 
>  /**
>   * rpmsg_sendto() - send a message across to the remote processor, specify
> dst
> @@ -820,11 +886,19 @@ EXPORT_SYMBOL(rpmsg_send);
>  int rpmsg_sendto(struct rpmsg_endpoint *ept, void *data, int len, u32 dst)
>  {
>  	struct rpmsg_device *rpdev = ept->rpdev;
> +
> +	return rpdev->sendto(ept, data, len, dst);
Ditto

> +}
> +EXPORT_SYMBOL(rpmsg_sendto);
> +
> +static int virtio_rpmsg_sendto(struct rpmsg_endpoint *ept, void *data, int
> len,
> +			       u32 dst)
> +{
> +	struct rpmsg_device *rpdev = ept->rpdev;
>  	u32 src = ept->addr;
> 
>  	return rpmsg_send_offchannel_raw(rpdev, src, dst, data, len, true);
>  }
> -EXPORT_SYMBOL(rpmsg_sendto);
> 
>  /**
>   * rpmsg_send_offchannel() - send a message using explicit src/dst
> addresses
> @@ -851,10 +925,18 @@ int rpmsg_send_offchannel(struct rpmsg_endpoint
> *ept, u32 src, u32 dst,
>  {
>  	struct rpmsg_device *rpdev = ept->rpdev;
> 
> -	return rpmsg_send_offchannel_raw(rpdev, src, dst, data, len, true);
> +	return rpdev->send_offchannel(ept, src, dst, data, len);
>  }
>  EXPORT_SYMBOL(rpmsg_send_offchannel);
> 
> +static int virtio_rpmsg_send_offchannel(struct rpmsg_endpoint *ept, u32
> src,
> +					u32 dst, void *data, int len)
> +{
> +	struct rpmsg_device *rpdev = ept->rpdev;
> +
> +	return rpmsg_send_offchannel_raw(rpdev, src, dst, data, len, true);
> +}
> +
>  /**
>   * rpmsg_send() - send a message across to the remote processor
>   * @ept: the rpmsg endpoint
> @@ -875,11 +957,18 @@ EXPORT_SYMBOL(rpmsg_send_offchannel);
>  int rpmsg_trysend(struct rpmsg_endpoint *ept, void *data, int len)
>  {
>  	struct rpmsg_device *rpdev = ept->rpdev;
> +
> +	return rpdev->trysend(ept, data, len);
ditto
> +}
> +EXPORT_SYMBOL(rpmsg_trysend);
> +
> +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;
> 
>  	return rpmsg_send_offchannel_raw(rpdev, src, dst, data, len, false);
>  }
> -EXPORT_SYMBOL(rpmsg_trysend);
> 
>  /**
>   * rpmsg_sendto() - send a message across to the remote processor, specify
> dst
> @@ -901,11 +990,19 @@ EXPORT_SYMBOL(rpmsg_trysend);
>  int rpmsg_trysendto(struct rpmsg_endpoint *ept, void *data, int len, u32
> dst)
>  {
>  	struct rpmsg_device *rpdev = ept->rpdev;
> +
> +	return rpdev->trysendto(ept, data, len, dst);
ditto
> +}
> +EXPORT_SYMBOL(rpmsg_trysendto);
> +
> +static int virtio_rpmsg_trysendto(struct rpmsg_endpoint *ept, void *data,
> +				  int len, u32 dst)
> +{
> +	struct rpmsg_device *rpdev = ept->rpdev;
>  	u32 src = ept->addr;
> 
>  	return rpmsg_send_offchannel_raw(rpdev, src, dst, data, len, false);
>  }
> -EXPORT_SYMBOL(rpmsg_trysendto);
> 
>  /**
>   * rpmsg_send_offchannel() - send a message using explicit src/dst
> addresses
> @@ -931,10 +1028,18 @@ int rpmsg_trysend_offchannel(struct
> rpmsg_endpoint *ept, u32 src, u32 dst,
>  {
>  	struct rpmsg_device *rpdev = ept->rpdev;
> 
> -	return rpmsg_send_offchannel_raw(rpdev, src, dst, data, len, false);
> +	return rpdev->trysend_offchannel(ept, src, dst, data, len);
ditto
>  }
>  EXPORT_SYMBOL(rpmsg_trysend_offchannel);
> 
> +static int virtio_rpmsg_trysend_offchannel(struct rpmsg_endpoint *ept,
> u32 src,
> +					   u32 dst, void *data, int len)
> +{
> +	struct rpmsg_device *rpdev = ept->rpdev;
> +
> +	return rpmsg_send_offchannel_raw(rpdev, src, dst, data, len, false);
> +}
> +
>  static int rpmsg_recv_single(struct virtproc_info *vrp, struct device *dev,
>  			     struct rpmsg_hdr *msg, unsigned int len)
>  {
> diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h
> index 3e3d5b1c154a..0b290ae18e70 100644
> --- a/include/linux/rpmsg.h
> +++ b/include/linux/rpmsg.h
> @@ -96,6 +96,10 @@ enum rpmsg_ns_flags {
>  #define RPMSG_ADDR_ANY		0xFFFFFFFF
> 
>  struct virtproc_info;
> +struct rpmsg_device;
> +struct rpmsg_endpoint;
> +
> +typedef void (*rpmsg_rx_cb_t)(struct rpmsg_device *, void *, int, void *,
> u32);
> 
>  /**
>   * rpmsg_device - device that belong to the rpmsg bus
> @@ -115,9 +119,24 @@ struct rpmsg_device {
>  	u32 dst;
>  	struct rpmsg_endpoint *ept;
>  	bool announce;
> -};
> 
> -typedef void (*rpmsg_rx_cb_t)(struct rpmsg_device *, void *, int, void *,
> u32);
> +	struct rpmsg_endpoint *(*create_ept)(struct rpmsg_device *rpdev,
> +					    rpmsg_rx_cb_t cb, void *priv, u32
> addr);
> +	void (*destroy_ept)(struct rpmsg_endpoint *ept);
> +
> +	int (*send)(struct rpmsg_endpoint *ept, void *data, int len);
> +	int (*sendto)(struct rpmsg_endpoint *ept, void *data, int len, u32
> dst);
> +	int (*send_offchannel)(struct rpmsg_endpoint *ept, u32 src, u32 dst,
> +				  void *data, int len);
> +
> +	int (*trysend)(struct rpmsg_endpoint *ept, void *data, int len);
> +	int (*trysendto)(struct rpmsg_endpoint *ept, void *data, int len, u32
> dst);
> +	int (*trysend_offchannel)(struct rpmsg_endpoint *ept, u32 src, u32
> dst,
> +			     void *data, int len);
> +
> +	int (*announce_create)(struct rpmsg_device *ept);
> +	int (*announce_destroy)(struct rpmsg_device *ept);
> +};
It will be nice to document if ops are mandatory or optional.

Regards,
Loic
> 
>  /**
>   * struct rpmsg_endpoint - binds a local rpmsg address to its user
> --
> 2.5.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[toc] | [prev] | [next] | [standalone]

#1465803

FromBjorn Andersson <bjorn.andersson@linaro.org>
Date2016-08-19 03:40 +0200
Message-ID<s7GWR-7sc-13@gated-at.bofh.it>
In reply to#1465217
On Thu 18 Aug 05:14 PDT 2016, Loic PALLARDY wrote:

> > diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c
[..]
> >  struct rpmsg_endpoint *rpmsg_create_ept(struct rpmsg_device *rpdev,
> >  					rpmsg_rx_cb_t cb, void *priv, u32
> > addr)
> >  {
> > -	return __rpmsg_create_ept(rpdev->vrp, rpdev, cb, priv, addr);
> > +	return rpdev->create_ept(rpdev, cb, priv, addr);
> Hi Bjorn,
> 
> It will be good to test if pointer is valid before calling function.
> 

Per the rpmsg_send() implementation, I can make it loud but friendlier
by:

if (WARN_ON(!rpdev))
	return -EINVAL;

[..]
> > 
> > +static const struct rpmsg_device virtio_rpmsg_ops = {
> > +	.create_ept = virtio_rpmsg_create_ept,
> > +	.destroy_ept = virtio_rpmsg_destroy_ept,
> > +	.send = virtio_rpmsg_send,
> > +	.sendto = virtio_rpmsg_sendto,
> > +	.send_offchannel = virtio_rpmsg_send_offchannel,
> > +	.trysend = virtio_rpmsg_trysend,
> > +	.trysendto = virtio_rpmsg_trysendto,
> > +	.trysend_offchannel = virtio_rpmsg_trysend_offchannel,
> > +	.announce_create = virtio_rpmsg_announce_create,
> > +	.announce_destroy = virtio_rpmsg_announce_destroy,
> > +};
> Why not creating a dedicated ops struct like other framework?
> Here ops are mixed with other parameters.
> 

That's a good suggestion...

> > +
> >  /*
> >   * create an rpmsg channel using its name and address info.
> >   * this function will be used to create both static and dynamic
> > @@ -511,6 +568,9 @@ static struct rpmsg_device
> > *rpmsg_create_channel(struct virtproc_info *vrp,
> >  	if (!rpdev)
> >  		return NULL;
> > 
> > +	/* Assign callbacks for rpmsg_channel */
> > +	*rpdev = virtio_rpmsg_ops;
> It is not a simple affectation behind this operation, more a memcopy of the complete struct.
> Not easy to read from my pov.

...and would clean this up. I'll do that.

> > +
> >  	rpdev->vrp = vrp;
> >  	rpdev->src = chinfo->src;
> >  	rpdev->dst = chinfo->dst;
> > @@ -793,11 +853,17 @@ out:
> >  int rpmsg_send(struct rpmsg_endpoint *ept, void *data, int len)
> >  {
> >  	struct rpmsg_device *rpdev = ept->rpdev;
> > +
> > +	return rpdev->send(ept, data, len);
> Test pointer before using it

Yeah, this would follow from the earlier patch.

> 
> > +}
> > +
[..]
> > diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h
[..]
> > u32);
> > +	struct rpmsg_endpoint *(*create_ept)(struct rpmsg_device *rpdev,
> > +					    rpmsg_rx_cb_t cb, void *priv, u32
> > addr);
> > +	void (*destroy_ept)(struct rpmsg_endpoint *ept);
> > +
> > +	int (*send)(struct rpmsg_endpoint *ept, void *data, int len);
> > +	int (*sendto)(struct rpmsg_endpoint *ept, void *data, int len, u32
> > dst);
> > +	int (*send_offchannel)(struct rpmsg_endpoint *ept, u32 src, u32 dst,
> > +				  void *data, int len);
> > +
> > +	int (*trysend)(struct rpmsg_endpoint *ept, void *data, int len);
> > +	int (*trysendto)(struct rpmsg_endpoint *ept, void *data, int len, u32
> > dst);
> > +	int (*trysend_offchannel)(struct rpmsg_endpoint *ept, u32 src, u32
> > dst,
> > +			     void *data, int len);
> > +
> > +	int (*announce_create)(struct rpmsg_device *ept);
> > +	int (*announce_destroy)(struct rpmsg_device *ept);
> > +};
> It will be nice to document if ops are mandatory or optional.
> 

I'll break them out in a ops struct and throw in some kerneldoc in both
cases.

Thanks for the feedback!

Regards,
Bjorn

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web