Linux-ARM-MSM Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 0/3] Add API to get rpmsg message max length
@ 2019-09-05 14:27 Arnaud Pouliquen
  2019-09-05 14:27 ` [PATCH 1/3] rpmsg: core: add API to get message length Arnaud Pouliquen
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Arnaud Pouliquen @ 2019-09-05 14:27 UTC (permalink / raw)
  To: Ohad Ben-Cohen, Bjorn Andersson, linux-kernel, linux-remoteproc,
	linux-arm-msm
  Cc: arnaud.pouliquen, Suman Anna, Fabien DESSENNE, linux-stm32

As introduction on the get_mtu api can impacts some rpmsg drivers,
i propose to discuss it separately.
The "rpmsg: core: add API to get message length" patch is extracted from
https://lkml.org/lkml/2019/9/4/556
In addition 2 patches implement the API for impacted rpmsg drivers.
The rpmsg tty client driver will be resent in a second step.

In this patchset the get_mpu is considered mandatory. The main reason is
that the rpmsg clients do not have access to the mtu information that is
platform dependent.

Notice that the GLINK and and SMD drivers have to be validated on target,
I don't have device to validate by myself...
Only a compilation check has been executed.

Arnaud Pouliquen (3):
  rpmsg: core: add API to get message length
  rpmsg: glink: implement get_mtu ops
  rpmsg: smd: implement get_mtu ops

 drivers/rpmsg/qcom_glink_native.c | 24 ++++++++++++++++++++++++
 drivers/rpmsg/qcom_smd.c          |  8 ++++++++
 drivers/rpmsg/rpmsg_core.c        | 21 +++++++++++++++++++++
 drivers/rpmsg/rpmsg_internal.h    |  2 ++
 drivers/rpmsg/virtio_rpmsg_bus.c  | 10 ++++++++++
 include/linux/rpmsg.h             | 10 ++++++++++
 6 files changed, 75 insertions(+)

-- 
2.7.4


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 1/3] rpmsg: core: add API to get message length
  2019-09-05 14:27 [PATCH 0/3] Add API to get rpmsg message max length Arnaud Pouliquen
@ 2019-09-05 14:27 ` Arnaud Pouliquen
  2019-09-05 14:42   ` Jeffrey Hugo
  2019-09-05 14:27 ` [PATCH 2/3] rpmsg: glink: implement get_mtu ops Arnaud Pouliquen
  2019-09-05 14:27 ` [PATCH 3/3] rpmsg: smd: " Arnaud Pouliquen
  2 siblings, 1 reply; 8+ messages in thread
From: Arnaud Pouliquen @ 2019-09-05 14:27 UTC (permalink / raw)
  To: Ohad Ben-Cohen, Bjorn Andersson, linux-kernel, linux-remoteproc,
	linux-arm-msm
  Cc: arnaud.pouliquen, Suman Anna, Fabien DESSENNE, linux-stm32

Return the rpmsg buffer size for sending message, so rpmsg users
can split a long message in several sub rpmsg buffers.

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
---
 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 e330ec4dfc33..a6ef54c4779a 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_mtu)
+		return -ENOTSUPP;
+
+	return ept->ops->get_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 3fc83cd50e98..12b9e72adc75 100644
--- a/drivers/rpmsg/rpmsg_internal.h
+++ b/drivers/rpmsg/rpmsg_internal.h
@@ -47,6 +47,7 @@ struct rpmsg_device_ops {
  * @trysendto:		see @rpmsg_trysendto(), optional
  * @trysend_offchannel:	see @rpmsg_trysend_offchannel(), optional
  * @poll:		see @rpmsg_poll(), optional
+ * @get_mtu:		see @get_mpu(), required
  *
  * Indirection table for the operations that a rpmsg backend should implement.
  * In addition to @destroy_ept, the backend must at least implement @send and
@@ -66,6 +67,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_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 376ebbf880d6..6e48fdf24555 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -175,6 +175,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_rpmsg_get_mtu(struct rpmsg_endpoint *ept);
 
 static const struct rpmsg_endpoint_ops virtio_endpoint_ops = {
 	.destroy_ept = virtio_rpmsg_destroy_ept,
@@ -184,6 +185,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_mtu = virtio_rpmsg_get_mtu,
 };
 
 /**
@@ -699,6 +701,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_rpmsg_get_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..88d7892ca93d 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 inline 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 */
-- 
2.7.4


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 2/3] rpmsg: glink: implement get_mtu ops
  2019-09-05 14:27 [PATCH 0/3] Add API to get rpmsg message max length Arnaud Pouliquen
  2019-09-05 14:27 ` [PATCH 1/3] rpmsg: core: add API to get message length Arnaud Pouliquen
