[PATCH V2 0/3] rpmsg and glink signaling API support

[PATCH V2 0/3] rpmsg and glink signaling API support

[Deepak Kumar Singh]

[Change from V1]
Fixed most of the review comments in V1.

Deepak Kumar Singh (3):
  rpmsg: core: Add signal API support
  rpmsg: glink: Add support to handle signals command
  rpmsg: char: Add TIOCMGET/TIOCMSET ioctl support

 drivers/rpmsg/qcom_glink_native.c | 77 +++++++++++++++++++++++++++++++++++++++
 drivers/rpmsg/rpmsg_char.c        | 47 ++++++++++++++++++++++--
 drivers/rpmsg/rpmsg_core.c        | 21 +++++++++++
 drivers/rpmsg/rpmsg_internal.h    |  2 +
 include/linux/rpmsg.h             | 14 +++++++
 5 files changed, 157 insertions(+), 4 deletions(-)

-- 
2.7.4

[PATCH V2 1/3] rpmsg: core: Add signal API support

[Deepak Kumar Singh]

Some transports like Glink support the state notifications between
clients using signals similar to serial protocol signals.
Local glink client drivers can send and receive signals to glink
clients running on remote processors.

Add APIs to support sending and receiving of signals by rpmsg clients.

Signed-off-by: Deepak Kumar Singh <quic_deesin@quicinc.com>
---
 drivers/rpmsg/rpmsg_core.c     | 21 +++++++++++++++++++++
 drivers/rpmsg/rpmsg_internal.h |  2 ++
 include/linux/rpmsg.h          | 14 ++++++++++++++
 3 files changed, 37 insertions(+)

diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
index d3eb600..6712418 100644
--- a/drivers/rpmsg/rpmsg_core.c
+++ b/drivers/rpmsg/rpmsg_core.c
@@ -328,6 +328,24 @@ 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
+ *
+ * Return: 0 on success and an appropriate error value on failure.
+ */
+int rpmsg_set_flow_control(struct rpmsg_endpoint *ept, bool enable)
+{
+	if (WARN_ON(!ept))
+		return -EINVAL;
+	if (!ept->ops->set_flow_control)
+		return -ENXIO;
+
+	return ept->ops->set_flow_control(ept, enable);
+}
+EXPORT_SYMBOL(rpmsg_set_flow_control);
+
+/**
  * rpmsg_get_mtu() - get maximum transmission buffer size for sending message.
  * @ept: the rpmsg endpoint
  *
@@ -535,6 +553,9 @@ static int rpmsg_dev_probe(struct device *dev)
 
 		rpdev->ept = ept;
 		rpdev->src = ept->addr;
+
+		if (rpdrv->signals)
+			ept->sig_cb = rpdrv->signals;
 	}
 
 	err = rpdrv->probe(rpdev);
diff --git a/drivers/rpmsg/rpmsg_internal.h b/drivers/rpmsg/rpmsg_internal.h
index b1245d3..35c2197 100644
--- a/drivers/rpmsg/rpmsg_internal.h
+++ b/drivers/rpmsg/rpmsg_internal.h
@@ -53,6 +53,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.
@@ -73,6 +74,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);
 	ssize_t (*get_mtu)(struct rpmsg_endpoint *ept);
 };
 
diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h
index 02fa911..06d090c 100644
--- a/include/linux/rpmsg.h
+++ b/include/linux/rpmsg.h
@@ -62,12 +62,14 @@ struct rpmsg_device {
 };
 
 typedef int (*rpmsg_rx_cb_t)(struct rpmsg_device *, void *, int, void *, u32);
+typedef int (*rpmsg_rx_sig_t)(struct rpmsg_device *, void *, u32);
 
 /**
  * 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
+ * @sig_cb: rx serial signal handler
  * @cb_lock: must be taken before accessing/changing @cb
  * @addr: local rpmsg address
  * @priv: private data for the driver's use
@@ -90,6 +92,7 @@ struct rpmsg_endpoint {
 	struct rpmsg_device *rpdev;
 	struct kref refcount;
 	rpmsg_rx_cb_t cb;
+	rpmsg_rx_sig_t sig_cb;
 	struct mutex cb_lock;
 	u32 addr;
 	void *priv;
@@ -111,6 +114,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 (*signals)(struct rpmsg_device *rpdev, void *priv, u32);
 };
 
 static inline u16 rpmsg16_to_cpu(struct rpmsg_device *rpdev, __rpmsg16 val)
@@ -188,6 +192,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);
+
 #else
 
 static inline int rpmsg_register_device(struct rpmsg_device *rpdev)
@@ -306,6 +312,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)
+{
+	/* 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

[PATCH V2 2/3] rpmsg: glink: Add support to handle signals command

[Deepak Kumar Singh]

Remote peripherals send signal notifications over glink with commandID 15.

Add support to send and receive the signal command and convert the signals
from NATIVE to TIOCM while receiving and vice versa while sending.

Signed-off-by: Chris Lew <quic_clew@quicinc.com>
Signed-off-by: Deepak Kumar Singh <quic_deesin@quicinc.com>
---
 drivers/rpmsg/qcom_glink_native.c | 77 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 77 insertions(+)

diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
index 3f377a7..d673d65 100644
--- a/drivers/rpmsg/qcom_glink_native.c
+++ b/drivers/rpmsg/qcom_glink_native.c
@@ -17,6 +17,7 @@
 #include <linux/rpmsg.h>
 #include <linux/sizes.h>
 #include <linux/slab.h>
+#include <linux/termios.h>
 #include <linux/workqueue.h>
 #include <linux/mailbox_client.h>
 
@@ -205,9 +206,16 @@ static const struct rpmsg_endpoint_ops glink_endpoint_ops;
 #define RPM_CMD_TX_DATA_CONT		12
 #define RPM_CMD_READ_NOTIF		13
 #define RPM_CMD_RX_DONE_W_REUSE		14
+#define RPM_CMD_SIGNALS			15
 
 #define GLINK_FEATURE_INTENTLESS	BIT(1)
 
+#define NATIVE_DTR_SIG			BIT(31)
+#define NATIVE_CTS_SIG			BIT(30)
+#define NATIVE_CD_SIG			BIT(29)
+#define NATIVE_RI_SIG			BIT(28)
+#define	SIG_MASK			0x0fff;
+
 static void qcom_glink_rx_done_work(struct work_struct *work);
 
 static struct glink_channel *qcom_glink_alloc_channel(struct qcom_glink *glink,
@@ -1003,6 +1011,70 @@ static int qcom_glink_rx_open_ack(struct qcom_glink *glink, unsigned int lcid)
 	return 0;
 }
 
+/**
+ * qcom_glink_set_flow_control() - convert a signal cmd to wire format and
+ * 				   transmit
+ * @ept:	Rpmsg endpoint for channel.
+ * @enable:	True/False - enable or disable flow control
+ *
+ * Return: 0 on success or standard Linux error code.
+ */
+static int qcom_glink_set_flow_control(struct rpmsg_endpoint *ept, bool enable)
+{
+	struct glink_channel *channel = to_glink_channel(ept);
+	struct qcom_glink *glink = channel->glink;
+	struct glink_msg msg;
+	u32 sigs;
+
+	/**
+	 * convert signals from TIOCM to NATIVE
+	 * sigs = TIOCM_DTR|TIOCM_RTS
+	 */
+	if (enable)
+		sigs |= NATIVE_DTR_SIG | NATIVE_CTS_SIG;
+	else
+		sigs |= ~(NATIVE_DTR_SIG | NATIVE_CTS_SIG);
+
+	msg.cmd = cpu_to_le16(RPM_CMD_SIGNALS);
+	msg.param1 = cpu_to_le16(channel->lcid);
+	msg.param2 = cpu_to_le32(sigs);
+
+	return qcom_glink_tx(glink, &msg, sizeof(msg), NULL, 0, true);
+}
+
+static int qcom_glink_handle_signals(struct qcom_glink *glink,
+				     unsigned int rcid, unsigned int sigs)
+{
+	struct glink_channel *channel;
+	unsigned long flags;
+
+	spin_lock_irqsave(&glink->idr_lock, flags);
+	channel = idr_find(&glink->rcids, rcid);
+	spin_unlock_irqrestore(&glink->idr_lock, flags);
+	if (!channel) {
+		dev_err(glink->dev, "signal for non-existing channel\n");
+		return -EINVAL;
+	}
+
+	if (!channel->ept.sig_cb)
+		return 0;
+
+	/* convert signals from NATIVE to TIOCM */
+	if (sigs & NATIVE_DTR_SIG)
+		sigs |= TIOCM_DSR;
+	if (sigs & NATIVE_CTS_SIG)
+		sigs |= TIOCM_CTS;
+	if (sigs & NATIVE_CD_SIG)
+		sigs |= TIOCM_CD;
+	if (sigs & NATIVE_RI_SIG)
+		sigs |= TIOCM_RI;
+	sigs &= SIG_MASK;
+
+	channel->ept.sig_cb(channel->ept.rpdev, channel->ept.priv, sigs);
+
+	return 0;
+}
+
 static irqreturn_t qcom_glink_native_intr(int irq, void *data)
 {
 	struct qcom_glink *glink = data;
@@ -1067,6 +1139,10 @@ static irqreturn_t qcom_glink_native_intr(int irq, void *data)
 			qcom_glink_handle_intent_req_ack(glink, param1, param2);
 			qcom_glink_rx_advance(glink, ALIGN(sizeof(msg), 8));
 			break;
+		case RPM_CMD_SIGNALS:
+			qcom_glink_handle_signals(glink, param1, param2);
+			qcom_glink_rx_advance(glink, ALIGN(sizeof(msg), 8));
+			break;
 		default:
 			dev_err(glink->dev, "unhandled rx cmd: %d\n", cmd);
 			ret = -EINVAL;
@@ -1442,6 +1518,7 @@ static const struct rpmsg_endpoint_ops glink_endpoint_ops = {
 	.sendto = qcom_glink_sendto,
 	.trysend = qcom_glink_trysend,
 	.trysendto = qcom_glink_trysendto,
+	.set_flow_control = qcom_glink_set_flow_control,
 };
 
 static void qcom_glink_rpdev_release(struct device *dev)
-- 
2.7.4

[PATCH V2 3/3] rpmsg: char: Add TIOCMGET/TIOCMSET ioctl support

[Deepak Kumar Singh]

Add TICOMGET and TIOCMSET ioctl support for rpmsg char device nodes
to get/set the low level transport signals.

Signed-off-by: Chris Lew <quic_clew@quicinc.com>
Signed-off-by: Deepak Kumar Singh <quic_deesin@quicinc.com>
---
 drivers/rpmsg/rpmsg_char.c | 47 ++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 43 insertions(+), 4 deletions(-)

diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
index b5907b8..c03a118 100644
--- a/drivers/rpmsg/rpmsg_char.c
+++ b/drivers/rpmsg/rpmsg_char.c
@@ -19,6 +19,7 @@
 #include <linux/rpmsg.h>
 #include <linux/skbuff.h>
 #include <linux/slab.h>
+#include <linux/termios.h>
 #include <linux/uaccess.h>
 #include <uapi/linux/rpmsg.h>
 
@@ -74,6 +75,9 @@ struct rpmsg_eptdev {
 	spinlock_t queue_lock;
 	struct sk_buff_head queue;
 	wait_queue_head_t readq;
+
+	u32 rsigs;
+	bool sig_pending;
 };
 
 static int rpmsg_eptdev_destroy(struct device *dev, void *data)
@@ -112,7 +116,18 @@ static int rpmsg_ept_cb(struct rpmsg_device *rpdev, void *buf, int len,
 	skb_queue_tail(&eptdev->queue, skb);
 	spin_unlock(&eptdev->queue_lock);
 
-	/* wake up any blocking processes, waiting for new data */
+	wake_up_interruptible(&eptdev->readq);
+
+	return 0;
+}
+
+static int rpmsg_sigs_cb(struct rpmsg_device *rpdev, void *priv, u32 sigs)
+{
+	struct rpmsg_eptdev *eptdev = priv;
+
+	eptdev->rsigs = sigs;
+	eptdev->sig_pending = true;
+
 	wake_up_interruptible(&eptdev->readq);
 
 	return 0;
@@ -137,6 +152,7 @@ static int rpmsg_eptdev_open(struct inode *inode, struct file *filp)
 		return -EINVAL;
 	}
 
+	ept->sig_cb = rpmsg_sigs_cb;
 	eptdev->ept = ept;
 	filp->private_data = eptdev;
 
@@ -155,6 +171,7 @@ static int rpmsg_eptdev_release(struct inode *inode, struct file *filp)
 		eptdev->ept = NULL;
 	}
 	mutex_unlock(&eptdev->ept_lock);
