Re: [PATCH v9 1/6] rpmsg: Process all available messages in virtqueue callback
On Tue, Apr 9, 2013 at 1:56 AM, Tivy, Robert rt...@ti.com wrote:
Shouldn't the virtqueue_kick() be called only when we successfully added a
buffer with virtqueue_add_buf()?
Definitely, thanks for noticing!
Take 2:
diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
index a59684b..4ade672 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -776,22 +776,11 @@ out:
}
EXPORT_SYMBOL(rpmsg_send_offchannel_raw);
-/* called when an rx buffer is used, and it's time to digest a message */
-static void rpmsg_recv_done(struct virtqueue *rvq)
+static int rpmsg_recv_single(struct virtproc_info *vrp, struct device *dev,
+struct rpmsg_hdr *msg, unsigned int len)
{
- struct rpmsg_hdr *msg;
- unsigned int len;
struct rpmsg_endpoint *ept;
struct scatterlist sg;
- struct virtproc_info *vrp = rvq-vdev-priv;
- struct device *dev = rvq-vdev-dev;
- int err;
-
- msg = virtqueue_get_buf(rvq, len);
- if (!msg) {
- dev_err(dev, uhm, incoming signal, but no used buffer ?\n);
- return;
- }
dev_dbg(dev, From: 0x%x, To: 0x%x, Len: %d, Flags: %d, Reserved: %d\n,
msg-src, msg-dst, msg-len,
@@ -806,7 +795,7 @@ static void rpmsg_recv_done(struct virtqueue *rvq)
if (len RPMSG_BUF_SIZE ||
msg-len (len - sizeof(struct rpmsg_hdr))) {
dev_warn(dev, inbound msg too big: (%d, %d)\n, len, msg-len);
- return;
+ return -EINVAL;
}
/* use the dst addr to fetch the callback of the appropriate user */
@@ -842,11 +831,42 @@ static void rpmsg_recv_done(struct virtqueue *rvq)
err = virtqueue_add_buf(vrp-rvq, sg, 0, 1, msg, GFP_KERNEL);
if (err 0) {
dev_err(dev, failed to add a virtqueue buffer: %d\n, err);
+ return err;
+ }
+
+ return 0;
+}
+
+/* called when an rx buffer is used, and it's time to digest a message */
+static void rpmsg_recv_done(struct virtqueue *rvq)
+{
+ struct virtproc_info *vrp = rvq-vdev-priv;
+ struct device *dev = rvq-vdev-dev;
+ struct rpmsg_hdr *msg;
+ unsigned int len, msgs_received = 0;
+ int err;
+
+ msg = virtqueue_get_buf(rvq, len);
+ if (!msg) {
+ dev_err(dev, uhm, incoming signal, but no used buffer ?\n);
return;
}
+ while (msg) {
+ err = rpmsg_recv_single(vrp, dev, msg, len);
+ if (err)
+ break;
+
+ msgs_received++;
+
+ msg = virtqueue_get_buf(rvq, len);
+ };
+
+ dev_dbg(dev, Received %u messages\n, msgs_received);
+
/* tell the remote processor we added another available rx buffer */
- virtqueue_kick(vrp-rvq);
+ if (msgs_received)
+ virtqueue_kick(vrp-rvq);
}
/*
Thanks for the rewrite, looks better. I'll give it a try and let you know
how it goes.
Thanks!
Ohad.
___
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
RE: [PATCH v9 1/6] rpmsg: Process all available messages in virtqueue callback
Just one small fix needed (see below) and it's good-to-go.
-Original Message-
From: Ohad Ben-Cohen [mailto:o...@wizery.com]
Sent: Tuesday, April 09, 2013 1:26 AM
On Tue, Apr 9, 2013 at 1:56 AM, Tivy, Robert rt...@ti.com wrote:
Shouldn't the virtqueue_kick() be called only when we successfully
added a buffer with virtqueue_add_buf()?
Definitely, thanks for noticing!
Take 2:
diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c
b/drivers/rpmsg/virtio_rpmsg_bus.c
index a59684b..4ade672 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -776,22 +776,11 @@ out:
}
EXPORT_SYMBOL(rpmsg_send_offchannel_raw);
-/* called when an rx buffer is used, and it's time to digest a message
*/
-static void rpmsg_recv_done(struct virtqueue *rvq)
+static int rpmsg_recv_single(struct virtproc_info *vrp, struct device
*dev,
+struct rpmsg_hdr *msg, unsigned int len)
{
- struct rpmsg_hdr *msg;
- unsigned int len;
struct rpmsg_endpoint *ept;
struct scatterlist sg;
- struct virtproc_info *vrp = rvq-vdev-priv;
- struct device *dev = rvq-vdev-dev;
- int err;
This new function also uses an 'int err;', so the above line should not be
removed.
-
- msg = virtqueue_get_buf(rvq, len);
- if (!msg) {
- dev_err(dev, uhm, incoming signal, but no used buffer
?\n);
- return;
- }
dev_dbg(dev, From: 0x%x, To: 0x%x, Len: %d, Flags: %d,
Reserved: %d\n,
msg-src, msg-dst, msg-len,
@@ -806,7 +795,7 @@ static void rpmsg_recv_done(struct virtqueue *rvq)
if (len RPMSG_BUF_SIZE ||
msg-len (len - sizeof(struct rpmsg_hdr))) {
dev_warn(dev, inbound msg too big: (%d, %d)\n, len,
msg-len);
- return;
+ return -EINVAL;
}
/* use the dst addr to fetch the callback of the appropriate
user */
@@ -842,11 +831,42 @@ static void rpmsg_recv_done(struct virtqueue
*rvq)
err = virtqueue_add_buf(vrp-rvq, sg, 0, 1, msg, GFP_KERNEL);
if (err 0) {
dev_err(dev, failed to add a virtqueue buffer: %d\n,
err);
+ return err;
+ }
+
+ return 0;
+}
+
+/* called when an rx buffer is used, and it's time to digest a message
*/
+static void rpmsg_recv_done(struct virtqueue *rvq)
+{
+ struct virtproc_info *vrp = rvq-vdev-priv;
+ struct device *dev = rvq-vdev-dev;
+ struct rpmsg_hdr *msg;
+ unsigned int len, msgs_received = 0;
+ int err;
+
+ msg = virtqueue_get_buf(rvq, len);
+ if (!msg) {
+ dev_err(dev, uhm, incoming signal, but no used buffer
?\n);
return;
}
+ while (msg) {
+ err = rpmsg_recv_single(vrp, dev, msg, len);
+ if (err)
+ break;
+
+ msgs_received++;
+
+ msg = virtqueue_get_buf(rvq, len);
+ };
+
+ dev_dbg(dev, Received %u messages\n, msgs_received);
+
/* tell the remote processor we added another available rx
buffer */
- virtqueue_kick(vrp-rvq);
+ if (msgs_received)
+ virtqueue_kick(vrp-rvq);
}
/*
Thanks for the rewrite, looks better. I'll give it a try and let you
know how it goes.
Thanks!
Ohad.
I added the above-mentioned 'int err;' to my tree's rpmsg_recv_single() and
tested it out, it worked well. So once that is corrected, you can add:
Acked-by: Robert Tivy rt...@ti.com
Regards,
- Rob
___
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
RE: [PATCH v9 1/6] rpmsg: Process all available messages in virtqueue callback
-Original Message-
From: Ohad Ben-Cohen [mailto:o...@wizery.com]
Sent: Sunday, April 07, 2013 5:51 AM
On Fri, Mar 29, 2013 at 4:41 AM, Robert Tivy rt...@ti.com wrote:
Change virtqueue callback function rpmsg_recv_done() to process all
available messages instead of just one message.
Signed-off-by: Robert Tivy rt...@ti.com
I'm thinking instead of adding an indentation level, let's split the
_recv function.
This is what I have in mind - let me know if it works for you - thanks:
diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c
b/drivers/rpmsg/virtio_rpmsg_bus.c
index a59684b..42b9872 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -776,22 +776,11 @@ out:
}
EXPORT_SYMBOL(rpmsg_send_offchannel_raw);
-/* called when an rx buffer is used, and it's time to digest a message
*/
-static void rpmsg_recv_done(struct virtqueue *rvq)
+static int rpmsg_recv_single(struct virtproc_info *vrp, struct device
*dev,
+struct rpmsg_hdr *msg, unsigned int len)
{
- struct rpmsg_hdr *msg;
- unsigned int len;
struct rpmsg_endpoint *ept;
struct scatterlist sg;
- struct virtproc_info *vrp = rvq-vdev-priv;
- struct device *dev = rvq-vdev-dev;
- int err;
-
- msg = virtqueue_get_buf(rvq, len);
- if (!msg) {
- dev_err(dev, uhm, incoming signal, but no used buffer
?\n);
- return;
- }
dev_dbg(dev, From: 0x%x, To: 0x%x, Len: %d, Flags: %d,
Reserved: %d\n,
msg-src, msg-dst, msg-len,
@@ -806,7 +795,7 @@ static void rpmsg_recv_done(struct virtqueue *rvq)
if (len RPMSG_BUF_SIZE ||
msg-len (len - sizeof(struct rpmsg_hdr))) {
dev_warn(dev, inbound msg too big: (%d, %d)\n, len,
msg-len);
- return;
+ return -EINVAL;
}
/* use the dst addr to fetch the callback of the appropriate
user */
@@ -842,9 +831,39 @@ static void rpmsg_recv_done(struct virtqueue *rvq)
err = virtqueue_add_buf(vrp-rvq, sg, 0, 1, msg, GFP_KERNEL);
if (err 0) {
dev_err(dev, failed to add a virtqueue buffer: %d\n,
err);
+ return err;
+ }
+
+ return 0;
+}
+
+/* called when an rx buffer is used, and it's time to digest a message
*/
+static void rpmsg_recv_done(struct virtqueue *rvq)
+{
+ struct virtproc_info *vrp = rvq-vdev-priv;
+ struct device *dev = rvq-vdev-dev;
+ struct rpmsg_hdr *msg;
+ unsigned int len, msgs_received = 0;
+ int err;
+
+ msg = virtqueue_get_buf(rvq, len);
+ if (!msg) {
+ dev_err(dev, uhm, incoming signal, but no used buffer
?\n);
return;
}
+ while (msg) {
+ err = rpmsg_recv_single(vrp, dev, msg, len);
+ if (err)
+ break;
+
+ msgs_received++;
+
+ msg = virtqueue_get_buf(rvq, len);
+ };
+
+ dev_dbg(dev, Received %u messages\n, msgs_received);
+
/* tell the remote processor we added another available rx
buffer */
virtqueue_kick(vrp-rvq);
}
Shouldn't the virtqueue_kick() be called only when we successfully added a
buffer with virtqueue_add_buf()?
If so, we should make it:
if (msgs_received)
/* tell the remote processor we added another available rx
buffer */
virtqueue_kick(vrp-rvp);
}
Thanks for the rewrite, looks better. I'll give it a try and let you know how
it goes.
Regards,
- Rob
___
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH v9 1/6] rpmsg: Process all available messages in virtqueue callback
On Fri, Mar 29, 2013 at 4:41 AM, Robert Tivy rt...@ti.com wrote:
Change virtqueue callback function rpmsg_recv_done() to process all
available messages instead of just one message.
Signed-off-by: Robert Tivy rt...@ti.com
I'm thinking instead of adding an indentation level, let's split the
_recv function.
This is what I have in mind - let me know if it works for you - thanks:
diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
index a59684b..42b9872 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -776,22 +776,11 @@ out:
}
EXPORT_SYMBOL(rpmsg_send_offchannel_raw);
-/* called when an rx buffer is used, and it's time to digest a message */
-static void rpmsg_recv_done(struct virtqueue *rvq)
+static int rpmsg_recv_single(struct virtproc_info *vrp, struct device *dev,
+struct rpmsg_hdr *msg, unsigned int len)
{
- struct rpmsg_hdr *msg;
- unsigned int len;
struct rpmsg_endpoint *ept;
struct scatterlist sg;
- struct virtproc_info *vrp = rvq-vdev-priv;
- struct device *dev = rvq-vdev-dev;
- int err;
-
- msg = virtqueue_get_buf(rvq, len);
- if (!msg) {
- dev_err(dev, uhm, incoming signal, but no used buffer ?\n);
- return;
- }
dev_dbg(dev, From: 0x%x, To: 0x%x, Len: %d, Flags: %d, Reserved: %d\n,
msg-src, msg-dst, msg-len,
@@ -806,7 +795,7 @@ static void rpmsg_recv_done(struct virtqueue *rvq)
if (len RPMSG_BUF_SIZE ||
msg-len (len - sizeof(struct rpmsg_hdr))) {
dev_warn(dev, inbound msg too big: (%d, %d)\n, len, msg-len);
- return;
+ return -EINVAL;
}
/* use the dst addr to fetch the callback of the appropriate user */
@@ -842,9 +831,39 @@ static void rpmsg_recv_done(struct virtqueue *rvq)
err = virtqueue_add_buf(vrp-rvq, sg, 0, 1, msg, GFP_KERNEL);
if (err 0) {
dev_err(dev, failed to add a virtqueue buffer: %d\n, err);
+ return err;
+ }
+
+ return 0;
+}
+
+/* called when an rx buffer is used, and it's time to digest a message */
+static void rpmsg_recv_done(struct virtqueue *rvq)
+{
+ struct virtproc_info *vrp = rvq-vdev-priv;
+ struct device *dev = rvq-vdev-dev;
+ struct rpmsg_hdr *msg;
+ unsigned int len, msgs_received = 0;
+ int err;
+
+ msg = virtqueue_get_buf(rvq, len);
+ if (!msg) {
+ dev_err(dev, uhm, incoming signal, but no used buffer ?\n);
return;
}
+ while (msg) {
+ err = rpmsg_recv_single(vrp, dev, msg, len);
+ if (err)
+ break;
+
+ msgs_received++;
+
+ msg = virtqueue_get_buf(rvq, len);
+ };
+
+ dev_dbg(dev, Received %u messages\n, msgs_received);
+
/* tell the remote processor we added another available rx buffer */
virtqueue_kick(vrp-rvq);
}
___
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