@ 2019-09-05 14:27 ` Arnaud Pouliquen
  2019-09-05 14:27 ` [PATCH 3/3] rpmsg: smd: " Arnaud Pouliquen
  2 siblings, 0 replies; 8+ messages in thread
From: Arnaud Pouliquen @ 2019-09-05 14:27 UTC (permalink / raw)
  To: Ohad Ben-Cohen, Bjorn Andersson, linux-kernel, linux-remoteproc,
	linux-arm-msm
  Cc: arnaud.pouliquen, Suman Anna, Fabien DESSENNE, linux-stm32

Implement the get_mtu ops to return the maximum size of
the message that can be sent.

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
---
 drivers/rpmsg/qcom_glink_native.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
index 621f1afd4d6b..8a477416a38a 100644
--- a/drivers/rpmsg/qcom_glink_native.c
+++ b/drivers/rpmsg/qcom_glink_native.c
@@ -1312,6 +1312,29 @@ static int qcom_glink_trysend(struct rpmsg_endpoint *ept, void *data, int len)
 	return __qcom_glink_send(channel, data, len, false);
 }
 
+static ssize_t qcom_glink_get_mtu(struct rpmsg_endpoint *ept)
+{
+	struct glink_channel *channel = to_glink_channel(ept);
+	size_t mtu_size = 0;
+	struct qcom_glink *glink = channel->glink;
+	struct glink_core_rx_intent *tmp;
+	unsigned long flags;
+	int iid = 0;
+
+	if (!glink->intentless) {
+		spin_lock_irqsave(&channel->intent_lock, flags);
+		idr_for_each_entry(&channel->riids, tmp, iid) {
+			if (tmp->size > mtu_size && !tmp->in_use)
+				mtu_size = tmp->size;
+		}
+		spin_unlock_irqrestore(&channel->intent_lock, flags);
+
+		return mtu_size;
+	} else {
+		return qcom_glink_tx_avail(glink);
+	}
+}
+
 /*
  * Finds the device_node for the glink child interested in this channel.
  */
@@ -1345,6 +1368,7 @@ static const struct rpmsg_endpoint_ops glink_endpoint_ops = {
 	.destroy_ept = qcom_glink_destroy_ept,
 	.send = qcom_glink_send,
 	.trysend = qcom_glink_trysend,
+	.get_mtu = qcom_glink_get_mtu,
 };
 
 static void qcom_glink_rpdev_release(struct device *dev)
-- 
2.7.4


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 3/3] rpmsg: smd: implement get_mtu ops
  2019-09-05 14:27 [PATCH 0/3] Add API to get rpmsg message max length Arnaud Pouliquen
  2019-09-05 14:27 ` [PATCH 1/3] rpmsg: core: add API to get message length Arnaud Pouliquen
  2019-09-05 14:27 ` [PATCH 2/3] rpmsg: glink: implement get_mtu ops Arnaud Pouliquen
@ 2019-09-05 14:27 ` " Arnaud Pouliquen
  2 siblings, 0 replies; 8+ messages in thread
From: Arnaud Pouliquen @ 2019-09-05 14:27 UTC (permalink / raw)
  To: Ohad Ben-Cohen, Bjorn Andersson, linux-kernel, linux-remoteproc,
	linux-arm-msm
  Cc: arnaud.pouliquen, Suman Anna, Fabien DESSENNE, linux-stm32

Implement the get_mtu ops to return the maximum size of
the message that can be sent.

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
---
 drivers/rpmsg/qcom_smd.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/rpmsg/qcom_smd.c b/drivers/rpmsg/qcom_smd.c
index 4abbeea782fa..f233f8d85062 100644
--- a/drivers/rpmsg/qcom_smd.c
+++ b/drivers/rpmsg/qcom_smd.c
@@ -989,6 +989,13 @@ static __poll_t qcom_smd_poll(struct rpmsg_endpoint *ept,
 	return mask;
 }
 
+static ssize_t qcom_smd_get_mtu(struct rpmsg_endpoint *ept)
+{
+	struct qcom_smd_endpoint *qsept = to_smd_endpoint(ept);
+
+	return qcom_smd_get_tx_avail(qsept->qsch);
+}
+
 /*
  * Finds the device_node for the smd child interested in this channel.
  */
@@ -1040,6 +1047,7 @@ static const struct rpmsg_endpoint_ops qcom_smd_endpoint_ops = {
 	.send = qcom_smd_send,
 	.trysend = qcom_smd_trysend,
 	.poll = qcom_smd_poll,
+	.get_mtu = qcom_smd_get_mtu,
 };
 
 static void qcom_smd_release_device(struct device *dev)
-- 
2.7.4


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/3] rpmsg: core: add API to get message length
  2019-09-05 14:27 ` [PATCH 1/3] rpmsg: core: add API to get message length Arnaud Pouliquen
