[PATCH] dmaengine: qcom: gpi: Fix SE ID and SE Protocol fields widths in SCRATCH_0

[PATCH] dmaengine: qcom: gpi: Fix SE ID and SE Protocol fields widths in SCRATCH_0

[Chandana Kishori Chiluveru]

GPII_n_CH_k_SCRATCH_0_SEID is currently defined as GENMASK(2, 0), only
3 bits wide. Serial engine indices >= 8 get silently truncated by
FIELD_PREP()/u32_encode_bits() when written into the scratch register,
causing the GPI channel to bind to the wrong serial engine. The
channel allocate command still succeeds (it doesn't require the
target SE to respond), but the subsequent channel start command times
out waiting for hardware to complete the handoff to the (incorrectly
bound) SE, and gpi_send_cmd() returns -EIO leading to GSI usecase failures
for SE8 and SE9 serial engines.

Older documentation, such as the SDM845 hardware programming guide,
describes the SE ID field as bits [2:0]. More recent QUP hardware
documentation defines the field as bits [3:0]. Use the wider definition
to match current hardware documentation while remaining compatible with
existing platforms and enabling serial engine IDs greater than 7.

Also update the SE Protocol field definition to match the documented
register layout and keep both fields correctly packed.

Fixes: 5d0c3533a19f ("dmaengine: qcom: Add GPI dma driver")
Signed-off-by: Chandana Kishori Chiluveru <chandana.chiluveru@oss.qualcomm.com>
---
 drivers/dma/qcom/gpi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/qcom/gpi.c b/drivers/dma/qcom/gpi.c
