Re: [PATCH v7 18/19] dmaengine: ti: k3-udma-v2: Update glue layer to support PKTDMA V2

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Improper teardown and disable sequences for PKTDMA V2 hardware, causing state corruption and timeouts.
- [High] Incomplete and contradictory configuration of PKTDMA V2 channels and flows when TISCI is absent, leading to memory corruption and missing settings.
- [High] The driver fails to poll for the completion of the channel autopairing sequence for PKTDMA V2, returning before the hardware is ready.
--

--- Patch [18]: [PATCH v7 18/19] dmaengine: ti: k3-udma-v2: Update glue layer to support PKTDMA V2 ---
commit e70a50b02c5e24b0a06466a8296de08ac78f41b1
Author: Sai Sree Kartheek Adivi <s-adivi@ti.com>

    dmaengine: ti: k3-udma-v2: Update glue layer to support PKTDMA V2
    
    Update glue layer to support PKTDMA V2 for non DMAengine users.

> diff --git a/drivers/dma/ti/k3-udma-glue.c b/drivers/dma/ti/k3-udma-glue.c
> index bec4deb480e60..07952c4efee2d 100644
> --- a/drivers/dma/ti/k3-udma-glue.c
> +++ b/drivers/dma/ti/k3-udma-glue.c
> @@ -267,6 +267,9 @@ static int k3_udma_glue_cfg_tx_chn(struct k3_udma_glue_tx_channel *tx_chn)
>  	const struct udma_tisci_rm *tisci_rm = tx_chn->common.tisci_rm;
>  	struct ti_sci_msg_rm_udmap_tx_ch_cfg req;
>  
> +	if (!tisci_rm->tisci)
> +		return 0;
> +

[Severity: High]
Does returning early here skip all TX channel configuration for PKTDMA V2?

This skips configuring the fetch size, pause on error, and other
essential parameters for the channel.