@ 2019-09-05 14:42   ` Jeffrey Hugo
  2019-09-05 16:02     ` Arnaud Pouliquen
  0 siblings, 1 reply; 8+ messages in thread
From: Jeffrey Hugo @ 2019-09-05 14:42 UTC (permalink / raw)
  To: Arnaud Pouliquen
  Cc: Ohad Ben-Cohen, Bjorn Andersson, lkml, linux-remoteproc, MSM,
	Suman Anna, Fabien DESSENNE, linux-stm32

On Thu, Sep 5, 2019 at 8:35 AM Arnaud Pouliquen <arnaud.pouliquen@st.com> wrote:
>
> Return the rpmsg buffer size for sending message, so rpmsg users
> can split a long message in several sub rpmsg buffers.
>
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
> ---
>  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 e330ec4dfc33..a6ef54c4779a 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.
> + */

What is the intent of this?

The term "mtu" is "maximum transfer unit" - ie the largest payload of
data that could possibly be sent, however at any one point in time,
that might not be able to be accommodated.

I don't think this is implemented correctly.  In GLINK and SMD, you've
not implemented MTU, you've implemented "how much can I send at this
point in time".  To me, this is not mtu.

In the case of SMD, you could get the fifo size and return that as the
mtu, but since you seem to be wanting to use this from the TTY layer
to determine how much can be sent at a particular point in time, I
don't think you actually want mtu.

For GLINK, I don't actually think you can get a mtu based on the
design, but I'm trying to remember from 5-6 years ago when we designed
it.  It would be possible that a larger intent would be made available
later.

I think you need to first determine if you are actually looking for
mtu, or "how much data can I send right now", because right now, it
isn't clear.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/3] rpmsg: core: add API to get message length
  2019-09-05 14:42   ` Jeffrey Hugo
@ 2019-09-05 16:02     ` Arnaud Pouliquen
  2019-09-05 16:18       ` Jeffrey Hugo
  0 siblings, 1 reply; 8+ messages in thread
From: Arnaud Pouliquen @ 2019-09-05 16:02 UTC (permalink / raw)
  To: Jeffrey Hugo
  Cc: Ohad Ben-Cohen, Bjorn Andersson, lkml, linux-remoteproc, MSM,
	Suman Anna, Fabien DESSENNE, linux-stm32

Hi Jeffrey,


On 9/5/19 4:42 PM, Jeffrey Hugo wrote:
> On Thu, Sep 5, 2019 at 8:35 AM Arnaud Pouliquen <arnaud.pouliquen@st.com> wrote:
>>
>> Return the rpmsg buffer size for sending message, so rpmsg users
>> can split a long message in several sub rpmsg buffers.
>>
>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
>> ---
>>   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 e330ec4dfc33..a6ef54c4779a 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.
>> + */
> 
> What is the intent of this?
> 
> The term "mtu" is "maximum transfer unit" - ie the largest payload of
> data that could possibly be sent, however at any one point in time,
> that might not be able to be accommodated.
I was not aware that the MTU has to be static in time. And I'm not 
enough expert to be able challenge this.
The use of the MTU initially came from a Bjorn request and IMHO makes 
sense in RPMSG protocol as other protocols. The aim here is not to 
guaranty the available size but to provide to rpmsg client a packet size 
information that is not available today at rpmsg client level.
For instance for the virtio rpmsg bus we provide the size of a vring 
buffer, not the total size available in the vring.

> 
> I don't think this is implemented correctly.  In GLINK and SMD, you've
> not implemented MTU, you've implemented "how much can I send at this
> point in time".  To me, this is not mtu.
If MTU has to be static i agree with you.
> 
> In the case of SMD, you could get the fifo size and return that as the
> mtu, but since you seem to be wanting to use this from the TTY layer
> to determine how much can be sent at a particular point in time, I
> don't think you actually want mtu.
Please forget the TTY for the moment, The mtu is used to help the tty 
framework to split the buffer to write. The size is then adjusted on write.
For SMD i can provide the fifo_size,or a division of this size to 
"limit" congestion.
would this make sense for you?
> 
> For GLINK, I don't actually think you can get a mtu based on the
> design, but I'm trying to remember from 5-6 years ago when we designed
> it.  It would be possible that a larger intent would be made available
> later.
Is it possible to have the largest intent? or it's not deterministic.
> 
> I think you need to first determine if you are actually looking for
> mtu, or "how much data can I send right now", because right now, it
> isn't clear.
> 
In my view it is the MTU. "how much data can I send right now" is an 
information that is very volatile as buffers can be shared between 
several clients, therefore unusable.

An alternative would be to make this ops optional, but that would mean
that some generic clients would not be compatible with SMD and/or Glink 
drivers.

Thanks,
Arnaud

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/3] rpmsg: core: add API to get message length
  2019-09-05 16:02     ` Arnaud Pouliquen
