[PATCH] rpmsg: glink: use only lower 16-bits of param2 for CMD_OPEN name length

[PATCH] rpmsg: glink: use only lower 16-bits of param2 for CMD_OPEN name length

[Jonathan Marek]

The name len field of the CMD_OPEN packet is only 16-bits and the upper
16-bits of "param2" are a different field, which can be nonzero in certain
situations, and CMD_OPEN packets can be unexpectedly dropped because of
this.

Fix this by masking out the upper 16 bits of param2.

(the commit in this Fixes tag is not where the original code was introduced
but it should be far back enough not to matter)

Fixes: 835764ddd9af ("rpmsg: glink: Move the common glink protocol implementation to glink_native.c")
Signed-off-by: Jonathan Marek <jonathan@marek.ca>
---
 drivers/rpmsg/qcom_glink_native.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
index 0b2f290069080..e4933b823238c 100644
--- a/drivers/rpmsg/qcom_glink_native.c
+++ b/drivers/rpmsg/qcom_glink_native.c
@@ -1204,7 +1204,7 @@ void qcom_glink_native_rx(struct qcom_glink *glink)
 			ret = qcom_glink_rx_open_ack(glink, param1);
 			break;
 		case GLINK_CMD_OPEN:
-			ret = qcom_glink_rx_defer(glink, param2);
+			ret = qcom_glink_rx_defer(glink, param2 & 0xffff);
 			break;
 		case GLINK_CMD_TX_DATA:
 		case GLINK_CMD_TX_DATA_CONT:
-- 
2.45.1

Re: [PATCH] rpmsg: glink: use only lower 16-bits of param2 for CMD_OPEN name length

[Dmitry Baryshkov]

On Mon, Oct 07, 2024 at 12:47:22AM GMT, Jonathan Marek wrote:
> The name len field of the CMD_OPEN packet is only 16-bits and the upper
> 16-bits of "param2" are a different field, which can be nonzero in certain
> situations, and CMD_OPEN packets can be unexpectedly dropped because of
> this.

Any idea about the upper 16 bits? Should we care about that data too?

> 
> Fix this by masking out the upper 16 bits of param2.
> 
> (the commit in this Fixes tag is not where the original code was introduced
> but it should be far back enough not to matter)

Let's be more precise:

Fixes: b4f8e52b89f6 ("rpmsg: Introduce Qualcomm RPM glink driver")

> 
> Fixes: 835764ddd9af ("rpmsg: glink: Move the common glink protocol implementation to glink_native.c")
> Signed-off-by: Jonathan Marek <jonathan@marek.ca>
> ---
>  drivers/rpmsg/qcom_glink_native.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
> index 0b2f290069080..e4933b823238c 100644
> --- a/drivers/rpmsg/qcom_glink_native.c
> +++ b/drivers/rpmsg/qcom_glink_native.c
> @@ -1204,7 +1204,7 @@ void qcom_glink_native_rx(struct qcom_glink *glink)
>  			ret = qcom_glink_rx_open_ack(glink, param1);
>  			break;
>  		case GLINK_CMD_OPEN:
> -			ret = qcom_glink_rx_defer(glink, param2);
> +			ret = qcom_glink_rx_defer(glink, param2 & 0xffff);
>  			break;
>  		case GLINK_CMD_TX_DATA:
>  		case GLINK_CMD_TX_DATA_CONT:
> -- 
> 2.45.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH] rpmsg: glink: use only lower 16-bits of param2 for CMD_OPEN name length

[Jonathan Marek]

On 10/7/24 9:16 AM, Dmitry Baryshkov wrote:
> On Mon, Oct 07, 2024 at 12:47:22AM GMT, Jonathan Marek wrote:
>> The name len field of the CMD_OPEN packet is only 16-bits and the upper
>> 16-bits of "param2" are a different field, which can be nonzero in certain
>> situations, and CMD_OPEN packets can be unexpectedly dropped because of
>> this.
> 
> Any idea about the upper 16 bits? Should we care about that data too?
> 

Its a "prio" value, it should be OK to ignore it.

Re: [PATCH] rpmsg: glink: use only lower 16-bits of param2 for CMD_OPEN name length

[Dmitry Baryshkov]

On Mon, Oct 07, 2024 at 09:19:05AM GMT, Jonathan Marek wrote:
> On 10/7/24 9:16 AM, Dmitry Baryshkov wrote:
> > On Mon, Oct 07, 2024 at 12:47:22AM GMT, Jonathan Marek wrote:
> > > The name len field of the CMD_OPEN packet is only 16-bits and the upper
> > > 16-bits of "param2" are a different field, which can be nonzero in certain
> > > situations, and CMD_OPEN packets can be unexpectedly dropped because of
> > > this.
> > 
> > Any idea about the upper 16 bits? Should we care about that data too?
> > 
> 
> Its a "prio" value, it should be OK to ignore it.

Could you please document it in the commit message and in a comment next
to the qcom_glink_rx_defer() call?

-- 
With best wishes
Dmitry