| Message ID | 1503559302-3744-12-git-send-email-sricharan@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@codeaurora.org> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> Acked-by: Arun Kumar Neelakantam <aneela@codeaurora.org> 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); > } >
Hi Sricharan, Minor bug in this patch. On 8/24/2017 12:21 AM, Sricharan R wrote: [..] > @@ -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); I think GFP_KERNEL should be changed to GFP_ATOMIC if the mutex is changed to spinlock. Thanks, Chris
Hi Chris, On 8/29/2017 3:21 AM, Chris Lew wrote: > Hi Sricharan, > > Minor bug in this patch. > > On 8/24/2017 12:21 AM, Sricharan R wrote: > [..] >> @@ -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); > > I think GFP_KERNEL should be changed to GFP_ATOMIC if the mutex is changed to spinlock. > ha, thanks for the catch. Regards, Sricharan
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); }