@ 2019-09-05 16:18       ` Jeffrey Hugo
  2019-09-10  9:56         ` Arnaud Pouliquen
  0 siblings, 1 reply; 8+ messages in thread
From: Jeffrey Hugo @ 2019-09-05 16:18 UTC (permalink / raw)
  To: Arnaud Pouliquen
  Cc: Ohad Ben-Cohen, Bjorn Andersson, lkml, linux-remoteproc, MSM,
	Suman Anna, Fabien DESSENNE, linux-stm32

On Thu, Sep 5, 2019 at 10:02 AM Arnaud Pouliquen
<arnaud.pouliquen@st.com> wrote:
>
> Hi Jeffrey,
>
>
> On 9/5/19 4:42 PM, Jeffrey Hugo wrote:
> > On Thu, Sep 5, 2019 at 8:35 AM Arnaud Pouliquen <arnaud.pouliquen@st.com> wrote:
> >>
> >> Return the rpmsg buffer size for sending message, so rpmsg users
> >> can split a long message in several sub rpmsg buffers.
> >>
> >> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
> >> ---
> >>   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 e330ec4dfc33..a6ef54c4779a 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.
> >> + */
> >
> > What is the intent of this?
> >
> > The term "mtu" is "maximum transfer unit" - ie the largest payload of
> > data that could possibly be sent, however at any one point in time,
> > that might not be able to be accommodated.
> I was not aware that the MTU has to be static in time. And I'm not
> enough expert to be able challenge this.
> The use of the MTU initially came from a Bjorn request and IMHO makes
> sense in RPMSG protocol as other protocols. The aim here is not to
> guaranty the available size but to provide to rpmsg client a packet size
> information that is not available today at rpmsg client level.
> For instance for the virtio rpmsg bus we provide the size of a vring
> buffer, not the total size available in the vring.
>
> >
> > I don't think this is implemented correctly.  In GLINK and SMD, you've
> > not implemented MTU, you've implemented "how much can I send at this
> > point in time".  To me, this is not mtu.
> If MTU has to be static i agree with you.
> >
> > In the case of SMD, you could get the fifo size and return that as the
> > mtu, but since you seem to be wanting to use this from the TTY layer
> > to determine how much can be sent at a particular point in time, I
> > don't think you actually want mtu.
> Please forget the TTY for the moment, The mtu is used to help the tty
> framework to split the buffer to write. The size is then adjusted on write.
> For SMD i can provide the fifo_size,or a division of this size to
> "limit" congestion.
> would this make sense for you?

Historically, TTY over SMD (I'm basing this on my experience with the
downstream code) has operated in a streaming fasion, where it attempts
to put as much data as will fit in the fifo at that point in time.  So
you would have a "write_size_avail" operation that returns the amount
of free space in the fifo, and then the TTY client would attempt to
write that amount of data into the fifo.

In sort, the fifo size is the maximum that could be put into the
transport, but at any one point in time, there may be data sitting in
the fifo that the remote end has not yet procesed, which would limit
the amount of data you could put in the fifo to fifo_size - size of
data currently sitting in the fifo.

