From 810c03d9d7c978b4ee5287d8987043a9be1d614e Mon Sep 17 00:00:00 2001 From: Uwe Kleine-König Date: Tue, 21 Mar 2023 16:40:37 +0100 Subject: rpmsg: qcom_smd: Make qcom_smd_unregister_edge() return void MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This function returned zero unconditionally. Convert it to return no value instead. This makes it more obvious what happens in the callers. One caller is converted to return zero explicitly. The only other caller (smd_subdev_stop() in drivers/remoteproc/qcom_common.c) already ignored the return value before. Signed-off-by: Uwe Kleine-König Signed-off-by: Bjorn Andersson Link: https://lore.kernel.org/r/20230321154039.355098-2-u.kleine-koenig@pengutronix.de --- drivers/rpmsg/qcom_smd.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) (limited to 'drivers') diff --git a/drivers/rpmsg/qcom_smd.c b/drivers/rpmsg/qcom_smd.c index 1044cf03c542..38352f5792f4 100644 --- a/drivers/rpmsg/qcom_smd.c +++ b/drivers/rpmsg/qcom_smd.c @@ -1533,7 +1533,7 @@ static int qcom_smd_remove_device(struct device *dev, void *data) * qcom_smd_unregister_edge() - release an edge and its children * @edge: edge reference acquired from qcom_smd_register_edge */ -int qcom_smd_unregister_edge(struct qcom_smd_edge *edge) +void qcom_smd_unregister_edge(struct qcom_smd_edge *edge) { int ret; @@ -1547,8 +1547,6 @@ int qcom_smd_unregister_edge(struct qcom_smd_edge *edge) mbox_free_channel(edge->mbox_chan); device_unregister(&edge->dev); - - return 0; } EXPORT_SYMBOL(qcom_smd_unregister_edge); @@ -1572,7 +1570,9 @@ static int qcom_smd_remove_edge(struct device *dev, void *data) { struct qcom_smd_edge *edge = to_smd_edge(dev); - return qcom_smd_unregister_edge(edge); + qcom_smd_unregister_edge(edge); + + return 0; } /* -- cgit v1.2.3 From 49446e573bf591eef71c1e8c7cf87ec19aa2569f Mon Sep 17 00:00:00 2001 From: Uwe Kleine-König Date: Tue, 21 Mar 2023 16:40:38 +0100 Subject: rpmsg: qcom_glink_rpm: Convert to platform remove callback returning void MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The .remove() callback for a platform driver returns an int which makes many driver authors wrongly assume it's possible to do error handling by returning an error code. However the value returned is (mostly) ignored and this typically results in resource leaks. To improve here there is a quest to make the remove callback return void. In the first step of this quest all drivers are converted to .remove_new() which already returns void. Trivially convert this driver from always returning zero in the remove callback to the void returning variant. Signed-off-by: Uwe Kleine-König Signed-off-by: Bjorn Andersson Link: https://lore.kernel.org/r/20230321154039.355098-3-u.kleine-koenig@pengutronix.de --- drivers/rpmsg/qcom_glink_rpm.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) (limited to 'drivers') diff --git a/drivers/rpmsg/qcom_glink_rpm.c b/drivers/rpmsg/qcom_glink_rpm.c index f94bb7d4f1ec..357272d7062e 100644 --- a/drivers/rpmsg/qcom_glink_rpm.c +++ b/drivers/rpmsg/qcom_glink_rpm.c @@ -361,7 +361,7 @@ static int glink_rpm_probe(struct platform_device *pdev) return 0; } -static int glink_rpm_remove(struct platform_device *pdev) +static void glink_rpm_remove(struct platform_device *pdev) { struct glink_rpm *rpm = platform_get_drvdata(pdev); struct qcom_glink *glink = rpm->glink; @@ -371,8 +371,6 @@ static int glink_rpm_remove(struct platform_device *pdev) qcom_glink_native_remove(glink); mbox_free_channel(rpm->mbox_chan); - - return 0; } static const struct of_device_id glink_rpm_of_match[] = { @@ -383,7 +381,7 @@ MODULE_DEVICE_TABLE(of, glink_rpm_of_match); static struct platform_driver glink_rpm_driver = { .probe = glink_rpm_probe, - .remove = glink_rpm_remove, + .remove_new = glink_rpm_remove, .driver = { .name = "qcom_glink_rpm", .of_match_table = glink_rpm_of_match, -- cgit v1.2.3 From 38be89514b88f53ff772d1e016a68b59814aef72 Mon Sep 17 00:00:00 2001 From: Uwe Kleine-König Date: Tue, 21 Mar 2023 16:40:39 +0100 Subject: rpmsg: qcom_smd: Convert to platform remove callback returning void MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The .remove() callback for a platform driver returns an int which makes many driver authors wrongly assume it's possible to do error handling by returning an error code. However the value returned is (mostly) ignored and this typically results in resource leaks. To improve here there is a quest to make the remove callback return void. In the first step of this quest all drivers are converted to .remove_new() which already returns void. qcom_smd_remove() always returned zero, though that isn't completely trivial to see. So explain that in a comment and convert to .remove_new(). Signed-off-by: Uwe Kleine-König Signed-off-by: Bjorn Andersson Link: https://lore.kernel.org/r/20230321154039.355098-4-u.kleine-koenig@pengutronix.de --- drivers/rpmsg/qcom_smd.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) (limited to 'drivers') diff --git a/drivers/rpmsg/qcom_smd.c b/drivers/rpmsg/qcom_smd.c index 38352f5792f4..7b9c298aa491 100644 --- a/drivers/rpmsg/qcom_smd.c +++ b/drivers/rpmsg/qcom_smd.c @@ -1579,15 +1579,13 @@ static int qcom_smd_remove_edge(struct device *dev, void *data) * Shut down all smd clients by making sure that each edge stops processing * events and scanning for new channels, then call destroy on the devices. */ -static int qcom_smd_remove(struct platform_device *pdev) +static void qcom_smd_remove(struct platform_device *pdev) { - int ret; - - ret = device_for_each_child(&pdev->dev, NULL, qcom_smd_remove_edge); - if (ret) - dev_warn(&pdev->dev, "can't remove smd device: %d\n", ret); - - return ret; + /* + * qcom_smd_remove_edge always returns zero, so there is no need to + * check the return value of device_for_each_child. + */ + device_for_each_child(&pdev->dev, NULL, qcom_smd_remove_edge); } static const struct of_device_id qcom_smd_of_match[] = { @@ -1598,7 +1596,7 @@ MODULE_DEVICE_TABLE(of, qcom_smd_of_match); static struct platform_driver qcom_smd_driver = { .probe = qcom_smd_probe, - .remove = qcom_smd_remove, + .remove_new = qcom_smd_remove, .driver = { .name = "qcom-smd", .of_match_table = qcom_smd_of_match, -- cgit v1.2.3 From 0a7eee89e79eb8b97d46e1e0001b9e2709795af5 Mon Sep 17 00:00:00 2001 From: Bjorn Andersson Date: Mon, 27 Mar 2023 07:46:16 -0700 Subject: rpmsg: glink: Transition intent request signaling to wait queue Transition the intent request acknowledgement to use a wait queue so that it's possible, in the next commit, to extend the wait to also wait for an incoming intent. Signed-off-by: Bjorn Andersson Reviewed-by: Chris Lew Signed-off-by: Bjorn Andersson Link: https://lore.kernel.org/r/20230327144617.3134175-2-quic_bjorande@quicinc.com --- drivers/rpmsg/qcom_glink_native.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) (limited to 'drivers') diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c index 01d2805fe30f..0b6291fc2816 100644 --- a/drivers/rpmsg/qcom_glink_native.c +++ b/drivers/rpmsg/qcom_glink_native.c @@ -16,6 +16,7 @@ #include #include #include +#include #include #include @@ -145,7 +146,7 @@ enum { * @open_req: completed once open-request has been received * @intent_req_lock: Synchronises multiple intent requests * @intent_req_result: Result of intent request - * @intent_req_comp: Completion for intent_req signalling + * @intent_req_wq: wait queue for intent_req signalling */ struct glink_channel { struct rpmsg_endpoint ept; @@ -175,8 +176,8 @@ struct glink_channel { struct completion open_req; struct mutex intent_req_lock; - bool intent_req_result; - struct completion intent_req_comp; + int intent_req_result; + wait_queue_head_t intent_req_wq; }; #define to_glink_channel(_ept) container_of(_ept, struct glink_channel, ept) @@ -221,7 +222,7 @@ static struct glink_channel *qcom_glink_alloc_channel(struct qcom_glink *glink, init_completion(&channel->open_req); init_completion(&channel->open_ack); - init_completion(&channel->intent_req_comp); + init_waitqueue_head(&channel->intent_req_wq); INIT_LIST_HEAD(&channel->done_intents); INIT_WORK(&channel->intent_work, qcom_glink_rx_done_work); @@ -420,13 +421,13 @@ static void qcom_glink_handle_intent_req_ack(struct qcom_glink *glink, } channel->intent_req_result = granted; - complete(&channel->intent_req_comp); + wake_up_all(&channel->intent_req_wq); } static void qcom_glink_intent_req_abort(struct glink_channel *channel) { channel->intent_req_result = 0; - complete(&channel->intent_req_comp); + wake_up_all(&channel->intent_req_wq); } /** @@ -1271,7 +1272,7 @@ static int qcom_glink_request_intent(struct qcom_glink *glink, mutex_lock(&channel->intent_req_lock); - reinit_completion(&channel->intent_req_comp); + WRITE_ONCE(channel->intent_req_result, -1); cmd.id = GLINK_CMD_RX_INTENT_REQ; cmd.cid = channel->lcid; @@ -1281,12 +1282,14 @@ static int qcom_glink_request_intent(struct qcom_glink *glink, if (ret) goto unlock; - ret = wait_for_completion_timeout(&channel->intent_req_comp, 10 * HZ); + ret = wait_event_timeout(channel->intent_req_wq, + READ_ONCE(channel->intent_req_result) >= 0, + 10 * HZ); if (!ret) { dev_err(glink->dev, "intent request timed out\n"); ret = -ETIMEDOUT; } else { - ret = channel->intent_req_result ? 0 : -ECANCELED; + ret = READ_ONCE(channel->intent_req_result) ? 0 : -ECANCELED; } unlock: -- cgit v1.2.3 From c05dfce0b89e3b58043805b6f4bdf30e3561d867 Mon Sep 17 00:00:00 2001 From: Bjorn Andersson Date: Mon, 27 Mar 2023 07:46:17 -0700 Subject: rpmsg: glink: Wait for intent, not just request ack In some implementations of the remote firmware, an intent request acknowledgment is sent when it's determined if the intent allocation will be fulfilled, but then the intent is queued after the acknowledgment. The result is that upon receiving a granted allocation request, the search for the newly allocated intent will fail and an additional request will be made. This will at best waste memory, but if the second request is rejected the transaction will be incorrectly rejected. Take the incoming intent into account in the wait to mitigate this problem. The above scenario can still happen, in the case that, on that same channel, an unrelated intent is delivered prior to the request acknowledgment and a separate process enters the send path and picks up the intent. Given that there's no relationship between the acknowledgment and the delivered (or to be delivered intent), there doesn't seem to be a solution to this problem. Signed-off-by: Bjorn Andersson Reviewed-by: Chris Lew [bjorn: Fixed spelling issues pointed out by checkpatch in commit message] Signed-off-by: Bjorn Andersson Link: https://lore.kernel.org/r/20230327144617.3134175-3-quic_bjorande@quicinc.com --- drivers/rpmsg/qcom_glink_native.c | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) (limited to 'drivers') diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c index 0b6291fc2816..196ca7d8df7c 100644 --- a/drivers/rpmsg/qcom_glink_native.c +++ b/drivers/rpmsg/qcom_glink_native.c @@ -146,6 +146,7 @@ enum { * @open_req: completed once open-request has been received * @intent_req_lock: Synchronises multiple intent requests * @intent_req_result: Result of intent request + * @intent_received: flag indicating that an intent has been received * @intent_req_wq: wait queue for intent_req signalling */ struct glink_channel { @@ -177,6 +178,7 @@ struct glink_channel { struct mutex intent_req_lock; int intent_req_result; + bool intent_received; wait_queue_head_t intent_req_wq; }; @@ -420,13 +422,13 @@ static void qcom_glink_handle_intent_req_ack(struct qcom_glink *glink, return; } - channel->intent_req_result = granted; + WRITE_ONCE(channel->intent_req_result, granted); wake_up_all(&channel->intent_req_wq); } static void qcom_glink_intent_req_abort(struct glink_channel *channel) { - channel->intent_req_result = 0; + WRITE_ONCE(channel->intent_req_result, 0); wake_up_all(&channel->intent_req_wq); } @@ -757,6 +759,11 @@ static void qcom_glink_handle_rx_done(struct qcom_glink *glink, kfree(intent); } spin_unlock_irqrestore(&channel->intent_lock, flags); + + if (reuse) { + WRITE_ONCE(channel->intent_received, true); + wake_up_all(&channel->intent_req_wq); + } } /** @@ -994,6 +1001,9 @@ static void qcom_glink_handle_intent(struct qcom_glink *glink, dev_err(glink->dev, "failed to store remote intent\n"); } + WRITE_ONCE(channel->intent_received, true); + wake_up_all(&channel->intent_req_wq); + kfree(msg); qcom_glink_rx_advance(glink, ALIGN(msglen, 8)); } @@ -1273,6 +1283,7 @@ static int qcom_glink_request_intent(struct qcom_glink *glink, mutex_lock(&channel->intent_req_lock); WRITE_ONCE(channel->intent_req_result, -1); + WRITE_ONCE(channel->intent_received, false); cmd.id = GLINK_CMD_RX_INTENT_REQ; cmd.cid = channel->lcid; @@ -1283,7 +1294,8 @@ static int qcom_glink_request_intent(struct qcom_glink *glink, goto unlock; ret = wait_event_timeout(channel->intent_req_wq, - READ_ONCE(channel->intent_req_result) >= 0, + READ_ONCE(channel->intent_req_result) >= 0 && + READ_ONCE(channel->intent_received), 10 * HZ); if (!ret) { dev_err(glink->dev, "intent request timed out\n"); -- cgit v1.2.3 From 7a68f9fa97357a0f2073c9c31ed4101da4fce93e Mon Sep 17 00:00:00 2001 From: Bjorn Andersson Date: Tue, 18 Apr 2023 09:30:17 -0700 Subject: rpmsg: glink: Propagate TX failures in intentless mode as well As support for splitting transmission over several messages using TX_DATA_CONT was introduced it does not immediately return the return value of qcom_glink_tx(). The result is that in the intentless case (i.e. intent == NULL), the code will continue to send all additional chunks. This is wasteful, and it's possible that the send operation could incorrectly indicate success, if the last chunk fits in the TX fifo. Fix the condition. Fixes: 8956927faed3 ("rpmsg: glink: Add TX_DATA_CONT command while sending") Reviewed-by: Chris Lew Signed-off-by: Bjorn Andersson Signed-off-by: Bjorn Andersson Link: https://lore.kernel.org/r/20230418163018.785524-2-quic_bjorande@quicinc.com --- drivers/rpmsg/qcom_glink_native.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) (limited to 'drivers') diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c index 196ca7d8df7c..916c0dbe4213 100644 --- a/drivers/rpmsg/qcom_glink_native.c +++ b/drivers/rpmsg/qcom_glink_native.c @@ -1371,8 +1371,9 @@ static int __qcom_glink_send(struct glink_channel *channel, ret = qcom_glink_tx(glink, &req, sizeof(req), data, chunk_size, wait); /* Mark intent available if we failed */ - if (ret && intent) { - intent->in_use = false; + if (ret) { + if (intent) + intent->in_use = false; return ret; } @@ -1393,8 +1394,9 @@ static int __qcom_glink_send(struct glink_channel *channel, chunk_size, wait); /* Mark intent available if we failed */ - if (ret && intent) { - intent->in_use = false; + if (ret) { + if (intent) + intent->in_use = false; break; } } -- cgit v1.2.3 From ba7a4754da1092decbeea3b29bf7d5946f1be400 Mon Sep 17 00:00:00 2001 From: Bjorn Andersson Date: Tue, 18 Apr 2023 09:30:18 -0700 Subject: rpmsg: glink: Consolidate TX_DATA and TX_DATA_CONT Rather than duplicating most of the code for constructing the initial TX_DATA and subsequent TX_DATA_CONT packets, roll them into a single loop. Signed-off-by: Bjorn Andersson Reviewed-by: Chris Lew Signed-off-by: Bjorn Andersson Link: https://lore.kernel.org/r/20230418163018.785524-3-quic_bjorande@quicinc.com --- drivers/rpmsg/qcom_glink_native.c | 46 +++++++++++---------------------------- 1 file changed, 13 insertions(+), 33 deletions(-) (limited to 'drivers') diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c index 916c0dbe4213..1beb40a1d3df 100644 --- a/drivers/rpmsg/qcom_glink_native.c +++ b/drivers/rpmsg/qcom_glink_native.c @@ -1324,7 +1324,7 @@ static int __qcom_glink_send(struct glink_channel *channel, int ret; unsigned long flags; int chunk_size = len; - int left_size = 0; + size_t offset = 0; if (!glink->intentless) { while (!intent) { @@ -1358,49 +1358,29 @@ static int __qcom_glink_send(struct glink_channel *channel, iid = intent->id; } - if (wait && chunk_size > SZ_8K) { - chunk_size = SZ_8K; - left_size = len - chunk_size; - } - req.msg.cmd = cpu_to_le16(GLINK_CMD_TX_DATA); - req.msg.param1 = cpu_to_le16(channel->lcid); - req.msg.param2 = cpu_to_le32(iid); - req.chunk_size = cpu_to_le32(chunk_size); - req.left_size = cpu_to_le32(left_size); - - ret = qcom_glink_tx(glink, &req, sizeof(req), data, chunk_size, wait); - - /* Mark intent available if we failed */ - if (ret) { - if (intent) - intent->in_use = false; - return ret; - } - - while (left_size > 0) { - data = (void *)((char *)data + chunk_size); - chunk_size = left_size; - if (chunk_size > SZ_8K) + while (offset < len) { + chunk_size = len - offset; + if (chunk_size > SZ_8K && wait) chunk_size = SZ_8K; - left_size -= chunk_size; - req.msg.cmd = cpu_to_le16(GLINK_CMD_TX_DATA_CONT); + req.msg.cmd = cpu_to_le16(offset == 0 ? GLINK_CMD_TX_DATA : GLINK_CMD_TX_DATA_CONT); req.msg.param1 = cpu_to_le16(channel->lcid); req.msg.param2 = cpu_to_le32(iid); req.chunk_size = cpu_to_le32(chunk_size); - req.left_size = cpu_to_le32(left_size); + req.left_size = cpu_to_le32(len - offset - chunk_size); - ret = qcom_glink_tx(glink, &req, sizeof(req), data, - chunk_size, wait); - - /* Mark intent available if we failed */ + ret = qcom_glink_tx(glink, &req, sizeof(req), data + offset, chunk_size, wait); if (ret) { + /* Mark intent available if we failed */ if (intent) intent->in_use = false; - break; + return ret; } + + offset += chunk_size; } - return ret; + + return 0; } static int qcom_glink_send(struct rpmsg_endpoint *ept, void *data, int len) -- cgit v1.2.3