[PATCH 0/6] rpmsg: glink: Misc improvements

[PATCH 0/6] rpmsg: glink: Misc improvements

[Bjorn Andersson]

This series refactors glink_native to move IRQ and mailbox handling to SMEM and
RPM driver, in preparation for more work. It then introduces the logic to fail
glink transactions and pending intent requests in the event of the edge being
torn down.

Bjorn Andersson (6):
  rpmsg: glink: Extract tx kick operation
  rpmsg: glink: smem: Wrap driver context
  rpmsg: glink: rpm: Wrap driver context
  rpmsg: glink: Move irq and mbox handling to transports
  rpmsg: glink: Fail qcom_glink_tx() once remove has been initiated
  rpmsg: glink: Cancel pending intent requests at removal

 drivers/remoteproc/qcom_common.h  |   3 +-
 drivers/rpmsg/qcom_glink_native.c |  93 ++++++++++-----------
 drivers/rpmsg/qcom_glink_native.h |   3 +-
 drivers/rpmsg/qcom_glink_rpm.c    |  94 ++++++++++++++++-----
 drivers/rpmsg/qcom_glink_smem.c   | 132 +++++++++++++++++++++++-------
 include/linux/rpmsg/qcom_glink.h  |  12 +--
 6 files changed, 228 insertions(+), 109 deletions(-)

-- 
2.37.3

[PATCH 1/6] rpmsg: glink: Extract tx kick operation

[Bjorn Andersson]

Refactor out the tx kick operations to its own function, in preparation
for pushing the details to the individual transports.

Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
---
 drivers/rpmsg/qcom_glink_native.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
index 115c0a1eddb1..5fd8b70271b7 100644
--- a/drivers/rpmsg/qcom_glink_native.c
+++ b/drivers/rpmsg/qcom_glink_native.c
@@ -303,6 +303,12 @@ static void qcom_glink_tx_write(struct qcom_glink *glink,
 	glink->tx_pipe->write(glink->tx_pipe, hdr, hlen, data, dlen);
 }
 