+	eptdev->sig_pending = false;
 
 	/* Discard all SKBs */
 	skb_queue_purge(&eptdev->queue);
@@ -265,6 +282,9 @@ static __poll_t rpmsg_eptdev_poll(struct file *filp, poll_table *wait)
 	if (!skb_queue_empty(&eptdev->queue))
 		mask |= EPOLLIN | EPOLLRDNORM;
 
+	if (eptdev->sig_pending)
+		mask |= EPOLLPRI;
+
 	mask |= rpmsg_poll(eptdev->ept, filp, wait);
 
 	return mask;
@@ -274,11 +294,30 @@ static long rpmsg_eptdev_ioctl(struct file *fp, unsigned int cmd,
 			       unsigned long arg)
 {
 	struct rpmsg_eptdev *eptdev = fp->private_data;
+	bool set;
+	u32 val;
+	int ret;
 
-	if (cmd != RPMSG_DESTROY_EPT_IOCTL)
-		return -EINVAL;
+	switch (cmd) {
+	case TIOCMGET:
+		eptdev->sig_pending = false;
+		ret = put_user(eptdev->rsigs, (int __user *)arg);
+		break;
+	case TIOCMSET:
+		ret = get_user(val, (int __user *)arg);
+		if (ret)
+			break;
+		set = (val & TIOCM_DTR) ? true : false;
+		ret = rpmsg_set_flow_control(eptdev->ept, set);
+		break;
+	case RPMSG_DESTROY_EPT_IOCTL:
+		ret = rpmsg_eptdev_destroy(&eptdev->dev, NULL);
+		break;
+	default:
+		ret = -EINVAL;
+	}
 
-	return rpmsg_eptdev_destroy(&eptdev->dev, NULL);
+	return ret;
 }
 
 static const struct file_operations rpmsg_eptdev_fops = {
-- 
2.7.4

Re: [PATCH V2 2/3] rpmsg: glink: Add support to handle signals command

[Bjorn Andersson]

On Tue 18 Jan 13:43 CST 2022, Deepak Kumar Singh wrote:

> Remote peripherals send signal notifications over glink with commandID 15.
> 
> Add support to send and receive the signal command and convert the signals
> from NATIVE to TIOCM while receiving and vice versa while sending.
> 
> Signed-off-by: Chris Lew <quic_clew@quicinc.com>

Co-developed-by: seems appropriate here, or you need to ensure the
author remains Chris, as his S-o-b comes first.

> Signed-off-by: Deepak Kumar Singh <quic_deesin@quicinc.com>
> ---
>  drivers/rpmsg/qcom_glink_native.c | 77 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 77 insertions(+)
> 
> diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
> index 3f377a7..d673d65 100644
> --- a/drivers/rpmsg/qcom_glink_native.c
> +++ b/drivers/rpmsg/qcom_glink_native.c
> @@ -17,6 +17,7 @@
>  #include <linux/rpmsg.h>
>  #include <linux/sizes.h>
>  #include <linux/slab.h>
> +#include <linux/termios.h>
>  #include <linux/workqueue.h>
>  #include <linux/mailbox_client.h>
>  
> @@ -205,9 +206,16 @@ static const struct rpmsg_endpoint_ops glink_endpoint_ops;
>  #define RPM_CMD_TX_DATA_CONT		12
>  #define RPM_CMD_READ_NOTIF		13
>  #define RPM_CMD_RX_DONE_W_REUSE		14
> +#define RPM_CMD_SIGNALS			15
>  
>  #define GLINK_FEATURE_INTENTLESS	BIT(1)
>  
> +#define NATIVE_DTR_SIG			BIT(31)

Seems reasonable to prefix these with GLINK_, perhaps GLINK_SIGNAL_DTR?

> +#define NATIVE_CTS_SIG			BIT(30)
> +#define NATIVE_CD_SIG			BIT(29)
> +#define NATIVE_RI_SIG			BIT(28)
> +#define	SIG_MASK			0x0fff;
> +
>  static void qcom_glink_rx_done_work(struct work_struct *work);
>  
>  static struct glink_channel *qcom_glink_alloc_channel(struct qcom_glink *glink,
> @@ -1003,6 +1011,70 @@ static int qcom_glink_rx_open_ack(struct qcom_glink *glink, unsigned int lcid)
>  	return 0;
>  }
>  
> +/**
> + * qcom_glink_set_flow_control() - convert a signal cmd to wire format and
> + * 				   transmit
> + * @ept:	Rpmsg endpoint for channel.
> + * @enable:	True/False - enable or disable flow control

"enable flow control" sounds sufficient (i.e. no need for True/False)
part.

Regards,
Bjorn

> + *
> + * Return: 0 on success or standard Linux error code.
> + */
> +static int qcom_glink_set_flow_control(struct rpmsg_endpoint *ept, bool enable)
> +{
> +	struct glink_channel *channel = to_glink_channel(ept);
> +	struct qcom_glink *glink = channel->glink;
> +	struct glink_msg msg;
> +	u32 sigs;
> +
> +	/**
> +	 * convert signals from TIOCM to NATIVE
> +	 * sigs = TIOCM_DTR|TIOCM_RTS
> +	 */
> +	if (enable)
> +		sigs |= NATIVE_DTR_SIG | NATIVE_CTS_SIG;
> +	else
> +		sigs |= ~(NATIVE_DTR_SIG | NATIVE_CTS_SIG);
> +
> +	msg.cmd = cpu_to_le16(RPM_CMD_SIGNALS);
> +	msg.param1 = cpu_to_le16(channel->lcid);
> +	msg.param2 = cpu_to_le32(sigs);
> +
> +	return qcom_glink_tx(glink, &msg, sizeof(msg), NULL, 0, true);
> +}
> +
> +static int qcom_glink_handle_signals(struct qcom_glink *glink,
> +				     unsigned int rcid, unsigned int sigs)
> +{
> +	struct glink_channel *channel;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&glink->idr_lock, flags);
> +	channel = idr_find(&glink->rcids, rcid);
> +	spin_unlock_irqrestore(&glink->idr_lock, flags);
> +	if (!channel) {
> +		dev_err(glink->dev, "signal for non-existing channel\n");
> +		return -EINVAL;
> +	}
> +
> +	if (!channel->ept.sig_cb)
> +		return 0;
> +
> +	/* convert signals from NATIVE to TIOCM */
> +	if (sigs & NATIVE_DTR_SIG)
> +		sigs |= TIOCM_DSR;
> +	if (sigs & NATIVE_CTS_SIG)
> +		sigs |= TIOCM_CTS;
> +	if (sigs & NATIVE_CD_SIG)
> +		sigs |= TIOCM_CD;
> +	if (sigs & NATIVE_RI_SIG)
> +		sigs |= TIOCM_RI;
> +	sigs &= SIG_MASK;
> +
> +	channel->ept.sig_cb(channel->ept.rpdev, channel->ept.priv, sigs);
> +
> +	return 0;
> +}
> +
>  static irqreturn_t qcom_glink_native_intr(int irq, void *data)
>  {
>  	struct qcom_glink *glink = data;
> @@ -1067,6 +1139,10 @@ static irqreturn_t qcom_glink_native_intr(int irq, void *data)
>  			qcom_glink_handle_intent_req_ack(glink, param1, param2);
>  			qcom_glink_rx_advance(glink, ALIGN(sizeof(msg), 8));
>  			break;
> +		case RPM_CMD_SIGNALS:
> +			qcom_glink_handle_signals(glink, param1, param2);
> +			qcom_glink_rx_advance(glink, ALIGN(sizeof(msg), 8));
> +			break;
>  		default:
>  			dev_err(glink->dev, "unhandled rx cmd: %d\n", cmd);
>  			ret = -EINVAL;
> @@ -1442,6 +1518,7 @@ static const struct rpmsg_endpoint_ops glink_endpoint_ops = {
>  	.sendto = qcom_glink_sendto,
>  	.trysend = qcom_glink_trysend,
>  	.trysendto = qcom_glink_trysendto,
> +	.set_flow_control = qcom_glink_set_flow_control,
>  };
>  
>  static void qcom_glink_rpdev_release(struct device *dev)
> -- 
> 2.7.4
> 

Re: [PATCH V2 1/3] rpmsg: core: Add signal API support

[Bjorn Andersson]

On Tue 18 Jan 13:43 CST 2022, Deepak Kumar Singh wrote:

> Some transports like Glink support the state notifications between
> clients using signals similar to serial protocol signals.
> Local glink client drivers can send and receive signals to glink
> clients running on remote processors.
> 
> Add APIs to support sending and receiving of signals by rpmsg clients.
> 
> Signed-off-by: Deepak Kumar Singh <quic_deesin@quicinc.com>
> ---
>  drivers/rpmsg/rpmsg_core.c     | 21 +++++++++++++++++++++
>  drivers/rpmsg/rpmsg_internal.h |  2 ++
>  include/linux/rpmsg.h          | 14 ++++++++++++++
>  3 files changed, 37 insertions(+)
> 
> diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
> index d3eb600..6712418 100644
> --- a/drivers/rpmsg/rpmsg_core.c
> +++ b/drivers/rpmsg/rpmsg_core.c
> @@ -328,6 +328,24 @@ 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
> + *
> + * Return: 0 on success and an appropriate error value on failure.
> + */
> +int rpmsg_set_flow_control(struct rpmsg_endpoint *ept, bool enable)

This API looks nice and clean and deals with flow control.

> +{
> +	if (WARN_ON(!ept))
> +		return -EINVAL;
> +	if (!ept->ops->set_flow_control)
> +		return -ENXIO;
> +
> +	return ept->ops->set_flow_control(ept, enable);
> +}
> +EXPORT_SYMBOL(rpmsg_set_flow_control);
> +
> +/**
>   * rpmsg_get_mtu() - get maximum transmission buffer size for sending message.
>   * @ept: the rpmsg endpoint
>   *
> @@ -535,6 +553,9 @@ static int rpmsg_dev_probe(struct device *dev)
>  
>  		rpdev->ept = ept;
>  		rpdev->src = ept->addr;
> +
> +		if (rpdrv->signals)
> +			ept->sig_cb = rpdrv->signals;
>  	}
>  
>  	err = rpdrv->probe(rpdev);
> diff --git a/drivers/rpmsg/rpmsg_internal.h b/drivers/rpmsg/rpmsg_internal.h
> index b1245d3..35c2197 100644
> --- a/drivers/rpmsg/rpmsg_internal.h
> +++ b/drivers/rpmsg/rpmsg_internal.h
> @@ -53,6 +53,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.
> @@ -73,6 +74,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);
>  	ssize_t (*get_mtu)(struct rpmsg_endpoint *ept);
>  };
>  
> diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h
> index 02fa911..06d090c 100644
> --- a/include/linux/rpmsg.h
> +++ b/include/linux/rpmsg.h
> @@ -62,12 +62,14 @@ struct rpmsg_device {
>  };
>  
>  typedef int (*rpmsg_rx_cb_t)(struct rpmsg_device *, void *, int, void *, u32);
> +typedef int (*rpmsg_rx_sig_t)(struct rpmsg_device *, void *, u32);

This callback however, is still using the original low level tty
signals.

Is there any reason why this can't be "rpmsg_flowcontrol_cb_t" and take
a boolean, so we get a clean interface in both directions?

Regards,
Bjorn

>  
>  /**
>   * 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
> + * @sig_cb: rx serial signal handler
>   * @cb_lock: must be taken before accessing/changing @cb
>   * @addr: local rpmsg address
>   * @priv: private data for the driver's use
> @@ -90,6 +92,7 @@ struct rpmsg_endpoint {
>  	struct rpmsg_device *rpdev;
>  	struct kref refcount;
>  	rpmsg_rx_cb_t cb;
> +	rpmsg_rx_sig_t sig_cb;
>  	struct mutex cb_lock;
>  	u32 addr;
>  	void *priv;
> @@ -111,6 +114,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 (*signals)(struct rpmsg_device *rpdev, void *priv, u32);
>  };
>  
>  static inline u16 rpmsg16_to_cpu(struct rpmsg_device *rpdev, __rpmsg16 val)
> @@ -188,6 +192,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);
> +
>  #else
>  
>  static inline int rpmsg_register_device(struct rpmsg_device *rpdev)
> @@ -306,6 +312,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)
> +{
> +	/* 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
> 

Re: [PATCH V2 2/3] rpmsg: glink: Add support to handle signals command

[Deepak Kumar Singh]

On 3/12/2022 2:39 AM, Bjorn Andersson wrote:
> On Tue 18 Jan 13:43 CST 2022, Deepak Kumar Singh wrote:
>
>> Remote peripherals send signal notifications over glink with commandID 15.
>>
>> Add support to send and receive the signal command and convert the signals
>> from NATIVE to TIOCM while receiving and vice versa while sending.
>>
>> Signed-off-by: Chris Lew <quic_clew@quicinc.com>
> Co-developed-by: seems appropriate here, or you need to ensure the
> author remains Chris, as his S-o-b comes first.
>
>> Signed-off-by: Deepak Kumar Singh <quic_deesin@quicinc.com>
>> ---
>>   drivers/rpmsg/qcom_glink_native.c | 77 +++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 77 insertions(+)
>>
>> diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
>> index 3f377a7..d673d65 100644
>> --- a/drivers/rpmsg/qcom_glink_native.c
>> +++ b/drivers/rpmsg/qcom_glink_native.c
>> @@ -17,6 +17,7 @@
>>   #include <linux/rpmsg.h>
>>   #include <linux/sizes.h>
>>   #include <linux/slab.h>
>> +#include <linux/termios.h>
>>   #include <linux/workqueue.h>
>>   #include <linux/mailbox_client.h>
>>   
>> @@ -205,9 +206,16 @@ static const struct rpmsg_endpoint_ops glink_endpoint_ops;
>>   #define RPM_CMD_TX_DATA_CONT		12
>>   #define RPM_CMD_READ_NOTIF		13
>>   #define RPM_CMD_RX_DONE_W_REUSE		14
>> +#define RPM_CMD_SIGNALS			15
>>   
>>   #define GLINK_FEATURE_INTENTLESS	BIT(1)
>>   
>> +#define NATIVE_DTR_SIG			BIT(31)
> Seems reasonable to prefix these with GLINK_, perhaps GLINK_SIGNAL_DTR?
>
>> +#define NATIVE_CTS_SIG			BIT(30)
>> +#define NATIVE_CD_SIG			BIT(29)
>> +#define NATIVE_RI_SIG			BIT(28)
>> +#define	SIG_MASK			0x0fff;
>> +
>>   static void qcom_glink_rx_done_work(struct work_struct *work);
>>   
>>   static struct glink_channel *qcom_glink_alloc_channel(struct qcom_glink *glink,
>> @@ -1003,6 +1011,70 @@ static int qcom_glink_rx_open_ack(struct qcom_glink *glink, unsigned int lcid)
>>   	return 0;
>>   }
>>   
>> +/**
>> + * qcom_glink_set_flow_control() - convert a signal cmd to wire format and
>> + * 				   transmit
>> + * @ept:	Rpmsg endpoint for channel.
>> + * @enable:	True/False - enable or disable flow control
> "enable flow control" sounds sufficient (i.e. no need for True/False)
> part.
>
> Regards,
> Bjorn

There are some user space clients which require both flow control on and 
off (DTR high/low).

So i guess true and false both are needed.

>> + *
>> + * Return: 0 on success or standard Linux error code.
>> + */
>> +static int qcom_glink_set_flow_control(struct rpmsg_endpoint *ept, bool enable)
>> +{
>> +	struct glink_channel *channel = to_glink_channel(ept);
>> +	struct qcom_glink *glink = channel->glink;
>> +	struct glink_msg msg;
>> +	u32 sigs;
>> +
>> +	/**
>> +	 * convert signals from TIOCM to NATIVE
>> +	 * sigs = TIOCM_DTR|TIOCM_RTS
>> +	 */
>> +	if (enable)
>> +		sigs |= NATIVE_DTR_SIG | NATIVE_CTS_SIG;
>> +	else
>> +		sigs |= ~(NATIVE_DTR_SIG | NATIVE_CTS_SIG);
>> +
>> +	msg.cmd = cpu_to_le16(RPM_CMD_SIGNALS);
>> +	msg.param1 = cpu_to_le16(channel->lcid);
>> +	msg.param2 = cpu_to_le32(sigs);
>> +
>> +	return qcom_glink_tx(glink, &msg, sizeof(msg), NULL, 0, true);
>> +}
>> +
>> +static int qcom_glink_handle_signals(struct qcom_glink *glink,
>> +				     unsigned int rcid, unsigned int sigs)
>> +{
>> +	struct glink_channel *channel;
>> +	unsigned long flags;
>> +
>> +	spin_lock_irqsave(&glink->idr_lock, flags);
>> +	channel = idr_find(&glink->rcids, rcid);
>> +	spin_unlock_irqrestore(&glink->idr_lock, flags);
>> +	if (!channel) {
>> +		dev_err(glink->dev, "signal for non-existing channel\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (!channel->ept.sig_cb)
>> +		return 0;
>> +
>> +	/* convert signals from NATIVE to TIOCM */
>> +	if (sigs & NATIVE_DTR_SIG)
>> +		sigs |= TIOCM_DSR;
>> +	if (sigs & NATIVE_CTS_SIG)
>> +		sigs |= TIOCM_CTS;
>> +	if (sigs & NATIVE_CD_SIG)
>> +		sigs |= TIOCM_CD;
>> +	if (sigs & NATIVE_RI_SIG)
>> +		sigs |= TIOCM_RI;
>> +	sigs &= SIG_MASK;
>> +
>> +	channel->ept.sig_cb(channel->ept.rpdev, channel->ept.priv, sigs);
>> +
>> +	return 0;
>> +}
>> +
>>   static irqreturn_t qcom_glink_native_intr(int irq, void *data)
>>   {
>>   	struct qcom_glink *glink = data;
>> @@ -1067,6 +1139,10 @@ static irqreturn_t qcom_glink_native_intr(int irq, void *data)
>>   			qcom_glink_handle_intent_req_ack(glink, param1, param2);
>>   			qcom_glink_rx_advance(glink, ALIGN(sizeof(msg), 8));
>>   			break;
>> +		case RPM_CMD_SIGNALS:
>> +			qcom_glink_handle_signals(glink, param1, param2);
>> +			qcom_glink_rx_advance(glink, ALIGN(sizeof(msg), 8));
>> +			break;
>>   		default:
>>   			dev_err(glink->dev, "unhandled rx cmd: %d\n", cmd);
>>   			ret = -EINVAL;
>> @@ -1442,6 +1518,7 @@ static const struct rpmsg_endpoint_ops glink_endpoint_ops = {
>>   	.sendto = qcom_glink_sendto,
>>   	.trysend = qcom_glink_trysend,
>>   	.trysendto = qcom_glink_trysendto,
>> +	.set_flow_control = qcom_glink_set_flow_control,
>>   };
>>   
>>   static void qcom_glink_rpdev_release(struct device *dev)
>> -- 
>> 2.7.4
>>

Re: [PATCH V2 0/3] rpmsg and glink signaling API support

[Arnaud POULIQUEN]

Hi all,

On 1/18/22 20:43, Deepak Kumar Singh wrote:
> [Change from V1]
> Fixed most of the review comments in V1.

This implementation works for the glink transport,
But how to manage such flow control for other transport
layer?
From my POV it is important that it is also usable for
by transport backends which doe not have such signaling.
The idea here is not to implement in other backends yet, but
at least to determine how it could be handled to avoid that
tomorrow this has to be reworked. 

More than that I wonder if the flow control could also be used
to solve the RPmsg protocol issue related to the channel
announcement [1][2]

[1] https://github.com/OpenAMP/open-amp/pull/160
[2] https://lore.kernel.org/lkml/20220316153001.662422-1-arnaud.pouliquen@foss.st.com/

Thanks,
Arnaud

> 
> Deepak Kumar Singh (3):
>   rpmsg: core: Add signal API support
>   rpmsg: glink: Add support to handle signals command
>   rpmsg: char: Add TIOCMGET/TIOCMSET ioctl support
> 
>  drivers/rpmsg/qcom_glink_native.c | 77 +++++++++++++++++++++++++++++++++++++++
>  drivers/rpmsg/rpmsg_char.c        | 47 ++++++++++++++++++++++--
>  drivers/rpmsg/rpmsg_core.c        | 21 +++++++++++
>  drivers/rpmsg/rpmsg_internal.h    |  2 +
>  include/linux/rpmsg.h             | 14 +++++++
>  5 files changed, 157 insertions(+), 4 deletions(-)
> 

Re: [PATCH V2 1/3] rpmsg: core: Add signal API support

[Arnaud POULIQUEN]

On 3/11/22 22:11, Bjorn Andersson wrote:
> On Tue 18 Jan 13:43 CST 2022, Deepak Kumar Singh wrote:
> 
>> Some transports like Glink support the state notifications between
>> clients using signals similar to serial protocol signals.
>> Local glink client drivers can send and receive signals to glink
>> clients running on remote processors.
>>
>> Add APIs to support sending and receiving of signals by rpmsg clients.
>>
>> Signed-off-by: Deepak Kumar Singh <quic_deesin@quicinc.com>
>> ---
>>  drivers/rpmsg/rpmsg_core.c     | 21 +++++++++++++++++++++
>>  drivers/rpmsg/rpmsg_internal.h |  2 ++
>>  include/linux/rpmsg.h          | 14 ++++++++++++++
>>  3 files changed, 37 insertions(+)
>>
>> diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
>> index d3eb600..6712418 100644
>> --- a/drivers/rpmsg/rpmsg_core.c
>> +++ b/drivers/rpmsg/rpmsg_core.c
>> @@ -328,6 +328,24 @@ 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
>> + *
>> + * Return: 0 on success and an appropriate error value on failure.
>> + */
>> +int rpmsg_set_flow_control(struct rpmsg_endpoint *ept, bool enable)
> 
> This API looks nice and clean and deals with flow control.

seems to me ambiguous API... what does it means flow control enable? Does it means that the flow control is enable or that the the local
endpoint is ready to receive?

Could we consider here that it is more a bind/unbind of the endpoint?

> 
>> +{
>> +	if (WARN_ON(!ept))
>> +		return -EINVAL;
>> +	if (!ept->ops->set_flow_control)
>> +		return -ENXIO;
>> +
>> +	return ept->ops->set_flow_control(ept, enable);
>> +}
>> +EXPORT_SYMBOL(rpmsg_set_flow_control);
>> +
>> +/**
>>   * rpmsg_get_mtu() - get maximum transmission buffer size for sending message.
>>   * @ept: the rpmsg endpoint
>>   *
>> @@ -535,6 +553,9 @@ static int rpmsg_dev_probe(struct device *dev)
>>  
>>  		rpdev->ept = ept;
>>  		rpdev->src = ept->addr;
>> +
>> +		if (rpdrv->signals)

seems an useless check

>> +			ept->sig_cb = rpdrv->signals;
>>  	}
>>  
>>  	err = rpdrv->probe(rpdev);
>> diff --git a/drivers/rpmsg/rpmsg_internal.h b/drivers/rpmsg/rpmsg_internal.h
>> index b1245d3..35c2197 100644
>> --- a/drivers/rpmsg/rpmsg_internal.h
>> +++ b/drivers/rpmsg/rpmsg_internal.h
>> @@ -53,6 +53,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.
>> @@ -73,6 +74,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);
>>  	ssize_t (*get_mtu)(struct rpmsg_endpoint *ept);
>>  };
>>  
>> diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h
>> index 02fa911..06d090c 100644
>> --- a/include/linux/rpmsg.h
>> +++ b/include/linux/rpmsg.h
>> @@ -62,12 +62,14 @@ struct rpmsg_device {
>>  };
>>  
>>  typedef int (*rpmsg_rx_cb_t)(struct rpmsg_device *, void *, int, void *, u32);
>> +typedef int (*rpmsg_rx_sig_t)(struct rpmsg_device *, void *, u32);
> 
> This callback however, is still using the original low level tty
> signals.
> 
> Is there any reason why this can't be "rpmsg_flowcontrol_cb_t" and take
> a boolean, so we get a clean interface in both directions?
> 
> Regards,
> Bjorn
> 
>>  
>>  /**
>>   * 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
>> + * @sig_cb: rx serial signal handler

Is it signaling for reception or transmission?

Regards,
Arnaud

>>   * @cb_lock: must be taken before accessing/changing @cb
>>   * @addr: local rpmsg address
>>   * @priv: private data for the driver's use
>> @@ -90,6 +92,7 @@ struct rpmsg_endpoint {
>>  	struct rpmsg_device *rpdev;
>>  	struct kref refcount;
>>  	rpmsg_rx_cb_t cb;
>> +	rpmsg_rx_sig_t sig_cb;
>>  	struct mutex cb_lock;
>>  	u32 addr;
>>  	void *priv;
>> @@ -111,6 +114,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 (*signals)(struct rpmsg_device *rpdev, void *priv, u32);
>>  };
>>  
>>  static inline u16 rpmsg16_to_cpu(struct rpmsg_device *rpdev, __rpmsg16 val)
>> @@ -188,6 +192,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);
>> +
>>  #else
>>  
>>  static inline int rpmsg_register_device(struct rpmsg_device *rpdev)
>> @@ -306,6 +312,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)
>> +{
>> +	/* 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
>>

Re: [PATCH V2 3/3] rpmsg: char: Add TIOCMGET/TIOCMSET ioctl support

[Arnaud POULIQUEN]

On 1/18/22 20:43, Deepak Kumar Singh wrote:
> Add TICOMGET and TIOCMSET ioctl support for rpmsg char device nodes
> to get/set the low level transport signals.
> 
> Signed-off-by: Chris Lew <quic_clew@quicinc.com>
> Signed-off-by: Deepak Kumar Singh <quic_deesin@quicinc.com>
> ---
>  drivers/rpmsg/rpmsg_char.c | 47 ++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 43 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
> index b5907b8..c03a118 100644
> --- a/drivers/rpmsg/rpmsg_char.c
> +++ b/drivers/rpmsg/rpmsg_char.c
> @@ -19,6 +19,7 @@
>  #include <linux/rpmsg.h>
>  #include <linux/skbuff.h>
>  #include <linux/slab.h>
> +#include <linux/termios.h>
>  #include <linux/uaccess.h>
>  #include <uapi/linux/rpmsg.h>
>  
> @@ -74,6 +75,9 @@ struct rpmsg_eptdev {
>  	spinlock_t queue_lock;
>  	struct sk_buff_head queue;
>  	wait_queue_head_t readq;
> +
> +	u32 rsigs;
> +	bool sig_pending;
>  };
>  
>  static int rpmsg_eptdev_destroy(struct device *dev, void *data)
> @@ -112,7 +116,18 @@ static int rpmsg_ept_cb(struct rpmsg_device *rpdev, void *buf, int len,
>  	skb_queue_tail(&eptdev->queue, skb);
>  	spin_unlock(&eptdev->queue_lock);
>  
> -	/* wake up any blocking processes, waiting for new data */
> +	wake_up_interruptible(&eptdev->readq);
> +
> +	return 0;
> +}
> +
> +static int rpmsg_sigs_cb(struct rpmsg_device *rpdev, void *priv, u32 sigs)
> +{
> +	struct rpmsg_eptdev *eptdev = priv;
> +
> +	eptdev->rsigs = sigs;
> +	eptdev->sig_pending = true;
> +
>  	wake_up_interruptible(&eptdev->readq);

Regarding the Glink code, the callback is used to be informed that the remote
is ready to send (DSR) and to receive (CTS or DSR)
So I suppose that the transmission should also be conditioned by the sig_pending

That said tell me if I'm wrong but look to me that what is implemented here is the 
 hardware flow control already managed by the TTY interface. What about using the
TTY interface in this case?

And What about using the "software flow control" instead? [1]

[1] https://en.wikipedia.org/wiki/Software_flow_control

>  
>  	return 0;
> @@ -137,6 +152,7 @@ static int rpmsg_eptdev_open(struct inode *inode, struct file *filp)
>  		return -EINVAL;
>  	}
>  
> +	ept->sig_cb = rpmsg_sigs_cb;
>  	eptdev->ept = ept;
>  	filp->private_data = eptdev;
>  
> @@ -155,6 +171,7 @@ static int rpmsg_eptdev_release(struct inode *inode, struct file *filp)
>  		eptdev->ept = NULL;
>  	}
>  	mutex_unlock(&eptdev->ept_lock);
> +	eptdev->sig_pending = false;
>  
>  	/* Discard all SKBs */
>  	skb_queue_purge(&eptdev->queue);
> @@ -265,6 +282,9 @@ static __poll_t rpmsg_eptdev_poll(struct file *filp, poll_table *wait)
>  	if (!skb_queue_empty(&eptdev->queue))
>  		mask |= EPOLLIN | EPOLLRDNORM;
>  
> +	if (eptdev->sig_pending)
> +		mask |= EPOLLPRI;
> +
>  	mask |= rpmsg_poll(eptdev->ept, filp, wait);
>  
>  	return mask;
> @@ -274,11 +294,30 @@ static long rpmsg_eptdev_ioctl(struct file *fp, unsigned int cmd,
>  			       unsigned long arg)
>  {
>  	struct rpmsg_eptdev *eptdev = fp->private_data;
> +	bool set;
> +	u32 val;
> +	int ret;
>  
> -	if (cmd != RPMSG_DESTROY_EPT_IOCTL)
> -		return -EINVAL;
> +	switch (cmd) {
> +	case TIOCMGET:
> +		eptdev->sig_pending = false;
> +		ret = put_user(eptdev->rsigs, (int __user *)arg);
> +		break;
> +	case TIOCMSET:
> +		ret = get_user(val, (int __user *)arg);
> +		if (ret)
> +			break;
> +		set = (val & TIOCM_DTR) ? true : false;
> +		ret = rpmsg_set_flow_control(eptdev->ept, set);
> +		break;

Could this directly be handled by the driver on open close?
If application wants to suspend the link it could just close de /dev/rpmsgX.
 
Regards,
Arnaud

> +	case RPMSG_DESTROY_EPT_IOCTL:
> +		ret = rpmsg_eptdev_destroy(&eptdev->dev, NULL);
> +		break;
> +	default:
> +		ret = -EINVAL;
> +	}
>  
> -	return rpmsg_eptdev_destroy(&eptdev->dev, NULL);
> +	return ret;
>  }
>  
>  static const struct file_operations rpmsg_eptdev_fops = {

Re: [PATCH V2 1/3] rpmsg: core: Add signal API support

[Deepak Kumar Singh]

On 3/23/2022 4:27 PM, Arnaud POULIQUEN wrote:
>
> On 3/11/22 22:11, Bjorn Andersson wrote:
>> On Tue 18 Jan 13:43 CST 2022, Deepak Kumar Singh wrote:
>>
>>> Some transports like Glink support the state notifications between
>>> clients using signals similar to serial protocol signals.
>>> Local glink client drivers can send and receive signals to glink
>>> clients running on remote processors.
>>>
>>> Add APIs to support sending and receiving of signals by rpmsg clients.
>>>
>>> Signed-off-by: Deepak Kumar Singh <quic_deesin@quicinc.com>
>>> ---
>>>   drivers/rpmsg/rpmsg_core.c     | 21 +++++++++++++++++++++
>>>   drivers/rpmsg/rpmsg_internal.h |  2 ++
>>>   include/linux/rpmsg.h          | 14 ++++++++++++++
>>>   3 files changed, 37 insertions(+)
>>>
>>> diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
>>> index d3eb600..6712418 100644
>>> --- a/drivers/rpmsg/rpmsg_core.c
>>> +++ b/drivers/rpmsg/rpmsg_core.c
>>> @@ -328,6 +328,24 @@ 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
>>> + *
>>> + * Return: 0 on success and an appropriate error value on failure.
>>> + */
>>> +int rpmsg_set_flow_control(struct rpmsg_endpoint *ept, bool enable)
>> This API looks nice and clean and deals with flow control.
> seems to me ambiguous API... what does it means flow control enable? Does it means that the flow control is enable or that the the local
> endpoint is ready to receive?
This means that local endpoint is ready to receive data.
>
> Could we consider here that it is more a bind/unbind of the endpoint?

Endpoint will remain bind, only data on that endpoint is not expected if 
flow control is disabled.

If flow control is enabled remote client can send data.

>>> +{
>>> +	if (WARN_ON(!ept))
>>> +		return -EINVAL;
>>> +	if (!ept->ops->set_flow_control)
>>> +		return -ENXIO;
>>> +
>>> +	return ept->ops->set_flow_control(ept, enable);
>>> +}
>>> +EXPORT_SYMBOL(rpmsg_set_flow_control);
>>> +
>>> +/**
>>>    * rpmsg_get_mtu() - get maximum transmission buffer size for sending message.
>>>    * @ept: the rpmsg endpoint
>>>    *
>>> @@ -535,6 +553,9 @@ static int rpmsg_dev_probe(struct device *dev)
>>>   
>>>   		rpdev->ept = ept;
>>>   		rpdev->src = ept->addr;
>>> +
>>> +		if (rpdrv->signals)
> seems an useless check

Some rpmsg cleints may not want to deal with flow control and not 
provide signal callback.

In such case this check is needed.

>>> +			ept->sig_cb = rpdrv->signals;
>>>   	}
>>>   
>>>   	err = rpdrv->probe(rpdev);
>>> diff --git a/drivers/rpmsg/rpmsg_internal.h b/drivers/rpmsg/rpmsg_internal.h
>>> index b1245d3..35c2197 100644
>>> --- a/drivers/rpmsg/rpmsg_internal.h
>>> +++ b/drivers/rpmsg/rpmsg_internal.h
>>> @@ -53,6 +53,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.
>>> @@ -73,6 +74,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);
>>>   	ssize_t (*get_mtu)(struct rpmsg_endpoint *ept);
>>>   };
>>>   
>>> diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h
>>> index 02fa911..06d090c 100644
>>> --- a/include/linux/rpmsg.h
>>> +++ b/include/linux/rpmsg.h
>>> @@ -62,12 +62,14 @@ struct rpmsg_device {
>>>   };
>>>   
>>>   typedef int (*rpmsg_rx_cb_t)(struct rpmsg_device *, void *, int, void *, u32);
>>> +typedef int (*rpmsg_rx_sig_t)(struct rpmsg_device *, void *, u32);
>> This callback however, is still using the original low level tty
>> signals.
>>
>> Is there any reason why this can't be "rpmsg_flowcontrol_cb_t" and take
>> a boolean, so we get a clean interface in both directions?
>>
>> Regards,
>> Bjorn
>>
>>>   
>>>   /**
>>>    * 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
>>> + * @sig_cb: rx serial signal handler
> Is it signaling for reception or transmission?
Remote is signalling for transmission.
>
> Regards,
> Arnaud
>
>>>    * @cb_lock: must be taken before accessing/changing @cb
>>>    * @addr: local rpmsg address
>>>    * @priv: private data for the driver's use
>>> @@ -90,6 +92,7 @@ struct rpmsg_endpoint {
>>>   	struct rpmsg_device *rpdev;
>>>   	struct kref refcount;
>>>   	rpmsg_rx_cb_t cb;
>>> +	rpmsg_rx_sig_t sig_cb;
>>>   	struct mutex cb_lock;
>>>   	u32 addr;
>>>   	void *priv;
>>> @@ -111,6 +114,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 (*signals)(struct rpmsg_device *rpdev, void *priv, u32);
>>>   };
>>>   
>>>   static inline u16 rpmsg16_to_cpu(struct rpmsg_device *rpdev, __rpmsg16 val)
>>> @@ -188,6 +192,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);
>>> +
>>>   #else
>>>   
>>>   static inline int rpmsg_register_device(struct rpmsg_device *rpdev)
>>> @@ -306,6 +312,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)
>>> +{
>>> +	/* 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
>>>

Re: [PATCH V2 3/3] rpmsg: char: Add TIOCMGET/TIOCMSET ioctl support

[Deepak Kumar Singh]

On 3/23/2022 7:08 PM, Arnaud POULIQUEN wrote:
>
> On 1/18/22 20:43, Deepak Kumar Singh wrote:
>> Add TICOMGET and TIOCMSET ioctl support for rpmsg char device nodes
>> to get/set the low level transport signals.
>>
>> Signed-off-by: Chris Lew <quic_clew@quicinc.com>
>> Signed-off-by: Deepak Kumar Singh <quic_deesin@quicinc.com>
>> ---
>>   drivers/rpmsg/rpmsg_char.c | 47 ++++++++++++++++++++++++++++++++++++++++++----
>>   1 file changed, 43 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
>> index b5907b8..c03a118 100644
>> --- a/drivers/rpmsg/rpmsg_char.c
>> +++ b/drivers/rpmsg/rpmsg_char.c
>> @@ -19,6 +19,7 @@
>>   #include <linux/rpmsg.h>
>>   #include <linux/skbuff.h>
>>   #include <linux/slab.h>
>> +#include <linux/termios.h>
>>   #include <linux/uaccess.h>
>>   #include <uapi/linux/rpmsg.h>
>>   
>> @@ -74,6 +75,9 @@ struct rpmsg_eptdev {
>>   	spinlock_t queue_lock;
>>   	struct sk_buff_head queue;
>>   	wait_queue_head_t readq;
>> +
>> +	u32 rsigs;
>> +	bool sig_pending;
>>   };
>>   
>>   static int rpmsg_eptdev_destroy(struct device *dev, void *data)
>> @@ -112,7 +116,18 @@ static int rpmsg_ept_cb(struct rpmsg_device *rpdev, void *buf, int len,
>>   	skb_queue_tail(&eptdev->queue, skb);
>>   	spin_unlock(&eptdev->queue_lock);
>>   
>> -	/* wake up any blocking processes, waiting for new data */
>> +	wake_up_interruptible(&eptdev->readq);
>> +
>> +	return 0;
>> +}
>> +
>> +static int rpmsg_sigs_cb(struct rpmsg_device *rpdev, void *priv, u32 sigs)
>> +{
>> +	struct rpmsg_eptdev *eptdev = priv;
>> +
>> +	eptdev->rsigs = sigs;
>> +	eptdev->sig_pending = true;
>> +
>>   	wake_up_interruptible(&eptdev->readq);
> Regarding the Glink code, the callback is used to be informed that the remote
> is ready to send (DSR) and to receive (CTS or DSR)
> So I suppose that the transmission should also be conditioned by the sig_pending

I think client need to get signal value before starting transmission, so 
that it knows that

it good to transmit data. Also it is not be enforced for every client. 
Some clients may not require

to use signalling/flow control.

>
> That said tell me if I'm wrong but look to me that what is implemented here is the
>   hardware flow control already managed by the TTY interface. What about using the
> TTY interface in this case?

Correct. But some clients are using rpmsg char driver directly and don't 
go through tty interface.

So we are incorporating tty like interface here(flow control).

> And What about using the "software flow control" instead? [1]
>
> [1] https://en.wikipedia.org/wiki/Software_flow_control
>
>>   
>>   	return 0;
>> @@ -137,6 +152,7 @@ static int rpmsg_eptdev_open(struct inode *inode, struct file *filp)
>>   		return -EINVAL;
>>   	}
>>   
>> +	ept->sig_cb = rpmsg_sigs_cb;
>>   	eptdev->ept = ept;
>>   	filp->private_data = eptdev;
>>   
>> @@ -155,6 +171,7 @@ static int rpmsg_eptdev_release(struct inode *inode, struct file *filp)
>>   		eptdev->ept = NULL;
>>   	}
>>   	mutex_unlock(&eptdev->ept_lock);
>> +	eptdev->sig_pending = false;
>>   
>>   	/* Discard all SKBs */
>>   	skb_queue_purge(&eptdev->queue);
>> @@ -265,6 +282,9 @@ static __poll_t rpmsg_eptdev_poll(struct file *filp, poll_table *wait)
>>   	if (!skb_queue_empty(&eptdev->queue))
>>   		mask |= EPOLLIN | EPOLLRDNORM;
>>   
>> +	if (eptdev->sig_pending)
>> +		mask |= EPOLLPRI;
>> +
>>   	mask |= rpmsg_poll(eptdev->ept, filp, wait);
>>   
>>   	return mask;
>> @@ -274,11 +294,30 @@ static long rpmsg_eptdev_ioctl(struct file *fp, unsigned int cmd,
>>   			       unsigned long arg)
>>   {
>>   	struct rpmsg_eptdev *eptdev = fp->private_data;
>> +	bool set;
>> +	u32 val;
>> +	int ret;
>>   
>> -	if (cmd != RPMSG_DESTROY_EPT_IOCTL)
>> -		return -EINVAL;
>> +	switch (cmd) {
>> +	case TIOCMGET:
>> +		eptdev->sig_pending = false;
>> +		ret = put_user(eptdev->rsigs, (int __user *)arg);
>> +		break;
>> +	case TIOCMSET:
>> +		ret = get_user(val, (int __user *)arg);
>> +		if (ret)
>> +			break;
>> +		set = (val & TIOCM_DTR) ? true : false;
>> +		ret = rpmsg_set_flow_control(eptdev->ept, set);
>> +		break;
> Could this directly be handled by the driver on open close?
> If application wants to suspend the link it could just close de /dev/rpmsgX.
All clients may not require setting flow control.
>   
> Regards,
> Arnaud
>
>> +	case RPMSG_DESTROY_EPT_IOCTL:
>> +		ret = rpmsg_eptdev_destroy(&eptdev->dev, NULL);
>> +		break;
>> +	default:
>> +		ret = -EINVAL;
>> +	}
>>   
>> -	return rpmsg_eptdev_destroy(&eptdev->dev, NULL);
>> +	return ret;
>>   }
>>   
>>   static const struct file_operations rpmsg_eptdev_fops = {

Re: [PATCH V2 1/3] rpmsg: core: Add signal API support

[Arnaud POULIQUEN]

Hello Deepak,

On 3/29/22 13:00, Deepak Kumar Singh wrote:
> 
> On 3/23/2022 4:27 PM, Arnaud POULIQUEN wrote:
>>
>> On 3/11/22 22:11, Bjorn Andersson wrote:
>>> On Tue 18 Jan 13:43 CST 2022, Deepak Kumar Singh wrote:
>>>
>>>> Some transports like Glink support the state notifications between
>>>> clients using signals similar to serial protocol signals.
>>>> Local glink client drivers can send and receive signals to glink
>>>> clients running on remote processors.
>>>>
>>>> Add APIs to support sending and receiving of signals by rpmsg clients.
>>>>
>>>> Signed-off-by: Deepak Kumar Singh <quic_deesin@quicinc.com>
>>>> ---
>>>>   drivers/rpmsg/rpmsg_core.c     | 21 +++++++++++++++++++++
>>>>   drivers/rpmsg/rpmsg_internal.h |  2 ++
>>>>   include/linux/rpmsg.h          | 14 ++++++++++++++
>>>>   3 files changed, 37 insertions(+)
>>>>
>>>> diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
>>>> index d3eb600..6712418 100644
>>>> --- a/drivers/rpmsg/rpmsg_core.c
>>>> +++ b/drivers/rpmsg/rpmsg_core.c
>>>> @@ -328,6 +328,24 @@ 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
>>>> + *
>>>> + * Return: 0 on success and an appropriate error value on failure.
>>>> + */
>>>> +int rpmsg_set_flow_control(struct rpmsg_endpoint *ept, bool enable)
>>> This API looks nice and clean and deals with flow control.
>> seems to me ambiguous API... what does it means flow control enable?
>> Does it means that the flow control is enable or that the the local
>> endpoint is ready to receive?
> This means that local endpoint is ready to receive data.
>>
>> Could we consider here that it is more a bind/unbind of the endpoint?
> 
> Endpoint will remain bind, only data on that endpoint is not expected if
> flow control is disabled.
> 
> If flow control is enabled remote client can send data.
> 
>>>> +{
>>>> +    if (WARN_ON(!ept))
>>>> +        return -EINVAL;
>>>> +    if (!ept->ops->set_flow_control)
>>>> +        return -ENXIO;
>>>> +
>>>> +    return ept->ops->set_flow_control(ept, enable);
>>>> +}
>>>> +EXPORT_SYMBOL(rpmsg_set_flow_control);
>>>> +
>>>> +/**
>>>>    * rpmsg_get_mtu() - get maximum transmission buffer size for
>>>> sending message.
>>>>    * @ept: the rpmsg endpoint
>>>>    *
>>>> @@ -535,6 +553,9 @@ static int rpmsg_dev_probe(struct device *dev)
>>>>             rpdev->ept = ept;
>>>>           rpdev->src = ept->addr;
>>>> +
>>>> +        if (rpdrv->signals)
>> seems an useless check
> 
> Some rpmsg cleints may not want to deal with flow control and not
> provide signal callback.
> 
> In such case this check is needed.
> 

my point here is that if rpdrv->signals is NULL,
then  ept->sig_cb will be NULL with or without the check.

>>>> +            ept->sig_cb = rpdrv->signals;
>>>>       }
>>>>         err = rpdrv->probe(rpdev);
>>>> diff --git a/drivers/rpmsg/rpmsg_internal.h
>>>> b/drivers/rpmsg/rpmsg_internal.h
>>>> index b1245d3..35c2197 100644
>>>> --- a/drivers/rpmsg/rpmsg_internal.h
>>>> +++ b/drivers/rpmsg/rpmsg_internal.h
>>>> @@ -53,6 +53,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.
>>>> @@ -73,6 +74,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);
>>>>       ssize_t (*get_mtu)(struct rpmsg_endpoint *ept);
>>>>   };
>>>>   diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h
>>>> index 02fa911..06d090c 100644
>>>> --- a/include/linux/rpmsg.h
>>>> +++ b/include/linux/rpmsg.h
>>>> @@ -62,12 +62,14 @@ struct rpmsg_device {
>>>>   };
>>>>     typedef int (*rpmsg_rx_cb_t)(struct rpmsg_device *, void *, int,
>>>> void *, u32);
>>>> +typedef int (*rpmsg_rx_sig_t)(struct rpmsg_device *, void *, u32);
>>> This callback however, is still using the original low level tty
>>> signals.
>>>
>>> Is there any reason why this can't be "rpmsg_flowcontrol_cb_t" and take
>>> a boolean, so we get a clean interface in both directions?
>>>
>>> Regards,
>>> Bjorn
>>>
>>>>     /**
>>>>    * 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
>>>> + * @sig_cb: rx serial signal handler
>> Is it signaling for reception or transmission?
> Remote is signalling for transmission.

So that the remote side is ready to receive, right?

Perhaps "@sig_cb: remote signaling callback handler" would be
less ambiguous?

Regards,
Arnaud

>>
>> Regards,
>> Arnaud
>>
>>>>    * @cb_lock: must be taken before accessing/changing @cb
>>>>    * @addr: local rpmsg address
>>>>    * @priv: private data for the driver's use
>>>> @@ -90,6 +92,7 @@ struct rpmsg_endpoint {
>>>>       struct rpmsg_device *rpdev;
>>>>       struct kref refcount;
>>>>       rpmsg_rx_cb_t cb;
>>>> +    rpmsg_rx_sig_t sig_cb;
>>>>       struct mutex cb_lock;
>>>>       u32 addr;
>>>>       void *priv;
>>>> @@ -111,6 +114,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 (*signals)(struct rpmsg_device *rpdev, void *priv, u32);
>>>>   };
>>>>     static inline u16 rpmsg16_to_cpu(struct rpmsg_device *rpdev,
>>>> __rpmsg16 val)
>>>> @@ -188,6 +192,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);
>>>> +
>>>>   #else
>>>>     static inline int rpmsg_register_device(struct rpmsg_device *rpdev)
>>>> @@ -306,6 +312,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)
>>>> +{
>>>> +    /* 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
>>>>

Re: [PATCH V2 2/3] rpmsg: glink: Add support to handle signals command

[Arnaud POULIQUEN]

On 3/23/22 08:20, Deepak Kumar Singh wrote:
> 
> On 3/12/2022 2:39 AM, Bjorn Andersson wrote:
>> On Tue 18 Jan 13:43 CST 2022, Deepak Kumar Singh wrote:
>>
>>> Remote peripherals send signal notifications over glink with
>>> commandID 15.
>>>
>>> Add support to send and receive the signal command and convert the
>>> signals
>>> from NATIVE to TIOCM while receiving and vice versa while sending.
>>>
>>> Signed-off-by: Chris Lew <quic_clew@quicinc.com>
>> Co-developed-by: seems appropriate here, or you need to ensure the
>> author remains Chris, as his S-o-b comes first.
>>
>>> Signed-off-by: Deepak Kumar Singh <quic_deesin@quicinc.com>
>>> ---
>>>   drivers/rpmsg/qcom_glink_native.c | 77
>>> +++++++++++++++++++++++++++++++++++++++
>>>   1 file changed, 77 insertions(+)
>>>
>>> diff --git a/drivers/rpmsg/qcom_glink_native.c
>>> b/drivers/rpmsg/qcom_glink_native.c
>>> index 3f377a7..d673d65 100644
>>> --- a/drivers/rpmsg/qcom_glink_native.c
>>> +++ b/drivers/rpmsg/qcom_glink_native.c
>>> @@ -17,6 +17,7 @@
>>>   #include <linux/rpmsg.h>
>>>   #include <linux/sizes.h>
>>>   #include <linux/slab.h>
>>> +#include <linux/termios.h>
>>>   #include <linux/workqueue.h>
>>>   #include <linux/mailbox_client.h>
>>>   @@ -205,9 +206,16 @@ static const struct rpmsg_endpoint_ops
>>> glink_endpoint_ops;
>>>   #define RPM_CMD_TX_DATA_CONT        12
>>>   #define RPM_CMD_READ_NOTIF        13
>>>   #define RPM_CMD_RX_DONE_W_REUSE        14
>>> +#define RPM_CMD_SIGNALS            15
>>>     #define GLINK_FEATURE_INTENTLESS    BIT(1)
>>>   +#define NATIVE_DTR_SIG            BIT(31)
>> Seems reasonable to prefix these with GLINK_, perhaps GLINK_SIGNAL_DTR?
>>
>>> +#define NATIVE_CTS_SIG            BIT(30)
>>> +#define NATIVE_CD_SIG            BIT(29)
>>> +#define NATIVE_RI_SIG            BIT(28)
>>> +#define    SIG_MASK            0x0fff;
>>> +
>>>   static void qcom_glink_rx_done_work(struct work_struct *work);
>>>     static struct glink_channel *qcom_glink_alloc_channel(struct
>>> qcom_glink *glink,
>>> @@ -1003,6 +1011,70 @@ static int qcom_glink_rx_open_ack(struct
>>> qcom_glink *glink, unsigned int lcid)
>>>       return 0;
>>>   }
>>>   +/**
>>> + * qcom_glink_set_flow_control() - convert a signal cmd to wire
>>> format and
>>> + *                    transmit
>>> + * @ept:    Rpmsg endpoint for channel.
>>> + * @enable:    True/False - enable or disable flow control
>> "enable flow control" sounds sufficient (i.e. no need for True/False)
>> part.
>>
>> Regards,
>> Bjorn
> 
> There are some user space clients which require both flow control on and
> off (DTR high/low).
> 
> So i guess true and false both are needed.
> 
>>> + *
>>> + * Return: 0 on success or standard Linux error code.
>>> + */
>>> +static int qcom_glink_set_flow_control(struct rpmsg_endpoint *ept,
>>> bool enable)
>>> +{
>>> +    struct glink_channel *channel = to_glink_channel(ept);
>>> +    struct qcom_glink *glink = channel->glink;
>>> +    struct glink_msg msg;
>>> +    u32 sigs;
>>> +
>>> +    /**
>>> +     * convert signals from TIOCM to NATIVE
>>> +     * sigs = TIOCM_DTR|TIOCM_RTS
>>> +     */
>>> +    if (enable)
>>> +        sigs |= NATIVE_DTR_SIG | NATIVE_CTS_SIG;
>>> +    else
>>> +        sigs |= ~(NATIVE_DTR_SIG | NATIVE_CTS_SIG);
>>> +
>>> +    msg.cmd = cpu_to_le16(RPM_CMD_SIGNALS);
>>> +    msg.param1 = cpu_to_le16(channel->lcid);
>>> +    msg.param2 = cpu_to_le32(sigs);
>>> +
>>> +    return qcom_glink_tx(glink, &msg, sizeof(msg), NULL, 0, true);
>>> +}
>>> +
>>> +static int qcom_glink_handle_signals(struct qcom_glink *glink,
>>> +                     unsigned int rcid, unsigned int sigs)
>>> +{
>>> +    struct glink_channel *channel;
>>> +    unsigned long flags;
>>> +
>>> +    spin_lock_irqsave(&glink->idr_lock, flags);
>>> +    channel = idr_find(&glink->rcids, rcid);
>>> +    spin_unlock_irqrestore(&glink->idr_lock, flags);
>>> +    if (!channel) {
>>> +        dev_err(glink->dev, "signal for non-existing channel\n");
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    if (!channel->ept.sig_cb)
>>> +        return 0;
>>> +
>>> +    /* convert signals from NATIVE to TIOCM */
>>> +    if (sigs & NATIVE_DTR_SIG)

Regarding specs seems that DTR is from the DTE (Data Terminal Equipment)
to the DCE (Data Communication Equipement)
NATIVE_DSR_SIG instead?

Regards
Arnaud

>>> +        sigs |= TIOCM_DSR;
>>> +    if (sigs & NATIVE_CTS_SIG)
>>> +        sigs |= TIOCM_CTS;
>>> +    if (sigs & NATIVE_CD_SIG)
>>> +        sigs |= TIOCM_CD;
>>> +    if (sigs & NATIVE_RI_SIG)
>>> +        sigs |= TIOCM_RI;
>>> +    sigs &= SIG_MASK;
>>> +
>>> +    channel->ept.sig_cb(channel->ept.rpdev, channel->ept.priv, sigs);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>>   static irqreturn_t qcom_glink_native_intr(int irq, void *data)
>>>   {
>>>       struct qcom_glink *glink = data;
>>> @@ -1067,6 +1139,10 @@ static irqreturn_t qcom_glink_native_intr(int
>>> irq, void *data)
>>>               qcom_glink_handle_intent_req_ack(glink, param1, param2);
>>>               qcom_glink_rx_advance(glink, ALIGN(sizeof(msg), 8));
>>>               break;
>>> +        case RPM_CMD_SIGNALS:
>>> +            qcom_glink_handle_signals(glink, param1, param2);
>>> +            qcom_glink_rx_advance(glink, ALIGN(sizeof(msg), 8));
>>> +            break;
>>>           default:
>>>               dev_err(glink->dev, "unhandled rx cmd: %d\n", cmd);
>>>               ret = -EINVAL;
>>> @@ -1442,6 +1518,7 @@ static const struct rpmsg_endpoint_ops
>>> glink_endpoint_ops = {
>>>       .sendto = qcom_glink_sendto,
>>>       .trysend = qcom_glink_trysend,
>>>       .trysendto = qcom_glink_trysendto,
>>> +    .set_flow_control = qcom_glink_set_flow_control,
>>>   };
>>>     static void qcom_glink_rpdev_release(struct device *dev)
>>> -- 
>>> 2.7.4
>>>

Re: [PATCH V2 3/3] rpmsg: char: Add TIOCMGET/TIOCMSET ioctl support

[Arnaud POULIQUEN]

On 3/29/22 14:25, Deepak Kumar Singh wrote:
> 
> On 3/23/2022 7:08 PM, Arnaud POULIQUEN wrote:
>>
>> On 1/18/22 20:43, Deepak Kumar Singh wrote:
>>> Add TICOMGET and TIOCMSET ioctl support for rpmsg char device nodes
>>> to get/set the low level transport signals.
>>>
>>> Signed-off-by: Chris Lew <quic_clew@quicinc.com>
>>> Signed-off-by: Deepak Kumar Singh <quic_deesin@quicinc.com>
>>> ---
>>>   drivers/rpmsg/rpmsg_char.c | 47
>>> ++++++++++++++++++++++++++++++++++++++++++----
>>>   1 file changed, 43 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
>>> index b5907b8..c03a118 100644
>>> --- a/drivers/rpmsg/rpmsg_char.c
>>> +++ b/drivers/rpmsg/rpmsg_char.c
>>> @@ -19,6 +19,7 @@
>>>   #include <linux/rpmsg.h>
>>>   #include <linux/skbuff.h>
>>>   #include <linux/slab.h>
>>> +#include <linux/termios.h>
>>>   #include <linux/uaccess.h>
>>>   #include <uapi/linux/rpmsg.h>
>>>   @@ -74,6 +75,9 @@ struct rpmsg_eptdev {
>>>       spinlock_t queue_lock;
>>>       struct sk_buff_head queue;
>>>       wait_queue_head_t readq;
>>> +
>>> +    u32 rsigs;
>>> +    bool sig_pending;
>>>   };
>>>     static int rpmsg_eptdev_destroy(struct device *dev, void *data)
>>> @@ -112,7 +116,18 @@ static int rpmsg_ept_cb(struct rpmsg_device
>>> *rpdev, void *buf, int len,
>>>       skb_queue_tail(&eptdev->queue, skb);
>>>       spin_unlock(&eptdev->queue_lock);
>>>   -    /* wake up any blocking processes, waiting for new data */
>>> +    wake_up_interruptible(&eptdev->readq);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int rpmsg_sigs_cb(struct rpmsg_device *rpdev, void *priv, u32
>>> sigs)
>>> +{
>>> +    struct rpmsg_eptdev *eptdev = priv;
>>> +
>>> +    eptdev->rsigs = sigs;
>>> +    eptdev->sig_pending = true;
>>> +
>>>       wake_up_interruptible(&eptdev->readq);
>> Regarding the Glink code, the callback is used to be informed that the
>> remote
>> is ready to send (DSR) and to receive (CTS or DSR)
>> So I suppose that the transmission should also be conditioned by the
>> sig_pending
> 
> I think client need to get signal value before starting transmission, so
> that it knows that
> 
> it good to transmit data. Also it is not be enforced for every client.
> Some clients may not require
> 
> to use signalling/flow control.
> 
>>
>> That said tell me if I'm wrong but look to me that what is implemented
>> here is the
>>   hardware flow control already managed by the TTY interface. What
>> about using the
>> TTY interface in this case?
> 
> Correct. But some clients are using rpmsg char driver directly and don't
> go through tty interface.
> 
> So we are incorporating tty like interface here(flow control).

This is the point I would like to highlight to be sure that it is the
good direction.

From my windows I would say if application wants a basic link it uses
the rpmsg_char, if it wants more complex link with TIOCMGET and 
TIOCMSET signaling it should migrate on rmsg tty as the link is now
available (so implementing signaling in rpmsg_tty instead of rpmsg char).

Anyway here I only share my opinion. It is not my role to give the direction.

> 
>> And What about using the "software flow control" instead? [1]
>>
>> [1] https://en.wikipedia.org/wiki/Software_flow_control
>>
>>>         return 0;
>>> @@ -137,6 +152,7 @@ static int rpmsg_eptdev_open(struct inode *inode,
>>> struct file *filp)
>>>           return -EINVAL;
>>>       }
>>>   +    ept->sig_cb = rpmsg_sigs_cb;
>>>       eptdev->ept = ept;
>>>       filp->private_data = eptdev;
>>>   @@ -155,6 +171,7 @@ static int rpmsg_eptdev_release(struct inode
>>> *inode, struct file *filp)
>>>           eptdev->ept = NULL;
>>>       }
>>>       mutex_unlock(&eptdev->ept_lock);
>>> +    eptdev->sig_pending = false;
>>>         /* Discard all SKBs */
>>>       skb_queue_purge(&eptdev->queue);
>>> @@ -265,6 +282,9 @@ static __poll_t rpmsg_eptdev_poll(struct file
>>> *filp, poll_table *wait)
>>>       if (!skb_queue_empty(&eptdev->queue))
>>>           mask |= EPOLLIN | EPOLLRDNORM;
>>>   +    if (eptdev->sig_pending)
>>> +        mask |= EPOLLPRI;
>>> +
>>>       mask |= rpmsg_poll(eptdev->ept, filp, wait);
>>>         return mask;
>>> @@ -274,11 +294,30 @@ static long rpmsg_eptdev_ioctl(struct file *fp,
>>> unsigned int cmd,
>>>                      unsigned long arg)
>>>   {
>>>       struct rpmsg_eptdev *eptdev = fp->private_data;
>>> +    bool set;
>>> +    u32 val;
>>> +    int ret;
>>>   -    if (cmd != RPMSG_DESTROY_EPT_IOCTL)
>>> -        return -EINVAL;
>>> +    switch (cmd) {
>>> +    case TIOCMGET:
>>> +        eptdev->sig_pending = false;
>>> +        ret = put_user(eptdev->rsigs, (int __user *)arg);
>>> +        break;
>>> +    case TIOCMSET:
>>> +        ret = get_user(val, (int __user *)arg);
>>> +        if (ret)
>>> +            break;
>>> +        set = (val & TIOCM_DTR) ? true : false;
>>> +        ret = rpmsg_set_flow_control(eptdev->ept, set);
>>> +        break;
>> Could this directly be handled by the driver on open close?
>> If application wants to suspend the link it could just close de
>> /dev/rpmsgX.
> All clients may not require setting flow control.

Agree, but this could be conditioned by rpdrv->signals, right?
And this could avoid to expose controls 
- in open/close the rpmsg_set_flow_control would be called,
- in rpmsg_eptdev_write_iter an error would be returned ( or 0)
  if remote side has suspended the transmission.

But perhaps you need more that a ON/OFF flow control?

Regards,
Arnaud

>>   Regards,
>> Arnaud
>>
>>> +    case RPMSG_DESTROY_EPT_IOCTL:
>>> +        ret = rpmsg_eptdev_destroy(&eptdev->dev, NULL);
>>> +        break;
>>> +    default:
>>> +        ret = -EINVAL;
>>> +    }
>>>   -    return rpmsg_eptdev_destroy(&eptdev->dev, NULL);
>>> +    return ret;
>>>   }
>>>     static const struct file_operations rpmsg_eptdev_fops = {

Re: [PATCH V2 0/3] rpmsg and glink signaling API support

[Arnaud POULIQUEN]

Hi,

On 3/23/22 11:17, Arnaud POULIQUEN wrote:
> Hi all,
> 
> On 1/18/22 20:43, Deepak Kumar Singh wrote:
>> [Change from V1]
>> Fixed most of the review comments in V1.
> 
> This implementation works for the glink transport,
> But how to manage such flow control for other transport
> layer?
> From my POV it is important that it is also usable for
> by transport backends which doe not have such signaling.
> The idea here is not to implement in other backends yet, but
> at least to determine how it could be handled to avoid that
> tomorrow this has to be reworked. 

FYI, I've started some dev, trying to adapt the implementation to
the virtio backend to move forward with this topic.

It is only a POC for time being based on a new ROPMsg service for the
flow control.

Linux code is available here:
https://github.com/arnopo/linux/commits/signalling

openamp library associated code is available here:
https://github.com/arnopo/open-amp/commits/flow_ctrl

I hope to find some time next month to continue my dev and send patches
to the mailing list. 

Regards,
Arnaud


> 
> More than that I wonder if the flow control could also be used
> to solve the RPmsg protocol issue related to the channel
> announcement [1][2]
> 
> [1] https://github.com/OpenAMP/open-amp/pull/160
> [2] https://lore.kernel.org/lkml/20220316153001.662422-1-arnaud.pouliquen@foss.st.com/
> 
> Thanks,
> Arnaud



> 
>>
>> Deepak Kumar Singh (3):
>>   rpmsg: core: Add signal API support
>>   rpmsg: glink: Add support to handle signals command
>>   rpmsg: char: Add TIOCMGET/TIOCMSET ioctl support
>>
>>  drivers/rpmsg/qcom_glink_native.c | 77 +++++++++++++++++++++++++++++++++++++++
>>  drivers/rpmsg/rpmsg_char.c        | 47 ++++++++++++++++++++++--
>>  drivers/rpmsg/rpmsg_core.c        | 21 +++++++++++
>>  drivers/rpmsg/rpmsg_internal.h    |  2 +
>>  include/linux/rpmsg.h             | 14 +++++++
>>  5 files changed, 157 insertions(+), 4 deletions(-)
>>