Skip to contentSkip to conversation

Add chrdev and name query support for GLINK

PATCH In review Chris Lew 3 participants 10 messages Apr 26, 2018 – Apr 30, 2018

Subsystems (4)
Device tree bindings Arm Qualcomm mailing list Arm Qualcomm support Remote processor messaging (rpmsg)
lore
Share & tools
lore

Chris Lew

author
#0 lore raw

v3 cover Add chrdev and name query support for GLINK

Add support for the GLINK rpmsg transport to register a rpmsg chrdev. This will create the rpmsg_ctrl nodes for userspace clients to open rpmsg epts. Create a label property that will help userspace clients distinguish between the different GLINK links. The rpmsg chrdev allocation is done by allocating a local channel which also allocates an ept. We need to add some guards against edge cases for this chrdev because it will never fully open.

Changes since v2:
- Revert change to make glink attribute table const

Changes since v1:
- Add explanation to dt-bindings commit message
- Add patch complete_all the open_req/ack variables
- Add patch to prevent null pointer dereference in chrdev channel release
- Change chrdev allocation to use glink channel allocation
- Change glink attr struct to const

Chris Lew (6):
dt-bindings: soc: qcom: Add label for GLINK bindings
rpmsg: glink: Store edge name for glink device
rpmsg: glink: Use complete_all for open states
rpmsg: Guard against null endpoint ops in destroy
rpmsg: glink: Add support for rpmsg glink chrdev
rpmsg: glink: Expose rpmsg name attr for glink

 .../devicetree/bindings/soc/qcom/qcom,glink.txt    |  5 ++
 drivers/rpmsg/qcom_glink_native.c                  | 68 +++++++++++++++++++++-
 drivers/rpmsg/rpmsg_core.c                         |  2 +-
 3 files changed, 71 insertions(+), 4 deletions(-)
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
author
#1 lore raw

PATCH v3 1/6 dt-bindings: soc: qcom: Add label for GLINK bindings

There are GLINK clients who open the same channel on multiple GLINK links. These clients need a way to distinguish which remoteproc they are communicating to. Add a label property to identify the edge this node represents.

Signed-off-by Chris Lew <[email protected]>

---

Changes since v1:
- Add explanation for label in commit message

 Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt | 5 +++++
 1 file changed, 5 insertions(+)
Documentation/devicetree/bindings/soc/qcom/qcom,glink.txt +5 -0
index 9663cab52246..0b8cc533ca83 100644
@@ -10,6 +10,11 @@ edge.
 	Value type: <stringlist>
 	Definition: must be "qcom,glink-rpm"
 
+- label:
+	Usage: optional
+	Value type: <string>
+	Definition: should specify the subsystem name this edge corresponds to.
+
 - interrupts:
 	Usage: required
 	Value type: <prop-encoded-array>
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
maintainer
#2 lore raw

Re: [PATCH v3 1/6] dt-bindings: soc: qcom: Add label for GLINK bindings

Quoted 4 lines · Chris Lew

There are GLINK clients who open the same channel on multiple GLINK links. These clients need a way to distinguish which remoteproc they are communicating to. Add a label property to identify the edge this node represents.

Isn't there a better way? "label" is really intended for things that would be printed on a sticker to be read by a human.

Rob

author
#3 lore raw

PATCH v3 2/6 rpmsg: glink: Store edge name for glink device

In reply to Chris Lew

Channels may need to identify the edge their channel was probed for. Store the edge name by reading the label property from device tree or default to the node name.

Signed-off-by Chris Lew <[email protected]>

---

Changes since v1:
- None

 drivers/rpmsg/qcom_glink_native.c | 6 ++++++
 1 file changed, 6 insertions(+)
