[PATCH 0/3] rpmsg: glink: smem: robustness and debuggability fixes

[PATCH 0/3] rpmsg: glink: smem: robustness and debuggability fixes

[Chunkai Deng]

This series makes three small improvements to the SMEM GLINK transport:

1. Use modulo instead of subtraction for FIFO tail wrap-around in
   rx_advance, so the index is always normalised back into [0, length)
   regardless of the advance count.

2. Add WARN_ON_ONCE checks in the FIFO read/write helpers to catch and
   report broken index invariants early, rather than proceeding with an
   out-of-bounds offset into the FIFO.

3. Pass dev_name(&smem->dev) as the IRQ name instead of the static string
   "glink-smem". On platforms with multiple remoteprocs each SMEM GLINK
   instance registers its own IRQ, and the unique device name makes each
   entry in /proc/interrupts identifiable without adding a new struct field.

Signed-off-by: Chunkai Deng <chunkai.deng@oss.qualcomm.com>
---
Chunkai Deng (3):
      rpmsg: glink: smem: Use device name as IRQ name
      rpmsg: glink: smem: Add WARN_ON_ONCE for FIFO index invariants
      rpmsg: glink: smem: Use modulo for FIFO tail wrap-around in rx_advance

 drivers/rpmsg/qcom_glink_smem.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)
---
base-commit: f7af91adc230aa99e23330ecf85bc9badd9780ad
change-id: 20260601-rpmsg-improvements-643a6e133f66

Best regards,
--  
Chunkai Deng <chunkai.deng@oss.qualcomm.com>

[PATCH 1/3] rpmsg: glink: smem: Use device name as IRQ name

[Chunkai Deng]

A SoC typically has multiple remoteprocs (ADSP, MPSS, CDSP, etc.), each
registering its own SMEM GLINK instance. With the static name "glink-smem"
all instances appear identically in /proc/interrupts, making it impossible
to associate an IRQ with a specific remote processor.

Pass dev_name(&smem->dev) to devm_request_irq() instead. The device name
is already set to "<parent>:<edge-node>" which uniquely identifies each
instance without requiring an additional field in the driver struct.

Signed-off-by: Chunkai Deng <chunkai.deng@oss.qualcomm.com>
---
 drivers/rpmsg/qcom_glink_smem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/rpmsg/qcom_glink_smem.c b/drivers/rpmsg/qcom_glink_smem.c