SMD channels have dedicated fifos, and are assumed to be used for a
single client.  If the channel is muxed between multiple clients, and
you want to manage "congestion", that would need to be managed at a
layer above SMD.

> >
> > For GLINK, I don't actually think you can get a mtu based on the
> > design, but I'm trying to remember from 5-6 years ago when we designed
> > it.  It would be possible that a larger intent would be made available
> > later.
> Is it possible to have the largest intent? or it's not deterministic.

Not really.  I think GLINK defines a maximum size that it can handle
as an intent (something like uint32_max), however there is no
guarantee that any particular client will support that.  If you
attempt to have the MTU as the max that GLINK supports, and a client
never queues an intent that large, the data will never be able to be
transmitted.  The MTU is really based on the the whims of the remote
side, and I don't recall if there is a way in the GLINK protocol to
query that.  If I recall correctly, there is a way to request the
remote side queue an intent of a specific size, which the remote side
can either do (success) or reject the request (failure).

In my mind, there should be a valid scenario in which a client can
transmit data of a size equal to the MTU (although the client may need
to wait for that to happen), however I don't have a simple answer on
how to determine that value in a generic way for GLINK.

> >
> > I think you need to first determine if you are actually looking for
> > mtu, or "how much data can I send right now", because right now, it
> > isn't clear.
> >
> In my view it is the MTU. "how much data can I send right now" is an
> information that is very volatile as buffers can be shared between
> several clients, therefore unusable.

Thats valid.  If you want MTU, then I think you need to fix the
GLINK/SMD implementations since those are not providing the correct
information.  Unfortunately, GLINK is complicated.  I think Bjorn
should chime in on what he thinks would be valid behavior for GLINK.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/3] rpmsg: core: add API to get message length
  2019-09-05 16:18       ` Jeffrey Hugo
@ 2019-09-10  9:56         ` Arnaud Pouliquen
  0 siblings, 0 replies; 8+ messages in thread
From: Arnaud Pouliquen @ 2019-09-10  9:56 UTC (permalink / raw)
  To: Jeffrey Hugo, Bjorn Andersson
  Cc: Ohad Ben-Cohen, lkml, linux-remoteproc, MSM, Suman Anna,
	Fabien DESSENNE, linux-stm32



On 9/5/19 6:18 PM, Jeffrey Hugo wrote:
> On Thu, Sep 5, 2019 at 10:02 AM Arnaud Pouliquen
> <arnaud.pouliquen@st.com> wrote:
>>
>> Hi Jeffrey,
>>
>>
>> On 9/5/19 4:42 PM, Jeffrey Hugo wrote:
>>> On Thu, Sep 5, 2019 at 8:35 AM Arnaud Pouliquen <arnaud.pouliquen@st.com> wrote:
>>>>
>>>> Return the rpmsg buffer size for sending message, so rpmsg users
>>>> can split a long message in several sub rpmsg buffers.
>>>>
>>>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
>>>> ---
>>>>    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 e330ec4dfc33..a6ef54c4779a 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.
>>>> + */
>>>
>>> What is the intent of this?
>>>
>>> The term "mtu" is "maximum transfer unit" - ie the largest payload of
>>> data that could possibly be sent, however at any one point in time,
>>> that might not be able to be accommodated.
>> I was not aware that the MTU has to be static in time. And I'm not
>> enough expert to be able challenge this.
>> The use of the MTU initially came from a Bjorn request and IMHO makes
>> sense in RPMSG protocol as other protocols. The aim here is not to
>> guaranty the available size but to provide to rpmsg client a packet size
>> information that is not available today at rpmsg client level.
>> For instance for the virtio rpmsg bus we provide the size of a vring
>> buffer, not the total size available in the vring.
>>
>>>
>>> I don't think this is implemented correctly.  In GLINK and SMD, you've
>>> not implemented MTU, you've implemented "how much can I send at this
>>> point in time".  To me, this is not mtu.
>> If MTU has to be static i agree with you.
>>>
>>> In the case of SMD, you could get the fifo size and return that as the
>>> mtu, but since you seem to be wanting to use this from the TTY layer
>>> to determine how much can be sent at a particular point in time, I
>>> don't think you actually want mtu.
>> Please forget the TTY for the moment, The mtu is used to help the tty
>> framework to split the buffer to write. The size is then adjusted on write.
>> For SMD i can provide the fifo_size,or a division of this size to
>> "limit" congestion.
>> would this make sense for you?
> 
> Historically, TTY over SMD (I'm basing this on my experience with the
> downstream code) has operated in a streaming fasion, where it attempts
> to put as much data as will fit in the fifo at that point in time.  So
> you would have a "write_size_avail" operation that returns the amount
> of free space in the fifo, and then the TTY client would attempt to
> write that amount of data into the fifo.
> 
> In sort, the fifo size is the maximum that could be put into the
> transport, but at any one point in time, there may be data sitting in
> the fifo that the remote end has not yet procesed, which would limit
> the amount of data you could put in the fifo to fifo_size - size of
> data currently sitting in the fifo.
Regarding __qcom_smd_send function. if message length is higher than 
fifo_size EINVAL error is returned, else if send failed EAGAIN error is 
returned ( meaning that it is busy and that client has to retry to sent 
it later).
Based on this behavior, seems make sense to return channel->fifo_size
as MTU value.

