Re: [PATCH v8 2/4] dmaengine: qcom: gpi: Add lock/unlock TREs for multi-owner I2C transfers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] Setting TRE_FLAGS_IEOB on Lock/Unlock TREs causes multiple completion events per descriptor, corrupting the vchan queue.
- [High] Dangling CHAIN flag on the final DMA TRE for I2C Read transfers.
- [High] Asymmetric Lock/Unlock TRE generation causes the RX channel to permanently lock.
- [High] Insufficient DMA transfer ring space check leads to partial TRE enqueue and hardware lockup.
- [High] Missing CHAIN bit on GO TRE before Unlock TRE.
--

--- Patch [2]: [PATCH v8 2/4] dmaengine: qcom: gpi: Add lock/unlock TREs for multi-owner I2C transfers ---
commit cc47faf2611d1c6105d08f28b502e364ce9bde15
Author: Mukesh Kumar Savaliya <mukesh.savaliya@oss.qualcomm.com>
Subject: dmaengine: qcom: gpi: Add lock/unlock TREs for multi-owner I2C transfers

This commit adds support for emitting GPI hardware lock and unlock TREs to
serialize access to a shared QUP-based I2C controller. It simplifies the
client interface with a lock_action selector and increases the maximum TRE
count to accommodate the newly added elements.

> diff --git a/drivers/dma/qcom/gpi.c b/drivers/dma/qcom/gpi.c
> index a5055a6273af6..1e70d2adfdff0 100644
> --- a/drivers/dma/qcom/gpi.c
> +++ b/drivers/dma/qcom/gpi.c
> @@ -518,7 +527,7 @@ struct gpii {
>  	bool ieob_set;
>  };
>  
> -#define MAX_TRE 3
> +#define MAX_TRE 5
>  
>  struct gpi_desc {
>  	struct virt_dma_desc vd;

[Severity: High]
Since the maximum number of TREs has increased, does gpi_prep_slave_sg()
also need to be updated to account for the new elements?

It appears gpi_prep_slave_sg() still hardcodes the maximum required TRE
slots to 3:

gpi_prep_slave_sg() {
        ...
        nr_tre = 3;
        ...
        nr = gpi_ring_num_elements_avail(ch_ring);
        if (nr < nr_tre) {
        ...
}

If the ring only has 3 slots available but 5 TREs are generated, could this
cause gpi_queue_xfer() to silently drop the extra TREs and dispatch a
truncated chain to the hardware?

> [ ... ]
> @@ -1625,12 +1634,27 @@ static int gpi_create_i2c_tre(struct gchan *chan, struct gpi_desc *desc,
>  			      unsigned long flags)
>  {
>  	struct gpi_i2c_config *i2c = chan->config;
> +	enum gpi_lock_action lock_action = i2c->lock_action;
>  	struct device *dev = chan->gpii->gpi_dev->dev;
>  	unsigned int tre_idx = 0;
>  	dma_addr_t address;
>  	struct gpi_tre *tre;
>  	unsigned int i;
>  
> +	/* Optional lock TRE before transfer */
> +	if (lock_action == GPI_LOCK_ACQUIRE) {
> +		tre = &desc->tre[tre_idx];
> +		tre_idx++;
> +
> +		tre->dword[0] = 0;
> +		tre->dword[1] = 0;
> +		tre->dword[2] = 0;
> +		tre->dword[3] = u32_encode_bits(1, TRE_LOCK);
> +		tre->dword[3] |= u32_encode_bits(1, TRE_FLAGS_IEOB);

[Severity: Critical]
Could setting the Interrupt on End of Block (IEOB) flag here on the LOCK
TRE cause multiple completion events to fire for a single descriptor?

The DMA TRE also sets the IEOT flag later in this function. If both events
are generated, gpi_process_xfer_compl_event() might call vchan_next_desc()
multiple times, popping unrelated descriptors from the queue and potentially
causing premature completions or use-after-free conditions.

[Severity: High]
Will this logic cause the RX channel to acquire a lock that it never
releases?

For an I2C read transfer, lock_action == GPI_LOCK_ACQUIRE unconditionally
appends a LOCK TRE to the descriptor for both the TX and RX channels.
However, the logic later in this function explicitly suppresses the UNLOCK
TRE when i2c->op != I2C_READ. Is it possible the RX channel sequencer gets
permanently locked this way?

> +		tre->dword[3] |= u32_encode_bits(0, TRE_MINOR_TYPE);
> +		tre->dword[3] |= u32_encode_bits(3, TRE_MAJOR_TYPE);
> +	}
> +
> [ ... ]
> @@ -1690,6 +1714,24 @@ static int gpi_create_i2c_tre(struct gchan *chan, struct gpi_desc *desc,
>  
>  		if (!(flags & DMA_PREP_INTERRUPT))
>  			tre->dword[3] |= u32_encode_bits(1, TRE_FLAGS_BEI);
> +
> +		/* If multi-owner and this is the release boundary, chain it */
> +		if (i2c->lock_action == GPI_LOCK_RELEASE)
> +			tre->dword[3] |= u32_encode_bits(1, TRE_FLAGS_CHAIN);
> +	}
> +
> +	/* Optional unlock TRE after transfer */
> +	if (lock_action == GPI_LOCK_RELEASE && i2c->op != I2C_READ) {
> +		tre = &desc->tre[tre_idx];
> +		tre_idx++;

[Severity: High]
Can this sequence leave a dangling CHAIN flag on the final DMA TRE?

When preparing an I2C read transfer with GPI_LOCK_RELEASE, the DMA block
unconditionally sets the CHAIN flag on the DMA TRE. However, the UNLOCK TRE
is explicitly skipped here because of the (i2c->op != I2C_READ) condition.

Could this cause the hardware DMA sequencer to fetch an out-of-bounds or
stale TRE since it expects another descriptor to follow?

[Severity: High]
When configuring the TX channel for a multi-message read transfer, does
the GO TRE need the CHAIN bit instead of the LINK bit if an UNLOCK TRE
is going to follow it?

If multi_msg is true, earlier code in gpi_create_i2c_tre() sets
TRE_FLAGS_LINK on the GO TRE. Since the DMA TRE is skipped for TX channels
during reads, the next TRE appended will be this UNLOCK TRE. Without the
CHAIN bit on the GO TRE, might the hardware sequencer halt before processing
the UNLOCK TRE?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708051023.2872304-1-mukesh.savaliya@oss.qualcomm.com?part=2