From a9011726c4bb37e5d6a7279bf47fcc19cd9d3e1a Mon Sep 17 00:00:00 2001 From: Arvind Yadav Date: Thu, 8 Mar 2018 15:06:07 +0530 Subject: [PATCH 1/8] rpmsg: glink: use put_device() if device_register fail if device_register() returned an error! Always use put_device() to give up the reference initialized. And unregister device for other return error. Signed-off-by: Arvind Yadav Signed-off-by: Bjorn Andersson --- drivers/rpmsg/qcom_glink_smem.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/rpmsg/qcom_glink_smem.c b/drivers/rpmsg/qcom_glink_smem.c index 892f2b92a4d8..3fa9d43e2c87 100644 --- a/drivers/rpmsg/qcom_glink_smem.c +++ b/drivers/rpmsg/qcom_glink_smem.c @@ -217,6 +217,7 @@ struct qcom_glink *qcom_glink_smem_register(struct device *parent, ret = device_register(dev); if (ret) { pr_err("failed to register glink edge\n"); + put_device(dev); return ERR_PTR(ret); } @@ -299,7 +300,7 @@ struct qcom_glink *qcom_glink_smem_register(struct device *parent, return glink; err_put_dev: - put_device(dev); + device_unregister(dev); return ERR_PTR(ret); } From be5acd246daa04edad9d758b8be37e5e2f989243 Mon Sep 17 00:00:00 2001 From: Arvind Yadav Date: Thu, 8 Mar 2018 15:06:08 +0530 Subject: [PATCH 2/8] rpmsg: smd: use put_device() if device_register fail if device_register() returned an error! Always use put_device() to give up the reference initialized. unregister device for other return error. Signed-off-by: Arvind Yadav Signed-off-by: Bjorn Andersson --- drivers/rpmsg/qcom_smd.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/rpmsg/qcom_smd.c b/drivers/rpmsg/qcom_smd.c index 92d0c6a7a837..ff8101a768bc 100644 --- a/drivers/rpmsg/qcom_smd.c +++ b/drivers/rpmsg/qcom_smd.c @@ -1408,6 +1408,7 @@ struct qcom_smd_edge *qcom_smd_register_edge(struct device *parent, ret = device_register(&edge->dev); if (ret) { pr_err("failed to register smd edge\n"); + put_device(&edge->dev); return ERR_PTR(ret); } @@ -1428,7 +1429,7 @@ struct qcom_smd_edge *qcom_smd_register_edge(struct device *parent, return edge; unregister_dev: - put_device(&edge->dev); + device_unregister(&edge->dev); return ERR_PTR(ret); } EXPORT_SYMBOL(qcom_smd_register_edge); From 33e3820dda8876792bd8135db633c741a07263be Mon Sep 17 00:00:00 2001 From: Bjorn Andersson Date: Tue, 13 Feb 2018 11:04:11 -0800 Subject: [PATCH 3/8] rpmsg: smd: Use spinlock in tx path By switching the tx_lock to a spinlock we allow clients to use rpmsg_trysend() from atomic context. The mutex was interruptable as it was previously held for the duration of some client waiting for available space in the FIFO, but this was recently changed to only be held temporarily - allowing us to replace it with a spinlock. Tested-by: Srinivas Kandagatla Signed-off-by: Bjorn Andersson --- drivers/rpmsg/qcom_smd.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/drivers/rpmsg/qcom_smd.c b/drivers/rpmsg/qcom_smd.c index ff8101a768bc..76b1c00b4a19 100644 --- a/drivers/rpmsg/qcom_smd.c +++ b/drivers/rpmsg/qcom_smd.c @@ -205,7 +205,7 @@ struct qcom_smd_channel { struct smd_channel_info_pair *info; struct smd_channel_info_word_pair *info_word; - struct mutex tx_lock; + spinlock_t tx_lock; wait_queue_head_t fblockread_event; void *tx_fifo; @@ -729,6 +729,7 @@ static int __qcom_smd_send(struct qcom_smd_channel *channel, const void *data, { __le32 hdr[5] = { cpu_to_le32(len), }; int tlen = sizeof(hdr) + len; + unsigned long flags; int ret; /* Word aligned channels only accept word size aligned data */ @@ -739,9 +740,11 @@ static int __qcom_smd_send(struct qcom_smd_channel *channel, const void *data, if (tlen >= channel->fifo_size) return -EINVAL; - ret = mutex_lock_interruptible(&channel->tx_lock); - if (ret) - return ret; + /* Highlight the fact that if we enter the loop below we might sleep */ + if (wait) + might_sleep(); + + spin_lock_irqsave(&channel->tx_lock, flags); while (qcom_smd_get_tx_avail(channel) < tlen && channel->state == SMD_CHANNEL_OPENED) { @@ -753,7 +756,7 @@ static int __qcom_smd_send(struct qcom_smd_channel *channel, const void *data, SET_TX_CHANNEL_FLAG(channel, fBLOCKREADINTR, 0); /* Wait without holding the tx_lock */ - mutex_unlock(&channel->tx_lock); + spin_unlock_irqrestore(&channel->tx_lock, flags); ret = wait_event_interruptible(channel->fblockread_event, qcom_smd_get_tx_avail(channel) >= tlen || @@ -761,9 +764,7 @@ static int __qcom_smd_send(struct qcom_smd_channel *channel, const void *data, if (ret) return ret; - ret = mutex_lock_interruptible(&channel->tx_lock); - if (ret) - return ret; + spin_lock_irqsave(&channel->tx_lock, flags); SET_TX_CHANNEL_FLAG(channel, fBLOCKREADINTR, 1); } @@ -787,7 +788,7 @@ static int __qcom_smd_send(struct qcom_smd_channel *channel, const void *data, qcom_smd_signal_channel(channel); out_unlock: - mutex_unlock(&channel->tx_lock); + spin_unlock_irqrestore(&channel->tx_lock, flags); return ret; } @@ -1090,7 +1091,7 @@ static struct qcom_smd_channel *qcom_smd_create_channel(struct qcom_smd_edge *ed if (!channel->name) return ERR_PTR(-ENOMEM); - mutex_init(&channel->tx_lock); + spin_lock_init(&channel->tx_lock); spin_lock_init(&channel->recv_lock); init_waitqueue_head(&channel->fblockread_event); init_waitqueue_head(&channel->state_change_event); From 29fc9b3873607d01b1ff1ae077982cf5629010af Mon Sep 17 00:00:00 2001 From: Bjorn Andersson Date: Tue, 13 Feb 2018 11:04:04 -0800 Subject: [PATCH 4/8] rpmsg: glink: Use spinlock in tx path Switch the tx_lock to a spinlock we allow clients to use rpmsg_trysend() from atomic context. In order to allow clients to sleep while waiting for space in the FIFO we release the lock temporarily around the delay; which should be replaced by sending a READ_NOTIF and waiting for the remote to signal us that space has been made available. Tested-by: Srinivas Kandagatla Signed-off-by: Bjorn Andersson --- drivers/rpmsg/qcom_glink_native.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c index e0f31ed096a5..768ef542a841 100644 --- a/drivers/rpmsg/qcom_glink_native.c +++ b/drivers/rpmsg/qcom_glink_native.c @@ -113,7 +113,7 @@ struct qcom_glink { spinlock_t rx_lock; struct list_head rx_queue; - struct mutex tx_lock; + spinlock_t tx_lock; spinlock_t idr_lock; struct idr lcids; @@ -288,15 +288,14 @@ static int qcom_glink_tx(struct qcom_glink *glink, const void *data, size_t dlen, bool wait) { unsigned int tlen = hlen + dlen; - int ret; + unsigned long flags; + int ret = 0; /* Reject packets that are too big */ if (tlen >= glink->tx_pipe->length) return -EINVAL; - ret = mutex_lock_interruptible(&glink->tx_lock); - if (ret) - return ret; + spin_lock_irqsave(&glink->tx_lock, flags); while (qcom_glink_tx_avail(glink) < tlen) { if (!wait) { @@ -304,7 +303,12 @@ static int qcom_glink_tx(struct qcom_glink *glink, goto out; } + /* Wait without holding the tx_lock */ + spin_unlock_irqrestore(&glink->tx_lock, flags); + usleep_range(10000, 15000); + + spin_lock_irqsave(&glink->tx_lock, flags); } qcom_glink_tx_write(glink, hdr, hlen, data, dlen); @@ -313,7 +317,7 @@ static int qcom_glink_tx(struct qcom_glink *glink, mbox_client_txdone(glink->mbox_chan, 0); out: - mutex_unlock(&glink->tx_lock); + spin_unlock_irqrestore(&glink->tx_lock, flags); return ret; } @@ -1567,7 +1571,7 @@ struct qcom_glink *qcom_glink_native_probe(struct device *dev, glink->features = features; glink->intentless = intentless; - mutex_init(&glink->tx_lock); + spin_lock_init(&glink->tx_lock); spin_lock_init(&glink->rx_lock); INIT_LIST_HEAD(&glink->rx_queue); INIT_WORK(&glink->rx_work, qcom_glink_work); From 2bd9b4385fd7ece4f0c64f38bea7726a810a06af Mon Sep 17 00:00:00 2001 From: Bjorn Andersson Date: Thu, 15 Mar 2018 11:12:44 -0700 Subject: [PATCH 5/8] Revert "rpmsg: smd: Create device for all channels" In an effort to pick up channels that are in a funky state we optimistically tried to open all channels that we found, with the addition that we failed if the other side did not handshake the opening. But as we're starting the modem a second time all channels are found - in a "funky" state - and we try to open them. But the modem firmware requires the IPCRTR to be up in order to initialize. So any channels we try to open before that will fail and will not be opened again. This takes care of the regression, at the cost of reintroducing the previous behavior of handling of channels with "funky" states. Reverts commit c12fc4519f60 ("rpmsg: smd: Create device for all channels") Reported-by: Srinivas Kandagatla Signed-off-by: Bjorn Andersson --- drivers/rpmsg/qcom_smd.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/drivers/rpmsg/qcom_smd.c b/drivers/rpmsg/qcom_smd.c index 76b1c00b4a19..b062e9d6e25f 100644 --- a/drivers/rpmsg/qcom_smd.c +++ b/drivers/rpmsg/qcom_smd.c @@ -1235,6 +1235,11 @@ static void qcom_channel_state_worker(struct work_struct *work) if (channel->state != SMD_CHANNEL_CLOSED) continue; + remote_state = GET_RX_CHANNEL_INFO(channel, state); + if (remote_state != SMD_CHANNEL_OPENING && + remote_state != SMD_CHANNEL_OPENED) + continue; + if (channel->registered) continue; From 6ddf12d397aee75dbd24dd02be07c1372e3008f6 Mon Sep 17 00:00:00 2001 From: Bjorn Andersson Date: Tue, 27 Mar 2018 14:06:41 -0700 Subject: [PATCH 6/8] rpmsg: smd: Fix container_of macros The container_of macros should not use the same name for the parameter as the member to use for lookup, as this will result in a compilation error unless the passed parameter has the same name as the member. Signed-off-by: Bjorn Andersson --- drivers/rpmsg/qcom_smd.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/rpmsg/qcom_smd.c b/drivers/rpmsg/qcom_smd.c index b062e9d6e25f..35089039a335 100644 --- a/drivers/rpmsg/qcom_smd.c +++ b/drivers/rpmsg/qcom_smd.c @@ -167,9 +167,9 @@ struct qcom_smd_endpoint { struct qcom_smd_channel *qsch; }; -#define to_smd_device(_rpdev) container_of(_rpdev, struct qcom_smd_device, rpdev) +#define to_smd_device(r) container_of(r, struct qcom_smd_device, rpdev) #define to_smd_edge(d) container_of(d, struct qcom_smd_edge, dev) -#define to_smd_endpoint(ept) container_of(ept, struct qcom_smd_endpoint, ept) +#define to_smd_endpoint(e) container_of(e, struct qcom_smd_endpoint, ept) /** * struct qcom_smd_channel - smd channel struct From 7586516ca043d55ed1ca563df72bc216c948cad4 Mon Sep 17 00:00:00 2001 From: Bjorn Andersson Date: Tue, 27 Mar 2018 14:06:42 -0700 Subject: [PATCH 7/8] rpmsg: Only invoke announce_create for rpdev with endpoints For special rpmsg devices without a primary endpoint there is nothing to announce so don't call the backend announce create function if we didn't create an endpoint. Signed-off-by: Bjorn Andersson --- drivers/rpmsg/rpmsg_core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c index 5a081762afcc..920a02f0462c 100644 --- a/drivers/rpmsg/rpmsg_core.c +++ b/drivers/rpmsg/rpmsg_core.c @@ -442,7 +442,7 @@ static int rpmsg_dev_probe(struct device *dev) goto out; } - if (rpdev->ops->announce_create) + if (ept && rpdev->ops->announce_create) err = rpdev->ops->announce_create(rpdev); out: return err; From 0d72038c303c616a63415a07366f916b5edc3830 Mon Sep 17 00:00:00 2001 From: Bjorn Andersson Date: Tue, 27 Mar 2018 14:06:43 -0700 Subject: [PATCH 8/8] rpmsg: smd: Use announce_create to process any receive work It is possible that incoming data arrives before the client driver has reached a point in the probe method where adequate context for handling the incoming message has been established. In the event that the client's callback function returns an error the message will be left on the FIFO and by invoking the receive handler after the device has been probed the message will be picked off the FIFO and the callback invoked again. Signed-off-by: Bjorn Andersson --- drivers/rpmsg/qcom_smd.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/drivers/rpmsg/qcom_smd.c b/drivers/rpmsg/qcom_smd.c index 35089039a335..5ce9bf7b897d 100644 --- a/drivers/rpmsg/qcom_smd.c +++ b/drivers/rpmsg/qcom_smd.c @@ -997,8 +997,26 @@ static struct device_node *qcom_smd_match_channel(struct device_node *edge_node, return NULL; } +static int qcom_smd_announce_create(struct rpmsg_device *rpdev) +{ + struct qcom_smd_endpoint *qept = to_smd_endpoint(rpdev->ept); + struct qcom_smd_channel *channel = qept->qsch; + unsigned long flags; + bool kick_state; + + spin_lock_irqsave(&channel->recv_lock, flags); + kick_state = qcom_smd_channel_intr(channel); + spin_unlock_irqrestore(&channel->recv_lock, flags); + + if (kick_state) + schedule_work(&channel->edge->state_work); + + return 0; +} + static const struct rpmsg_device_ops qcom_smd_device_ops = { .create_ept = qcom_smd_create_ept, + .announce_create = qcom_smd_announce_create, }; static const struct rpmsg_endpoint_ops qcom_smd_endpoint_ops = {