index a5055a6273af..8f09eaf4e714 100644
--- a/drivers/dma/qcom/gpi.c
+++ b/drivers/dma/qcom/gpi.c
@@ -207,8 +207,8 @@ enum CNTXT_OFFS {
 
 /* Scratch registers */
 #define GPII_n_CH_k_SCRATCH_0_OFFS(n, k)	(0x20060 + (0x4000 * (n)) + (0x80 * (k)))
-#define GPII_n_CH_k_SCRATCH_0_SEID		GENMASK(2, 0)
-#define GPII_n_CH_k_SCRATCH_0_PROTO		GENMASK(7, 4)
+#define GPII_n_CH_k_SCRATCH_0_SEID		GENMASK(3, 0)
+#define GPII_n_CH_k_SCRATCH_0_PROTO		GENMASK(6, 4)
 #define GPII_n_CH_k_SCRATCH_0_PAIR		GENMASK(20, 16)
 #define GPII_n_CH_k_SCRATCH_0(pair, proto, seid)		\
 			     (FIELD_PREP(GPII_n_CH_k_SCRATCH_0_PAIR, pair)	| \

---
base-commit: 34cf6dafc47441dfb6b356a095b89c3585a93714
change-id: 20260710-fix_se_id_and_se_protocol_fields-d7f0fe616596

Best regards,
--  
Chandana Kishori Chiluveru <chandana.chiluveru@oss.qualcomm.com>

Re: [PATCH] dmaengine: qcom: gpi: Fix SE ID and SE Protocol fields widths in SCRATCH_0

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

Pre-existing issues:
- [High] Error cleanup paths in gpi_ch_init() incorrectly reset the wrong channel.
- [High] Missing dma_wmb() before ringing the hardware doorbell in `gpi_issue_pending()`, which may cause the DMA engine to read stale descriptor memory.
- [High] Missing dma_rmb() in `gpi_process_events()` after observing the hardware read pointer, risking speculative reads of stale event data.
- [High] Flawed virt-dma descriptor submission logic in `gpi_issue_pending()` causes descriptors to be silently skipped.
--

--- Patch [1]: [PATCH] dmaengine: qcom: gpi: Fix SE ID and SE Protocol fields widths in SCRATCH_0 ---
commit cf1da458f619102482edf1e06ebdff45f28db2b5
Author: Chandana Kishori Chiluveru <chandana.chiluveru@oss.qualcomm.com>

dmaengine: qcom: gpi: Fix SE ID and SE Protocol fields widths in SCRATCH_0

This commit corrects the widths of the SE ID and SE Protocol fields in the
SCRATCH_0 register definition for the GPI DMA driver. It updates the SE ID
field to 4 bits to match newer QUP hardware documentation and avoid silently
truncating serial engine indices. The SE Protocol field is also updated to
keep both fields correctly packed.

> diff --git a/drivers/dma/qcom/gpi.c b/drivers/dma/qcom/gpi.c
> index a5055a6273af6..8f09eaf4e714c 100644
> --- a/drivers/dma/qcom/gpi.c
> +++ b/drivers/dma/qcom/gpi.c

[ ... ]

While reviewing this patch, I noticed a few pre-existing issues in
drivers/dma/qcom/gpi.c:

drivers/dma/qcom/gpi.c:gpi_ch_init() {
    ...
	error_start_chan:
		for (i = i - 1; i >= 0; i--) {
			gpi_stop_chan(&gpii->gchan[i]);
			gpi_send_cmd(gpii, gchan, GPI_CH_CMD_RESET);
		}
		i = 2;
	error_alloc_chan:
		for (i = i - 1; i >= 0; i--)
			gpi_reset_chan(gchan, GPI_CH_CMD_DE_ALLOC);
    ...
}

[Severity: High]
This is a pre-existing issue, but does this cleanup path reset the correct
channel?

If gpi_start_chan() fails during initialization, the error handler iterates
backward to stop and reset the previously initialized channels. However,
instead of passing the iterated channel &gpii->gchan[i] to gpi_send_cmd()
and gpi_reset_chan(), it passes the function argument gchan.

Could this leave the actual channel that needs to be rolled back active
and partially allocated, while repeatedly resetting the argument channel?
This might lead to channel state corruption and resource leaks.

drivers/dma/qcom/gpi.c:gpi_issue_pending() {
    ...
	gpi_desc = to_gpi_desc(vd);
	for (i = 0; i < gpi_desc->num_tre; i++) {
		tre = &gpi_desc->tre[i];
		gpi_queue_xfer(gpii, gchan, tre, &wp);
	}

	gpi_desc->db = ch_ring->wp;
	gpi_write_ch_db(gchan, &gchan->ch_ring, gpi_desc->db);
    ...
}

[Severity: High]
This isn't a bug introduced by this patch, but is a memory barrier required
before ringing the hardware doorbell?

gpi_issue_pending() writes Transfer Ring Elements to DMA ring buffer
memory via gpi_queue_xfer(), which uses memcpy(). It then immediately rings
the hardware doorbell via gpi_write_ch_db(), which maps to writel_relaxed().

Because writel_relaxed() does not guarantee memory write visibility before
the I/O write, and there is no memory barrier between the descriptor writes
and the doorbell write, is it possible for the DMA hardware to observe the
doorbell write and fetch the elements before the CPU's memory writes have
reached RAM?

Could this result in the DMA processing stale or zeroed memory?

drivers/dma/qcom/gpi.c:gpi_process_events() {
    ...
	cntxt_rp = gpi_read_reg(gpii, gpii->ev_ring_rp_lsb_reg);
	rp = to_virtual(ev_ring, cntxt_rp);

	do {
		while (rp != ev_ring->rp) {
			gpi_event = ev_ring->rp;
    ...
}

[Severity: High]
This is a pre-existing issue, but is a read memory barrier needed after
reading the hardware pointer?

gpi_process_events() reads the hardware event ring read pointer using
gpi_read_reg(), which uses readl_relaxed(). It then immediately enters a
loop that dereferences memory from the event ring.

Because readl_relaxed() does not contain an implicit memory barrier, is the
CPU permitted to speculatively prefetch the event ring memory before the
read of the hardware pointer register completes?

Could this result in the driver processing stale event data that hasn't yet
been updated by the DMA hardware?

drivers/dma/qcom/gpi.c:gpi_issue_pending() {
    ...
	/* move all submitted descriptors to issued list */
	spin_lock_irqsave(&gchan->vc.lock, flags);
	if (vchan_issue_pending(&gchan->vc))
		vd = list_last_entry(&gchan->vc.desc_issued,
				     struct virt_dma_desc, node);
	spin_unlock_irqrestore(&gchan->vc.lock, flags);
    ...
}

[Severity: High]
This isn't a bug introduced by this patch, but does this logic successfully
handle multiple submitted descriptors?

vchan_issue_pending() moves all newly submitted descriptors from the
desc_submitted to the desc_issued list. gpi_issue_pending() then retrieves
only a single descriptor using list_last_entry(), and queues only this last
descriptor's elements to the hardware ring.

If a client submits multiple descriptors before issue_pending() is called,
would the earlier descriptors be silently ignored? Furthermore, when the last
descriptor completes, gpi_process_xfer_compl_event() uses vchan_next_desc()
which retrieves the first descriptor.

Could this mismatch lead to incorrect DMA completion notifications?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260720-fix_se_id_and_se_protocol_fields-v1-1-e41dbb107d97@oss.qualcomm.com?part=1

Re: [PATCH] dmaengine: qcom: gpi: Fix SE ID and SE Protocol fields widths in SCRATCH_0

[Konrad Dybcio]

On 7/20/26 5:27 PM, Chandana Kishori Chiluveru wrote:
> GPII_n_CH_k_SCRATCH_0_SEID is currently defined as GENMASK(2, 0), only
> 3 bits wide. Serial engine indices >= 8 get silently truncated by
> FIELD_PREP()/u32_encode_bits() when written into the scratch register,
> causing the GPI channel to bind to the wrong serial engine. The
> channel allocate command still succeeds (it doesn't require the
> target SE to respond), but the subsequent channel start command times
> out waiting for hardware to complete the handoff to the (incorrectly
> bound) SE, and gpi_send_cmd() returns -EIO leading to GSI usecase failures
> for SE8 and SE9 serial engines.
> 
> Older documentation, such as the SDM845 hardware programming guide,
> describes the SE ID field as bits [2:0]. More recent QUP hardware
> documentation defines the field as bits [3:0]. Use the wider definition
> to match current hardware documentation while remaining compatible with
> existing platforms and enabling serial engine IDs greater than 7.
> 
> Also update the SE Protocol field definition to match the documented
> register layout and keep both fields correctly packed.
> 
> Fixes: 5d0c3533a19f ("dmaengine: qcom: Add GPI dma driver")
> Signed-off-by: Chandana Kishori Chiluveru <chandana.chiluveru@oss.qualcomm.com>
> ---
>  drivers/dma/qcom/gpi.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/dma/qcom/gpi.c b/drivers/dma/qcom/gpi.c
> index a5055a6273af..8f09eaf4e714 100644
> --- a/drivers/dma/qcom/gpi.c
> +++ b/drivers/dma/qcom/gpi.c
> @@ -207,8 +207,8 @@ enum CNTXT_OFFS {
>  
>  /* Scratch registers */
>  #define GPII_n_CH_k_SCRATCH_0_OFFS(n, k)	(0x20060 + (0x4000 * (n)) + (0x80 * (k)))
> -#define GPII_n_CH_k_SCRATCH_0_SEID		GENMASK(2, 0)
> -#define GPII_n_CH_k_SCRATCH_0_PROTO		GENMASK(7, 4)
> +#define GPII_n_CH_k_SCRATCH_0_SEID		GENMASK(3, 0)
> +#define GPII_n_CH_k_SCRATCH_0_PROTO		GENMASK(6, 4)

There exist protocols, such as QSPI with id=9, so this surely can't
be right (unless there's another bitfield for that?)

Konrad

Re: [PATCH] dmaengine: qcom: gpi: Fix SE ID and SE Protocol fields widths in SCRATCH_0

[Chandana Kishori Chiluveru]

HI Konrad

On 7/20/26 08:47, Konrad Dybcio wrote:
> On 7/20/26 5:27 PM, Chandana Kishori Chiluveru wrote:
>> GPII_n_CH_k_SCRATCH_0_SEID is currently defined as GENMASK(2, 0), only
>> 3 bits wide. Serial engine indices >= 8 get silently truncated by
>> FIELD_PREP()/u32_encode_bits() when written into the scratch register,
>> causing the GPI channel to bind to the wrong serial engine. The
>> channel allocate command still succeeds (it doesn't require the
>> target SE to respond), but the subsequent channel start command times
>> out waiting for hardware to complete the handoff to the (incorrectly
>> bound) SE, and gpi_send_cmd() returns -EIO leading to GSI usecase failures
>> for SE8 and SE9 serial engines.
>>
>> Older documentation, such as the SDM845 hardware programming guide,
>> describes the SE ID field as bits [2:0]. More recent QUP hardware
>> documentation defines the field as bits [3:0]. Use the wider definition
>> to match current hardware documentation while remaining compatible with
>> existing platforms and enabling serial engine IDs greater than 7.
>>
>> Also update the SE Protocol field definition to match the documented
>> register layout and keep both fields correctly packed.
>>
>> Fixes: 5d0c3533a19f ("dmaengine: qcom: Add GPI dma driver")
>> Signed-off-by: Chandana Kishori Chiluveru <chandana.chiluveru@oss.qualcomm.com>
>> ---
>>   drivers/dma/qcom/gpi.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/dma/qcom/gpi.c b/drivers/dma/qcom/gpi.c
>> index a5055a6273af..8f09eaf4e714 100644
>> --- a/drivers/dma/qcom/gpi.c
>> +++ b/drivers/dma/qcom/gpi.c
>> @@ -207,8 +207,8 @@ enum CNTXT_OFFS {
>>   
>>   /* Scratch registers */
>>   #define GPII_n_CH_k_SCRATCH_0_OFFS(n, k)	(0x20060 + (0x4000 * (n)) + (0x80 * (k)))
>> -#define GPII_n_CH_k_SCRATCH_0_SEID		GENMASK(2, 0)
>> -#define GPII_n_CH_k_SCRATCH_0_PROTO		GENMASK(7, 4)
>> +#define GPII_n_CH_k_SCRATCH_0_SEID		GENMASK(3, 0)
>> +#define GPII_n_CH_k_SCRATCH_0_PROTO		GENMASK(6, 4)
> There exist protocols, such as QSPI with id=9, so this surely can't

Its my bad, The PROTO field should be [8:4]. I will correct this in the 
next patch

> be right (unless there's another bitfield for that?)
>
> Konrad
>