index 62adc4db2317..edab912557ac 100644
--- a/drivers/rpmsg/qcom_glink_smem.c
+++ b/drivers/rpmsg/qcom_glink_smem.c
@@ -307,7 +307,7 @@ struct qcom_glink_smem *qcom_glink_smem_register(struct device *parent,
 	smem->irq = of_irq_get(smem->dev.of_node, 0);
 	ret = devm_request_irq(&smem->dev, smem->irq, qcom_glink_smem_intr,
 			       IRQF_NO_SUSPEND | IRQF_NO_AUTOEN,
-			       "glink-smem", smem);
+			       dev_name(&smem->dev), smem);
 	if (ret) {
 		dev_err(&smem->dev, "failed to request IRQ\n");
 		goto err_put_dev;

-- 
2.34.1

Re: [PATCH 1/3] rpmsg: glink: smem: Use device name as IRQ name

[Dmitry Baryshkov]

On Wed, Jun 03, 2026 at 06:14:28PM +0800, Chunkai Deng wrote:
> A SoC typically has multiple remoteprocs (ADSP, MPSS, CDSP, etc.), each
> registering its own SMEM GLINK instance. With the static name "glink-smem"
> all instances appear identically in /proc/interrupts, making it impossible
> to associate an IRQ with a specific remote processor.
> 
> Pass dev_name(&smem->dev) to devm_request_irq() instead. The device name
> is already set to "<parent>:<edge-node>" which uniquely identifies each
> instance without requiring an additional field in the driver struct.
> 
> Signed-off-by: Chunkai Deng <chunkai.deng@oss.qualcomm.com>
> ---
>  drivers/rpmsg/qcom_glink_smem.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>


-- 
With best wishes
Dmitry

[PATCH 2/3] rpmsg: glink: smem: Add WARN_ON_ONCE for FIFO index invariants

[Chunkai Deng]

The FIFO read/write helpers assume the head and tail indices stay within
[0, pipe->native.length) and use them directly as offsets into the
mapped FIFO region. If that invariant is ever broken, the subsequent
memcpy or memcpy_fromio would access memory outside the FIFO.

Add WARN_ON_ONCE checks in these helpers so a broken invariant is
caught and reported once, and the out-of-bounds access is skipped,
instead of proceeding silently.

Signed-off-by: Chunkai Deng <chunkai.deng@oss.qualcomm.com>
---
 drivers/rpmsg/qcom_glink_smem.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/rpmsg/qcom_glink_smem.c b/drivers/rpmsg/qcom_glink_smem.c
index edab912557ac..42ad315d7910 100644
--- a/drivers/rpmsg/qcom_glink_smem.c
+++ b/drivers/rpmsg/qcom_glink_smem.c
@@ -86,9 +86,14 @@ static size_t glink_smem_rx_avail(struct qcom_glink_pipe *np)
 	tail = le32_to_cpu(*pipe->tail);
 
 	if (head < tail)
-		return pipe->native.length - tail + head;
+		len = pipe->native.length - tail + head;
 	else
-		return head - tail;
+		len = head - tail;
+
+	if (WARN_ON_ONCE(len > pipe->native.length))
+		len = 0;
+
+	return len;
 }
 
 static void glink_smem_rx_peek(struct qcom_glink_pipe *np,
@@ -103,6 +108,9 @@ static void glink_smem_rx_peek(struct qcom_glink_pipe *np,
 	if (tail >= pipe->native.length)
 		tail -= pipe->native.length;
 
+	if (WARN_ON_ONCE(tail >= pipe->native.length))
+		return;
+
 	len = min_t(size_t, count, pipe->native.length - tail);
 	if (len)
 		memcpy_fromio(data, pipe->fifo + tail, len);
@@ -141,6 +149,9 @@ static size_t glink_smem_tx_avail(struct qcom_glink_pipe *np)
 	else
 		avail = tail - head;
 
+	if (WARN_ON_ONCE(avail > pipe->native.length))
+		avail = 0;
+
 	if (avail < (FIFO_FULL_RESERVE + TX_BLOCKED_CMD_RESERVE))
 		avail = 0;
 	else
@@ -155,6 +166,9 @@ static unsigned int glink_smem_tx_write_one(struct glink_smem_pipe *pipe,
 {
 	size_t len;
 
+	if (WARN_ON_ONCE(head >= pipe->native.length))
+		return head;
+
 	len = min_t(size_t, count, pipe->native.length - head);
 	if (len)
 		memcpy(pipe->fifo + head, data, len);

-- 
2.34.1

Re: [PATCH 2/3] rpmsg: glink: smem: Add WARN_ON_ONCE for FIFO index invariants

[Dmitry Baryshkov]

On Wed, Jun 03, 2026 at 06:14:29PM +0800, Chunkai Deng wrote:
> The FIFO read/write helpers assume the head and tail indices stay within
> [0, pipe->native.length) and use them directly as offsets into the
> mapped FIFO region. If that invariant is ever broken, the subsequent
> memcpy or memcpy_fromio would access memory outside the FIFO.
> 
> Add WARN_ON_ONCE checks in these helpers so a broken invariant is
> caught and reported once, and the out-of-bounds access is skipped,

I think the comma should not be here. Instead please add the Fixes and
cc:stable tags.

> instead of proceeding silently.
> 
> Signed-off-by: Chunkai Deng <chunkai.deng@oss.qualcomm.com>
> ---
>  drivers/rpmsg/qcom_glink_smem.c | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/rpmsg/qcom_glink_smem.c b/drivers/rpmsg/qcom_glink_smem.c
> index edab912557ac..42ad315d7910 100644
> --- a/drivers/rpmsg/qcom_glink_smem.c
> +++ b/drivers/rpmsg/qcom_glink_smem.c
> @@ -86,9 +86,14 @@ static size_t glink_smem_rx_avail(struct qcom_glink_pipe *np)
>  	tail = le32_to_cpu(*pipe->tail);
>  
>  	if (head < tail)
> -		return pipe->native.length - tail + head;
> +		len = pipe->native.length - tail + head;
>  	else
> -		return head - tail;
> +		len = head - tail;
> +
> +	if (WARN_ON_ONCE(len > pipe->native.length))
> +		len = 0;
> +
> +	return len;
>  }
>  
>  static void glink_smem_rx_peek(struct qcom_glink_pipe *np,
> @@ -103,6 +108,9 @@ static void glink_smem_rx_peek(struct qcom_glink_pipe *np,
>  	if (tail >= pipe->native.length)
>  		tail -= pipe->native.length;
>  
> +	if (WARN_ON_ONCE(tail >= pipe->native.length))
> +		return;
> +
>  	len = min_t(size_t, count, pipe->native.length - tail);
>  	if (len)
>  		memcpy_fromio(data, pipe->fifo + tail, len);
> @@ -141,6 +149,9 @@ static size_t glink_smem_tx_avail(struct qcom_glink_pipe *np)
>  	else
>  		avail = tail - head;
>  
> +	if (WARN_ON_ONCE(avail > pipe->native.length))
> +		avail = 0;
> +
>  	if (avail < (FIFO_FULL_RESERVE + TX_BLOCKED_CMD_RESERVE))
>  		avail = 0;
>  	else
> @@ -155,6 +166,9 @@ static unsigned int glink_smem_tx_write_one(struct glink_smem_pipe *pipe,
>  {
>  	size_t len;
>  
> +	if (WARN_ON_ONCE(head >= pipe->native.length))
> +		return head;
> +
>  	len = min_t(size_t, count, pipe->native.length - head);
>  	if (len)
>  		memcpy(pipe->fifo + head, data, len);
> 
> -- 
> 2.34.1
> 

-- 
With best wishes
Dmitry

[PATCH 3/3] rpmsg: glink: smem: Use modulo for FIFO tail wrap-around in rx_advance

[Chunkai Deng]

glink_smem_rx_advance() wraps the tail index with a single subtraction,
which only corrects for one full wrap. The advance count is derived from
remote-supplied packet fields (up to sizeof(glink_msg) + 0xffff bytes);
if such a count reaches or exceeds pipe->native.length, the tail remains
outside [0, length) after the subtraction and the next FIFO access uses
an out-of-bounds offset.

Use modulo so the tail is always normalised back into [0, length),
keeping it consistent with the index bounds enforced by the WARN_ON_ONCE
checks added to the FIFO helpers.

Signed-off-by: Chunkai Deng <chunkai.deng@oss.qualcomm.com>
---
 drivers/rpmsg/qcom_glink_smem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/rpmsg/qcom_glink_smem.c b/drivers/rpmsg/qcom_glink_smem.c
index 42ad315d7910..4f143921b719 100644
--- a/drivers/rpmsg/qcom_glink_smem.c
+++ b/drivers/rpmsg/qcom_glink_smem.c
@@ -129,7 +129,7 @@ static void glink_smem_rx_advance(struct qcom_glink_pipe *np,
 
 	tail += count;
 	if (tail >= pipe->native.length)
-		tail -= pipe->native.length;
+		tail %= pipe->native.length;
 
 	*pipe->tail = cpu_to_le32(tail);
 }

-- 
2.34.1

Re: [PATCH 3/3] rpmsg: glink: smem: Use modulo for FIFO tail wrap-around in rx_advance

[Dmitry Baryshkov]

On Wed, Jun 03, 2026 at 06:14:30PM +0800, Chunkai Deng wrote:
> glink_smem_rx_advance() wraps the tail index with a single subtraction,
> which only corrects for one full wrap. The advance count is derived from
> remote-supplied packet fields (up to sizeof(glink_msg) + 0xffff bytes);
> if such a count reaches or exceeds pipe->native.length, the tail remains

Would not such a packet already cause issues as it will overflow the
FIFO?

> outside [0, length) after the subtraction and the next FIFO access uses
> an out-of-bounds offset.
> 
> Use modulo so the tail is always normalised back into [0, length),
> keeping it consistent with the index bounds enforced by the WARN_ON_ONCE
> checks added to the FIFO helpers.
> 
> Signed-off-by: Chunkai Deng <chunkai.deng@oss.qualcomm.com>
> ---
>  drivers/rpmsg/qcom_glink_smem.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/rpmsg/qcom_glink_smem.c b/drivers/rpmsg/qcom_glink_smem.c
> index 42ad315d7910..4f143921b719 100644
> --- a/drivers/rpmsg/qcom_glink_smem.c
> +++ b/drivers/rpmsg/qcom_glink_smem.c
> @@ -129,7 +129,7 @@ static void glink_smem_rx_advance(struct qcom_glink_pipe *np,
>  
>  	tail += count;
>  	if (tail >= pipe->native.length)
> -		tail -= pipe->native.length;
> +		tail %= pipe->native.length;
>  
>  	*pipe->tail = cpu_to_le32(tail);
>  }
> 
> -- 
> 2.34.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH 3/3] rpmsg: glink: smem: Use modulo for FIFO tail wrap-around in rx_advance

[Chunkai Deng]

On 6/3/2026 8:01 PM, Dmitry Baryshkov wrote:

> On Wed, Jun 03, 2026 at 06:14:30PM +0800, Chunkai Deng wrote:
>> glink_smem_rx_advance() wraps the tail index with a single subtraction,
>> which only corrects for one full wrap. The advance count is derived from
>> remote-supplied packet fields (up to sizeof(glink_msg) + 0xffff bytes);
>> if such a count reaches or exceeds pipe->native.length, the tail remains
> Would not such a packet already cause issues as it will overflow the
> FIFO?

Yes -- looking at qcom_glink_rx_data() again, the existing avail check
("avail < sizeof(hdr) + chunk_size") makes the scenario I described in
the commit message effectively unreachable in practice: if a remote
ever supplied a chunk_size such that the advance count would exceed
pipe->native.length, that check would never pass and we would return
-EAGAIN indefinitely without ever reaching rx_advance.  So the
out-of-bounds offset I described would only arise from a malformed
remote, and at that point modulo does not make anything more
trustworthy.

As mentioned in my reply to Bjorn, I’ll split this patch out and
discuss its optimization separately.

Thanks,
Chunkai

>> outside [0, length) after the subtraction and the next FIFO access uses
>> an out-of-bounds offset.
>>
>> Use modulo so the tail is always normalised back into [0, length),
>> keeping it consistent with the index bounds enforced by the WARN_ON_ONCE
>> checks added to the FIFO helpers.
>>
>> Signed-off-by: Chunkai Deng <chunkai.deng@oss.qualcomm.com>
>> ---
>>  drivers/rpmsg/qcom_glink_smem.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/rpmsg/qcom_glink_smem.c b/drivers/rpmsg/qcom_glink_smem.c
>> index 42ad315d7910..4f143921b719 100644
>> --- a/drivers/rpmsg/qcom_glink_smem.c
>> +++ b/drivers/rpmsg/qcom_glink_smem.c
>> @@ -129,7 +129,7 @@ static void glink_smem_rx_advance(struct qcom_glink_pipe *np,
>>  
>>  	tail += count;
>>  	if (tail >= pipe->native.length)
>> -		tail -= pipe->native.length;
>> +		tail %= pipe->native.length;
>>  
>>  	*pipe->tail = cpu_to_le32(tail);
>>  }
>>
>> -- 
>> 2.34.1
>>

Re: [PATCH 3/3] rpmsg: glink: smem: Use modulo for FIFO tail wrap-around in rx_advance

[Bjorn Andersson]

On Wed, Jun 03, 2026 at 06:14:30PM +0800, Chunkai Deng wrote:
> glink_smem_rx_advance() wraps the tail index with a single subtraction,
> which only corrects for one full wrap. The advance count is derived from
> remote-supplied packet fields (up to sizeof(glink_msg) + 0xffff bytes);
> if such a count reaches or exceeds pipe->native.length, the tail remains
> outside [0, length) after the subtraction and the next FIFO access uses
> an out-of-bounds offset.
> 
> Use modulo so the tail is always normalised back into [0, length),
> keeping it consistent with the index bounds enforced by the WARN_ON_ONCE
> checks added to the FIFO helpers.
> 
> Signed-off-by: Chunkai Deng <chunkai.deng@oss.qualcomm.com>
> ---
>  drivers/rpmsg/qcom_glink_smem.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/rpmsg/qcom_glink_smem.c b/drivers/rpmsg/qcom_glink_smem.c
> index 42ad315d7910..4f143921b719 100644
> --- a/drivers/rpmsg/qcom_glink_smem.c
> +++ b/drivers/rpmsg/qcom_glink_smem.c
> @@ -129,7 +129,7 @@ static void glink_smem_rx_advance(struct qcom_glink_pipe *np,
>  
>  	tail += count;
>  	if (tail >= pipe->native.length)
> -		tail -= pipe->native.length;
> +		tail %= pipe->native.length;

It seems unlikely that the "tail" will point to the start of a valid
header after this. How can we make sure this is more robust?

Regards,
Bjorn

>  
>  	*pipe->tail = cpu_to_le32(tail);
>  }
> 
> -- 
> 2.34.1
> 

Re: [PATCH 3/3] rpmsg: glink: smem: Use modulo for FIFO tail wrap-around in rx_advance

[Chunkai Deng]

On 6/15/2026 6:54 AM, Bjorn Andersson wrote:

> On Wed, Jun 03, 2026 at 06:14:30PM +0800, Chunkai Deng wrote:
>> glink_smem_rx_advance() wraps the tail index with a single subtraction,
>> which only corrects for one full wrap. The advance count is derived from
>> remote-supplied packet fields (up to sizeof(glink_msg) + 0xffff bytes);
>> if such a count reaches or exceeds pipe->native.length, the tail remains
>> outside [0, length) after the subtraction and the next FIFO access uses
>> an out-of-bounds offset.
>>
>> Use modulo so the tail is always normalised back into [0, length),
>> keeping it consistent with the index bounds enforced by the WARN_ON_ONCE
>> checks added to the FIFO helpers.
>>
>> Signed-off-by: Chunkai Deng <chunkai.deng@oss.qualcomm.com>
>> ---
>>  drivers/rpmsg/qcom_glink_smem.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/rpmsg/qcom_glink_smem.c b/drivers/rpmsg/qcom_glink_smem.c
>> index 42ad315d7910..4f143921b719 100644
>> --- a/drivers/rpmsg/qcom_glink_smem.c
>> +++ b/drivers/rpmsg/qcom_glink_smem.c
>> @@ -129,7 +129,7 @@ static void glink_smem_rx_advance(struct qcom_glink_pipe *np,
>>  
>>  	tail += count;
>>  	if (tail >= pipe->native.length)
>> -		tail -= pipe->native.length;
>> +		tail %= pipe->native.length;
> It seems unlikely that the "tail" will point to the start of a valid
> header after this. How can we make sure this is more robust?
>
> Regards,
> Bjorn

Agreed -- modulo only normalises the offset arithmetically; the
protocol state is already desynchronised at that point and the tail
won't land on a valid header boundary. The patch hides the symptom
(OOB access) without addressing the underlying issue (a bogus
remote-supplied size).

I'll drop this patch in v2 and keep just the WARN_ON_ONCE patch as a
defensive backstop.

If a proper hardening of the receive path is wanted (e.g. validating
chunk_size against rx_pipe->length on header peek, mirroring the
existing "tlen >= tx_pipe->length" check on the TX side), I'll send
it as a separate patch so it can be discussed on its own.

Thanks,
Chunkai

>>  
>>  	*pipe->tail = cpu_to_le32(tail);
>>  }
>>
>> -- 
>> 2.34.1
>>