Glink transport support signals to exchange state notification between
local and remote side clients. Adding support to send/receive the signal
command and notify the clients through callback and POLL notification.
Changes since v3:
- Correct the TICOMGET case handling as per new rpmsg_get_signals prototype
- Update the rpmsg_get_signals function header
Changes since v2:
- Modify the rpmsg_get_signals function prototype
Changes since v1:
- Split the patches as per functional areas like core, char, glink
- Add set, clear mask for TIOCMSET
- Merge the char signal callback and POLLPRI patches
Arun Kumar Neelakantam (4):
rpmsg: core: Add signal API support
rpmsg: glink: Add support to handle signals command
rpmsg: char: Add TIOCMGET/TIOCMSET ioctl support
rpmsg: char: Add signal callback and POLLPRI support
drivers/rpmsg/qcom_glink_native.c | 126 ++++++++++++++++++++++++++++++++++++++
drivers/rpmsg/rpmsg_char.c | 75 ++++++++++++++++++++++-
drivers/rpmsg/rpmsg_core.c | 41 +++++++++++++
drivers/rpmsg/rpmsg_internal.h | 5 ++
include/linux/rpmsg.h | 26 ++++++++
5 files changed, 270 insertions(+), 3 deletions(-)
--
2.7.4
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 <[email protected]>
Signed-off-by: Arun Kumar Neelakantam <[email protected]>
---
drivers/rpmsg/qcom_glink_native.c | 126 ++++++++++++++++++++++++++++++++++++++
1 file changed, 126 insertions(+)
diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
index fc8ef66..68e039a 100644
--- a/drivers/rpmsg/qcom_glink_native.c
+++ b/drivers/rpmsg/qcom_glink_native.c
@@ -1,5 +1,6 @@
// SPDX-License-Identifier: GPL-2.0
/*
+ * Copyright (c) 2018, The Linux Foundation.
* Copyright (c) 2016-2017, Linaro Ltd
*/
@@ -17,6 +18,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>
@@ -150,6 +152,8 @@ enum {
* @intent_req_lock: Synchronises multiple intent requests
* @intent_req_result: Result of intent request
* @intent_req_comp: Completion for intent_req signalling
+ * @lsigs: local side signals
+ * @rsigs: remote side signals
*/
struct glink_channel {
struct rpmsg_endpoint ept;
@@ -181,6 +185,10 @@ struct glink_channel {
struct mutex intent_req_lock;
bool intent_req_result;
struct completion intent_req_comp;
+
+ unsigned int lsigs;
+ unsigned int rsigs;
+
};
#define to_glink_channel(_ept) container_of(_ept, struct glink_channel, ept)
@@ -201,9 +209,15 @@ 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)
+
static void qcom_glink_rx_done_work(struct work_struct *work);
static struct glink_channel *qcom_glink_alloc_channel(struct qcom_glink *glink,
@@ -975,6 +989,76 @@ static int qcom_glink_rx_open_ack(struct qcom_glink *glink, unsigned int lcid)
return 0;
}
+/**
+ * qcom_glink_send_signals() - convert a signal cmd to wire format and transmit
+ * @glink: The transport to transmit on.
+ * @channel: The glink channel
+ * @sigs: The signals to encode.
+ *
+ * Return: 0 on success or standard Linux error code.
+ */
+static int qcom_glink_send_signals(struct qcom_glink *glink,
+ struct glink_channel *channel,
+ u32 sigs)
+{
+ struct glink_msg msg;
+
+ /* convert signals from TIOCM to NATIVE */
+ sigs &= 0x0fff;
+ if (sigs & TIOCM_DTR)
+ sigs |= NATIVE_DTR_SIG;
+ if (sigs & TIOCM_RTS)
+ sigs |= NATIVE_CTS_SIG;
+ if (sigs & TIOCM_CD)
+ sigs |= NATIVE_CD_SIG;
+ if (sigs & TIOCM_RI)
+ sigs |= NATIVE_RI_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 signals)
+{
+ struct glink_channel *channel;
+ unsigned long flags;
+ u32 old;
+
+ 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;
+ }
+
+ old = channel->rsigs;
+
+ /* convert signals from NATIVE to TIOCM */
+ if (signals & NATIVE_DTR_SIG)
+ signals |= TIOCM_DSR;
+ if (signals & NATIVE_CTS_SIG)
+ signals |= TIOCM_CTS;
+ if (signals & NATIVE_CD_SIG)
+ signals |= TIOCM_CD;
+ if (signals & NATIVE_RI_SIG)
+ signals |= TIOCM_RI;
+ signals &= 0x0fff;
+
+ channel->rsigs = signals;
+
+ if (channel->ept.sig_cb) {
+ channel->ept.sig_cb(channel->ept.rpdev, channel->ept.priv,
+ old, channel->rsigs);
+ }
+
+ return 0;
+}
+
static irqreturn_t qcom_glink_native_intr(int irq, void *data)
{
struct qcom_glink *glink = data;
@@ -1036,6 +1120,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;
@@ -1340,6 +1428,42 @@ static int qcom_glink_trysend(struct rpmsg_endpoint *ept, void *data, int len)
return __qcom_glink_send(channel, data, len, false);
}
+static int qcom_glink_get_sigs(struct rpmsg_endpoint *ept)
+{
+ struct glink_channel *channel = to_glink_channel(ept);
+
+ return channel->rsigs;
+}
+
+static int qcom_glink_set_sigs(struct rpmsg_endpoint *ept, u32 set, u32 clear)
+{
+ struct glink_channel *channel = to_glink_channel(ept);
+ struct qcom_glink *glink = channel->glink;
+ u32 sigs = channel->lsigs;
+
+ if (set & TIOCM_DTR)
+ sigs |= TIOCM_DTR;
+ if (set & TIOCM_RTS)
+ sigs |= TIOCM_RTS;
+ if (set & TIOCM_CD)
+ sigs |= TIOCM_CD;
+ if (set & TIOCM_RI)
+ sigs |= TIOCM_RI;
+
+ if (clear & TIOCM_DTR)
+ sigs &= ~TIOCM_DTR;
+ if (clear & TIOCM_RTS)
+ sigs &= ~TIOCM_RTS;
+ if (clear & TIOCM_CD)
+ sigs &= ~TIOCM_CD;
+ if (clear & TIOCM_RI)
+ sigs &= ~TIOCM_RI;
+
+ channel->lsigs = sigs;
+
+ return qcom_glink_send_signals(glink, channel, sigs);
+}
+
/*
* Finds the device_node for the glink child interested in this channel.
*/
@@ -1373,6 +1497,8 @@ static const struct rpmsg_endpoint_ops glink_endpoint_ops = {
.destroy_ept = qcom_glink_destroy_ept,
.send = qcom_glink_send,
.trysend = qcom_glink_trysend,
+ .get_signals = qcom_glink_get_sigs,
+ .set_signals = qcom_glink_set_sigs,
};
static void qcom_glink_rpdev_release(struct device *dev)
--
2.7.4
Register a callback to get the signal notifications from rpmsg and
send POLLPRI mask to indicate the signal change in POLL system call.
Signed-off-by: Arun Kumar Neelakantam <[email protected]>
---
drivers/rpmsg/rpmsg_char.c | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
index e2f92f3..ae15d4f 100644
--- a/drivers/rpmsg/rpmsg_char.c
+++ b/drivers/rpmsg/rpmsg_char.c
@@ -64,6 +64,7 @@ struct rpmsg_ctrldev {
* @queue_lock: synchronization of @queue operations
* @queue: incoming message queue
* @readq: wait object for incoming queue
+ * @sig_pending:state of signal notification
*/
struct rpmsg_eptdev {
struct device dev;
@@ -78,6 +79,8 @@ struct rpmsg_eptdev {
spinlock_t queue_lock;
struct sk_buff_head queue;
wait_queue_head_t readq;
+
+ bool sig_pending;
};
static int rpmsg_eptdev_destroy(struct device *dev, void *data)
@@ -122,6 +125,19 @@ static int rpmsg_ept_cb(struct rpmsg_device *rpdev, void *buf, int len,
return 0;
}
+static int rpmsg_sigs_cb(struct rpmsg_device *rpdev, void *priv,
+ u32 old, u32 new)
+{
+ struct rpmsg_eptdev *eptdev = priv;
+
+ eptdev->sig_pending = true;
+
+ /* wake up any blocking processes, waiting for signal notification */
+ wake_up_interruptible(&eptdev->readq);
+ return 0;
+}
+
+
static int rpmsg_eptdev_open(struct inode *inode, struct file *filp)
{
struct rpmsg_eptdev *eptdev = cdev_to_eptdev(inode->i_cdev);
@@ -138,6 +154,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;
@@ -156,6 +173,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);
@@ -266,6 +284,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 |= POLLPRI;
+
mask |= rpmsg_poll(eptdev->ept, filp, wait);
return mask;
@@ -309,6 +330,7 @@ static long rpmsg_eptdev_ioctl(struct file *fp, unsigned int cmd,
switch (cmd) {
case TIOCMGET:
+ eptdev->sig_pending = false;
ret = rpmsg_get_signals(eptdev->ept);
if (ret >= 0)
ret = put_user(ret, (int __user *)arg);
--
2.7.4
On Wed, May 13, 2020 at 10:41:09AM +0530, Arun Kumar Neelakantam 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 <[email protected]>
> Signed-off-by: Arun Kumar Neelakantam <[email protected]>
> ---
> drivers/rpmsg/qcom_glink_native.c | 126 ++++++++++++++++++++++++++++++++++++++
> 1 file changed, 126 insertions(+)
>
> diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
> index fc8ef66..68e039a 100644
> --- a/drivers/rpmsg/qcom_glink_native.c
> +++ b/drivers/rpmsg/qcom_glink_native.c
> @@ -1,5 +1,6 @@
> // SPDX-License-Identifier: GPL-2.0
> /*
> + * Copyright (c) 2018, The Linux Foundation.
Again, not sure what this is for.
> * Copyright (c) 2016-2017, Linaro Ltd
> */
>
> @@ -17,6 +18,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>
>
> @@ -150,6 +152,8 @@ enum {
> * @intent_req_lock: Synchronises multiple intent requests
> * @intent_req_result: Result of intent request
> * @intent_req_comp: Completion for intent_req signalling
> + * @lsigs: local side signals
> + * @rsigs: remote side signals
> */
> struct glink_channel {
> struct rpmsg_endpoint ept;
> @@ -181,6 +185,10 @@ struct glink_channel {
> struct mutex intent_req_lock;
> bool intent_req_result;
> struct completion intent_req_comp;
> +
> + unsigned int lsigs;
> + unsigned int rsigs;
> +
> };
>
> #define to_glink_channel(_ept) container_of(_ept, struct glink_channel, ept)
> @@ -201,9 +209,15 @@ 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)
Please align theres with the BIT(1) of _INTENTLESS.
> +
> static void qcom_glink_rx_done_work(struct work_struct *work);
>
> static struct glink_channel *qcom_glink_alloc_channel(struct qcom_glink *glink,
> @@ -975,6 +989,76 @@ static int qcom_glink_rx_open_ack(struct qcom_glink *glink, unsigned int lcid)
> return 0;
> }
>
> +/**
> + * qcom_glink_send_signals() - convert a signal cmd to wire format and transmit
s/signal cmd/signal cmd
> + * @glink: The transport to transmit on.
> + * @channel: The glink channel
> + * @sigs: The signals to encode.
> + *
> + * Return: 0 on success or standard Linux error code.
> + */
> +static int qcom_glink_send_signals(struct qcom_glink *glink,
> + struct glink_channel *channel,
> + u32 sigs)
> +{
> + struct glink_msg msg;
> +
> + /* convert signals from TIOCM to NATIVE */
> + sigs &= 0x0fff;
> + if (sigs & TIOCM_DTR)
> + sigs |= NATIVE_DTR_SIG;
> + if (sigs & TIOCM_RTS)
> + sigs |= NATIVE_CTS_SIG;
> + if (sigs & TIOCM_CD)
> + sigs |= NATIVE_CD_SIG;
> + if (sigs & TIOCM_RI)
> + sigs |= NATIVE_RI_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 signals)
> +{
> + struct glink_channel *channel;
> + unsigned long flags;
> + u32 old;
> +
> + 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;
> + }
> +
> + old = channel->rsigs;
> +
> + /* convert signals from NATIVE to TIOCM */
> + if (signals & NATIVE_DTR_SIG)
> + signals |= TIOCM_DSR;
In qcom_glink_send_signals(), TIOCM_DTR (and not TIOCM_DSR) converts to
NATIVE_DTR_SIG.
> + if (signals & NATIVE_CTS_SIG)
> + signals |= TIOCM_CTS;
Similarly, NATIVE_CTS_SIG converts to TIOCM_RTS. I would have expected to have
a match between what gets sent and what comes back up. If this is intentional
please at some comments to justify your choices.
> + if (signals & NATIVE_CD_SIG)
> + signals |= TIOCM_CD;
> + if (signals & NATIVE_RI_SIG)
> + signals |= TIOCM_RI;
> + signals &= 0x0fff;
> +
> + channel->rsigs = signals;
> +
> + if (channel->ept.sig_cb) {
> + channel->ept.sig_cb(channel->ept.rpdev, channel->ept.priv,
> + old, channel->rsigs);
What is the relevance of old and channel->rsigs? They are certainly not used in
patch 4.
> + }
> +
> + return 0;
> +}
> +
> static irqreturn_t qcom_glink_native_intr(int irq, void *data)
> {
> struct qcom_glink *glink = data;
> @@ -1036,6 +1120,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;
> @@ -1340,6 +1428,42 @@ static int qcom_glink_trysend(struct rpmsg_endpoint *ept, void *data, int len)
> return __qcom_glink_send(channel, data, len, false);
> }
>
> +static int qcom_glink_get_sigs(struct rpmsg_endpoint *ept)
> +{
> + struct glink_channel *channel = to_glink_channel(ept);
> +
> + return channel->rsigs;
> +}
> +
> +static int qcom_glink_set_sigs(struct rpmsg_endpoint *ept, u32 set, u32 clear)
> +{
> + struct glink_channel *channel = to_glink_channel(ept);
> + struct qcom_glink *glink = channel->glink;
> + u32 sigs = channel->lsigs;
> +
> + if (set & TIOCM_DTR)
> + sigs |= TIOCM_DTR;
> + if (set & TIOCM_RTS)
> + sigs |= TIOCM_RTS;
> + if (set & TIOCM_CD)
> + sigs |= TIOCM_CD;
> + if (set & TIOCM_RI)
> + sigs |= TIOCM_RI;
> +
> + if (clear & TIOCM_DTR)
> + sigs &= ~TIOCM_DTR;
> + if (clear & TIOCM_RTS)
> + sigs &= ~TIOCM_RTS;
> + if (clear & TIOCM_CD)
> + sigs &= ~TIOCM_CD;
> + if (clear & TIOCM_RI)
> + sigs &= ~TIOCM_RI;
> +
> + channel->lsigs = sigs;
> +
> + return qcom_glink_send_signals(glink, channel, sigs);
> +}
> +
> /*
> * Finds the device_node for the glink child interested in this channel.
> */
> @@ -1373,6 +1497,8 @@ static const struct rpmsg_endpoint_ops glink_endpoint_ops = {
> .destroy_ept = qcom_glink_destroy_ept,
> .send = qcom_glink_send,
> .trysend = qcom_glink_trysend,
> + .get_signals = qcom_glink_get_sigs,
> + .set_signals = qcom_glink_set_sigs,
> };
>
> static void qcom_glink_rpdev_release(struct device *dev)
> --
> 2.7.4
On Wed, May 13, 2020 at 10:41:07AM +0530, Arun Kumar Neelakantam wrote:
> Glink transport support signals to exchange state notification between
> local and remote side clients. Adding support to send/receive the signal
> command and notify the clients through callback and POLL notification.
From a general point of view it is hard to have an understanding of what this
patchset does and if it is the right solution due to a lack of context.
The cover letter and changelogs are sparse to a point where a
reviewer not familiar with the QC/Glink implementation can't offer much more
than syntactic comments.
Thanks,
Mathieu
>
> Changes since v3:
> - Correct the TICOMGET case handling as per new rpmsg_get_signals prototype
> - Update the rpmsg_get_signals function header
>
> Changes since v2:
> - Modify the rpmsg_get_signals function prototype
>
> Changes since v1:
> - Split the patches as per functional areas like core, char, glink
> - Add set, clear mask for TIOCMSET
> - Merge the char signal callback and POLLPRI patches
>
> Arun Kumar Neelakantam (4):
> rpmsg: core: Add signal API support
> rpmsg: glink: Add support to handle signals command
> rpmsg: char: Add TIOCMGET/TIOCMSET ioctl support
> rpmsg: char: Add signal callback and POLLPRI support
>
> drivers/rpmsg/qcom_glink_native.c | 126 ++++++++++++++++++++++++++++++++++++++
> drivers/rpmsg/rpmsg_char.c | 75 ++++++++++++++++++++++-
> drivers/rpmsg/rpmsg_core.c | 41 +++++++++++++
> drivers/rpmsg/rpmsg_internal.h | 5 ++
> include/linux/rpmsg.h | 26 ++++++++
> 5 files changed, 270 insertions(+), 3 deletions(-)
>
> --
> 2.7.4