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

[Conor Walsh]

> 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 at intel.com>
>> Reviewed-by: Kevin Laatz <kevin.laatz at 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.

I will assign rte_dma_get_dev_id(name) to a variable and check it before 
use in v5.

>
>> +	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.

I think I will actually remove the "dev->dev_private = NULL" in v5 as 
this is handled by the lib.

Thanks,

Conor.

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