Re: [PATCH v5 1/2] rpmsg: core: add API to get message length

From: Arnaud Pouliquen
Date: Wed Sep 04 2019 - 04:14:50 EST

Hi Suman

On 9/3/19 6:06 PM, Suman Anna wrote:
Hi Arnaud,

On 9/3/19 4:49 AM, Arnaud Pouliquen wrote:
hi Suman

On 8/29/19 12:34 AM, Suman Anna wrote:
Hi Arnaud,

On 8/28/19 10:19 AM, Arnaud Pouliquen wrote:
Return the rpmsg buffer size for sending message, so rpmsg users
can split a long message in several sub rpmsg buffers.

Thanks for the patch, I also have a need for the same to be able to
compute permissible payload size. Minor comments below.

Thanks for your review. i will update it ASAP. Then if you need it and
ack it, i suppose that we could request Bjorn to integrate it in a first
step, if the rpmsg tty driver has not a level of quality sufficient to
be accepted...

Yeah, this patch can always be merged independently ahead of the rpmsg
tty driver. Anyways, the tty patch will have to be picked up by a
separate maintainer right. So, it would be nice to get the revised
version get into 5.4

Sure, I plan to send a new version of the series today.
I would prefer not to split the series, just to simplify the review and the tests. if this patch is cherry-picked and integrated independently by Bjorn, I will simply sent a new version of the rpmsg tty driver without it.

Regards
Arnaud


regards
Suman


Regards
Arnaud


Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@xxxxxx>
---
V4 to V5 :
ÂÂ - rename rpmsg_get_buf_payload_size to rpmsg_get_mtu

 drivers/rpmsg/rpmsg_core.c | 21 +++++++++++++++++++++
 drivers/rpmsg/rpmsg_internal.h | 2 ++
 drivers/rpmsg/virtio_rpmsg_bus.c | 10 ++++++++++
 include/linux/rpmsg.h | 10 ++++++++++
 4 files changed, 43 insertions(+)

diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
index 8122807db380..daca2e24fc71 100644
--- a/drivers/rpmsg/rpmsg_core.c
+++ b/drivers/rpmsg/rpmsg_core.c
@@ -283,6 +283,27 @@ int rpmsg_trysend_offchannel(struct
rpmsg_endpoint *ept, u32 src, u32 dst,
 }
 EXPORT_SYMBOL(rpmsg_trysend_offchannel);
 +/**
+ * rpmsg_get_mtu() - get maximum transmission buffer size for
sending message.
+ * @ept: the rpmsg endpoint
+ *
+ * This function returns maximum buffer size available for a single
message.
+ *
+ * Return: the maximum transmission size on success and an
appropriate error
+ * value on failure.
+ */
+
+ssize_t rpmsg_get_mtu(struct rpmsg_endpoint *ept)
+{
+ÂÂÂ if (WARN_ON(!ept))
+ÂÂÂÂÂÂÂ return -EINVAL;
+ÂÂÂ if (!ept->ops->get_buf_mtu)

How about calling the ops just get_mtu or rename the function to follow
the ops name, like all the others.

+ÂÂÂÂÂÂÂ return -ENXIO;

Perhaps ENOTSUPP or EOPNOTSUPP.

+
+ÂÂÂ return ept->ops->get_buf_mtu(ept);
+}
+EXPORT_SYMBOL(rpmsg_get_mtu);
+
 /*
ÂÂ * match an rpmsg channel with a channel info struct.
ÂÂ * this is used to make sure we're not creating rpmsg devices for
channels
diff --git a/drivers/rpmsg/rpmsg_internal.h
b/drivers/rpmsg/rpmsg_internal.h
index 0d791c30b7ea..645c402569ac 100644
--- a/drivers/rpmsg/rpmsg_internal.h
+++ b/drivers/rpmsg/rpmsg_internal.h
@@ -46,6 +46,7 @@ struct rpmsg_device_ops {
ÂÂ * @trysend:ÂÂÂÂÂÂÂ see @rpmsg_trysend(), required
ÂÂ * @trysendto:ÂÂÂÂÂÂÂ see @rpmsg_trysendto(), optional
ÂÂ * @trysend_offchannel:ÂÂÂ see @rpmsg_trysend_offchannel(), optional
+ * @get_buf_payload_size: see @rpmsg_get_buf_payload_size(), optional

Missed updating the kerneldoc to the new name.

ÂÂ *
ÂÂ * Indirection table for the operations that a rpmsg backend should
implement.
ÂÂ * In addition to @destroy_ept, the backend must at least implement
@send and
@@ -65,6 +66,7 @@ struct rpmsg_endpoint_ops {
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ void *data, int len);
ÂÂÂÂÂ __poll_t (*poll)(struct rpmsg_endpoint *ept, struct file *filp,
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ poll_table *wait);
+ÂÂÂ ssize_t (*get_buf_mtu)(struct rpmsg_endpoint *ept);
 };
  int rpmsg_register_device(struct rpmsg_device *rpdev);
diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c
b/drivers/rpmsg/virtio_rpmsg_bus.c
index e757f0038a1c..f80b1ad23e7e 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -178,6 +178,7 @@ 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);
+static ssize_t virtio_get_buf_mtu(struct rpmsg_endpoint *ept);

Minor nit, virtio_rpmsg_ prefix similar to all the other ops.

regards
Suman

  static const struct rpmsg_endpoint_ops virtio_endpoint_ops = {
ÂÂÂÂÂ .destroy_ept = virtio_rpmsg_destroy_ept,
@@ -187,6 +188,7 @@ static const struct rpmsg_endpoint_ops
virtio_endpoint_ops = {
ÂÂÂÂÂ .trysend = virtio_rpmsg_trysend,
ÂÂÂÂÂ .trysendto = virtio_rpmsg_trysendto,
ÂÂÂÂÂ .trysend_offchannel = virtio_rpmsg_trysend_offchannel,
+ÂÂÂ .get_buf_mtu = virtio_get_buf_mtu,
 };
  /**
@@ -702,6 +704,14 @@ static int
virtio_rpmsg_trysend_offchannel(struct rpmsg_endpoint *ept, u32 src,
ÂÂÂÂÂ return rpmsg_send_offchannel_raw(rpdev, src, dst, data, len,
false);
 }
 +static ssize_t virtio_get_buf_mtu(struct rpmsg_endpoint *ept)
+{
+ÂÂÂ struct rpmsg_device *rpdev = ept->rpdev;
+ÂÂÂ struct virtio_rpmsg_channel *vch = to_virtio_rpmsg_channel(rpdev);
+
+ÂÂÂ return vch->vrp->buf_size - sizeof(struct rpmsg_hdr);
+}
+
 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 9fe156d1c018..9d638bf2bdce 100644
--- a/include/linux/rpmsg.h
+++ b/include/linux/rpmsg.h
@@ -135,6 +135,8 @@ int rpmsg_trysend_offchannel(struct
rpmsg_endpoint *ept, u32 src, u32 dst,
 __poll_t rpmsg_poll(struct rpmsg_endpoint *ept, struct file *filp,
ÂÂÂÂÂÂÂÂÂÂÂÂÂ poll_table *wait);
 +ssize_t rpmsg_get_mtu(struct rpmsg_endpoint *ept);
+
 #else
  static inline int register_rpmsg_device(struct rpmsg_device *dev)
@@ -242,6 +244,14 @@ static inline __poll_t rpmsg_poll(struct
rpmsg_endpoint *ept,
ÂÂÂÂÂ return 0;
 }
 +static ssize_t rpmsg_get_mtu(struct rpmsg_endpoint *ept)
+{
+ÂÂÂ /* This shouldn't be possible */
+ÂÂÂ WARN_ON(1);
+
+ÂÂÂ return -ENXIO;
+}
+
 #endif /* IS_ENABLED(CONFIG_RPMSG) */
  /* use a macro to avoid include chaining to get THIS_MODULE */