On 6/15/23 16:50, Bjorn Andersson wrote: > On Thu, Jun 15, 2023 at 11:01:14AM +0200, Arnaud POULIQUEN wrote: >> >> >> On 6/14/23 17:54, Bjorn Andersson wrote: >>> On Sat, Apr 22, 2023 at 04:12:05PM +0530, Sarannya S wrote: >>>> From: Deepak Kumar Singh <quic_deesin@xxxxxxxxxxx> >>>> >>>> Some transports like Glink support the state notifications between >>>> clients using flow control signals similar to serial protocol signals. >>>> Local glink client drivers can send and receive flow control status >>>> to glink clients running on remote processors. >>>> >>>> Add APIs to support sending and receiving of flow control status by >>>> rpmsg clients. >>>> >>>> Signed-off-by: Deepak Kumar Singh <quic_deesin@xxxxxxxxxxx> >>>> Signed-off-by: Sarannya S <quic_sarannya@xxxxxxxxxxx> >>>> --- >>>> drivers/rpmsg/rpmsg_core.c | 21 +++++++++++++++++++++ >>>> drivers/rpmsg/rpmsg_internal.h | 2 ++ >>>> include/linux/rpmsg.h | 15 +++++++++++++++ >>>> 3 files changed, 38 insertions(+) >>>> >>>> diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c >>>> index a2207c0..e8bbe05 100644 >>>> --- a/drivers/rpmsg/rpmsg_core.c >>>> +++ b/drivers/rpmsg/rpmsg_core.c >>>> @@ -331,6 +331,25 @@ int rpmsg_trysend_offchannel(struct rpmsg_endpoint *ept, u32 src, u32 dst, >>>> EXPORT_SYMBOL(rpmsg_trysend_offchannel); >>>> >>>> /** >>>> + * rpmsg_set_flow_control() - sets/clears serial flow control signals >>>> + * @ept: the rpmsg endpoint >>>> + * @enable: pause/resume incoming data flow >>> >>> As shown in the discussion, it's still not clear what true/false means. >>> Also, let's try to clarify that it's a request for the other side to do >>> something: >>> >>> * rpmsg_set_flow_control() - request remote to pause/resume transmission >>> * ... >>> * @enable: flow restricted >>> * ... >>> >>> >>> PS. There's a stray space at the end of the line. >> >> The notion of flow restricted seems to me also ambiguous. It does >> not specify if the stream is limited in term of bandwidth or stopped. >> >> What about using XON/XOFF as specified in software flow control[1] >> >> XOFF Pause transmission >> XON Resume transmission >> >> or simply pause/resume definitions >> > > I agree, that's still ambiguous. > > I was concerned about expressing it such that the reader would assume > that calling this means there will be no more data coming, but there > might be things in the queues etc. Expressing it in terms of the state > of transmission is clearer. > > > /* > * rpmsg_set_flow_control() - request remote to pause/resume transmission > ... > * @enable: Pause transmission > ... > */ > > Does that sound okay and clear to you? Much better! I still have a nitpicking point :) What about replacing @enable variable by @pause to align the variable with the usage? /* * rpmsg_set_flow_control() - request remote to pause/resume transmission ... * @pause: set to 1 to pause transmission, to 0 to resume the transmission ... */ Thanks, Arnaud > > Thanks, > Bjorn > >> [1]https://en.wikipedia.org/wiki/Software_flow_control >> >> Regards, >> Arnaud >> >>> >>>> + * @dst: destination address of the endpoint >>>> + * >>>> + * Return: 0 on success and an appropriate error value on failure. >>>> + */ >>>> +int rpmsg_set_flow_control(struct rpmsg_endpoint *ept, bool enable, u32 dst) >>>> +{ >>>> + if (WARN_ON(!ept)) >>>> + return -EINVAL; >>>> + if (!ept->ops->set_flow_control) >>>> + return -ENXIO; >>>> + >>>> + return ept->ops->set_flow_control(ept, enable, dst); >>>> +} >>>> +EXPORT_SYMBOL_GPL(rpmsg_set_flow_control); >>>> + >>>> +/** >>>> * rpmsg_get_mtu() - get maximum transmission buffer size for sending message. >>>> * @ept: the rpmsg endpoint >>>> * >>>> @@ -539,6 +558,8 @@ static int rpmsg_dev_probe(struct device *dev) >>>> >>>> rpdev->ept = ept; >>>> rpdev->src = ept->addr; >>>> + >>>> + ept->flow_cb = rpdrv->flowcontrol; >>>> } >>>> >>>> err = rpdrv->probe(rpdev); >>>> diff --git a/drivers/rpmsg/rpmsg_internal.h b/drivers/rpmsg/rpmsg_internal.h >>>> index 39b646d..b6efd3e 100644 >>>> --- a/drivers/rpmsg/rpmsg_internal.h >>>> +++ b/drivers/rpmsg/rpmsg_internal.h >>>> @@ -55,6 +55,7 @@ struct rpmsg_device_ops { >>>> * @trysendto: see @rpmsg_trysendto(), optional >>>> * @trysend_offchannel: see @rpmsg_trysend_offchannel(), optional >>>> * @poll: see @rpmsg_poll(), optional >>>> + * @set_flow_control: see @rpmsg_set_flow_control(), optional >>>> * @get_mtu: see @rpmsg_get_mtu(), optional >>>> * >>>> * Indirection table for the operations that a rpmsg backend should implement. >>>> @@ -75,6 +76,7 @@ struct rpmsg_endpoint_ops { >>>> void *data, int len); >>>> __poll_t (*poll)(struct rpmsg_endpoint *ept, struct file *filp, >>>> poll_table *wait); >>>> + int (*set_flow_control)(struct rpmsg_endpoint *ept, bool enable, u32 dst); >>>> ssize_t (*get_mtu)(struct rpmsg_endpoint *ept); >>>> }; >>>> >>>> diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h >>>> index 523c98b..a0e9d38 100644 >>>> --- a/include/linux/rpmsg.h >>>> +++ b/include/linux/rpmsg.h >>>> @@ -64,12 +64,14 @@ struct rpmsg_device { >>>> }; >>>> >>>> typedef int (*rpmsg_rx_cb_t)(struct rpmsg_device *, void *, int, void *, u32); >>>> +typedef int (*rpmsg_flowcontrol_cb_t)(struct rpmsg_device *, void *, bool); >>>> >>>> /** >>>> * struct rpmsg_endpoint - binds a local rpmsg address to its user >>>> * @rpdev: rpmsg channel device >>>> * @refcount: when this drops to zero, the ept is deallocated >>>> * @cb: rx callback handler >>>> + * @flow_cb: remote flow control callback handler >>>> * @cb_lock: must be taken before accessing/changing @cb >>>> * @addr: local rpmsg address >>>> * @priv: private data for the driver's use >>>> @@ -92,6 +94,7 @@ struct rpmsg_endpoint { >>>> struct rpmsg_device *rpdev; >>>> struct kref refcount; >>>> rpmsg_rx_cb_t cb; >>>> + rpmsg_flowcontrol_cb_t flow_cb; >>>> struct mutex cb_lock; >>>> u32 addr; >>>> void *priv; >>>> @@ -106,6 +109,7 @@ struct rpmsg_endpoint { >>>> * @probe: invoked when a matching rpmsg channel (i.e. device) is found >>>> * @remove: invoked when the rpmsg channel is removed >>>> * @callback: invoked when an inbound message is received on the channel >>>> + * @flowcontrol: invoked when remote side flow control status is received >>> >>> s/status/request/ >>> >>> Regards, >>> Bjorn >>> >>>> */ >>>> struct rpmsg_driver { >>>> struct device_driver drv; >>>> @@ -113,6 +117,7 @@ struct rpmsg_driver { >>>> int (*probe)(struct rpmsg_device *dev); >>>> void (*remove)(struct rpmsg_device *dev); >>>> int (*callback)(struct rpmsg_device *, void *, int, void *, u32); >>>> + int (*flowcontrol)(struct rpmsg_device *, void *, bool); >>>> }; >>>> >>>> static inline u16 rpmsg16_to_cpu(struct rpmsg_device *rpdev, __rpmsg16 val) >>>> @@ -192,6 +197,8 @@ __poll_t rpmsg_poll(struct rpmsg_endpoint *ept, struct file *filp, >>>> >>>> ssize_t rpmsg_get_mtu(struct rpmsg_endpoint *ept); >>>> >>>> +int rpmsg_set_flow_control(struct rpmsg_endpoint *ept, bool enable, u32 dst); >>>> + >>>> #else >>>> >>>> static inline int rpmsg_register_device_override(struct rpmsg_device *rpdev, >>>> @@ -316,6 +323,14 @@ static inline ssize_t rpmsg_get_mtu(struct rpmsg_endpoint *ept) >>>> return -ENXIO; >>>> } >>>> >>>> +static inline int rpmsg_set_flow_control(struct rpmsg_endpoint *ept, bool enable, u32 dst) >>>> +{ >>>> + /* 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 */ >>>> -- >>>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, >>>> a Linux Foundation Collaborative Project >>>>
