|SUCCESS| [v1,2/2] dma/odm: avoid zero length DMA transfers

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/164741

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-06-01

Commit: 20260601101559.1925302-2-sthotton at marvell.com
Author: Vamsi Attunuru <vattunuru at marvell.com>
Subject: [PATCH v1 1/2] dma/odm: support dev to mem transfers

This patch adds support for device-to-memory and memory-to-device DMA
transfers by extending the existing memory-to-memory functionality.

> diff --git a/drivers/dma/odm/odm.c b/drivers/dma/odm/odm.c
> index 270808f4df..0633ce0b4b 100644
> --- a/drivers/dma/odm/odm.c
> +++ b/drivers/dma/odm/odm.c
> @@ -125,13 +125,41 @@ odm_disable(struct odm_dev *odm)
>  	return 0;
>  }
>  
> +static int
> +odm_get_ext_port_type(const struct rte_dma_vchan_conf *conf, uint8_t *ext_port)
> +{
> +	uint8_t coreid;
> +
> +	if (conf->src_port.port_type == RTE_DMA_PORT_PCIE) {
> +		coreid = conf->src_port.pcie.coreid;
> +	} else if (conf->dst_port.port_type == RTE_DMA_PORT_PCIE) {
> +		coreid = conf->dst_port.pcie.coreid;
> +	} else {
> +		*ext_port = ODM_EXT_PORT_NCB;
> +		return 0;
> +	}

Does this code allow simultaneous src and dst to be RTE_DMA_PORT_PCIE,
which would set coreid twice to potentially different values?

> +
> +	switch (coreid) {
> +	case 0:
> +		*ext_port = ODM_EXT_PORT_PEM0;
> +		return 0;
> +	case 1:
> +		*ext_port = ODM_EXT_PORT_PEM1;
> +		return 0;
> +	default:
> +		ODM_LOG(ERR, "Unsupported PCIe coreid %u (only 0 and 1 are valid)", coreid);
> +		return -EINVAL;
> +	}
> +}
> +
>  int
> -odm_vchan_setup(struct odm_dev *odm, int vchan, int nb_desc)
> +odm_vchan_setup(struct odm_dev *odm, int vchan, const struct rte_dma_vchan_conf *conf)
>  {
>  	struct odm_queue *vq = &odm->vq[vchan];
>  	int isize, csize, max_nb_desc, rc = 0;
>  	union odm_mbox_msg mbox_msg;
>  	const struct rte_memzone *mz;
> +	uint8_t ext_port;
>  	char name[32];
>  
>  	if (vq->iring_mz != NULL)
> @@ -140,10 +168,29 @@ odm_vchan_setup(struct odm_dev *odm, int vchan, int nb_desc)
>  	mbox_msg.u[0] = 0;
>  	mbox_msg.u[1] = 0;
>  
> +	switch (conf->direction) {
> +	case RTE_DMA_DIR_DEV_TO_MEM:
> +		vq->xtype = ODM_XTYPE_INBOUND;
> +		break;
> +	case RTE_DMA_DIR_MEM_TO_DEV:
> +		vq->xtype = ODM_XTYPE_OUTBOUND;
> +		break;
> +	case RTE_DMA_DIR_MEM_TO_MEM:
> +		vq->xtype = ODM_XTYPE_INTERNAL;
> +		break;
> +	default:
> +		ODM_LOG(ERR, "Unsupported DMA direction %d", conf->direction);
> +		return -EINVAL;
> +	}
> +
>  	/* ODM PF driver expects vfid starts from index 0 */
>  	mbox_msg.q.vfid = odm->vfid;
>  	mbox_msg.q.cmd = ODM_QUEUE_OPEN;
>  	mbox_msg.q.qidx = vchan;
> +	rc = odm_get_ext_port_type(conf, &ext_port);
> +	if (rc < 0)
> +		return rc;
> +	mbox_msg.q.ext_port = ext_port;

Does this code leak vq->iring_mz if odm_get_ext_port_type() returns an
error, given the check for vq->iring_mz != NULL earlier and the comment
about its cleanup in odm_vchan_destroy()?

>  	rc = send_mbox_to_pf(odm, &mbox_msg, &mbox_msg);
>  	if (rc < 0)
>  		return rc;

[ ... ]

> diff --git a/drivers/dma/odm/odm_dmadev.c b/drivers/dma/odm/odm_dmadev.c
> index a2f4ed9a8e..0211133bd4 100644
> --- a/drivers/dma/odm/odm_dmadev.c
> +++ b/drivers/dma/odm/odm_dmadev.c
> @@ -30,7 +30,8 @@ odm_dmadev_info_get(const struct rte_dma_dev *dev, struct rte_dma_info *dev_info
>  	dev_info->max_vchans = odm->max_qs;
>  	dev_info->nb_vchans = odm->num_qs;
>  	dev_info->dev_capa =
> -		(RTE_DMA_CAPA_MEM_TO_MEM | RTE_DMA_CAPA_OPS_COPY | RTE_DMA_CAPA_OPS_COPY_SG);
> +		(RTE_DMA_CAPA_MEM_TO_MEM | RTE_DMA_CAPA_OPS_COPY | RTE_DMA_CAPA_OPS_COPY_SG |
> +		 RTE_DMA_CAPA_MEM_TO_DEV | RTE_DMA_CAPA_DEV_TO_MEM);

Are there corresponding capability checks added when setting up the
device direction to validate that the requested direction matches the
advertised capabilities?

[ ... ]


Commit: 20260601101559.1925302-3-sthotton at marvell.com
Author: Shijith Thotton <sthotton at marvell.com>
Subject: [PATCH v1 2/2] dma/odm: avoid zero length DMA transfers

This patch adds validation to reject zero-length DMA operations.

> diff --git a/drivers/dma/odm/odm_dmadev.c b/drivers/dma/odm/odm_dmadev.c
> index 0211133bd4..7488b960fd 100644
> --- a/drivers/dma/odm/odm_dmadev.c
> +++ b/drivers/dma/odm/odm_dmadev.c
> @@ -110,6 +110,9 @@ odm_dmadev_copy(void *dev_private, uint16_t vchan, rte_iova_t src, rte_iova_t ds
>  	vq = &odm->vq[vchan];
>  	hdr.s.xtype = vq->xtype;
>  
> +	if (unlikely(!length))
> +		return -EINVAL;

This uses the implicit comparison forbidden by the coding standard. The
check should be written as 'if (unlikely(length == 0))'.

> +
>  	h = length;
>  	h |= ((uint64_t)length << 32);
>  
> @@ -262,14 +265,20 @@ odm_dmadev_copy_sg(void *dev_private, uint16_t vchan, const struct rte_dma_sge *
>  	pending_submit_len = vq->pending_submit_len;
>  	pending_submit_cnt = vq->pending_submit_cnt;
>  
> -	if (unlikely(nb_src > 4 || nb_dst > 4))
> +	if (unlikely(!nb_src || nb_src > 4 || !nb_dst || nb_dst > 4))
>  		return -EINVAL;

Same issue here - uses forbidden implicit comparisons. Should be
'nb_src == 0' and 'nb_dst == 0'.

>  
> -	for (i = 0; i < nb_src; i++)
> +	for (i = 0; i < nb_src; i++) {
> +		if (unlikely(!src[i].length))
> +			return -EINVAL;

Same issue - should be 'src[i].length == 0'.

>  		s_sz += src[i].length;
> +	}
>  
> -	for (i = 0; i < nb_dst; i++)
> +	for (i = 0; i < nb_dst; i++) {
> +		if (unlikely(!dst[i].length))
> +			return -EINVAL;

Same issue - should be 'dst[i].length == 0'.

>  		d_sz += dst[i].length;
> +	}
>  
>  	if (s_sz != d_sz)
>  		return -EINVAL;
> @@ -342,6 +351,9 @@ odm_dmadev_fill(void *dev_private, uint16_t vchan, uint64_t pattern, rte_iova_t
>  		.s.nlst = 1,
>  	};
>  
> +	if (unlikely(!length))
> +		return -EINVAL;

Same issue - should be 'length == 0'.

>  
>  	h = (uint64_t)length;
>