> 
> SMD channels have dedicated fifos, and are assumed to be used for a
> single client.  If the channel is muxed between multiple clients, and
> you want to manage "congestion", that would need to be managed at a
> layer above SMD.
Yes this is the flow control that could have to be in core or splitted 
between the core and the platform driver. We had a first discussion with 
Bjorn few month ago on this subject, the need was identified.

>>>
>>> For GLINK, I don't actually think you can get a mtu based on the
>>> design, but I'm trying to remember from 5-6 years ago when we designed
>>> it.  It would be possible that a larger intent would be made available
>>> later.
>> Is it possible to have the largest intent? or it's not deterministic.
> 
> Not really.  I think GLINK defines a maximum size that it can handle
> as an intent (something like uint32_max), however there is no
> guarantee that any particular client will support that.  If you
> attempt to have the MTU as the max that GLINK supports, and a client
> never queues an intent that large, the data will never be able to be
> transmitted.  The MTU is really based on the the whims of the remote
> side, and I don't recall if there is a way in the GLINK protocol to
> query that.  If I recall correctly, there is a way to request the
> remote side queue an intent of a specific size, which the remote side
> can either do (success) or reject the request (failure).
> 
> In my mind, there should be a valid scenario in which a client can
> transmit data of a size equal to the MTU (although the client may need
> to wait for that to happen), however I don't have a simple answer on
> how to determine that value in a generic way for GLINK.

So no simple way for GLINK :(, anyway thanks for you time and your 
expertise.

> 
>>>
>>> I think you need to first determine if you are actually looking for
>>> mtu, or "how much data can I send right now", because right now, it
>>> isn't clear.
>>>
>> In my view it is the MTU. "how much data can I send right now" is an
>> information that is very volatile as buffers can be shared between
>> several clients, therefore unusable.
> 
> Thats valid.  If you want MTU, then I think you need to fix the
> GLINK/SMD implementations since those are not providing the correct
> information.  Unfortunately, GLINK is complicated.  I think Bjorn
> should chime in on what he thinks would be valid behavior for GLINK.
> 
An alternative could be a DT property that could be defined depending on 
the remote side constraint.

Bjorn,
please, could you share your view to help to find a solution to 
implement the MTU for Glink? or should we simply consider the MTU ops as 
optional and in this case how to determine a default size?

Thanks,
Arnaud



^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, back to index

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-05 14:27 [PATCH 0/3] Add API to get rpmsg message max length Arnaud Pouliquen
2019-09-05 14:27 ` [PATCH 1/3] rpmsg: core: add API to get message length Arnaud Pouliquen
2019-09-05 14:42   ` Jeffrey Hugo
2019-09-05 16:02     ` Arnaud Pouliquen
2019-09-05 16:18       ` Jeffrey Hugo
2019-09-10  9:56         ` Arnaud Pouliquen
2019-09-05 14:27 ` [PATCH 2/3] rpmsg: glink: implement get_mtu ops Arnaud Pouliquen
2019-09-05 14:27 ` [PATCH 3/3] rpmsg: smd: " Arnaud Pouliquen

Linux-ARM-MSM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-arm-msm/0 linux-arm-msm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-arm-msm linux-arm-msm/ https://lore.kernel.org/linux-arm-msm \
		linux-arm-msm@vger.kernel.org linux-arm-msm@archiver.kernel.org
	public-inbox-index linux-arm-msm


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-arm-msm


AGPL code for this site: git clone https://public-inbox.org/ public-inbox