Date:   Wed, 21 Dec 2022 17:12:22 +0100
From:   Arnaud POULIQUEN <arnaud.pouliquen@...s.st.com>
To:     Sarannya S <quic_sarannya@...cinc.com>,
        <quic_bjorande@...cinc.com>, <swboyd@...omium.org>,
        <quic_clew@...cinc.com>, <mathieu.poirier@...aro.org>
CC:     <linux-kernel@...r.kernel.org>, <linux-arm-msm@...r.kernel.org>,
        <linux-remoteproc@...r.kernel.org>,
        Deepak Kumar Singh <quic_deesin@...cinc.com>,
        Bjorn Andersson <andersson@...nel.org>
Subject: Re: [PATCH V4 1/3] rpmsg: core: Add signal API support

Hello,

On 12/7/22 14:04, Sarannya S wrote:
> 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@...cinc.com>
> Signed-off-by: Sarannya S <quic_sarannya@...cinc.com>
> ---
>  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 d6dde00e..77aeba0 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:	enable or disable serial flow control

What does it mean "enable and disable serial flow control"?
Do you speak about the flow control feature or the data flow itself?

I guess it is the activation/deactivation of the data stream
regarding Bjorn's comment in V1:

"I therefore asked Deepak to change it so the rpmsg api would contain a
single "pause incoming data"/"resume incoming data" - given that this is
a wish that we've seen in a number of discussions."

For me this is the software flow control:
https://en.wikipedia.org/wiki/Software_flow_control

I would suggest not limiting the control only to activation/deactivation but to
offer more flexibility in terms of services. replace the boolean by a bitmap
would allow to extend it later.

For instance by introducing 2 definitions:

/* RPMSG pause transmission request:
 * sent to the remote endpoint to request to suspend its transmission */
 */
#define RPMSG_FC_PT_REQ  (1 << 0)

/* RPMSG resume transmission request
 * sent to the remote endpoint to allow to resume its transmission
 */
#define RPMSG_FC_RT_REQ  (1 << 1)

Then we could add (in a next step) some other flow controls such as
/* RPMSG pause transmission information
 * Sent to the remote endpoint to inform that no more data will be sent
 * until the reception of RPMSG_FC_RT_INFO
 */
#define RPMSG_FC_PT_INFO  (1 << 16)
#define RPMSG_FC_RT_INFO  (1 << 16)

> + * @dst:	destination address of the endpoint

Thanks to have integrated this in your patch!

Regards,
Arnaud

> + *
> + * 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(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
>   */
>  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 */

Powered by blists - more mailing lists