+static void qcom_glink_tx_kick(struct qcom_glink *glink)
+{
+	mbox_send_message(glink->mbox_chan, NULL);
+	mbox_client_txdone(glink->mbox_chan, 0);
+}
+
 static void qcom_glink_send_read_notify(struct qcom_glink *glink)
 {
 	struct glink_msg msg;
@@ -313,8 +319,7 @@ static void qcom_glink_send_read_notify(struct qcom_glink *glink)
 
 	qcom_glink_tx_write(glink, &msg, sizeof(msg), NULL, 0);
 
-	mbox_send_message(glink->mbox_chan, NULL);
-	mbox_client_txdone(glink->mbox_chan, 0);
+	qcom_glink_tx_kick(glink);
 }
 
 static int qcom_glink_tx(struct qcom_glink *glink,
@@ -355,9 +360,7 @@ static int qcom_glink_tx(struct qcom_glink *glink,
 	}
 
 	qcom_glink_tx_write(glink, hdr, hlen, data, dlen);
-
-	mbox_send_message(glink->mbox_chan, NULL);
-	mbox_client_txdone(glink->mbox_chan, 0);
+	qcom_glink_tx_kick(glink);
 
 out:
 	spin_unlock_irqrestore(&glink->tx_lock, flags);
@@ -1046,9 +1049,7 @@ static irqreturn_t qcom_glink_native_intr(int irq, void *data)
 			break;
 		case RPM_CMD_READ_NOTIF:
 			qcom_glink_rx_advance(glink, ALIGN(sizeof(msg), 8));
-
-			mbox_send_message(glink->mbox_chan, NULL);
-			mbox_client_txdone(glink->mbox_chan, 0);
+			qcom_glink_tx_kick(glink);
 			break;
 		case RPM_CMD_INTENT:
 			qcom_glink_handle_intent(glink, param1, param2, avail);
-- 
2.37.3

Re: [PATCH 1/6] rpmsg: glink: Extract tx kick operation

[Chris Lew]

On 1/9/2023 2:39 PM, Bjorn Andersson wrote:
> Refactor out the tx kick operations to its own function, in preparation
> for pushing the details to the individual transports.
> 
> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> ---

Reviewed-by: Chris Lew <quic_clew@quicinc.com>

>   drivers/rpmsg/qcom_glink_native.c | 17 +++++++++--------
>   1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
> index 115c0a1eddb1..5fd8b70271b7 100644
> --- a/drivers/rpmsg/qcom_glink_native.c
> +++ b/drivers/rpmsg/qcom_glink_native.c
> @@ -303,6 +303,12 @@ static void qcom_glink_tx_write(struct qcom_glink *glink,
>   	glink->tx_pipe->write(glink->tx_pipe, hdr, hlen, data, dlen);
>   }
>   
> +static void qcom_glink_tx_kick(struct qcom_glink *glink)
> +{
> +	mbox_send_message(glink->mbox_chan, NULL);
> +	mbox_client_txdone(glink->mbox_chan, 0);
> +}
> +
>   static void qcom_glink_send_read_notify(struct qcom_glink *glink)
>   {
>   	struct glink_msg msg;
> @@ -313,8 +319,7 @@ static void qcom_glink_send_read_notify(struct qcom_glink *glink)
>   
>   	qcom_glink_tx_write(glink, &msg, sizeof(msg), NULL, 0);
>   
> -	mbox_send_message(glink->mbox_chan, NULL);
> -	mbox_client_txdone(glink->mbox_chan, 0);
> +	qcom_glink_tx_kick(glink);
>   }
>   
>   static int qcom_glink_tx(struct qcom_glink *glink,
> @@ -355,9 +360,7 @@ static int qcom_glink_tx(struct qcom_glink *glink,
>   	}
>   
>   	qcom_glink_tx_write(glink, hdr, hlen, data, dlen);
> -
> -	mbox_send_message(glink->mbox_chan, NULL);
> -	mbox_client_txdone(glink->mbox_chan, 0);
> +	qcom_glink_tx_kick(glink);
>   
>   out:
>   	spin_unlock_irqrestore(&glink->tx_lock, flags);
> @@ -1046,9 +1049,7 @@ static irqreturn_t qcom_glink_native_intr(int irq, void *data)
>   			break;
>   		case RPM_CMD_READ_NOTIF:
>   			qcom_glink_rx_advance(glink, ALIGN(sizeof(msg), 8));
> -
> -			mbox_send_message(glink->mbox_chan, NULL);
> -			mbox_client_txdone(glink->mbox_chan, 0);
> +			qcom_glink_tx_kick(glink);
>   			break;
>   		case RPM_CMD_INTENT:
>   			qcom_glink_handle_intent(glink, param1, param2, avail);

[PATCH 2/6] rpmsg: glink: smem: Wrap driver context

[Bjorn Andersson]

The Glink SMEM driver allocates a struct device and hangs two
devres-allocated pipe objects thereon. To facilitate the move of
interrupt and mailbox handling to the driver, introduce a wrapper object
capturing the device, glink reference and remote processor id.

The type of the remoteproc reference is updated, as these are
specifically targetting the SMEM implementation.

Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
---
 drivers/remoteproc/qcom_common.h |  3 +-
 drivers/rpmsg/qcom_glink_smem.c  | 76 ++++++++++++++++++++------------
 include/linux/rpmsg/qcom_glink.h | 12 ++---
 3 files changed, 55 insertions(+), 36 deletions(-)

diff --git a/drivers/remoteproc/qcom_common.h b/drivers/remoteproc/qcom_common.h
index c35adf730be0..2747c7d9ba44 100644
--- a/drivers/remoteproc/qcom_common.h
+++ b/drivers/remoteproc/qcom_common.h
@@ -6,6 +6,7 @@
 #include "remoteproc_internal.h"
 #include <linux/soc/qcom/qmi.h>
 
+struct qcom_glink_smem;
 struct qcom_sysmon;
 
 struct qcom_rproc_glink {
@@ -15,7 +16,7 @@ struct qcom_rproc_glink {
 
 	struct device *dev;
 	struct device_node *node;
-	struct qcom_glink *edge;
+	struct qcom_glink_smem *edge;
 };
 
 struct qcom_rproc_subdev {
diff --git a/drivers/rpmsg/qcom_glink_smem.c b/drivers/rpmsg/qcom_glink_smem.c
index 579bc4443f6d..703e63fa5a86 100644
--- a/drivers/rpmsg/qcom_glink_smem.c
+++ b/drivers/rpmsg/qcom_glink_smem.c
@@ -33,6 +33,14 @@
 #define SMEM_GLINK_NATIVE_XPRT_FIFO_0		479
 #define SMEM_GLINK_NATIVE_XPRT_FIFO_1		480
 
+struct qcom_glink_smem {
+	struct device dev;
+
+	struct qcom_glink *glink;
+
+	u32 remote_pid;
+};
+
 struct glink_smem_pipe {
 	struct qcom_glink_pipe native;
 
@@ -41,7 +49,7 @@ struct glink_smem_pipe {
 
 	void *fifo;
 
-	int remote_pid;
+	struct qcom_glink_smem *smem;
 };
 
 #define to_smem_pipe(p) container_of(p, struct glink_smem_pipe, native)
@@ -49,13 +57,14 @@ struct glink_smem_pipe {
 static size_t glink_smem_rx_avail(struct qcom_glink_pipe *np)
 {
 	struct glink_smem_pipe *pipe = to_smem_pipe(np);
+	struct qcom_glink_smem *smem = pipe->smem;
 	size_t len;
 	void *fifo;
 	u32 head;
 	u32 tail;
 
 	if (!pipe->fifo) {
-		fifo = qcom_smem_get(pipe->remote_pid,
+		fifo = qcom_smem_get(smem->remote_pid,
 				     SMEM_GLINK_NATIVE_XPRT_FIFO_1, &len);
 		if (IS_ERR(fifo)) {
 			pr_err("failed to acquire RX fifo handle: %ld\n",
@@ -179,45 +188,49 @@ static void glink_smem_tx_write(struct qcom_glink_pipe *glink_pipe,
 
 static void qcom_glink_smem_release(struct device *dev)
 {
-	kfree(dev);
+	struct qcom_glink_smem *smem = container_of(dev, struct qcom_glink_smem, dev);
+
+	kfree(smem);
 }
 
-struct qcom_glink *qcom_glink_smem_register(struct device *parent,
-					    struct device_node *node)
+struct qcom_glink_smem *qcom_glink_smem_register(struct device *parent,
+						 struct device_node *node)
 {
 	struct glink_smem_pipe *rx_pipe;
 	struct glink_smem_pipe *tx_pipe;
 	struct qcom_glink *glink;
-	struct device *dev;
+	struct qcom_glink_smem *smem;
 	u32 remote_pid;
 	__le32 *descs;
 	size_t size;
 	int ret;
 
-	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
-	if (!dev)
+	smem = kzalloc(sizeof(*smem), GFP_KERNEL);
+	if (!smem)
 		return ERR_PTR(-ENOMEM);
 
-	dev->parent = parent;
-	dev->of_node = node;
-	dev->release = qcom_glink_smem_release;
-	dev_set_name(dev, "%s:%pOFn", dev_name(parent->parent), node);
-	ret = device_register(dev);
+	smem->dev.parent = parent;
+	smem->dev.of_node = node;
+	smem->dev.release = qcom_glink_smem_release;
+	dev_set_name(&smem->dev, "%s:%pOFn", dev_name(parent->parent), node);
+	ret = device_register(&smem->dev);
 	if (ret) {
 		pr_err("failed to register glink edge\n");
-		put_device(dev);
+		put_device(&smem->dev);
 		return ERR_PTR(ret);
 	}
 
-	ret = of_property_read_u32(dev->of_node, "qcom,remote-pid",
+	ret = of_property_read_u32(smem->dev.of_node, "qcom,remote-pid",
 				   &remote_pid);
 	if (ret) {
-		dev_err(dev, "failed to parse qcom,remote-pid\n");
+		dev_err(&smem->dev, "failed to parse qcom,remote-pid\n");
 		goto err_put_dev;
 	}
 
-	rx_pipe = devm_kzalloc(dev, sizeof(*rx_pipe), GFP_KERNEL);
-	tx_pipe = devm_kzalloc(dev, sizeof(*tx_pipe), GFP_KERNEL);
+	smem->remote_pid = remote_pid;
+
+	rx_pipe = devm_kzalloc(&smem->dev, sizeof(*rx_pipe), GFP_KERNEL);
+	tx_pipe = devm_kzalloc(&smem->dev, sizeof(*tx_pipe), GFP_KERNEL);
 	if (!rx_pipe || !tx_pipe) {
 		ret = -ENOMEM;
 		goto err_put_dev;
@@ -226,20 +239,20 @@ struct qcom_glink *qcom_glink_smem_register(struct device *parent,
 	ret = qcom_smem_alloc(remote_pid,
 			      SMEM_GLINK_NATIVE_XPRT_DESCRIPTOR, 32);
 	if (ret && ret != -EEXIST) {
-		dev_err(dev, "failed to allocate glink descriptors\n");
+		dev_err(&smem->dev, "failed to allocate glink descriptors\n");
 		goto err_put_dev;
 	}
 
 	descs = qcom_smem_get(remote_pid,
 			      SMEM_GLINK_NATIVE_XPRT_DESCRIPTOR, &size);
 	if (IS_ERR(descs)) {
-		dev_err(dev, "failed to acquire xprt descriptor\n");
+		dev_err(&smem->dev, "failed to acquire xprt descriptor\n");
 		ret = PTR_ERR(descs);
 		goto err_put_dev;
 	}
 
 	if (size != 32) {
-		dev_err(dev, "glink descriptor of invalid size\n");
+		dev_err(&smem->dev, "glink descriptor of invalid size\n");
 		ret = -EINVAL;
 		goto err_put_dev;
 	}
@@ -252,31 +265,31 @@ struct qcom_glink *qcom_glink_smem_register(struct device *parent,
 	ret = qcom_smem_alloc(remote_pid, SMEM_GLINK_NATIVE_XPRT_FIFO_0,
 			      SZ_16K);
 	if (ret && ret != -EEXIST) {
-		dev_err(dev, "failed to allocate TX fifo\n");
+		dev_err(&smem->dev, "failed to allocate TX fifo\n");
 		goto err_put_dev;
 	}
 
 	tx_pipe->fifo = qcom_smem_get(remote_pid, SMEM_GLINK_NATIVE_XPRT_FIFO_0,
 				      &tx_pipe->native.length);
 	if (IS_ERR(tx_pipe->fifo)) {
-		dev_err(dev, "failed to acquire TX fifo\n");
+		dev_err(&smem->dev, "failed to acquire TX fifo\n");
 		ret = PTR_ERR(tx_pipe->fifo);
 		goto err_put_dev;
 	}
 
+	rx_pipe->smem = smem;
 	rx_pipe->native.avail = glink_smem_rx_avail;
 	rx_pipe->native.peak = glink_smem_rx_peak;
 	rx_pipe->native.advance = glink_smem_rx_advance;
-	rx_pipe->remote_pid = remote_pid;
 
+	tx_pipe->smem = smem;
 	tx_pipe->native.avail = glink_smem_tx_avail;
 	tx_pipe->native.write = glink_smem_tx_write;
-	tx_pipe->remote_pid = remote_pid;
 
 	*rx_pipe->tail = 0;
 	*tx_pipe->head = 0;
 
-	glink = qcom_glink_native_probe(dev,
+	glink = qcom_glink_native_probe(&smem->dev,
 					GLINK_FEATURE_INTENT_REUSE,
 					&rx_pipe->native, &tx_pipe->native,
 					false);
@@ -285,17 +298,22 @@ struct qcom_glink *qcom_glink_smem_register(struct device *parent,
 		goto err_put_dev;
 	}
 
-	return glink;
+	smem->glink = glink;
+
+	return smem;
+
 
 err_put_dev:
-	device_unregister(dev);
+	device_unregister(&smem->dev);
 
 	return ERR_PTR(ret);
 }
 EXPORT_SYMBOL_GPL(qcom_glink_smem_register);
 
-void qcom_glink_smem_unregister(struct qcom_glink *glink)
+void qcom_glink_smem_unregister(struct qcom_glink_smem *smem)
 {
+	struct qcom_glink *glink = smem->glink;
+
 	qcom_glink_native_remove(glink);
 	qcom_glink_native_unregister(glink);
 }
diff --git a/include/linux/rpmsg/qcom_glink.h b/include/linux/rpmsg/qcom_glink.h
index 22fc3a69b683..bfbd48f435fa 100644
--- a/include/linux/rpmsg/qcom_glink.h
+++ b/include/linux/rpmsg/qcom_glink.h
@@ -5,7 +5,7 @@
 
 #include <linux/device.h>
 
-struct qcom_glink;
+struct qcom_glink_smem;
 
 #if IS_ENABLED(CONFIG_RPMSG_QCOM_GLINK)
 void qcom_glink_ssr_notify(const char *ssr_name);
@@ -15,20 +15,20 @@ static inline void qcom_glink_ssr_notify(const char *ssr_name) {}
 
 #if IS_ENABLED(CONFIG_RPMSG_QCOM_GLINK_SMEM)
 
-struct qcom_glink *qcom_glink_smem_register(struct device *parent,
-					    struct device_node *node);
-void qcom_glink_smem_unregister(struct qcom_glink *glink);
+struct qcom_glink_smem *qcom_glink_smem_register(struct device *parent,
+						 struct device_node *node);
+void qcom_glink_smem_unregister(struct qcom_glink_smem *glink);
 
 #else
 
-static inline struct qcom_glink *
+static inline struct qcom_glink_smem *
 qcom_glink_smem_register(struct device *parent,
 			 struct device_node *node)
 {
 	return NULL;
 }
 
-static inline void qcom_glink_smem_unregister(struct qcom_glink *glink) {}
+static inline void qcom_glink_smem_unregister(struct qcom_glink_smem *glink) {}
 #endif
 
 #endif
-- 
2.37.3

Re: [PATCH 2/6] rpmsg: glink: smem: Wrap driver context

[Chris Lew]

On 1/9/2023 2:39 PM, Bjorn Andersson wrote:
> The Glink SMEM driver allocates a struct device and hangs two
> devres-allocated pipe objects thereon. To facilitate the move of
> interrupt and mailbox handling to the driver, introduce a wrapper object
> capturing the device, glink reference and remote processor id.
> 
> The type of the remoteproc reference is updated, as these are
> specifically targetting the SMEM implementation.

s/targetting/targeting

> 
> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> ---
>   drivers/remoteproc/qcom_common.h |  3 +-
>   drivers/rpmsg/qcom_glink_smem.c  | 76 ++++++++++++++++++++------------
>   include/linux/rpmsg/qcom_glink.h | 12 ++---
>   3 files changed, 55 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/remoteproc/qcom_common.h b/drivers/remoteproc/qcom_common.h
> index c35adf730be0..2747c7d9ba44 100644
> --- a/drivers/remoteproc/qcom_common.h
> +++ b/drivers/remoteproc/qcom_common.h
> @@ -6,6 +6,7 @@
>   #include "remoteproc_internal.h"
>   #include <linux/soc/qcom/qmi.h>
>   
> +struct qcom_glink_smem;
>   struct qcom_sysmon;
>   
>   struct qcom_rproc_glink {
> @@ -15,7 +16,7 @@ struct qcom_rproc_glink {
>   
>   	struct device *dev;
>   	struct device_node *node;
> -	struct qcom_glink *edge;
> +	struct qcom_glink_smem *edge;
>   };
>   
>   struct qcom_rproc_subdev {
> diff --git a/drivers/rpmsg/qcom_glink_smem.c b/drivers/rpmsg/qcom_glink_smem.c
> index 579bc4443f6d..703e63fa5a86 100644
> --- a/drivers/rpmsg/qcom_glink_smem.c
> +++ b/drivers/rpmsg/qcom_glink_smem.c
> @@ -33,6 +33,14 @@
>   #define SMEM_GLINK_NATIVE_XPRT_FIFO_0		479
>   #define SMEM_GLINK_NATIVE_XPRT_FIFO_1		480
>   
> +struct qcom_glink_smem {
> +	struct device dev;
> +
> +	struct qcom_glink *glink;
> +
> +	u32 remote_pid;
> +};
> +
>   struct glink_smem_pipe {
>   	struct qcom_glink_pipe native;
>   
> @@ -41,7 +49,7 @@ struct glink_smem_pipe {
>   
>   	void *fifo;
>   
> -	int remote_pid;
> +	struct qcom_glink_smem *smem;
>   };
>   
>   #define to_smem_pipe(p) container_of(p, struct glink_smem_pipe, native)
> @@ -49,13 +57,14 @@ struct glink_smem_pipe {
>   static size_t glink_smem_rx_avail(struct qcom_glink_pipe *np)
>   {
>   	struct glink_smem_pipe *pipe = to_smem_pipe(np);
> +	struct qcom_glink_smem *smem = pipe->smem;
>   	size_t len;
>   	void *fifo;
>   	u32 head;
>   	u32 tail;
>   
>   	if (!pipe->fifo) {
> -		fifo = qcom_smem_get(pipe->remote_pid,
> +		fifo = qcom_smem_get(smem->remote_pid,
>   				     SMEM_GLINK_NATIVE_XPRT_FIFO_1, &len);
>   		if (IS_ERR(fifo)) {
>   			pr_err("failed to acquire RX fifo handle: %ld\n",
> @@ -179,45 +188,49 @@ static void glink_smem_tx_write(struct qcom_glink_pipe *glink_pipe,
>   
>   static void qcom_glink_smem_release(struct device *dev)
>   {
> -	kfree(dev);
> +	struct qcom_glink_smem *smem = container_of(dev, struct qcom_glink_smem, dev);
> +
> +	kfree(smem);
>   }
>   
> -struct qcom_glink *qcom_glink_smem_register(struct device *parent,
> -					    struct device_node *node)
> +struct qcom_glink_smem *qcom_glink_smem_register(struct device *parent,
> +						 struct device_node *node)
>   {
>   	struct glink_smem_pipe *rx_pipe;
>   	struct glink_smem_pipe *tx_pipe;
>   	struct qcom_glink *glink;
> -	struct device *dev;
> +	struct qcom_glink_smem *smem;

I think we're following reverse christmas tree in this file

>   	u32 remote_pid;
>   	__le32 *descs;
>   	size_t size;
>   	int ret;
>   
> -	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> -	if (!dev)
> +	smem = kzalloc(sizeof(*smem), GFP_KERNEL);
> +	if (!smem)
>   		return ERR_PTR(-ENOMEM);
>

Would it be proper to keep a pointer to dev and avoid all the changes to 
smem->dev use?

dev = &smem->dev;

> -	dev->parent = parent;
> -	dev->of_node = node;
> -	dev->release = qcom_glink_smem_release;
> -	dev_set_name(dev, "%s:%pOFn", dev_name(parent->parent), node);
> -	ret = device_register(dev);
> +	smem->dev.parent = parent;
> +	smem->dev.of_node = node;
> +	smem->dev.release = qcom_glink_smem_release;
> +	dev_set_name(&smem->dev, "%s:%pOFn", dev_name(parent->parent), node);
> +	ret = device_register(&smem->dev);
>   	if (ret) {
>   		pr_err("failed to register glink edge\n");
> -		put_device(dev);
> +		put_device(&smem->dev);
>   		return ERR_PTR(ret);
>   	}
>   
> -	ret = of_property_read_u32(dev->of_node, "qcom,remote-pid",
> +	ret = of_property_read_u32(smem->dev.of_node, "qcom,remote-pid",
>   				   &remote_pid);
>   	if (ret) {
> -		dev_err(dev, "failed to parse qcom,remote-pid\n");
> +		dev_err(&smem->dev, "failed to parse qcom,remote-pid\n");
>   		goto err_put_dev;
>   	}
>   
> -	rx_pipe = devm_kzalloc(dev, sizeof(*rx_pipe), GFP_KERNEL);
> -	tx_pipe = devm_kzalloc(dev, sizeof(*tx_pipe), GFP_KERNEL);
> +	smem->remote_pid = remote_pid;
> +
> +	rx_pipe = devm_kzalloc(&smem->dev, sizeof(*rx_pipe), GFP_KERNEL);
> +	tx_pipe = devm_kzalloc(&smem->dev, sizeof(*tx_pipe), GFP_KERNEL);
>   	if (!rx_pipe || !tx_pipe) {
>   		ret = -ENOMEM;
>   		goto err_put_dev;
> @@ -226,20 +239,20 @@ struct qcom_glink *qcom_glink_smem_register(struct device *parent,
>   	ret = qcom_smem_alloc(remote_pid,
>   			      SMEM_GLINK_NATIVE_XPRT_DESCRIPTOR, 32);
>   	if (ret && ret != -EEXIST) {
> -		dev_err(dev, "failed to allocate glink descriptors\n");
> +		dev_err(&smem->dev, "failed to allocate glink descriptors\n");
>   		goto err_put_dev;
>   	}
>   
>   	descs = qcom_smem_get(remote_pid,
>   			      SMEM_GLINK_NATIVE_XPRT_DESCRIPTOR, &size);
>   	if (IS_ERR(descs)) {
> -		dev_err(dev, "failed to acquire xprt descriptor\n");
> +		dev_err(&smem->dev, "failed to acquire xprt descriptor\n");
>   		ret = PTR_ERR(descs);
>   		goto err_put_dev;
>   	}
>   
>   	if (size != 32) {
> -		dev_err(dev, "glink descriptor of invalid size\n");
> +		dev_err(&smem->dev, "glink descriptor of invalid size\n");
>   		ret = -EINVAL;
>   		goto err_put_dev;
>   	}
> @@ -252,31 +265,31 @@ struct qcom_glink *qcom_glink_smem_register(struct device *parent,
>   	ret = qcom_smem_alloc(remote_pid, SMEM_GLINK_NATIVE_XPRT_FIFO_0,
>   			      SZ_16K);
>   	if (ret && ret != -EEXIST) {
> -		dev_err(dev, "failed to allocate TX fifo\n");
> +		dev_err(&smem->dev, "failed to allocate TX fifo\n");
>   		goto err_put_dev;
>   	}
>   
>   	tx_pipe->fifo = qcom_smem_get(remote_pid, SMEM_GLINK_NATIVE_XPRT_FIFO_0,
>   				      &tx_pipe->native.length);
>   	if (IS_ERR(tx_pipe->fifo)) {
> -		dev_err(dev, "failed to acquire TX fifo\n");
> +		dev_err(&smem->dev, "failed to acquire TX fifo\n");
>   		ret = PTR_ERR(tx_pipe->fifo);
>   		goto err_put_dev;
>   	}
>   
> +	rx_pipe->smem = smem;
>   	rx_pipe->native.avail = glink_smem_rx_avail;
>   	rx_pipe->native.peak = glink_smem_rx_peak;
>   	rx_pipe->native.advance = glink_smem_rx_advance;
> -	rx_pipe->remote_pid = remote_pid;
>   
> +	tx_pipe->smem = smem;
>   	tx_pipe->native.avail = glink_smem_tx_avail;
>   	tx_pipe->native.write = glink_smem_tx_write;
> -	tx_pipe->remote_pid = remote_pid;
>   
>   	*rx_pipe->tail = 0;
>   	*tx_pipe->head = 0;
>   
> -	glink = qcom_glink_native_probe(dev,
> +	glink = qcom_glink_native_probe(&smem->dev,
>   					GLINK_FEATURE_INTENT_REUSE,
>   					&rx_pipe->native, &tx_pipe->native,
>   					false);
> @@ -285,17 +298,22 @@ struct qcom_glink *qcom_glink_smem_register(struct device *parent,
>   		goto err_put_dev;
>   	}
>   
> -	return glink;
> +	smem->glink = glink;
> +
> +	return smem;
> +
>   
>   err_put_dev:
> -	device_unregister(dev);
> +	device_unregister(&smem->dev);
>   
>   	return ERR_PTR(ret);
>   }
>   EXPORT_SYMBOL_GPL(qcom_glink_smem_register);
>   
> -void qcom_glink_smem_unregister(struct qcom_glink *glink)
> +void qcom_glink_smem_unregister(struct qcom_glink_smem *smem)
>   {
> +	struct qcom_glink *glink = smem->glink;
> +
>   	qcom_glink_native_remove(glink);
>   	qcom_glink_native_unregister(glink);
>   }
> diff --git a/include/linux/rpmsg/qcom_glink.h b/include/linux/rpmsg/qcom_glink.h
> index 22fc3a69b683..bfbd48f435fa 100644
> --- a/include/linux/rpmsg/qcom_glink.h
> +++ b/include/linux/rpmsg/qcom_glink.h
> @@ -5,7 +5,7 @@
>   
>   #include <linux/device.h>
>   
> -struct qcom_glink;
> +struct qcom_glink_smem;
>   
>   #if IS_ENABLED(CONFIG_RPMSG_QCOM_GLINK)
>   void qcom_glink_ssr_notify(const char *ssr_name);
> @@ -15,20 +15,20 @@ static inline void qcom_glink_ssr_notify(const char *ssr_name) {}
>   
>   #if IS_ENABLED(CONFIG_RPMSG_QCOM_GLINK_SMEM)
>   
> -struct qcom_glink *qcom_glink_smem_register(struct device *parent,
> -					    struct device_node *node);
> -void qcom_glink_smem_unregister(struct qcom_glink *glink);
> +struct qcom_glink_smem *qcom_glink_smem_register(struct device *parent,
> +						 struct device_node *node);
> +void qcom_glink_smem_unregister(struct qcom_glink_smem *glink);
>   
>   #else
>   
> -static inline struct qcom_glink *
> +static inline struct qcom_glink_smem *
>   qcom_glink_smem_register(struct device *parent,
>   			 struct device_node *node)
>   {
>   	return NULL;
>   }
>   
> -static inline void qcom_glink_smem_unregister(struct qcom_glink *glink) {}
> +static inline void qcom_glink_smem_unregister(struct qcom_glink_smem *glink) {}
>   #endif
>   
>   #endif

Re: [PATCH 2/6] rpmsg: glink: smem: Wrap driver context

[Bjorn Andersson]

On Tue, Jan 24, 2023 at 10:30:42PM -0800, Chris Lew wrote:
> 
> 
> On 1/9/2023 2:39 PM, Bjorn Andersson wrote:
> > The Glink SMEM driver allocates a struct device and hangs two
> > devres-allocated pipe objects thereon. To facilitate the move of
> > interrupt and mailbox handling to the driver, introduce a wrapper object
> > capturing the device, glink reference and remote processor id.
> > 
> > The type of the remoteproc reference is updated, as these are
> > specifically targetting the SMEM implementation.
> 
> s/targetting/targeting
> 

Thank you.

> > 
> > Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> > ---
> >   drivers/remoteproc/qcom_common.h |  3 +-
> >   drivers/rpmsg/qcom_glink_smem.c  | 76 ++++++++++++++++++++------------
> >   include/linux/rpmsg/qcom_glink.h | 12 ++---
> >   3 files changed, 55 insertions(+), 36 deletions(-)
> > 
> > diff --git a/drivers/remoteproc/qcom_common.h b/drivers/remoteproc/qcom_common.h
> > index c35adf730be0..2747c7d9ba44 100644
> > --- a/drivers/remoteproc/qcom_common.h
> > +++ b/drivers/remoteproc/qcom_common.h
> > @@ -6,6 +6,7 @@
> >   #include "remoteproc_internal.h"
> >   #include <linux/soc/qcom/qmi.h>
> > +struct qcom_glink_smem;
> >   struct qcom_sysmon;
> >   struct qcom_rproc_glink {
> > @@ -15,7 +16,7 @@ struct qcom_rproc_glink {
> >   	struct device *dev;
> >   	struct device_node *node;
> > -	struct qcom_glink *edge;
> > +	struct qcom_glink_smem *edge;
> >   };
> >   struct qcom_rproc_subdev {
> > diff --git a/drivers/rpmsg/qcom_glink_smem.c b/drivers/rpmsg/qcom_glink_smem.c
> > index 579bc4443f6d..703e63fa5a86 100644
> > --- a/drivers/rpmsg/qcom_glink_smem.c
> > +++ b/drivers/rpmsg/qcom_glink_smem.c
> > @@ -33,6 +33,14 @@
> >   #define SMEM_GLINK_NATIVE_XPRT_FIFO_0		479
> >   #define SMEM_GLINK_NATIVE_XPRT_FIFO_1		480
> > +struct qcom_glink_smem {
> > +	struct device dev;
> > +
> > +	struct qcom_glink *glink;
> > +
> > +	u32 remote_pid;
> > +};
> > +
> >   struct glink_smem_pipe {
> >   	struct qcom_glink_pipe native;
> > @@ -41,7 +49,7 @@ struct glink_smem_pipe {
> >   	void *fifo;
> > -	int remote_pid;
> > +	struct qcom_glink_smem *smem;
> >   };
> >   #define to_smem_pipe(p) container_of(p, struct glink_smem_pipe, native)
> > @@ -49,13 +57,14 @@ struct glink_smem_pipe {
> >   static size_t glink_smem_rx_avail(struct qcom_glink_pipe *np)
> >   {
> >   	struct glink_smem_pipe *pipe = to_smem_pipe(np);
> > +	struct qcom_glink_smem *smem = pipe->smem;
> >   	size_t len;
> >   	void *fifo;
> >   	u32 head;
> >   	u32 tail;
> >   	if (!pipe->fifo) {
> > -		fifo = qcom_smem_get(pipe->remote_pid,
> > +		fifo = qcom_smem_get(smem->remote_pid,
> >   				     SMEM_GLINK_NATIVE_XPRT_FIFO_1, &len);
> >   		if (IS_ERR(fifo)) {
> >   			pr_err("failed to acquire RX fifo handle: %ld\n",
> > @@ -179,45 +188,49 @@ static void glink_smem_tx_write(struct qcom_glink_pipe *glink_pipe,
> >   static void qcom_glink_smem_release(struct device *dev)
> >   {
> > -	kfree(dev);
> > +	struct qcom_glink_smem *smem = container_of(dev, struct qcom_glink_smem, dev);
> > +
> > +	kfree(smem);
> >   }
> > -struct qcom_glink *qcom_glink_smem_register(struct device *parent,
> > -					    struct device_node *node)
> > +struct qcom_glink_smem *qcom_glink_smem_register(struct device *parent,
> > +						 struct device_node *node)
> >   {
> >   	struct glink_smem_pipe *rx_pipe;
> >   	struct glink_smem_pipe *tx_pipe;
> >   	struct qcom_glink *glink;
> > -	struct device *dev;
> > +	struct qcom_glink_smem *smem;
> 
> I think we're following reverse christmas tree in this file
> 
> >   	u32 remote_pid;
> >   	__le32 *descs;
> >   	size_t size;
> >   	int ret;
> > -	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> > -	if (!dev)
> > +	smem = kzalloc(sizeof(*smem), GFP_KERNEL);
> > +	if (!smem)
> >   		return ERR_PTR(-ENOMEM);
> > 
> 
> Would it be proper to keep a pointer to dev and avoid all the changes to
> smem->dev use?
> 
> dev = &smem->dev;
> 

That seems reasonable. Will respin accordingly.

Thanks,
Bjorn

[PATCH 3/6] rpmsg: glink: rpm: Wrap driver context

[Bjorn Andersson]

As with the SMEM driver update, wrap the RPM context in a struct to
facilitate the upcoming changes of moving IRQ and mailbox registration
to the driver.

Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
---
 drivers/rpmsg/qcom_glink_rpm.c | 44 ++++++++++++++++++++--------------
 1 file changed, 26 insertions(+), 18 deletions(-)

diff --git a/drivers/rpmsg/qcom_glink_rpm.c b/drivers/rpmsg/qcom_glink_rpm.c
index f64f45d1a735..6443843df6ca 100644
--- a/drivers/rpmsg/qcom_glink_rpm.c
+++ b/drivers/rpmsg/qcom_glink_rpm.c
@@ -53,6 +53,13 @@ struct glink_rpm_pipe {
 	void __iomem *fifo;
 };
 
+struct glink_rpm {
+	struct qcom_glink *glink;
+
+	struct glink_rpm_pipe rx_pipe;
+	struct glink_rpm_pipe tx_pipe;
+};
+
 static size_t glink_rpm_rx_avail(struct qcom_glink_pipe *glink_pipe)
 {
 	struct glink_rpm_pipe *pipe = to_rpm_pipe(glink_pipe);
@@ -257,8 +264,7 @@ static int glink_rpm_parse_toc(struct device *dev,
 static int glink_rpm_probe(struct platform_device *pdev)
 {
 	struct qcom_glink *glink;
-	struct glink_rpm_pipe *rx_pipe;
-	struct glink_rpm_pipe *tx_pipe;
+	struct glink_rpm *rpm;
 	struct device_node *np;
 	void __iomem *msg_ram;
 	size_t msg_ram_size;
@@ -266,9 +272,8 @@ static int glink_rpm_probe(struct platform_device *pdev)
 	struct resource r;
 	int ret;
 
-	rx_pipe = devm_kzalloc(&pdev->dev, sizeof(*rx_pipe), GFP_KERNEL);
-	tx_pipe = devm_kzalloc(&pdev->dev, sizeof(*tx_pipe), GFP_KERNEL);
-	if (!rx_pipe || !tx_pipe)
+	rpm = devm_kzalloc(&pdev->dev, sizeof(*rpm), GFP_KERNEL);
+	if (!rpm)
 		return -ENOMEM;
 
 	np = of_parse_phandle(dev->of_node, "qcom,rpm-msg-ram", 0);
@@ -283,36 +288,39 @@ static int glink_rpm_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	ret = glink_rpm_parse_toc(dev, msg_ram, msg_ram_size,
-				  rx_pipe, tx_pipe);
+				  &rpm->rx_pipe, &rpm->tx_pipe);
 	if (ret)
 		return ret;
 
 	/* Pipe specific accessors */
-	rx_pipe->native.avail = glink_rpm_rx_avail;
-	rx_pipe->native.peak = glink_rpm_rx_peak;
-	rx_pipe->native.advance = glink_rpm_rx_advance;
-	tx_pipe->native.avail = glink_rpm_tx_avail;
-	tx_pipe->native.write = glink_rpm_tx_write;
+	rpm->rx_pipe.native.avail = glink_rpm_rx_avail;
+	rpm->rx_pipe.native.peak = glink_rpm_rx_peak;
+	rpm->rx_pipe.native.advance = glink_rpm_rx_advance;
+	rpm->tx_pipe.native.avail = glink_rpm_tx_avail;
+	rpm->tx_pipe.native.write = glink_rpm_tx_write;
 
-	writel(0, tx_pipe->head);
-	writel(0, rx_pipe->tail);
+	writel(0, rpm->tx_pipe.head);
+	writel(0, rpm->rx_pipe.tail);
 
-	glink = qcom_glink_native_probe(&pdev->dev,
+	glink = qcom_glink_native_probe(dev,
 					0,
-					&rx_pipe->native,
-					&tx_pipe->native,
+					&rpm->rx_pipe.native,
+					&rpm->tx_pipe.native,
 					true);
 	if (IS_ERR(glink))
 		return PTR_ERR(glink);
 
-	platform_set_drvdata(pdev, glink);
+	rpm->glink = glink;
+
+	platform_set_drvdata(pdev, rpm);
 
 	return 0;
 }
 
 static int glink_rpm_remove(struct platform_device *pdev)
 {
-	struct qcom_glink *glink = platform_get_drvdata(pdev);
+	struct glink_rpm *rpm = platform_get_drvdata(pdev);
+	struct qcom_glink *glink = rpm->glink;
 
 	qcom_glink_native_remove(glink);
 
-- 
2.37.3

Re: [PATCH 3/6] rpmsg: glink: rpm: Wrap driver context

[Chris Lew]

On 1/9/2023 2:39 PM, Bjorn Andersson wrote:
> As with the SMEM driver update, wrap the RPM context in a struct to
> facilitate the upcoming changes of moving IRQ and mailbox registration
> to the driver.
> 
> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> ---

Reviewed-by: Chris Lew <quic_clew@quicinc.com>

>   drivers/rpmsg/qcom_glink_rpm.c | 44 ++++++++++++++++++++--------------
>   1 file changed, 26 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/rpmsg/qcom_glink_rpm.c b/drivers/rpmsg/qcom_glink_rpm.c
> index f64f45d1a735..6443843df6ca 100644
> --- a/drivers/rpmsg/qcom_glink_rpm.c
> +++ b/drivers/rpmsg/qcom_glink_rpm.c
> @@ -53,6 +53,13 @@ struct glink_rpm_pipe {
>   	void __iomem *fifo;
>   };
>   
> +struct glink_rpm {
> +	struct qcom_glink *glink;
> +
> +	struct glink_rpm_pipe rx_pipe;
> +	struct glink_rpm_pipe tx_pipe;
> +};
> +
>   static size_t glink_rpm_rx_avail(struct qcom_glink_pipe *glink_pipe)
>   {
>   	struct glink_rpm_pipe *pipe = to_rpm_pipe(glink_pipe);
> @@ -257,8 +264,7 @@ static int glink_rpm_parse_toc(struct device *dev,
>   static int glink_rpm_probe(struct platform_device *pdev)
>   {
>   	struct qcom_glink *glink;
> -	struct glink_rpm_pipe *rx_pipe;
> -	struct glink_rpm_pipe *tx_pipe;
> +	struct glink_rpm *rpm;
>   	struct device_node *np;
>   	void __iomem *msg_ram;
>   	size_t msg_ram_size;
> @@ -266,9 +272,8 @@ static int glink_rpm_probe(struct platform_device *pdev)
>   	struct resource r;
>   	int ret;
>   
> -	rx_pipe = devm_kzalloc(&pdev->dev, sizeof(*rx_pipe), GFP_KERNEL);
> -	tx_pipe = devm_kzalloc(&pdev->dev, sizeof(*tx_pipe), GFP_KERNEL);
> -	if (!rx_pipe || !tx_pipe)
> +	rpm = devm_kzalloc(&pdev->dev, sizeof(*rpm), GFP_KERNEL);
> +	if (!rpm)
>   		return -ENOMEM;
>   
>   	np = of_parse_phandle(dev->of_node, "qcom,rpm-msg-ram", 0);
> @@ -283,36 +288,39 @@ static int glink_rpm_probe(struct platform_device *pdev)
>   		return -ENOMEM;
>   
>   	ret = glink_rpm_parse_toc(dev, msg_ram, msg_ram_size,
> -				  rx_pipe, tx_pipe);
> +				  &rpm->rx_pipe, &rpm->tx_pipe);
>   	if (ret)
>   		return ret;
>   
>   	/* Pipe specific accessors */
> -	rx_pipe->native.avail = glink_rpm_rx_avail;
> -	rx_pipe->native.peak = glink_rpm_rx_peak;
> -	rx_pipe->native.advance = glink_rpm_rx_advance;
> -	tx_pipe->native.avail = glink_rpm_tx_avail;
> -	tx_pipe->native.write = glink_rpm_tx_write;
> +	rpm->rx_pipe.native.avail = glink_rpm_rx_avail;
> +	rpm->rx_pipe.native.peak = glink_rpm_rx_peak;
> +	rpm->rx_pipe.native.advance = glink_rpm_rx_advance;
> +	rpm->tx_pipe.native.avail = glink_rpm_tx_avail;
> +	rpm->tx_pipe.native.write = glink_rpm_tx_write;
>   
> -	writel(0, tx_pipe->head);
> -	writel(0, rx_pipe->tail);
> +	writel(0, rpm->tx_pipe.head);
> +	writel(0, rpm->rx_pipe.tail);
>   
> -	glink = qcom_glink_native_probe(&pdev->dev,
> +	glink = qcom_glink_native_probe(dev,
>   					0,
> -					&rx_pipe->native,
> -					&tx_pipe->native,
> +					&rpm->rx_pipe.native,
> +					&rpm->tx_pipe.native,
>   					true);
>   	if (IS_ERR(glink))
>   		return PTR_ERR(glink);
>   
> -	platform_set_drvdata(pdev, glink);
> +	rpm->glink = glink;
> +
> +	platform_set_drvdata(pdev, rpm);
>   
>   	return 0;
>   }
>   
>   static int glink_rpm_remove(struct platform_device *pdev)
>   {
> -	struct qcom_glink *glink = platform_get_drvdata(pdev);
> +	struct glink_rpm *rpm = platform_get_drvdata(pdev);
> +	struct qcom_glink *glink = rpm->glink;
>   
>   	qcom_glink_native_remove(glink);
>   

[PATCH 4/6] rpmsg: glink: Move irq and mbox handling to transports

[Bjorn Andersson]

Not all GLINK transports uses an interrupt and a mailbox instance. The
interrupt for RPM needs to be IRQF_NOSUSPEND, while it seems reasonable
for the SMEM interrupt to use irq_set_wake. The glink struct device is
constructed in the SMEM and RPM drivers but torn down in the core
driver.

Move the interrupt and kick handling into the SMEM and RPM driver, to
improve this and facilitate further improvements.

Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
---
 drivers/rpmsg/qcom_glink_native.c | 48 ++------------------------
 drivers/rpmsg/qcom_glink_native.h |  3 +-
 drivers/rpmsg/qcom_glink_rpm.c    | 50 ++++++++++++++++++++++++++-
 drivers/rpmsg/qcom_glink_smem.c   | 56 +++++++++++++++++++++++++++++--
 4 files changed, 108 insertions(+), 49 deletions(-)

diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
index 5fd8b70271b7..db5d946d5901 100644
--- a/drivers/rpmsg/qcom_glink_native.c
+++ b/drivers/rpmsg/qcom_glink_native.c
@@ -11,7 +11,6 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
-#include <linux/of_irq.h>
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
 #include <linux/rpmsg.h>
@@ -78,11 +77,8 @@ struct glink_core_rx_intent {
 /**
  * struct qcom_glink - driver context, relates to one remote subsystem
  * @dev:	reference to the associated struct device
- * @mbox_client: mailbox client
- * @mbox_chan:  mailbox channel
  * @rx_pipe:	pipe object for receive FIFO
  * @tx_pipe:	pipe object for transmit FIFO
- * @irq:	IRQ for signaling incoming events
  * @rx_work:	worker for handling received control messages
  * @rx_lock:	protects the @rx_queue
  * @rx_queue:	queue of received control messages to be processed in @rx_work
@@ -98,14 +94,9 @@ struct glink_core_rx_intent {
 struct qcom_glink {
 	struct device *dev;
 
-	struct mbox_client mbox_client;
-	struct mbox_chan *mbox_chan;
-
 	struct qcom_glink_pipe *rx_pipe;
 	struct qcom_glink_pipe *tx_pipe;
 
-	int irq;
-
 	struct work_struct rx_work;
 	spinlock_t rx_lock;
 	struct list_head rx_queue;
@@ -305,8 +296,7 @@ static void qcom_glink_tx_write(struct qcom_glink *glink,
 
 static void qcom_glink_tx_kick(struct qcom_glink *glink)
 {
-	mbox_send_message(glink->mbox_chan, NULL);
-	mbox_client_txdone(glink->mbox_chan, 0);
+	glink->tx_pipe->kick(glink->tx_pipe);
 }
 
 static void qcom_glink_send_read_notify(struct qcom_glink *glink)
@@ -1004,9 +994,8 @@ static int qcom_glink_rx_open_ack(struct qcom_glink *glink, unsigned int lcid)
 	return 0;
 }
 
-static irqreturn_t qcom_glink_native_intr(int irq, void *data)
+void qcom_glink_native_intr(struct qcom_glink *glink)
 {
-	struct qcom_glink *glink = data;
 	struct glink_msg msg;
 	unsigned int param1;
 	unsigned int param2;
@@ -1075,9 +1064,8 @@ static irqreturn_t qcom_glink_native_intr(int irq, void *data)
 		if (ret)
 			break;
 	}
-
-	return IRQ_HANDLED;
 }
+EXPORT_SYMBOL(qcom_glink_native_intr);
 
 /* Locally initiated rpmsg_create_ept */
 static struct glink_channel *qcom_glink_create_local(struct qcom_glink *glink,
@@ -1723,7 +1711,6 @@ struct qcom_glink *qcom_glink_native_probe(struct device *dev,
 					   struct qcom_glink_pipe *tx,
 					   bool intentless)
 {
-	int irq;
 	int ret;
 	struct qcom_glink *glink;
 
@@ -1754,27 +1741,6 @@ struct qcom_glink *qcom_glink_native_probe(struct device *dev,
 	if (ret)
 		dev_err(dev, "failed to add groups\n");
 
-	glink->mbox_client.dev = dev;
-	glink->mbox_client.knows_txdone = true;
-	glink->mbox_chan = mbox_request_channel(&glink->mbox_client, 0);
-	if (IS_ERR(glink->mbox_chan)) {
-		if (PTR_ERR(glink->mbox_chan) != -EPROBE_DEFER)
-			dev_err(dev, "failed to acquire IPC channel\n");
-		return ERR_CAST(glink->mbox_chan);
-	}
-
-	irq = of_irq_get(dev->of_node, 0);
-	ret = devm_request_irq(dev, irq,
-			       qcom_glink_native_intr,
-			       IRQF_NO_SUSPEND | IRQF_SHARED,
-			       "glink-native", glink);
-	if (ret) {
-		dev_err(dev, "failed to request IRQ\n");
-		return ERR_PTR(ret);
-	}
-
-	glink->irq = irq;
-
 	ret = qcom_glink_send_version(glink);
 	if (ret)
 		return ERR_PTR(ret);
@@ -1800,7 +1766,6 @@ void qcom_glink_native_remove(struct qcom_glink *glink)
 	int cid;
 	int ret;
 
-	disable_irq(glink->irq);
 	qcom_glink_cancel_rx_work(glink);
 
 	ret = device_for_each_child(glink->dev, NULL, qcom_glink_remove_device);
@@ -1817,15 +1782,8 @@ void qcom_glink_native_remove(struct qcom_glink *glink)
 
 	idr_destroy(&glink->lcids);
 	idr_destroy(&glink->rcids);
-	mbox_free_channel(glink->mbox_chan);
 }
 EXPORT_SYMBOL_GPL(qcom_glink_native_remove);
 
-void qcom_glink_native_unregister(struct qcom_glink *glink)
-{
-	device_unregister(glink->dev);
-}
-EXPORT_SYMBOL_GPL(qcom_glink_native_unregister);
-
 MODULE_DESCRIPTION("Qualcomm GLINK driver");
 MODULE_LICENSE("GPL v2");
diff --git a/drivers/rpmsg/qcom_glink_native.h b/drivers/rpmsg/qcom_glink_native.h
index e9a8671616c7..0129fe1b2b6c 100644
--- a/drivers/rpmsg/qcom_glink_native.h
+++ b/drivers/rpmsg/qcom_glink_native.h
@@ -24,6 +24,7 @@ struct qcom_glink_pipe {
 	void (*write)(struct qcom_glink_pipe *glink_pipe,
 		      const void *hdr, size_t hlen,
 		      const void *data, size_t dlen);
+	void (*kick)(struct qcom_glink_pipe *glink_pipe);
 };
 
 struct device;
@@ -35,6 +36,6 @@ struct qcom_glink *qcom_glink_native_probe(struct device *dev,
 					   struct qcom_glink_pipe *tx,
 					   bool intentless);
 void qcom_glink_native_remove(struct qcom_glink *glink);
+void qcom_glink_native_intr(struct qcom_glink *glink);
 
-void qcom_glink_native_unregister(struct qcom_glink *glink);
 #endif
diff --git a/drivers/rpmsg/qcom_glink_rpm.c b/drivers/rpmsg/qcom_glink_rpm.c
index 6443843df6ca..9136645d6251 100644
--- a/drivers/rpmsg/qcom_glink_rpm.c
+++ b/drivers/rpmsg/qcom_glink_rpm.c
@@ -11,6 +11,7 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
+#include <linux/of_irq.h>
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
 #include <linux/rpmsg.h>
@@ -56,6 +57,11 @@ struct glink_rpm_pipe {
 struct glink_rpm {
 	struct qcom_glink *glink;
 
+	int irq;
+
+	struct mbox_client mbox_client;
+	struct mbox_chan *mbox_chan;
+
 	struct glink_rpm_pipe rx_pipe;
 	struct glink_rpm_pipe tx_pipe;
 };
@@ -186,6 +192,24 @@ static void glink_rpm_tx_write(struct qcom_glink_pipe *glink_pipe,
 	writel(head, pipe->head);
 }
 
+static void glink_rpm_tx_kick(struct qcom_glink_pipe *glink_pipe)
+{
+	struct glink_rpm_pipe *pipe = to_rpm_pipe(glink_pipe);
+	struct glink_rpm *rpm = container_of(pipe, struct glink_rpm, tx_pipe);
+
+	mbox_send_message(rpm->mbox_chan, NULL);
+	mbox_client_txdone(rpm->mbox_chan, 0);
+}
+
+static irqreturn_t qcom_glink_rpm_intr(int irq, void *data)
+{
+	struct glink_rpm *rpm = data;
+
+	qcom_glink_native_intr(rpm->glink);
+
+	return IRQ_HANDLED;
+}
+
 static int glink_rpm_parse_toc(struct device *dev,
 			       void __iomem *msg_ram,
 			       size_t msg_ram_size,
@@ -292,12 +316,28 @@ static int glink_rpm_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	rpm->irq = of_irq_get(dev->of_node, 0);
+	ret = devm_request_irq(dev, rpm->irq, qcom_glink_rpm_intr,
+			       IRQF_NO_SUSPEND | IRQF_NO_AUTOEN,
+			       "glink-rpm", rpm);
+	if (ret) {
+		dev_err(dev, "failed to request IRQ\n");
+		return ret;
+	}
+
+	rpm->mbox_client.dev = dev;
+	rpm->mbox_client.knows_txdone = true;
+	rpm->mbox_chan = mbox_request_channel(&rpm->mbox_client, 0);
+	if (IS_ERR(rpm->mbox_chan))
+		return dev_err_probe(dev, PTR_ERR(rpm->mbox_chan), "failed to acquire IPC channel\n");
+
 	/* Pipe specific accessors */
 	rpm->rx_pipe.native.avail = glink_rpm_rx_avail;
 	rpm->rx_pipe.native.peak = glink_rpm_rx_peak;
 	rpm->rx_pipe.native.advance = glink_rpm_rx_advance;
 	rpm->tx_pipe.native.avail = glink_rpm_tx_avail;
 	rpm->tx_pipe.native.write = glink_rpm_tx_write;
+	rpm->tx_pipe.native.kick = glink_rpm_tx_kick;
 
 	writel(0, rpm->tx_pipe.head);
 	writel(0, rpm->rx_pipe.tail);
@@ -307,13 +347,17 @@ static int glink_rpm_probe(struct platform_device *pdev)
 					&rpm->rx_pipe.native,
 					&rpm->tx_pipe.native,
 					true);
-	if (IS_ERR(glink))
+	if (IS_ERR(glink)) {
+		mbox_free_channel(rpm->mbox_chan);
 		return PTR_ERR(glink);
+	}
 
 	rpm->glink = glink;
 
 	platform_set_drvdata(pdev, rpm);
 
+	enable_irq(rpm->irq);
+
 	return 0;
 }
 
@@ -322,8 +366,12 @@ static int glink_rpm_remove(struct platform_device *pdev)
 	struct glink_rpm *rpm = platform_get_drvdata(pdev);
 	struct qcom_glink *glink = rpm->glink;
 
+	disable_irq(rpm->irq);
+
 	qcom_glink_native_remove(glink);
 
+	mbox_free_channel(rpm->mbox_chan);
+
 	return 0;
 }
 
diff --git a/drivers/rpmsg/qcom_glink_smem.c b/drivers/rpmsg/qcom_glink_smem.c
index 703e63fa5a86..eec47ae98d67 100644
--- a/drivers/rpmsg/qcom_glink_smem.c
+++ b/drivers/rpmsg/qcom_glink_smem.c
@@ -7,8 +7,10 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
+#include <linux/of_irq.h>
 #include <linux/interrupt.h>
 #include <linux/platform_device.h>
+#include <linux/mailbox_client.h>
 #include <linux/mfd/syscon.h>
 #include <linux/slab.h>
 #include <linux/rpmsg.h>
@@ -36,8 +38,12 @@
 struct qcom_glink_smem {
 	struct device dev;
 
+	int irq;
 	struct qcom_glink *glink;
 
+	struct mbox_client mbox_client;
+	struct mbox_chan *mbox_chan;
+
 	u32 remote_pid;
 };
 
@@ -186,6 +192,24 @@ static void glink_smem_tx_write(struct qcom_glink_pipe *glink_pipe,
 	*pipe->head = cpu_to_le32(head);
 }
 
+static void glink_smem_tx_kick(struct qcom_glink_pipe *glink_pipe)
+{
+	struct glink_smem_pipe *pipe = to_smem_pipe(glink_pipe);
+	struct qcom_glink_smem *smem = pipe->smem;
+
+	mbox_send_message(smem->mbox_chan, NULL);
+	mbox_client_txdone(smem->mbox_chan, 0);
+}
+
+static irqreturn_t qcom_glink_smem_intr(int irq, void *data)
+{
+	struct qcom_glink_smem *smem = data;
+
+	qcom_glink_native_intr(smem->glink);
+
+	return IRQ_HANDLED;
+}
+
 static void qcom_glink_smem_release(struct device *dev)
 {
 	struct qcom_glink_smem *smem = container_of(dev, struct qcom_glink_smem, dev);
@@ -277,6 +301,24 @@ struct qcom_glink_smem *qcom_glink_smem_register(struct device *parent,
 		goto err_put_dev;
 	}
 
+	smem->irq = of_irq_get(smem->dev.of_node, 0);
+	ret = devm_request_irq(&smem->dev, smem->irq, qcom_glink_smem_intr,
+			       IRQF_NO_SUSPEND | IRQF_NO_AUTOEN,
+			       "glink-smem", smem);
+	if (ret) {
+		dev_err(&smem->dev, "failed to request IRQ\n");
+		goto err_put_dev;
+	}
+
+	smem->mbox_client.dev = &smem->dev;
+	smem->mbox_client.knows_txdone = true;
+	smem->mbox_chan = mbox_request_channel(&smem->mbox_client, 0);
+	if (IS_ERR(smem->mbox_chan)) {
+		ret = dev_err_probe(&smem->dev, PTR_ERR(smem->mbox_chan),
+				    "failed to acquire IPC channel\n");
+		goto err_put_dev;
+	}
+
 	rx_pipe->smem = smem;
 	rx_pipe->native.avail = glink_smem_rx_avail;
 	rx_pipe->native.peak = glink_smem_rx_peak;
@@ -285,6 +327,7 @@ struct qcom_glink_smem *qcom_glink_smem_register(struct device *parent,
 	tx_pipe->smem = smem;
 	tx_pipe->native.avail = glink_smem_tx_avail;
 	tx_pipe->native.write = glink_smem_tx_write;
+	tx_pipe->native.kick = glink_smem_tx_kick;
 
 	*rx_pipe->tail = 0;
 	*tx_pipe->head = 0;
@@ -295,13 +338,17 @@ struct qcom_glink_smem *qcom_glink_smem_register(struct device *parent,
 					false);
 	if (IS_ERR(glink)) {
 		ret = PTR_ERR(glink);
-		goto err_put_dev;
+		goto err_free_mbox;
 	}
 
 	smem->glink = glink;
 
+	enable_irq(smem->irq);
+
 	return smem;
 
+err_free_mbox:
+	mbox_free_channel(smem->mbox_chan);
 
 err_put_dev:
 	device_unregister(&smem->dev);
@@ -314,8 +361,13 @@ void qcom_glink_smem_unregister(struct qcom_glink_smem *smem)
 {
 	struct qcom_glink *glink = smem->glink;
 
+	disable_irq(smem->irq);
+
 	qcom_glink_native_remove(glink);
-	qcom_glink_native_unregister(glink);
+
+	device_unregister(&smem->dev);
+
+	mbox_free_channel(smem->mbox_chan);
 }
 EXPORT_SYMBOL_GPL(qcom_glink_smem_unregister);
 
-- 
2.37.3

Re: [PATCH 4/6] rpmsg: glink: Move irq and mbox handling to transports

[Chris Lew]

On 1/9/2023 2:39 PM, Bjorn Andersson wrote:
> Not all GLINK transports uses an interrupt and a mailbox instance. The
> interrupt for RPM needs to be IRQF_NOSUSPEND, while it seems reasonable
> for the SMEM interrupt to use irq_set_wake. The glink struct device is
> constructed in the SMEM and RPM drivers but torn down in the core
> driver.
> 
> Move the interrupt and kick handling into the SMEM and RPM driver, to
> improve this and facilitate further improvements.
> 
> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> ---
>   drivers/rpmsg/qcom_glink_native.c | 48 ++------------------------
>   drivers/rpmsg/qcom_glink_native.h |  3 +-
>   drivers/rpmsg/qcom_glink_rpm.c    | 50 ++++++++++++++++++++++++++-
>   drivers/rpmsg/qcom_glink_smem.c   | 56 +++++++++++++++++++++++++++++--
>   4 files changed, 108 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
> index 5fd8b70271b7..db5d946d5901 100644
> --- a/drivers/rpmsg/qcom_glink_native.c
> +++ b/drivers/rpmsg/qcom_glink_native.c
> @@ -11,7 +11,6 @@
>   #include <linux/module.h>
>   #include <linux/of.h>
>   #include <linux/of_address.h>
> -#include <linux/of_irq.h>
>   #include <linux/platform_device.h>
>   #include <linux/regmap.h>
>   #include <linux/rpmsg.h>
> @@ -78,11 +77,8 @@ struct glink_core_rx_intent {
>   /**
>    * struct qcom_glink - driver context, relates to one remote subsystem
>    * @dev:	reference to the associated struct device
> - * @mbox_client: mailbox client
> - * @mbox_chan:  mailbox channel
>    * @rx_pipe:	pipe object for receive FIFO
>    * @tx_pipe:	pipe object for transmit FIFO
> - * @irq:	IRQ for signaling incoming events
>    * @rx_work:	worker for handling received control messages
>    * @rx_lock:	protects the @rx_queue
>    * @rx_queue:	queue of received control messages to be processed in @rx_work
> @@ -98,14 +94,9 @@ struct glink_core_rx_intent {
>   struct qcom_glink {
>   	struct device *dev;
>   
> -	struct mbox_client mbox_client;
> -	struct mbox_chan *mbox_chan;
> -
>   	struct qcom_glink_pipe *rx_pipe;
>   	struct qcom_glink_pipe *tx_pipe;
>   
> -	int irq;
> -
>   	struct work_struct rx_work;
>   	spinlock_t rx_lock;
>   	struct list_head rx_queue;
> @@ -305,8 +296,7 @@ static void qcom_glink_tx_write(struct qcom_glink *glink,
>   
>   static void qcom_glink_tx_kick(struct qcom_glink *glink)
>   {
> -	mbox_send_message(glink->mbox_chan, NULL);
> -	mbox_client_txdone(glink->mbox_chan, 0);
> +	glink->tx_pipe->kick(glink->tx_pipe);

I think that we need to check that tx_pipe is not null or validate that 
tx_pipe is not null in the native register probe.

>   }
>   
>   static void qcom_glink_send_read_notify(struct qcom_glink *glink)
> @@ -1004,9 +994,8 @@ static int qcom_glink_rx_open_ack(struct qcom_glink *glink, unsigned int lcid)
>   	return 0;
>   }
>   
> -static irqreturn_t qcom_glink_native_intr(int irq, void *data)
> +void qcom_glink_native_intr(struct qcom_glink *glink)
>   {
> -	struct qcom_glink *glink = data;
>   	struct glink_msg msg;
>   	unsigned int param1;
>   	unsigned int param2;
> @@ -1075,9 +1064,8 @@ static irqreturn_t qcom_glink_native_intr(int irq, void *data)
>   		if (ret)
>   			break;
>   	}
> -
> -	return IRQ_HANDLED;
>   }
> +EXPORT_SYMBOL(qcom_glink_native_intr);
>   
>   /* Locally initiated rpmsg_create_ept */
>   static struct glink_channel *qcom_glink_create_local(struct qcom_glink *glink,
> @@ -1723,7 +1711,6 @@ struct qcom_glink *qcom_glink_native_probe(struct device *dev,
>   					   struct qcom_glink_pipe *tx,
>   					   bool intentless)
>   {
> -	int irq;
>   	int ret;
>   	struct qcom_glink *glink;
>   
> @@ -1754,27 +1741,6 @@ struct qcom_glink *qcom_glink_native_probe(struct device *dev,
>   	if (ret)
>   		dev_err(dev, "failed to add groups\n");
>   
> -	glink->mbox_client.dev = dev;
> -	glink->mbox_client.knows_txdone = true;
> -	glink->mbox_chan = mbox_request_channel(&glink->mbox_client, 0);
> -	if (IS_ERR(glink->mbox_chan)) {
> -		if (PTR_ERR(glink->mbox_chan) != -EPROBE_DEFER)
> -			dev_err(dev, "failed to acquire IPC channel\n");
> -		return ERR_CAST(glink->mbox_chan);
> -	}
> -
> -	irq = of_irq_get(dev->of_node, 0);
> -	ret = devm_request_irq(dev, irq,
> -			       qcom_glink_native_intr,
> -			       IRQF_NO_SUSPEND | IRQF_SHARED,
> -			       "glink-native", glink);
> -	if (ret) {
> -		dev_err(dev, "failed to request IRQ\n");
> -		return ERR_PTR(ret);
> -	}
> -
> -	glink->irq = irq;
> -
>   	ret = qcom_glink_send_version(glink);
>   	if (ret)
>   		return ERR_PTR(ret);
> @@ -1800,7 +1766,6 @@ void qcom_glink_native_remove(struct qcom_glink *glink)
>   	int cid;
>   	int ret;
>   
> -	disable_irq(glink->irq);
>   	qcom_glink_cancel_rx_work(glink);
>   
>   	ret = device_for_each_child(glink->dev, NULL, qcom_glink_remove_device);
> @@ -1817,15 +1782,8 @@ void qcom_glink_native_remove(struct qcom_glink *glink)
>   
>   	idr_destroy(&glink->lcids);
>   	idr_destroy(&glink->rcids);
> -	mbox_free_channel(glink->mbox_chan);
>   }
>   EXPORT_SYMBOL_GPL(qcom_glink_native_remove);
>   
> -void qcom_glink_native_unregister(struct qcom_glink *glink)
> -{
> -	device_unregister(glink->dev);
> -}
> -EXPORT_SYMBOL_GPL(qcom_glink_native_unregister);
> -
>   MODULE_DESCRIPTION("Qualcomm GLINK driver");
>   MODULE_LICENSE("GPL v2");
> diff --git a/drivers/rpmsg/qcom_glink_native.h b/drivers/rpmsg/qcom_glink_native.h
> index e9a8671616c7..0129fe1b2b6c 100644
> --- a/drivers/rpmsg/qcom_glink_native.h
> +++ b/drivers/rpmsg/qcom_glink_native.h
> @@ -24,6 +24,7 @@ struct qcom_glink_pipe {
>   	void (*write)(struct qcom_glink_pipe *glink_pipe,
>   		      const void *hdr, size_t hlen,
>   		      const void *data, size_t dlen);
> +	void (*kick)(struct qcom_glink_pipe *glink_pipe);
>   };
>   
>   struct device;
> @@ -35,6 +36,6 @@ struct qcom_glink *qcom_glink_native_probe(struct device *dev,
>   					   struct qcom_glink_pipe *tx,
>   					   bool intentless);
>   void qcom_glink_native_remove(struct qcom_glink *glink);
> +void qcom_glink_native_intr(struct qcom_glink *glink);
>

We could rename this away from qcom_glink_native_intr to something like 
qcom_glink_native_rx. Seeing this in the header, the purpose sounds a 
bit obscure.

> -void qcom_glink_native_unregister(struct qcom_glink *glink);
>   #endif
> diff --git a/drivers/rpmsg/qcom_glink_rpm.c b/drivers/rpmsg/qcom_glink_rpm.c
> index 6443843df6ca..9136645d6251 100644
> --- a/drivers/rpmsg/qcom_glink_rpm.c
> +++ b/drivers/rpmsg/qcom_glink_rpm.c
> @@ -11,6 +11,7 @@
>   #include <linux/module.h>
>   #include <linux/of.h>
>   #include <linux/of_address.h>
> +#include <linux/of_irq.h>
>   #include <linux/platform_device.h>
>   #include <linux/regmap.h>
>   #include <linux/rpmsg.h>
> @@ -56,6 +57,11 @@ struct glink_rpm_pipe {
>   struct glink_rpm {
>   	struct qcom_glink *glink;
>   
> +	int irq;
> +
> +	struct mbox_client mbox_client;
> +	struct mbox_chan *mbox_chan;
> +
>   	struct glink_rpm_pipe rx_pipe;
>   	struct glink_rpm_pipe tx_pipe;
>   };
> @@ -186,6 +192,24 @@ static void glink_rpm_tx_write(struct qcom_glink_pipe *glink_pipe,
>   	writel(head, pipe->head);
>   }
>   
> +static void glink_rpm_tx_kick(struct qcom_glink_pipe *glink_pipe)
> +{
> +	struct glink_rpm_pipe *pipe = to_rpm_pipe(glink_pipe);
> +	struct glink_rpm *rpm = container_of(pipe, struct glink_rpm, tx_pipe);
> +
> +	mbox_send_message(rpm->mbox_chan, NULL);
> +	mbox_client_txdone(rpm->mbox_chan, 0);
> +}
> +
> +static irqreturn_t qcom_glink_rpm_intr(int irq, void *data)
> +{
> +	struct glink_rpm *rpm = data;
> +
> +	qcom_glink_native_intr(rpm->glink);
> +
> +	return IRQ_HANDLED;
> +}
> +
>   static int glink_rpm_parse_toc(struct device *dev,
>   			       void __iomem *msg_ram,
>   			       size_t msg_ram_size,
> @@ -292,12 +316,28 @@ static int glink_rpm_probe(struct platform_device *pdev)
>   	if (ret)
>   		return ret;
>   
> +	rpm->irq = of_irq_get(dev->of_node, 0);
> +	ret = devm_request_irq(dev, rpm->irq, qcom_glink_rpm_intr,
> +			       IRQF_NO_SUSPEND | IRQF_NO_AUTOEN,
> +			       "glink-rpm", rpm);
> +	if (ret) {
> +		dev_err(dev, "failed to request IRQ\n");
> +		return ret;
> +	}
> +
> +	rpm->mbox_client.dev = dev;
> +	rpm->mbox_client.knows_txdone = true;
> +	rpm->mbox_chan = mbox_request_channel(&rpm->mbox_client, 0);
> +	if (IS_ERR(rpm->mbox_chan))
> +		return dev_err_probe(dev, PTR_ERR(rpm->mbox_chan), "failed to acquire IPC channel\n");
> +
>   	/* Pipe specific accessors */
>   	rpm->rx_pipe.native.avail = glink_rpm_rx_avail;
>   	rpm->rx_pipe.native.peak = glink_rpm_rx_peak;
>   	rpm->rx_pipe.native.advance = glink_rpm_rx_advance;
>   	rpm->tx_pipe.native.avail = glink_rpm_tx_avail;
>   	rpm->tx_pipe.native.write = glink_rpm_tx_write;
> +	rpm->tx_pipe.native.kick = glink_rpm_tx_kick;
>   
>   	writel(0, rpm->tx_pipe.head);
>   	writel(0, rpm->rx_pipe.tail);
> @@ -307,13 +347,17 @@ static int glink_rpm_probe(struct platform_device *pdev)
>   					&rpm->rx_pipe.native,
>   					&rpm->tx_pipe.native,
>   					true);
> -	if (IS_ERR(glink))
> +	if (IS_ERR(glink)) {
> +		mbox_free_channel(rpm->mbox_chan);
>   		return PTR_ERR(glink);
> +	}
>   
>   	rpm->glink = glink;
>   
>   	platform_set_drvdata(pdev, rpm);
>   
> +	enable_irq(rpm->irq);
> +
>   	return 0;
>   }
>   
> @@ -322,8 +366,12 @@ static int glink_rpm_remove(struct platform_device *pdev)
>   	struct glink_rpm *rpm = platform_get_drvdata(pdev);
>   	struct qcom_glink *glink = rpm->glink;
>   
> +	disable_irq(rpm->irq);
> +
>   	qcom_glink_native_remove(glink);
>   
> +	mbox_free_channel(rpm->mbox_chan);
> +
>   	return 0;
>   }
>   
> diff --git a/drivers/rpmsg/qcom_glink_smem.c b/drivers/rpmsg/qcom_glink_smem.c
> index 703e63fa5a86..eec47ae98d67 100644
> --- a/drivers/rpmsg/qcom_glink_smem.c
> +++ b/drivers/rpmsg/qcom_glink_smem.c
> @@ -7,8 +7,10 @@
>   #include <linux/module.h>
>   #include <linux/of.h>
>   #include <linux/of_address.h>
> +#include <linux/of_irq.h>
>   #include <linux/interrupt.h>
>   #include <linux/platform_device.h>
> +#include <linux/mailbox_client.h>
>   #include <linux/mfd/syscon.h>
>   #include <linux/slab.h>
>   #include <linux/rpmsg.h>
> @@ -36,8 +38,12 @@
>   struct qcom_glink_smem {
>   	struct device dev;
>   
> +	int irq;
>   	struct qcom_glink *glink;
>   
> +	struct mbox_client mbox_client;
> +	struct mbox_chan *mbox_chan;
> +
>   	u32 remote_pid;
>   };
>   
> @@ -186,6 +192,24 @@ static void glink_smem_tx_write(struct qcom_glink_pipe *glink_pipe,
>   	*pipe->head = cpu_to_le32(head);
>   }
>   
> +static void glink_smem_tx_kick(struct qcom_glink_pipe *glink_pipe)
> +{
> +	struct glink_smem_pipe *pipe = to_smem_pipe(glink_pipe);
> +	struct qcom_glink_smem *smem = pipe->smem;
> +
> +	mbox_send_message(smem->mbox_chan, NULL);
> +	mbox_client_txdone(smem->mbox_chan, 0);
> +}
> +
> +static irqreturn_t qcom_glink_smem_intr(int irq, void *data)
> +{
> +	struct qcom_glink_smem *smem = data;
> +
> +	qcom_glink_native_intr(smem->glink);
> +
> +	return IRQ_HANDLED;
> +}
> +
>   static void qcom_glink_smem_release(struct device *dev)
>   {
>   	struct qcom_glink_smem *smem = container_of(dev, struct qcom_glink_smem, dev);
> @@ -277,6 +301,24 @@ struct qcom_glink_smem *qcom_glink_smem_register(struct device *parent,
>   		goto err_put_dev;
>   	}
>   
> +	smem->irq = of_irq_get(smem->dev.of_node, 0);
> +	ret = devm_request_irq(&smem->dev, smem->irq, qcom_glink_smem_intr,
> +			       IRQF_NO_SUSPEND | IRQF_NO_AUTOEN,
> +			       "glink-smem", smem);

Are we adding dropping IRQF_NO_SUSPEND and adding enable irq wake for 
smem in follow up change?

> +	if (ret) {
> +		dev_err(&smem->dev, "failed to request IRQ\n");
> +		goto err_put_dev;
> +	}
> +
> +	smem->mbox_client.dev = &smem->dev;
> +	smem->mbox_client.knows_txdone = true;
> +	smem->mbox_chan = mbox_request_channel(&smem->mbox_client, 0);
> +	if (IS_ERR(smem->mbox_chan)) {
> +		ret = dev_err_probe(&smem->dev, PTR_ERR(smem->mbox_chan),
> +				    "failed to acquire IPC channel\n");
> +		goto err_put_dev;
> +	}
> +
>   	rx_pipe->smem = smem;
>   	rx_pipe->native.avail = glink_smem_rx_avail;
>   	rx_pipe->native.peak = glink_smem_rx_peak;
> @@ -285,6 +327,7 @@ struct qcom_glink_smem *qcom_glink_smem_register(struct device *parent,
>   	tx_pipe->smem = smem;
>   	tx_pipe->native.avail = glink_smem_tx_avail;
>   	tx_pipe->native.write = glink_smem_tx_write;
> +	tx_pipe->native.kick = glink_smem_tx_kick;
>   
>   	*rx_pipe->tail = 0;
>   	*tx_pipe->head = 0;
> @@ -295,13 +338,17 @@ struct qcom_glink_smem *qcom_glink_smem_register(struct device *parent,
>   					false);
>   	if (IS_ERR(glink)) {
>   		ret = PTR_ERR(glink);
> -		goto err_put_dev;
> +		goto err_free_mbox;
>   	}
>   
>   	smem->glink = glink;
>   
> +	enable_irq(smem->irq);
> +
>   	return smem;
>   
> +err_free_mbox:
> +	mbox_free_channel(smem->mbox_chan);
>   
>   err_put_dev:
>   	device_unregister(&smem->dev);
> @@ -314,8 +361,13 @@ void qcom_glink_smem_unregister(struct qcom_glink_smem *smem)
>   {
>   	struct qcom_glink *glink = smem->glink;
>   
> +	disable_irq(smem->irq);
> +
>   	qcom_glink_native_remove(glink);
> -	qcom_glink_native_unregister(glink);
> +
> +	device_unregister(&smem->dev);
> +
> +	mbox_free_channel(smem->mbox_chan);

This might need to be moved above device_unregister. I think the release 
function frees the smem structure.

>   }
>   EXPORT_SYMBOL_GPL(qcom_glink_smem_unregister);
>   

Re: [PATCH 4/6] rpmsg: glink: Move irq and mbox handling to transports

[Bjorn Andersson]

On Tue, Jan 24, 2023 at 10:55:47PM -0800, Chris Lew wrote:
> On 1/9/2023 2:39 PM, Bjorn Andersson wrote:
> > diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
[..]
> > @@ -305,8 +296,7 @@ static void qcom_glink_tx_write(struct qcom_glink *glink,
> >   static void qcom_glink_tx_kick(struct qcom_glink *glink)
> >   {
> > -	mbox_send_message(glink->mbox_chan, NULL);
> > -	mbox_client_txdone(glink->mbox_chan, 0);
> > +	glink->tx_pipe->kick(glink->tx_pipe);
> 
> I think that we need to check that tx_pipe is not null or validate that
> tx_pipe is not null in the native register probe.
> 

The function pointers are const, so it's only during development of a
transport that this could become NULL, and it's impossible to register
successfully without hitting that oops.

So I don't know if it's worth adding a runtime check for this. It's just
a handful of checks, but they would run trillions of times for no
purpose...

> >   }
> >   static void qcom_glink_send_read_notify(struct qcom_glink *glink)
[..]
> > @@ -35,6 +36,6 @@ struct qcom_glink *qcom_glink_native_probe(struct device *dev,
> >   					   struct qcom_glink_pipe *tx,
> >   					   bool intentless);
> >   void qcom_glink_native_remove(struct qcom_glink *glink);
> > +void qcom_glink_native_intr(struct qcom_glink *glink);
> > 
> 
> We could rename this away from qcom_glink_native_intr to something like
> qcom_glink_native_rx. Seeing this in the header, the purpose sounds a bit
> obscure.
> 

Perhaps qcom_glink_native_notify_rx()?

> > -void qcom_glink_native_unregister(struct qcom_glink *glink);
> >   #endif
> > diff --git a/drivers/rpmsg/qcom_glink_rpm.c b/drivers/rpmsg/qcom_glink_rpm.c
[..]
> > @@ -277,6 +301,24 @@ struct qcom_glink_smem *qcom_glink_smem_register(struct device *parent,
> >   		goto err_put_dev;
> >   	}
> > +	smem->irq = of_irq_get(smem->dev.of_node, 0);
> > +	ret = devm_request_irq(&smem->dev, smem->irq, qcom_glink_smem_intr,
> > +			       IRQF_NO_SUSPEND | IRQF_NO_AUTOEN,
> > +			       "glink-smem", smem);
> 
> Are we adding dropping IRQF_NO_SUSPEND and adding enable irq wake for smem
> in follow up change?
> 

Yes, while I haven't reviewed all the details of that discussion again,
I was planning to follow up with something on that after this has been
merged.

That way we can discuss/review that separately.

> > +	if (ret) {
> > +		dev_err(&smem->dev, "failed to request IRQ\n");
> > +		goto err_put_dev;
> > +	}
> > +
[..]
> > @@ -314,8 +361,13 @@ void qcom_glink_smem_unregister(struct qcom_glink_smem *smem)
> >   {
> >   	struct qcom_glink *glink = smem->glink;
> > +	disable_irq(smem->irq);
> > +
> >   	qcom_glink_native_remove(glink);
> > -	qcom_glink_native_unregister(glink);
> > +
> > +	device_unregister(&smem->dev);
> > +
> > +	mbox_free_channel(smem->mbox_chan);
> 
> This might need to be moved above device_unregister. I think the release
> function frees the smem structure.
> 

Yes, that looks correct.

Thank you for the review,
Bjorn

> >   }
> >   EXPORT_SYMBOL_GPL(qcom_glink_smem_unregister);

[PATCH 5/6] rpmsg: glink: Fail qcom_glink_tx() once remove has been initiated

[Bjorn Andersson]

Upon removing the glink edge communication is at best one-way. This
means that the very common scenario of glink requesting intents will not
be possible to serve.

Typically a successful transmission results in the client waiting for a
response, with some timeout and a mechanism for aborting that timeout.

Because of this, once the glink edge is defunct once removal is
commenced it's better to fail transmissions fast.

Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
---
 drivers/rpmsg/qcom_glink_native.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
index db5d946d5901..d81d0729493e 100644
--- a/drivers/rpmsg/qcom_glink_native.c
+++ b/drivers/rpmsg/qcom_glink_native.c
@@ -90,6 +90,7 @@ struct glink_core_rx_intent {
  * @intentless:	flag to indicate that there is no intent
  * @tx_avail_notify: Waitqueue for pending tx tasks
  * @sent_read_notify: flag to check cmd sent or not
+ * @abort_tx:	flag indicating that all tx attempts should fail
  */
 struct qcom_glink {
 	struct device *dev;
@@ -111,6 +112,8 @@ struct qcom_glink {
 	bool intentless;
 	wait_queue_head_t tx_avail_notify;
 	bool sent_read_notify;
+
+	bool abort_tx;
 };
 
 enum {
@@ -326,12 +329,22 @@ static int qcom_glink_tx(struct qcom_glink *glink,
 
 	spin_lock_irqsave(&glink->tx_lock, flags);
 
+	if (glink->abort_tx) {
+		ret = -EIO;
+		goto out;
+	}
+
 	while (qcom_glink_tx_avail(glink) < tlen) {
 		if (!wait) {
 			ret = -EAGAIN;
 			goto out;
 		}
 
+		if (glink->abort_tx) {
+			ret = -EIO;
+			goto out;
+		}
+
 		if (!glink->sent_read_notify) {
 			glink->sent_read_notify = true;
 			qcom_glink_send_read_notify(glink);
@@ -1763,11 +1776,18 @@ static int qcom_glink_remove_device(struct device *dev, void *data)
 void qcom_glink_native_remove(struct qcom_glink *glink)
 {
 	struct glink_channel *channel;
+	unsigned long flags;
 	int cid;
 	int ret;
 
 	qcom_glink_cancel_rx_work(glink);
 
+	/* Fail all attempts at sending messages */
+	spin_lock_irqsave(&glink->tx_lock, flags);
+	glink->abort_tx = true;
+	wake_up_all(&glink->tx_avail_notify);
+	spin_unlock_irqrestore(&glink->tx_lock, flags);
+
 	ret = device_for_each_child(glink->dev, NULL, qcom_glink_remove_device);
 	if (ret)
 		dev_warn(glink->dev, "Can't remove GLINK devices: %d\n", ret);
-- 
2.37.3

Re: [PATCH 5/6] rpmsg: glink: Fail qcom_glink_tx() once remove has been initiated

[Chris Lew]

On 1/9/2023 2:40 PM, Bjorn Andersson wrote:
> Upon removing the glink edge communication is at best one-way. This

glink edge, communication

> means that the very common scenario of glink requesting intents will not
> be possible to serve.
> 
> Typically a successful transmission results in the client waiting for a
> response, with some timeout and a mechanism for aborting that timeout.
> 
> Because of this, once the glink edge is defunct once removal is
> commenced it's better to fail transmissions fast.
> 
> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> ---

Reviewed-by: Chris Lew <quic_clew@quicinc.com>

>   drivers/rpmsg/qcom_glink_native.c | 20 ++++++++++++++++++++
>   1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
> index db5d946d5901..d81d0729493e 100644
> --- a/drivers/rpmsg/qcom_glink_native.c
> +++ b/drivers/rpmsg/qcom_glink_native.c
> @@ -90,6 +90,7 @@ struct glink_core_rx_intent {
>    * @intentless:	flag to indicate that there is no intent
>    * @tx_avail_notify: Waitqueue for pending tx tasks
>    * @sent_read_notify: flag to check cmd sent or not
> + * @abort_tx:	flag indicating that all tx attempts should fail
>    */
>   struct qcom_glink {
>   	struct device *dev;
> @@ -111,6 +112,8 @@ struct qcom_glink {
>   	bool intentless;
>   	wait_queue_head_t tx_avail_notify;
>   	bool sent_read_notify;
> +
> +	bool abort_tx;
>   };
>   
>   enum {
> @@ -326,12 +329,22 @@ static int qcom_glink_tx(struct qcom_glink *glink,
>   
>   	spin_lock_irqsave(&glink->tx_lock, flags);
>   
> +	if (glink->abort_tx) {
> +		ret = -EIO;
> +		goto out;
> +	}
> +
>   	while (qcom_glink_tx_avail(glink) < tlen) {
>   		if (!wait) {
>   			ret = -EAGAIN;
>   			goto out;
>   		}
>   
> +		if (glink->abort_tx) {
> +			ret = -EIO;
> +			goto out;
> +		}
> +
>   		if (!glink->sent_read_notify) {
>   			glink->sent_read_notify = true;
>   			qcom_glink_send_read_notify(glink);
> @@ -1763,11 +1776,18 @@ static int qcom_glink_remove_device(struct device *dev, void *data)
>   void qcom_glink_native_remove(struct qcom_glink *glink)
>   {
>   	struct glink_channel *channel;
> +	unsigned long flags;
>   	int cid;
>   	int ret;
>   
>   	qcom_glink_cancel_rx_work(glink);
>   
> +	/* Fail all attempts at sending messages */
> +	spin_lock_irqsave(&glink->tx_lock, flags);
> +	glink->abort_tx = true;
> +	wake_up_all(&glink->tx_avail_notify);
> +	spin_unlock_irqrestore(&glink->tx_lock, flags);
> +
>   	ret = device_for_each_child(glink->dev, NULL, qcom_glink_remove_device);
>   	if (ret)
>   		dev_warn(glink->dev, "Can't remove GLINK devices: %d\n", ret);

[PATCH 6/6] rpmsg: glink: Cancel pending intent requests at removal

[Bjorn Andersson]

During removal of the glink edge interrupts are disabled and no more
incoming messages are being serviced. In addition to the remote endpoint
being defunct that means that any outstanding requests for intents will
not be serviced, and qcom_glink_request_intent() will blindly wait for
up to 10 seconds.

Mark the intent request as not granted and complete the intent request
completion to fail the waiting client immediately.

Once the current intent request is failed, any potential clients waiting
for the intent request mutex will not enter the same wait, as the
qcom_glink_tx() call will fail fast.

Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
---
 drivers/rpmsg/qcom_glink_native.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
index d81d0729493e..bb14e7edeadc 100644
--- a/drivers/rpmsg/qcom_glink_native.c
+++ b/drivers/rpmsg/qcom_glink_native.c
@@ -423,6 +423,12 @@ static void qcom_glink_handle_intent_req_ack(struct qcom_glink *glink,
 	complete(&channel->intent_req_comp);
 }
 
+static void qcom_glink_intent_req_abort(struct glink_channel *channel)
+{
+	channel->intent_req_result = 0;
+	complete(&channel->intent_req_comp);
+}
+
 /**
  * qcom_glink_send_open_req() - send a RPM_CMD_OPEN request to the remote
  * @glink: Ptr to the glink edge
@@ -1788,6 +1794,12 @@ void qcom_glink_native_remove(struct qcom_glink *glink)
 	wake_up_all(&glink->tx_avail_notify);
 	spin_unlock_irqrestore(&glink->tx_lock, flags);
 
+	/* Abort any senders waiting for intent requests */
+	spin_lock_irqsave(&glink->idr_lock, flags);
+	idr_for_each_entry(&glink->lcids, channel, cid)
+		qcom_glink_intent_req_abort(channel);
+	spin_unlock_irqrestore(&glink->idr_lock, flags);
+
 	ret = device_for_each_child(glink->dev, NULL, qcom_glink_remove_device);
 	if (ret)
 		dev_warn(glink->dev, "Can't remove GLINK devices: %d\n", ret);
-- 
2.37.3

Re: [PATCH 6/6] rpmsg: glink: Cancel pending intent requests at removal

[Chris Lew]

On 1/9/2023 2:40 PM, Bjorn Andersson wrote:
> During removal of the glink edge interrupts are disabled and no more
> incoming messages are being serviced. In addition to the remote endpoint
> being defunct that means that any outstanding requests for intents will
> not be serviced, and qcom_glink_request_intent() will blindly wait for
> up to 10 seconds.
> 
> Mark the intent request as not granted and complete the intent request
> completion to fail the waiting client immediately.
> 
> Once the current intent request is failed, any potential clients waiting
> for the intent request mutex will not enter the same wait, as the
> qcom_glink_tx() call will fail fast.
> 
> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> ---

Reviewed-by: Chris Lew <quic_clew@quicinc.com>

>   drivers/rpmsg/qcom_glink_native.c | 12 ++++++++++++
>   1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
> index d81d0729493e..bb14e7edeadc 100644
> --- a/drivers/rpmsg/qcom_glink_native.c
> +++ b/drivers/rpmsg/qcom_glink_native.c
> @@ -423,6 +423,12 @@ static void qcom_glink_handle_intent_req_ack(struct qcom_glink *glink,
>   	complete(&channel->intent_req_comp);
>   }
>   
> +static void qcom_glink_intent_req_abort(struct glink_channel *channel)
> +{
> +	channel->intent_req_result = 0;
> +	complete(&channel->intent_req_comp);
> +}
> +
>   /**
>    * qcom_glink_send_open_req() - send a RPM_CMD_OPEN request to the remote
>    * @glink: Ptr to the glink edge
> @@ -1788,6 +1794,12 @@ void qcom_glink_native_remove(struct qcom_glink *glink)
>   	wake_up_all(&glink->tx_avail_notify);
>   	spin_unlock_irqrestore(&glink->tx_lock, flags);
>   
> +	/* Abort any senders waiting for intent requests */
> +	spin_lock_irqsave(&glink->idr_lock, flags);
> +	idr_for_each_entry(&glink->lcids, channel, cid)
> +		qcom_glink_intent_req_abort(channel);
> +	spin_unlock_irqrestore(&glink->idr_lock, flags);
> +
>   	ret = device_for_each_child(glink->dev, NULL, qcom_glink_remove_device);
>   	if (ret)
>   		dev_warn(glink->dev, "Can't remove GLINK devices: %d\n", ret);