drivers/rpmsg/qcom_glink_native.c +6 -0
index 768ef542a841..c3f548e2ff20 100644
@@ -101,6 +101,8 @@ struct glink_core_rx_intent {
 struct qcom_glink {
 	struct device *dev;
 
+	const char *name;
+
 	struct mbox_client mbox_client;
 	struct mbox_chan *mbox_chan;
 

@@ -1580,6 +1582,10 @@ struct qcom_glink *qcom_glink_native_probe(struct device *dev,
 	idr_init(&glink->lcids);
 	idr_init(&glink->rcids);
 
+	ret = of_property_read_string(dev->of_node, "label", &glink->name);
+	if (ret < 0)
+		glink->name = dev->of_node->name;
+
 	glink->mbox_client.dev = dev;
 	glink->mbox_client.knows_txdone = true;
 	glink->mbox_chan = mbox_request_channel(&glink->mbox_client, 0);
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
author
#4 lore raw

PATCH v3 3/6 rpmsg: glink: Use complete_all for open states

In reply to Chris Lew

The open_req and open_ack completion variables are the state variables to represet a remote channel as open. Use complete_all so there are no races with waiters and using completion_done.

Signed-off-by Chris Lew <[email protected]>

---

Changes since v1:
- New change

 drivers/rpmsg/qcom_glink_native.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/rpmsg/qcom_glink_native.c +2 -2
index c3f548e2ff20..120d6b9bb9f3 100644
@@ -957,7 +957,7 @@ static int qcom_glink_rx_open_ack(struct qcom_glink *glink, unsigned int lcid)
 		return -EINVAL;
 	}
 
-	complete(&channel->open_ack);
+	complete_all(&channel->open_ack);
 
 	return 0;
 }

@@ -1401,7 +1401,7 @@ static int qcom_glink_rx_open(struct qcom_glink *glink, unsigned int rcid,
 	channel->rcid = ret;
 	spin_unlock_irqrestore(&glink->idr_lock, flags);
 
-	complete(&channel->open_req);
+	complete_all(&channel->open_req);
 
 	if (create_device) {
 		rpdev = kzalloc(sizeof(*rpdev), GFP_KERNEL);
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
author
#5 lore raw

PATCH v3 4/6 rpmsg: Guard against null endpoint ops in destroy

In reply to Chris Lew

In RPMSG GLINK the chrdev device will allocate an ept as part of the rpdev creation. This device will not register endpoint ops even though it has an allocated ept. Protect against the case where the device is being destroyed.

Signed-off-by Chris Lew <[email protected]>

---

Changes since v1:
- New change

 drivers/rpmsg/rpmsg_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/rpmsg/rpmsg_core.c +1 -1
index 920a02f0462c..7bfe36afccc5 100644
@@ -88,7 +88,7 @@ struct rpmsg_endpoint *rpmsg_create_ept(struct rpmsg_device *rpdev,
  */
 void rpmsg_destroy_ept(struct rpmsg_endpoint *ept)
 {
-	if (ept)
+	if (ept && ept->ops)
 		ept->ops->destroy_ept(ept);
 }
 EXPORT_SYMBOL(rpmsg_destroy_ept);
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
#6 lore raw

Re: [PATCH v3 4/6] rpmsg: Guard against null endpoint ops in destroy

Hello Chris,

Quoted 20 lines · Chris Lew

In RPMSG GLINK the chrdev device will allocate an ept as part of the rpdev creation. This device will not register endpoint ops even though it has an allocated ept. Protect against the case where the device is being destroyed.

Signed-off-by Chris Lew <[email protected]>

---

Changes since v1:
- New change

 drivers/rpmsg/rpmsg_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/rpmsg/rpmsg_core.c +0 -0
index 920a02f0462c..7bfe36afccc5 100644

drivers/rpmsg/rpmsg_core.c struct rpmsg_endpoint *rpmsg_create_ept(struct rpmsg_device *rpdev, · line 94

Quoted 8 lines
drivers/rpmsg/rpmsg_core.c +1 -1
@@ -88,7 +88,7 @@ struct rpmsg_endpoint *rpmsg_create_ept(struct rpmsg_device *rpdev,
  */
 void rpmsg_destroy_ept(struct rpmsg_endpoint *ept)
 {
-	if (ept)
+	if (ept && ept->ops)
 		ept->ops->destroy_ept(ept);
 }
 EXPORT_SYMBOL(rpmsg_destroy_ept);

Would make sense that you also add test on ept->ops->destroy_ept. I guess that ops may not be null while destroy_ept pointer is.

Regards
Arnaud

#7 lore raw

Re: [PATCH v3 4/6] rpmsg: Guard against null endpoint ops in destroy

Quoted 26 lines · Arnaud Pouliquen

Hello Chris,

Quoted 23 lines · Chris Lew

In RPMSG GLINK the chrdev device will allocate an ept as part of the rpdev creation. This device will not register endpoint ops even though it has an allocated ept. Protect against the case where the device is being destroyed.

Signed-off-by Chris Lew <[email protected]>

---

Changes since v1:
- New change

 drivers/rpmsg/rpmsg_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/rpmsg/rpmsg_core.c +0 -1
index 920a02f0462c..7bfe36afccc5 100644
@@ -88,7 +88,7 @@ struct rpmsg_endpoint *rpmsg_create_ept(struct rpmsg_device *rpdev,
  */
 void rpmsg_destroy_ept(struct rpmsg_endpoint *ept)
 {
-	if (ept)
Quoted 8 lines

drivers/rpmsg/rpmsg_core.c struct rpmsg_endpoint *rpmsg_create_ept(struct rpmsg_device *rpdev, · line 94

Quoted 5 lines
drivers/rpmsg/rpmsg_core.c +1 -0
+	if (ept && ept->ops)
 		ept->ops->destroy_ept(ept);
 }
 EXPORT_SYMBOL(rpmsg_destroy_ept);

Would make sense that you also add test on ept->ops->destroy_ept. I guess that ops may not be null while destroy_ept pointer is.

Sorry i cross checked in rpmsg_endpoint_ops. destroy_ept is required. So my comment is not relevant.

Nevertheless do it make sense to have an endpoint without associated ops? I don't use rpmsg_char but I tried to figure out how rpmsg_create_ept is called in rpmsg_char without rpmsg_device ops...
Seems that qcom_glink_create_ept should be called, so ept->ops should point to glink_endpoint_ops struct, right?

Another point concerns the send and try send ops that are also required. Do you test rpmsg_trysend and/or rpmsg_send function call? seems that you should face the same issue...

Quoted 8 lines

Regards
Arnaud
--
To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

author
#8 lore raw

PATCH v3 5/6 rpmsg: glink: Add support for rpmsg glink chrdev

In reply to Chris Lew

RPMSG provides a char device interface to userspace. Probe the rpmsg chrdev channel to enable the rpmsg_ctrl device creation on glink transports.

Signed-off-by Chris Lew <[email protected]>

---

Changes since v1:
- Use qcom_glink_alloc_channel to create the chrdev rpmsg device

 drivers/rpmsg/qcom_glink_native.c | 40 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 39 insertions(+), 1 deletion(-)
drivers/rpmsg/qcom_glink_native.c +39 -1
index 120d6b9bb9f3..1418926f6110 100644
@@ -1166,7 +1166,7 @@ static int qcom_glink_announce_create(struct rpmsg_device *rpdev)
 	__be32 *val = defaults;
 	int size;
 
-	if (glink->intentless)
+	if (glink->intentless || !completion_done(&channel->open_ack))
 		return 0;
 
 	prop = of_find_property(np, "qcom,intents", NULL);

@@ -1552,6 +1552,40 @@ static void qcom_glink_work(struct work_struct *work)
 	}
 }
 
+static void qcom_glink_device_release(struct device *dev)
+{
+	struct rpmsg_device *rpdev = to_rpmsg_device(dev);
+	struct glink_channel *channel = to_glink_channel(rpdev->ept);
+
+	/* Release qcom_glink_alloc_channel() reference */
+	kref_put(&channel->refcount, qcom_glink_channel_release);
+	kfree(rpdev);
+}
+
+static int qcom_glink_create_chrdev(struct qcom_glink *glink)
+{
+	struct rpmsg_device *rpdev;
+	struct glink_channel *channel;
+
+	rpdev = kzalloc(sizeof(*rpdev), GFP_KERNEL);
+	if (!rpdev)
+		return -ENOMEM;
+
+	channel = qcom_glink_alloc_channel(glink, "rpmsg_chrdev");
+	if (IS_ERR(channel)) {
+		kfree(rpdev);
+		return PTR_ERR(channel);
+	}
+	channel->rpdev = rpdev;
+
+	rpdev->ept = &channel->ept;
+	rpdev->ops = &glink_device_ops;
+	rpdev->dev.parent = glink->dev;
+	rpdev->dev.release = qcom_glink_device_release;
+
+	return rpmsg_chrdev_register_device(rpdev);
+}
+
 struct qcom_glink *qcom_glink_native_probe(struct device *dev,
 					   unsigned long features,
 					   struct qcom_glink_pipe *rx,

@@ -1611,6 +1645,10 @@ struct qcom_glink *qcom_glink_native_probe(struct device *dev,
 	if (ret)
 		return ERR_PTR(ret);
 
+	ret = qcom_glink_create_chrdev(glink);
+	if (ret)
+		dev_err(glink->dev, "failed to register chrdev\n");
+
 	return glink;
 }
 EXPORT_SYMBOL_GPL(qcom_glink_native_probe);
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
author
#9 lore raw

PATCH v3 6/6 rpmsg: glink: Expose rpmsg name attr for glink

In reply to Chris Lew

Expose the name field as an attr so clients listening to uevents for rpmsg can identify the edge the events correspond to.

Signed-off-by Chris Lew <[email protected]>

---

Changes since v2:
- Remove const on attribute struct

Changes since v1:
- Add const to attribute struct
- Get name from glink channel

 drivers/rpmsg/qcom_glink_native.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)
drivers/rpmsg/qcom_glink_native.c +18 -0
index 1418926f6110..c2983a53058f 100644
@@ -1552,6 +1552,22 @@ static void qcom_glink_work(struct work_struct *work)
 	}
 }
 
+static ssize_t rpmsg_name_show(struct device *dev,
+			       struct device_attribute *attr, char *buf)
+{
+	struct rpmsg_device *rpdev = to_rpmsg_device(dev);
+	struct glink_channel *channel = to_glink_channel(rpdev->ept);
+
+	return snprintf(buf, RPMSG_NAME_SIZE, "%s\n", channel->glink->name);
+}
+static DEVICE_ATTR_RO(rpmsg_name);
+
+static struct attribute *qcom_glink_attrs[] = {
+	&dev_attr_rpmsg_name.attr,
+	NULL
+};
+ATTRIBUTE_GROUPS(qcom_glink);
+
 static void qcom_glink_device_release(struct device *dev)
 {
 	struct rpmsg_device *rpdev = to_rpmsg_device(dev);

@@ -1601,6 +1617,8 @@ struct qcom_glink *qcom_glink_native_probe(struct device *dev,
 		return ERR_PTR(-ENOMEM);
 
 	glink->dev = dev;
+	glink->dev->groups = qcom_glink_groups;
+
 	glink->tx_pipe = tx;
 	glink->rx_pipe = rx;
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project