2023-03-27 14:49:56

by Bjorn Andersson

Subject: [PATCH 0/2] rpmsg: glink: Fix + cleanup in __qcom_glink_send()

Found these two things to fix/cleanup while poking around in the glink
code.

Bjorn Andersson (2):
rpmsg: glink: Propagate TX failures in intentless mode as well
rpmsg: glink: Consolidate TX_DATA and TX_DATA_CONT

drivers/rpmsg/qcom_glink_native.c | 50 ++++++++++---------------------
1 file changed, 16 insertions(+), 34 deletions(-)

--
2.25.1

2023-03-27 14:50:22

by Bjorn Andersson

Subject: [PATCH 1/2] rpmsg: glink: Propagate TX failures in intentless mode as well

As support for splitting transmision 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")
Signed-off-by: Bjorn Andersson <[email protected]>
---
drivers/rpmsg/qcom_glink_native.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
index 01d2805fe30f..62634d020d13 100644
--- a/drivers/rpmsg/qcom_glink_native.c
+++ b/drivers/rpmsg/qcom_glink_native.c
@@ -1356,8 +1356,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;
}

@@ -1378,8 +1379,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;
}
}
--
2.25.1

2023-03-28 23:08:36

by Chris Lew

Subject: Re: [PATCH 1/2] rpmsg: glink: Propagate TX failures in intentless mode as well

On 3/27/2023 7:41 AM, Bjorn Andersson wrote:
> As support for splitting transmision 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")
> Signed-off-by: Bjorn Andersson <[email protected]>
> ---

s/transmision/transmission

Reviewed-by: Chris Lew <[email protected]>

> drivers/rpmsg/qcom_glink_native.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
> index 01d2805fe30f..62634d020d13 100644
> --- a/drivers/rpmsg/qcom_glink_native.c
> +++ b/drivers/rpmsg/qcom_glink_native.c
> @@ -1356,8 +1356,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;
> }
>
> @@ -1378,8 +1379,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;
> }
> }
>