Re: [PATCH v2 11/20] rpmsg: glink: Fix idr_lock from mutex to spinlock

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 8/24/2017 12:51 PM, Sricharan R wrote:

The channel members lcids, rcids synchronised using
the idr_lock is accessed in both atomic/non-atomic
contexts. The readers are not currently synchronised.
That no correct, so add the readers as well under the
lock and use a spinlock.

Signed-off-by: Sricharan R <sricharan@xxxxxxxxxxxxxx>
Signed-off-by: Bjorn Andersson <bjorn.andersson@xxxxxxxxxx>

Acked-by: Arun Kumar Neelakantam <aneela@xxxxxxxxxxxxxx>

Regards,
Arun N

---
  drivers/rpmsg/qcom_glink_native.c | 58 +++++++++++++++++++++++++++------------
  1 file changed, 40 insertions(+), 18 deletions(-)

diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
index 777ac6b..588a56c 100644
--- a/drivers/rpmsg/qcom_glink_native.c
+++ b/drivers/rpmsg/qcom_glink_native.c
@@ -92,7 +92,7 @@ struct qcom_glink {
struct mutex tx_lock; - struct mutex idr_lock; 
+	spinlock_t idr_lock;
  	struct idr lcids;
  	struct idr rcids;
  	unsigned long features;
@@ -309,14 +309,15 @@ static int qcom_glink_send_open_req(struct qcom_glink *glink,
  	int name_len = strlen(channel->name) + 1;
  	int req_len = ALIGN(sizeof(req.msg) + name_len, 8);
  	int ret;
+	unsigned long flags;
kref_get(&channel->refcount); - mutex_lock(&glink->idr_lock); 
+	spin_lock_irqsave(&glink->idr_lock, flags);
  	ret = idr_alloc_cyclic(&glink->lcids, channel,
  			       RPM_GLINK_CID_MIN, RPM_GLINK_CID_MAX,
  			       GFP_KERNEL);
-	mutex_unlock(&glink->idr_lock);
+	spin_unlock_irqrestore(&glink->idr_lock, flags);
  	if (ret < 0)
  		return ret;
@@ -334,10 +335,10 @@ static int qcom_glink_send_open_req(struct qcom_glink *glink, 
  	return 0;
remove_idr: 
-	mutex_lock(&glink->idr_lock);
+	spin_lock_irqsave(&glink->idr_lock, flags);
  	idr_remove(&glink->lcids, channel->lcid);
  	channel->lcid = 0;
-	mutex_unlock(&glink->idr_lock);
+	spin_unlock_irqrestore(&glink->idr_lock, flags);
return ret; 
  }
@@ -463,6 +464,7 @@ static int qcom_glink_rx_data(struct qcom_glink *glink, size_t avail)
  	unsigned int chunk_size;
  	unsigned int left_size;
  	unsigned int rcid;
+	unsigned long flags;
if (avail < sizeof(hdr)) { 
  		dev_dbg(glink->dev, "Not enough data in fifo\n");
@@ -482,7 +484,9 @@ static int qcom_glink_rx_data(struct qcom_glink *glink, size_t avail)
  		return -EINVAL;
rcid = le16_to_cpu(hdr.msg.param1); 
+	spin_lock_irqsave(&glink->idr_lock, flags);
  	channel = idr_find(&glink->rcids, rcid);
+	spin_unlock_irqrestore(&glink->idr_lock, flags);
  	if (!channel) {
  		dev_dbg(glink->dev, "Data on non-existing channel\n");
@@ -543,11 +547,13 @@ static int qcom_glink_rx_open_ack(struct qcom_glink *glink, unsigned int lcid) 
  {
  	struct glink_channel *channel;
+ spin_lock(&glink->idr_lock); 
  	channel = idr_find(&glink->lcids, lcid);
  	if (!channel) {
  		dev_err(glink->dev, "Invalid open ack packet\n");
  		return -EINVAL;
  	}
+	spin_unlock(&glink->idr_lock);
complete(&channel->open_ack); @@ -621,6 +627,7 @@ static struct glink_channel *qcom_glink_create_local(struct qcom_glink *glink, 
  {
  	struct glink_channel *channel;
  	int ret;
+	unsigned long flags;
channel = qcom_glink_alloc_channel(glink, name); 
  	if (IS_ERR(channel))
@@ -644,9 +651,9 @@ static struct glink_channel *qcom_glink_create_local(struct qcom_glink *glink,
err_timeout: 
  	/* qcom_glink_send_open_req() did register the channel in lcids*/
-	mutex_lock(&glink->idr_lock);
+	spin_lock_irqsave(&glink->idr_lock, flags);
  	idr_remove(&glink->lcids, channel->lcid);
-	mutex_unlock(&glink->idr_lock);
+	spin_unlock_irqrestore(&glink->idr_lock, flags);
release_channel: 
  	/* Release qcom_glink_send_open_req() reference */
@@ -703,11 +710,14 @@ static struct rpmsg_endpoint *qcom_glink_create_ept(struct rpmsg_device *rpdev,
  	const char *name = chinfo.name;
  	int cid;
  	int ret;
+	unsigned long flags;
+ spin_lock_irqsave(&glink->idr_lock, flags); 
  	idr_for_each_entry(&glink->rcids, channel, cid) {
  		if (!strcmp(channel->name, name))
  			break;
  	}
+	spin_unlock_irqrestore(&glink->idr_lock, flags);
if (!channel) { 
  		channel = qcom_glink_create_local(glink, name);
@@ -829,11 +839,14 @@ static int qcom_glink_rx_open(struct qcom_glink *glink, unsigned int rcid,
  	struct device_node *node;
  	int lcid;
  	int ret;
+	unsigned long flags;
+ spin_lock_irqsave(&glink->idr_lock, flags); 
  	idr_for_each_entry(&glink->lcids, channel, lcid) {
  		if (!strcmp(channel->name, name))
  			break;
  	}
+	spin_unlock_irqrestore(&glink->idr_lock, flags);
if (!channel) { 
  		channel = qcom_glink_alloc_channel(glink, name);
@@ -844,15 +857,15 @@ static int qcom_glink_rx_open(struct qcom_glink *glink, unsigned int rcid,
  		create_device = true;
  	}
- mutex_lock(&glink->idr_lock); 
+	spin_lock_irqsave(&glink->idr_lock, flags);
  	ret = idr_alloc(&glink->rcids, channel, rcid, rcid + 1, GFP_KERNEL);
  	if (ret < 0) {
  		dev_err(glink->dev, "Unable to insert channel into rcid list\n");
-		mutex_unlock(&glink->idr_lock);
+		spin_unlock_irqrestore(&glink->idr_lock, flags);
  		goto free_channel;
  	}
  	channel->rcid = ret;
-	mutex_unlock(&glink->idr_lock);
+	spin_unlock_irqrestore(&glink->idr_lock, flags);
complete(&channel->open_req); @@ -886,10 +899,10 @@ static int qcom_glink_rx_open(struct qcom_glink *glink, unsigned int rcid, 
  free_rpdev:
  	kfree(rpdev);
  rcid_remove:
-	mutex_lock(&glink->idr_lock);
+	spin_lock_irqsave(&glink->idr_lock, flags);
  	idr_remove(&glink->rcids, channel->rcid);
  	channel->rcid = 0;
-	mutex_unlock(&glink->idr_lock);
+	spin_unlock_irqrestore(&glink->idr_lock, flags);
  free_channel:
  	/* Release the reference, iff we took it */
  	if (create_device)
@@ -902,8 +915,11 @@ static void qcom_glink_rx_close(struct qcom_glink *glink, unsigned int rcid)
  {
  	struct rpmsg_channel_info chinfo;
  	struct glink_channel *channel;
+	unsigned long flags;
+ spin_lock_irqsave(&glink->idr_lock, flags); 
  	channel = idr_find(&glink->rcids, rcid);
+	spin_unlock_irqrestore(&glink->idr_lock, flags);
  	if (WARN(!channel, "close request on unknown channel\n"))
  		return;
@@ -917,10 +933,10 @@ static void qcom_glink_rx_close(struct qcom_glink *glink, unsigned int rcid) qcom_glink_send_close_ack(glink, channel->rcid); - mutex_lock(&glink->idr_lock); 
+	spin_lock_irqsave(&glink->idr_lock, flags);
  	idr_remove(&glink->rcids, channel->rcid);
  	channel->rcid = 0;
-	mutex_unlock(&glink->idr_lock);
+	spin_unlock_irqrestore(&glink->idr_lock, flags);
kref_put(&channel->refcount, qcom_glink_channel_release); 
  }
@@ -928,15 +944,18 @@ static void qcom_glink_rx_close(struct qcom_glink *glink, unsigned int rcid)
  static void qcom_glink_rx_close_ack(struct qcom_glink *glink, unsigned int lcid)
  {
  	struct glink_channel *channel;
+	unsigned long flags;
+ spin_lock_irqsave(&glink->idr_lock, flags); 
  	channel = idr_find(&glink->lcids, lcid);
-	if (WARN(!channel, "close ack on unknown channel\n"))
+	if (WARN(!channel, "close ack on unknown channel\n")) {
+		spin_unlock_irqrestore(&glink->idr_lock, flags);
  		return;
+	}
- mutex_lock(&glink->idr_lock); 
  	idr_remove(&glink->lcids, channel->lcid);
  	channel->lcid = 0;
-	mutex_unlock(&glink->idr_lock);
+	spin_unlock_irqrestore(&glink->idr_lock, flags);
kref_put(&channel->refcount, qcom_glink_channel_release); 
  }
@@ -1017,7 +1036,7 @@ struct qcom_glink *qcom_glink_native_probe(struct device *dev,
  	INIT_LIST_HEAD(&glink->rx_queue);
  	INIT_WORK(&glink->rx_work, qcom_glink_work);
- mutex_init(&glink->idr_lock); 
+	spin_lock_init(&glink->idr_lock);
  	idr_init(&glink->lcids);
  	idr_init(&glink->rcids);
@@ -1060,6 +1079,7 @@ void qcom_glink_native_remove(struct qcom_glink *glink) 
  	struct glink_channel *channel;
  	int cid;
  	int ret;
+	unsigned long flags;
disable_irq(glink->irq); 
  	cancel_work_sync(&glink->rx_work);
@@ -1068,12 +1088,14 @@ void qcom_glink_native_remove(struct qcom_glink *glink)
  	if (ret)
  		dev_warn(glink->dev, "Can't remove GLINK devices: %d\n", ret);
+ spin_lock_irqsave(&glink->idr_lock, flags); 
  	/* Release any defunct local channels, waiting for close-ack */
  	idr_for_each_entry(&glink->lcids, channel, cid)
  		kref_put(&channel->refcount, qcom_glink_channel_release);
idr_destroy(&glink->lcids); 
  	idr_destroy(&glink->rcids);
+	spin_unlock_irqrestore(&glink->idr_lock, flags);
  	mbox_free_channel(glink->mbox_chan);
  }


--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux