Re: [dpdk-dev] [PATCH v4 02/11] dma/ioat: create dmadev instances on PCI probe

[Bruce Richardson <bruce.richardson@intel.com>]

On Fri, Sep 17, 2021 at 03:42:18PM +0000, Conor Walsh wrote:
> When a suitable device is found during the PCI probe, create a dmadev
> instance for each channel. Internal structures and HW definitions required
> for device creation are also included.
> 
> Signed-off-by: Conor Walsh <conor.walsh@intel.com>
> Reviewed-by: Kevin Laatz <kevin.laatz@intel.com>
> ---
>  drivers/dma/ioat/ioat_dmadev.c   | 119 ++++++++++++++++++++++++++++++-
>  drivers/dma/ioat/ioat_hw_defs.h  |  45 ++++++++++++
>  drivers/dma/ioat/ioat_internal.h |  24 +++++++
>  3 files changed, 186 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/dma/ioat/ioat_dmadev.c b/drivers/dma/ioat/ioat_dmadev.c
> index f3491d45b1..b815d30bcf 100644
> --- a/drivers/dma/ioat/ioat_dmadev.c
> +++ b/drivers/dma/ioat/ioat_dmadev.c
> @@ -4,6 +4,7 @@
>  

<snip>

> +/* Destroy a DMA device. */
> +static int
> +ioat_dmadev_destroy(const char *name)
> +{
> +	struct rte_dma_dev *dev;
> +	struct ioat_dmadev *ioat;
> +	int ret;
> +
> +	if (!name) {
> +		IOAT_PMD_ERR("Invalid device name");
> +		return -EINVAL;
> +	}
> +
> +	dev = &rte_dma_devices[rte_dma_get_dev_id(name)];
> +	if (!dev) {
> +		IOAT_PMD_ERR("Invalid device name (%s)", name);
> +		return -EINVAL;
> +	}
> +

I think you need to independently check the return value from
rte_dma_get_dev_id, rather than assuming when it returns an error value the
resultant index location will hold a null pointer.

> +	ioat = dev->dev_private;
> +	if (!ioat) {
> +		IOAT_PMD_ERR("Error getting dev_private");
> +		return -EINVAL;
> +	}
> +
> +	dev->dev_private = NULL;
> +	rte_free(ioat->desc_ring);
> +
> +	ret = rte_dma_pmd_release(name);

The rte_dma_pmd_allocate function reserves memory for the private data, so
the release function should free that memory too. However, you have
assigned private_data to NULL just above, so that probably won't work.

> +	if (ret)
> +		IOAT_PMD_DEBUG("Device cleanup failed");
> +
> +	return 0;
> +}
> +

<snip>