From: Mathieu Poirier <mathieu.poirier@linaro.org>
To: Arnaud POULIQUEN <arnaud.pouliquen@st.com>
Cc: "ohad@wizery.com" <ohad@wizery.com>,
"bjorn.andersson@linaro.org" <bjorn.andersson@linaro.org>,
"guennadi.liakhovetski@linux.intel.com"
<guennadi.liakhovetski@linux.intel.com>,
"linux-remoteproc@vger.kernel.org"
<linux-remoteproc@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2 3/9] rpmsg: virtio: Move from virtio to rpmsg byte conversion
Date: Thu, 15 Oct 2020 13:46:20 -0600 [thread overview]
Message-ID: <20201015194620.GE1450102@xps15> (raw)
In-Reply-To: <0264d2f6-22c7-6c6d-0db4-05fd9e0b1121@st.com>
On Wed, Oct 14, 2020 at 07:04:32PM +0200, Arnaud POULIQUEN wrote:
>
>
> On 10/14/20 1:25 AM, Mathieu Poirier wrote:
> > Use rpmsg byte conversion functions in order for the RPMSG
> > headers and generic functions to be used by external entities.
> >
> > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> > ---
> > drivers/rpmsg/virtio_rpmsg_bus.c | 60 +++++++++++++++++++-------------
> > 1 file changed, 35 insertions(+), 25 deletions(-)
> >
> > diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> > index 9006fc7f73d0..793fe924671f 100644
> > --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> > +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> > @@ -19,11 +19,11 @@
> > #include <linux/mutex.h>
> > #include <linux/of_device.h>
> > #include <linux/rpmsg.h>
> > +#include <linux/rpmsg_byteorder.h>
> > #include <linux/scatterlist.h>
> > #include <linux/slab.h>
> > #include <linux/sched.h>
> > #include <linux/virtio.h>
> > -#include <linux/virtio_byteorder.h>
> > #include <linux/virtio_ids.h>
> > #include <linux/virtio_config.h>
> > #include <linux/wait.h>
> > @@ -85,11 +85,11 @@ struct virtproc_info {
> > * Every message sent(/received) on the rpmsg bus begins with this header.
> > */
> > struct rpmsg_hdr {
> > - __virtio32 src;
> > - __virtio32 dst;
> > - __virtio32 reserved;
> > - __virtio16 len;
> > - __virtio16 flags;
> > + __rpmsg32 src;
> > + __rpmsg32 dst;
> > + __rpmsg32 reserved;
> > + __rpmsg16 len;
> > + __rpmsg16 flags;
> > u8 data[];
> > } __packed;
> >
> > @@ -107,8 +107,8 @@ struct rpmsg_hdr {
> > */
> > struct rpmsg_ns_msg {
> > char name[RPMSG_NAME_SIZE];
> > - __virtio32 addr;
> > - __virtio32 flags;
> > + __rpmsg32 addr;
> > + __rpmsg32 flags;
> > } __packed;
> >
> > /**
> > @@ -280,6 +280,14 @@ static struct rpmsg_endpoint *__rpmsg_create_ept(struct virtproc_info *vrp,
> > return NULL;
> > }
> >
> > +static bool virtio_rpmsg_is_little_endian(struct rpmsg_device *rpdev)
> > +{
> > + struct virtio_rpmsg_channel *vch = to_virtio_rpmsg_channel(rpdev);
> > + struct virtproc_info *vrp = vch->vrp;
> > +
> > + return virtio_is_little_endian(vrp->vdev);
> > +}
>
> Regarding this i wonder if the endianess could not be a rpmsg_device field that would be set on
> __rpmsg_create_channel?
> I don't think that the endianess could change, so perhaps no need to call ops for each conversion
> using interface implemented in rpmsg.h...
That's a valid point - I certainly don't expect the endianness to change
dynamically.
> But perhaps I missed something?
>
> > +
> > static struct rpmsg_endpoint *virtio_rpmsg_create_ept(struct rpmsg_device *rpdev,
> > rpmsg_rx_cb_t cb,
> > void *priv,
> > @@ -336,8 +344,8 @@ static int virtio_rpmsg_announce_create(struct rpmsg_device *rpdev)
> > struct rpmsg_ns_msg nsm;
> >
> > strncpy(nsm.name, rpdev->id.name, RPMSG_NAME_SIZE);
> > - nsm.addr = cpu_to_virtio32(vrp->vdev, rpdev->ept->addr);
> > - nsm.flags = cpu_to_virtio32(vrp->vdev, RPMSG_NS_CREATE);
> > + nsm.addr = cpu_to_rpmsg32(rpdev, rpdev->ept->addr);
> > + nsm.flags = cpu_to_rpmsg32(rpdev, RPMSG_NS_CREATE);
> >
> > err = rpmsg_sendto(rpdev->ept, &nsm, sizeof(nsm), RPMSG_NS_ADDR);
> > if (err)
> > @@ -360,8 +368,8 @@ static int virtio_rpmsg_announce_destroy(struct rpmsg_device *rpdev)
> > struct rpmsg_ns_msg nsm;
> >
> > strncpy(nsm.name, rpdev->id.name, RPMSG_NAME_SIZE);
> > - nsm.addr = cpu_to_virtio32(vrp->vdev, rpdev->ept->addr);
> > - nsm.flags = cpu_to_virtio32(vrp->vdev, RPMSG_NS_DESTROY);
> > + nsm.addr = cpu_to_rpmsg32(rpdev, rpdev->ept->addr);
> > + nsm.flags = cpu_to_rpmsg32(rpdev, RPMSG_NS_DESTROY);
> >
> > err = rpmsg_sendto(rpdev->ept, &nsm, sizeof(nsm), RPMSG_NS_ADDR);
> > if (err)
> > @@ -372,6 +380,7 @@ static int virtio_rpmsg_announce_destroy(struct rpmsg_device *rpdev)
> > }
> >
> > static const struct rpmsg_device_ops virtio_rpmsg_ops = {
> > + .is_little_endian = virtio_rpmsg_is_little_endian,
> > .create_ept = virtio_rpmsg_create_ept,
> > .announce_create = virtio_rpmsg_announce_create,
> > .announce_destroy = virtio_rpmsg_announce_destroy,
> > @@ -613,10 +622,10 @@ static int rpmsg_send_offchannel_raw(struct rpmsg_device *rpdev,
> > }
> > }
> >
> > - msg->len = cpu_to_virtio16(vrp->vdev, len);
> > + msg->len = cpu_to_rpmsg16(rpdev, len);
> > msg->flags = 0;
> > - msg->src = cpu_to_virtio32(vrp->vdev, src);
> > - msg->dst = cpu_to_virtio32(vrp->vdev, dst);
> > + msg->src = cpu_to_rpmsg32(rpdev, src);
> > + msg->dst = cpu_to_rpmsg32(rpdev, dst);
> > msg->reserved = 0;
> > memcpy(msg->data, data, len);
> >
> > @@ -705,14 +714,15 @@ static int rpmsg_recv_single(struct virtproc_info *vrp, struct device *dev,
> > {
> > struct rpmsg_endpoint *ept;
> > struct scatterlist sg;
> > - unsigned int msg_len = virtio16_to_cpu(vrp->vdev, msg->len);
> > + bool little_endian = rpmsg_is_little_endian();
> > + unsigned int msg_len = __rpmsg16_to_cpu(little_endian, msg->len);
> > int err;
> >
> > dev_dbg(dev, "From: 0x%x, To: 0x%x, Len: %d, Flags: %d, Reserved: %d\n",
> > - virtio32_to_cpu(vrp->vdev, msg->src),
> > - virtio32_to_cpu(vrp->vdev, msg->dst), msg_len,
> > - virtio16_to_cpu(vrp->vdev, msg->flags),
> > - virtio32_to_cpu(vrp->vdev, msg->reserved));
> > + __rpmsg32_to_cpu(little_endian, msg->src),
> > + __rpmsg32_to_cpu(little_endian, msg->dst), msg_len,
> > + __rpmsg16_to_cpu(little_endian, msg->flags),
> > + __rpmsg32_to_cpu(little_endian, msg->reserved));
>
> Nitpicking: sometime rpmsgXX_to_cpu is used, sometime __rpmsgXX_to_cpu,
> Perhaps only one API should be used... But i don't see any blocking point to use both...:)
Here we have to use __rpmsgXX_to_CPU() because the rpmsg_device is not
available. It could be fixed but would require a serious amount of refactoring
for little value. Moreover since this rpmsg_recv_single() is specific to the
virtIO implementation the end result is the same.
>
> Thanks,
> Arnaud
>
> > #if defined(CONFIG_DYNAMIC_DEBUG)
> > dynamic_hex_dump("rpmsg_virtio RX: ", DUMP_PREFIX_NONE, 16, 1,
> > msg, sizeof(*msg) + msg_len, true);
> > @@ -731,7 +741,7 @@ static int rpmsg_recv_single(struct virtproc_info *vrp, struct device *dev,
> > /* use the dst addr to fetch the callback of the appropriate user */
> > mutex_lock(&vrp->endpoints_lock);
> >
> > - ept = idr_find(&vrp->endpoints, virtio32_to_cpu(vrp->vdev, msg->dst));
> > + ept = idr_find(&vrp->endpoints, __rpmsg32_to_cpu(little_endian, msg->dst));
> >
> > /* let's make sure no one deallocates ept while we use it */
> > if (ept)
> > @@ -745,7 +755,7 @@ static int rpmsg_recv_single(struct virtproc_info *vrp, struct device *dev,
> >
> > if (ept->cb)
> > ept->cb(ept->rpdev, msg->data, msg_len, ept->priv,
> > - virtio32_to_cpu(vrp->vdev, msg->src));
> > + __rpmsg32_to_cpu(little_endian, msg->src));
> >
> > mutex_unlock(&ept->cb_lock);
> >
> > @@ -853,13 +863,13 @@ static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void *data, int len,
> >
> > strncpy(chinfo.name, msg->name, sizeof(chinfo.name));
> > chinfo.src = RPMSG_ADDR_ANY;
> > - chinfo.dst = virtio32_to_cpu(vrp->vdev, msg->addr);
> > + chinfo.dst = rpmsg32_to_cpu(rpdev, msg->addr);
> >
> > dev_info(dev, "%sing channel %s addr 0x%x\n",
> > - virtio32_to_cpu(vrp->vdev, msg->flags) & RPMSG_NS_DESTROY ?
> > + rpmsg32_to_cpu(rpdev, msg->flags) & RPMSG_NS_DESTROY ?
> > "destroy" : "creat", msg->name, chinfo.dst);
> >
> > - if (virtio32_to_cpu(vrp->vdev, msg->flags) & RPMSG_NS_DESTROY) {
> > + if (rpmsg32_to_cpu(rpdev, msg->flags) & RPMSG_NS_DESTROY) {
> > ret = rpmsg_unregister_device(&vrp->vdev->dev, &chinfo);
> > if (ret)
> > dev_err(dev, "rpmsg_destroy_channel failed: %d\n", ret);
> >
next prev parent reply other threads:[~2020-10-15 19:46 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-10-13 23:25 [PATCH v2 0/9] rpmsg: Make RPMSG name service modular Mathieu Poirier
2020-10-13 23:25 ` [PATCH v2 1/9] rpmsg: Move rpmsg_endpoint_ops to rpmsg.h Mathieu Poirier
2020-10-15 8:48 ` Arnaud POULIQUEN
2020-10-13 23:25 ` [PATCH v2 2/9] rpmsg: Introduce __rpmsg{16|32|64} types Mathieu Poirier
2020-10-14 16:31 ` Arnaud POULIQUEN
2020-10-15 19:32 ` Mathieu Poirier
2020-10-16 9:41 ` Arnaud POULIQUEN
2020-10-13 23:25 ` [PATCH v2 3/9] rpmsg: virtio: Move from virtio to rpmsg byte conversion Mathieu Poirier
2020-10-14 17:04 ` Arnaud POULIQUEN
2020-10-15 19:46 ` Mathieu Poirier [this message]
2020-10-13 23:25 ` [PATCH v2 4/9] rpmsg: Move rpmsg_hr and rpmsg_ns_msg to header file Mathieu Poirier
2020-10-15 8:33 ` Arnaud POULIQUEN
2020-10-15 20:19 ` Mathieu Poirier
2020-10-16 9:45 ` Arnaud POULIQUEN
2020-10-13 23:25 ` [PATCH v2 5/9] rpmsg: virtio: Rename rpmsg_create_channel Mathieu Poirier
2020-10-13 23:25 ` [PATCH v2 6/9] rpmsg: core: Add channel creation internal API Mathieu Poirier
2020-10-13 23:25 ` [PATCH v2 7/9] rpmsg: virtio: Add rpmsg channel device ops Mathieu Poirier
2020-10-13 23:25 ` [PATCH v2 8/9] rpmsg: Make rpmsg_{register|unregister}_device() public Mathieu Poirier
2020-10-15 8:47 ` Arnaud POULIQUEN
2020-10-13 23:25 ` [PATCH v2 9/9] rpmsg: Turn name service into a stand alone driver Mathieu Poirier
2020-10-14 10:18 ` [PATCH v2 0/9] rpmsg: Make RPMSG name service modular Guennadi Liakhovetski
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20201015194620.GE1450102@xps15 \
--to=mathieu.poirier@linaro.org \
--cc=arnaud.pouliquen@st.com \
--cc=bjorn.andersson@linaro.org \
--cc=guennadi.liakhovetski@linux.intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-remoteproc@vger.kernel.org \
--cc=ohad@wizery.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.