>  	memset(&req, 0, sizeof(req));
>  
[ ... ]
> @@ -525,21 +528,26 @@ int k3_udma_glue_enable_tx_chn(struct k3_udma_glue_tx_channel *tx_chn)
>  {
>  	int ret;
>  
> -	ret = xudma_navss_psil_pair(tx_chn->common.udmax,
> -				    tx_chn->common.src_thread,
> -				    tx_chn->common.dst_thread);
> -	if (ret) {
> -		dev_err(tx_chn->common.dev, "PSI-L request err %d\n", ret);
> -		return ret;
> -	}
> +	if (tx_chn->common.udmax->match_data->version == K3_UDMA_V1) {
> +		ret = xudma_navss_psil_pair(tx_chn->common.udmax,
> +					    tx_chn->common.src_thread,
> +					    tx_chn->common.dst_thread);
> +		if (ret) {
> +			dev_err(tx_chn->common.dev, "PSI-L request err %d\n", ret);
> +			return ret;
> +		}
>  
> -	tx_chn->psil_paired = true;
> +		tx_chn->psil_paired = true;
>  
> -	xudma_tchanrt_write(tx_chn->udma_tchanx, UDMA_CHAN_RT_PEER_RT_EN_REG,
> -			    UDMA_PEER_RT_EN_ENABLE);
> +		xudma_tchanrt_write(tx_chn->udma_tchanx, UDMA_CHAN_RT_PEER_RT_EN_REG,
> +				    UDMA_PEER_RT_EN_ENABLE);
>  
> -	xudma_tchanrt_write(tx_chn->udma_tchanx, UDMA_CHAN_RT_CTL_REG,
> -			    UDMA_CHAN_RT_CTL_EN);
> +		xudma_tchanrt_write(tx_chn->udma_tchanx, UDMA_CHAN_RT_CTL_REG,
> +				    UDMA_CHAN_RT_CTL_EN);
> +	} else {
> +		xudma_tchanrt_write(tx_chn->udma_tchanx, UDMA_CHAN_RT_CTL_REG,
> +				    UDMA_CHAN_RT_CTL_AUTOPAIR | UDMA_CHAN_RT_CTL_EN);
> +	}

[Severity: High]
Does this need to poll for the completion of the channel autopairing
sequence?

By returning immediately after setting UDMA_CHAN_RT_CTL_AUTOPAIR, the caller
might begin submitting descriptors before the hardware is fully initialized
or paired. The core k3-udma-v2.c dmaengine driver polls for
UDMA_CHAN_RT_CTL_PAIR_COMPLETE after setting this bit.

[Severity: High]
Since this adds V2-specific setup using UDMA_CHAN_RT_CTL_AUTOPAIR, do we
also need to update k3_udma_glue_tdown_tx_chn() and 
k3_udma_glue_disable_tx_chn() for V2 hardware?

Currently k3_udma_glue_tdown_tx_chn() unconditionally executes a direct
register write to UDMA_CHAN_RT_CTL_REG with just EN and TDOWN bits, which
will clobber the UDMA_CHAN_RT_CTL_AUTOPAIR bit set here. This abruptly
unpairs the channel while attempting to tear it down.

Additionally, k3_udma_glue_disable_tx_chn() unconditionally writes 0 to
UDMA_CHAN_RT_PEER_RT_EN_REG without checking the hardware version.

>  
>  	k3_udma_glue_dump_tx_rt_chn(tx_chn, "txchn en");
>  	return 0;
[ ... ]
> @@ -725,11 +732,18 @@ static int k3_udma_glue_cfg_rx_chn(struct k3_udma_glue_rx_channel *rx_chn)
>  	req.rx_chan_type = TI_SCI_RM_UDMAP_CHAN_TYPE_PKT_PBRR;
>  	req.rx_atype = rx_chn->common.atype_asel;
>  
> +	if (!tisci_rm->tisci) {
> +		// TODO: look at the chan settings
> +		xudma_rchanrt_write(rx_chn->udma_rchanx, UDMA_CHAN_RT_CFG_REG,
> +				    UDMA_CHAN_CFG_FILT_EINFO | UDMA_CHAN_CFG_FILT_PSWORDS);
> +		return 0;
> +	}
> +

[Severity: High]
Does hardcoding UDMA_CHAN_CFG_FILT_EINFO and UDMA_CHAN_CFG_FILT_PSWORDS
miss the rest of the actual channel configuration, like the fetch size?

>  	req.nav_id = tisci_rm->tisci_dev_id;
>  	ret = tisci_rm->tisci_udmap_ops->rx_ch_cfg(tisci_rm->tisci, &req);
>  	if (ret)
[ ... ]
> @@ -826,6 +843,14 @@ static int k3_udma_glue_cfg_rx_flow(struct k3_udma_glue_rx_channel *rx_chn,
>  		rx_ringfdq_id = k3_ringacc_get_ring_id(flow->ringrxfdq);
>  	}
>  
> +	if (!tisci_rm->tisci) {
> +		xudma_rflowrt_write(flow->udma_rflow, UDMA_RX_FLOWRT_RFA,
> +				    UDMA_FLOWRT_RFA_RX_EINFO_PRESENT |
> +				    UDMA_FLOWRT_RFA_RX_PSINFO_PRESENT);
> +		rx_chn->flows_ready++;
> +		return 0;
> +	}
> +

[Severity: High]
Will forcing these flags cause memory corruption if the software
descriptors weren't configured with space for this metadata?

By unconditionally forcing UDMA_FLOWRT_RFA_RX_EINFO_PRESENT and
UDMA_FLOWRT_RFA_RX_PSINFO_PRESENT into the UDMA_RX_FLOWRT_RFA register,
the hardware will write extended protocol information into the receive
buffers. If the software did not allocate space for them (checked via
rx_chn->common.epib and rx_chn->common.psdata_size in the TISCI path),
could this overwrite adjacent packet data or exceed descriptor boundaries?

>  	memset(&req, 0, sizeof(req));
>  
>  	req.valid_params =
[ ... ]
> @@ -1406,21 +1437,26 @@ int k3_udma_glue_enable_rx_chn(struct k3_udma_glue_rx_channel *rx_chn)
>  	if (rx_chn->flows_ready < rx_chn->flow_num)
>  		return -EINVAL;
>  
> -	ret = xudma_navss_psil_pair(rx_chn->common.udmax,
> -				    rx_chn->common.src_thread,
> -				    rx_chn->common.dst_thread);
> -	if (ret) {
> -		dev_err(rx_chn->common.dev, "PSI-L request err %d\n", ret);
> -		return ret;
> -	}
> +	if (rx_chn->common.udmax->match_data->version == K3_UDMA_V1) {
> +		ret = xudma_navss_psil_pair(rx_chn->common.udmax,
> +					    rx_chn->common.src_thread,
> +					    rx_chn->common.dst_thread);
> +		if (ret) {
> +			dev_err(rx_chn->common.dev, "PSI-L request err %d\n", ret);
> +			return ret;
> +		}
>  
> -	rx_chn->psil_paired = true;
> +		rx_chn->psil_paired = true;
>  
> -	xudma_rchanrt_write(rx_chn->udma_rchanx, UDMA_CHAN_RT_CTL_REG,
> -			    UDMA_CHAN_RT_CTL_EN);
> +		xudma_rchanrt_write(rx_chn->udma_rchanx, UDMA_CHAN_RT_CTL_REG,
> +				    UDMA_CHAN_RT_CTL_EN);
>  
> -	xudma_rchanrt_write(rx_chn->udma_rchanx, UDMA_CHAN_RT_PEER_RT_EN_REG,
> -			    UDMA_PEER_RT_EN_ENABLE);
> +		xudma_rchanrt_write(rx_chn->udma_rchanx, UDMA_CHAN_RT_PEER_RT_EN_REG,
> +				    UDMA_PEER_RT_EN_ENABLE);
> +	} else {
> +		xudma_rchanrt_write(rx_chn->udma_rchanx, UDMA_CHAN_RT_CTL_REG,
> +				    UDMA_CHAN_RT_CTL_AUTOPAIR |  UDMA_CHAN_RT_CTL_EN);
> +	}

[Severity: High]
Does this also need to poll for the completion of the channel autopairing
sequence, similar to the TX side?

[Severity: High]
Since V2 channel enable logic is added here, do k3_udma_glue_tdown_rx_chn()
and k3_udma_glue_disable_rx_chn() also need updates for V2 hardware?

k3_udma_glue_tdown_rx_chn() currently writes the teardown trigger to
UDMA_CHAN_RT_PEER_RT_EN_REG, which V2 hardware ignores for teardown. The
UDMA_CHAN_RT_CTL_EN bit would never clear, leading to a permanent failure
in the polling loop (RX tdown timeout).

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260724102115.2854-1-s-adivi@ti.com?part=18