[PATCH 0/5] Fix IDXD PCI device close

[PATCH 0/5] Fix IDXD PCI device close

[Kevin Laatz]

This patchset addresses the device close for IDXD PCI devices.
Initially, there was a memory leak reported by ASAN for the 'pci' member
of the 'idxd_dmadev' struct due to a missing free. In addition, this
patch set corrects the behaviour of the device close function to ensure
the cleanup is completed as expected.

Applications which use DMA devices should call rte_dma_close() for each
device probed in order to ensure proper cleanup of the devices. This has
been added to applications where DMA devices are commonly used.

Kevin Laatz (5):
  dma/idxd: fix memory leak in pci close
  dma/idxd: fix memory leak due to free on incorrect pointer
  app/test: close dma devices during cleanup
  app/testpmd: stop and close dmadevs at exit
  examples/dma: fix missing dma close

 app/test-pmd/testpmd.c           |  9 ++++++++
 app/test/test.c                  |  6 ++++++
 drivers/dma/idxd/idxd_common.c   |  1 +
 drivers/dma/idxd/idxd_internal.h |  2 ++
 drivers/dma/idxd/idxd_pci.c      | 36 +++++++++++++++++++++++++-------
 examples/dma/dmafwd.c            |  6 ++++++
 6 files changed, 52 insertions(+), 8 deletions(-)

-- 
2.31.1

[PATCH 1/5] dma/idxd: fix memory leak in pci close

[Kevin Laatz]

ASAN reports a memory leak for the 'pci' pointer in the 'idxd_dmadev'
struct.

This is fixed by free'ing the struct when the last queue on the PCI
device is being closed.

Fixes: 9449330a8458 ("dma/idxd: create dmadev instances on PCI probe")
Cc: stable@dpdk.org
Cc: bruce.richardson@intel.com

Reported-by: Xingguang He <xingguang.he@intel.com>
Signed-off-by: Kevin Laatz <kevin.laatz@intel.com>
---
 drivers/dma/idxd/idxd_common.c   |  1 +
 drivers/dma/idxd/idxd_internal.h |  2 ++
 drivers/dma/idxd/idxd_pci.c      | 34 +++++++++++++++++++++++++-------
 3 files changed, 30 insertions(+), 7 deletions(-)

diff --git a/drivers/dma/idxd/idxd_common.c b/drivers/dma/idxd/idxd_common.c
index ea6413cc7a..57c4c1655d 100644
--- a/drivers/dma/idxd/idxd_common.c
+++ b/drivers/dma/idxd/idxd_common.c
@@ -597,6 +597,7 @@ idxd_dmadev_create(const char *name, struct rte_device *dev,
 	dmadev->fp_obj->dev_private = idxd;
 
 	idxd->dmadev->state = RTE_DMA_DEV_READY;
+	rte_atomic16_inc(&idxd->u.pci->ref_count);
 
 	return 0;
 
diff --git a/drivers/dma/idxd/idxd_internal.h b/drivers/dma/idxd/idxd_internal.h
index 3375600217..180a8587c6 100644
--- a/drivers/dma/idxd/idxd_internal.h
+++ b/drivers/dma/idxd/idxd_internal.h
@@ -7,6 +7,7 @@
 
 #include <rte_dmadev_pmd.h>
 #include <rte_spinlock.h>
+#include <rte_atomic.h>
 
 #include "idxd_hw_defs.h"
 
@@ -33,6 +34,7 @@ struct idxd_pci_common {
 	rte_spinlock_t lk;
 
 	uint8_t wq_cfg_sz;
+	rte_atomic16_t ref_count;
 	volatile struct rte_idxd_bar0 *regs;
 	volatile uint32_t *wq_regs_base;
 	volatile struct rte_idxd_grpcfg *grp_regs;
diff --git a/drivers/dma/idxd/idxd_pci.c b/drivers/dma/idxd/idxd_pci.c
index 9ca1ec64e9..7036eb938d 100644
--- a/drivers/dma/idxd/idxd_pci.c
+++ b/drivers/dma/idxd/idxd_pci.c
@@ -6,6 +6,7 @@
 #include <rte_devargs.h>
 #include <rte_dmadev_pmd.h>
 #include <rte_malloc.h>
+#include <rte_atomic.h>
 
 #include "idxd_internal.h"
 
@@ -115,20 +116,38 @@ idxd_pci_dev_close(struct rte_dma_dev *dev)
 {
 	struct idxd_dmadev *idxd = dev->fp_obj->dev_private;
 	uint8_t err_code;
+	int is_last_wq;
 
-	/* disable the device */
-	err_code = idxd_pci_dev_command(idxd, idxd_disable_dev);
-	if (err_code) {
-		IDXD_PMD_ERR("Error disabling device: code %#x", err_code);
-		return err_code;
+	if (idxd_is_wq_enabled(idxd)) {
+		/* disable the wq */
+		err_code = idxd_pci_dev_command(idxd, idxd_disable_wq);
+		if (err_code) {
+			IDXD_PMD_ERR("Error disabling wq: code %#x", err_code);
+			return err_code;
+		}
+		IDXD_PMD_DEBUG("IDXD WQ disabled OK");
 	}
-	IDXD_PMD_DEBUG("IDXD Device disabled OK");
 
 	/* free device memory */
 	IDXD_PMD_DEBUG("Freeing device driver memory");
 	rte_free(idxd->batch_idx_ring);
 	rte_free(idxd->desc_ring);
 
+	/* if this is the last WQ on the device, disable the device and free
+	 * the PCI struct
+	 */
+	is_last_wq = rte_atomic16_dec_and_test(&idxd->u.pci->ref_count);
+	if (is_last_wq) {
+		/* disable the device */
+		err_code = idxd_pci_dev_command(idxd, idxd_disable_dev);
+		if (err_code) {
+			IDXD_PMD_ERR("Error disabling device: code %#x", err_code);
+			return err_code;
+		}
+		IDXD_PMD_DEBUG("IDXD device disabled OK");
+		rte_free(idxd->u.pci);
+	}
+
 	return 0;
 }
 
@@ -159,12 +178,13 @@ init_pci_device(struct rte_pci_device *dev, struct idxd_dmadev *idxd,
 	uint8_t lg2_max_batch, lg2_max_copy_size;
 	unsigned int i, err_code;
 
-	pci = malloc(sizeof(*pci));
+	pci = rte_malloc(NULL, sizeof(*pci), 0);
 	if (pci == NULL) {
 		IDXD_PMD_ERR("%s: Can't allocate memory", __func__);
 		err_code = -1;
 		goto err;
 	}
+	memset(pci, 0, sizeof(*pci));
 	rte_spinlock_init(&pci->lk);
 
 	/* assign the bar registers, and then configure device */
-- 
2.31.1

Re: [PATCH 1/5] dma/idxd: fix memory leak in pci close

[Bruce Richardson]

On Fri, Apr 08, 2022 at 03:15:00PM +0100, Kevin Laatz wrote:
> ASAN reports a memory leak for the 'pci' pointer in the 'idxd_dmadev'
> struct.
> 
> This is fixed by free'ing the struct when the last queue on the PCI
> device is being closed.
> 
> Fixes: 9449330a8458 ("dma/idxd: create dmadev instances on PCI probe")
> Cc: stable@dpdk.org
> Cc: bruce.richardson@intel.com
> 
> Reported-by: Xingguang He <xingguang.he@intel.com>
> Signed-off-by: Kevin Laatz <kevin.laatz@intel.com>
> ---
Acked-by: Bruce Richardson <bruce.richardson@intel.com>

[PATCH 2/5] dma/idxd: fix memory leak due to free on incorrect pointer

[Kevin Laatz]

During PCI device close, any allocated memory needs to be free'd.
Currently, one of the free's is being called on an incorrect idxd_dmadev
struct member, namely 'batch_idx_ring', causing a memleak from the
pointer that should have been free'd.
This patch fixes this memleak by calling free on the correct pointer.

Fixes: 9449330a8458 ("dma/idxd: create dmadev instances on PCI probe")
Cc: stable@dpdk.org
Cc: bruce.richardson@intel.com

Signed-off-by: Kevin Laatz <kevin.laatz@intel.com>
---
 drivers/dma/idxd/idxd_pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/dma/idxd/idxd_pci.c b/drivers/dma/idxd/idxd_pci.c
index 7036eb938d..fdb1f15750 100644
--- a/drivers/dma/idxd/idxd_pci.c
+++ b/drivers/dma/idxd/idxd_pci.c
@@ -130,7 +130,7 @@ idxd_pci_dev_close(struct rte_dma_dev *dev)
 
 	/* free device memory */
 	IDXD_PMD_DEBUG("Freeing device driver memory");
-	rte_free(idxd->batch_idx_ring);
+	rte_free(idxd->batch_comp_ring);
 	rte_free(idxd->desc_ring);
 
 	/* if this is the last WQ on the device, disable the device and free
-- 
2.31.1

Re: [PATCH 2/5] dma/idxd: fix memory leak due to free on incorrect pointer

[Bruce Richardson]

On Fri, Apr 08, 2022 at 03:15:01PM +0100, Kevin Laatz wrote:
> During PCI device close, any allocated memory needs to be free'd.
> Currently, one of the free's is being called on an incorrect idxd_dmadev
> struct member, namely 'batch_idx_ring', causing a memleak from the
> pointer that should have been free'd.
> This patch fixes this memleak by calling free on the correct pointer.
> 
> Fixes: 9449330a8458 ("dma/idxd: create dmadev instances on PCI probe")
> Cc: stable@dpdk.org
> Cc: bruce.richardson@intel.com
> 
> Signed-off-by: Kevin Laatz <kevin.laatz@intel.com>
> ---
>  drivers/dma/idxd/idxd_pci.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/dma/idxd/idxd_pci.c b/drivers/dma/idxd/idxd_pci.c
> index 7036eb938d..fdb1f15750 100644
> --- a/drivers/dma/idxd/idxd_pci.c
> +++ b/drivers/dma/idxd/idxd_pci.c
> @@ -130,7 +130,7 @@ idxd_pci_dev_close(struct rte_dma_dev *dev)
>  
>  	/* free device memory */
>  	IDXD_PMD_DEBUG("Freeing device driver memory");
> -	rte_free(idxd->batch_idx_ring);
> +	rte_free(idxd->batch_comp_ring);
>  	rte_free(idxd->desc_ring);
>  
This is largely my fault, I expect, for being "smart" and allocating the
memory for both arrays from the one allocation. To clarify things, we need
to:
1) update the commit log message explaining why it's the wrong pointer,
i.e. that the two are in the one memory reservation
2) similarly add a comment to the rte_free call noting that it frees the
idx_ring too.

Alternatively, we can also consider adjusting the allocation code so both
arrays are allocated separately, and on free are freed similarly. We would,
however, need to double check that doing so introduces no perf hit.

/Bruce

Re: [PATCH 2/5] dma/idxd: fix memory leak due to free on incorrect pointer

[Kevin Laatz]

On 08/04/2022 15:52, Bruce Richardson wrote:
> On Fri, Apr 08, 2022 at 03:15:01PM +0100, Kevin Laatz wrote:
>> During PCI device close, any allocated memory needs to be free'd.
>> Currently, one of the free's is being called on an incorrect idxd_dmadev
>> struct member, namely 'batch_idx_ring', causing a memleak from the
>> pointer that should have been free'd.
>> This patch fixes this memleak by calling free on the correct pointer.
>>
>> Fixes: 9449330a8458 ("dma/idxd: create dmadev instances on PCI probe")
>> Cc: stable@dpdk.org
>> Cc: bruce.richardson@intel.com
>>
>> Signed-off-by: Kevin Laatz <kevin.laatz@intel.com>
>> ---
>>   drivers/dma/idxd/idxd_pci.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/dma/idxd/idxd_pci.c b/drivers/dma/idxd/idxd_pci.c
>> index 7036eb938d..fdb1f15750 100644
>> --- a/drivers/dma/idxd/idxd_pci.c
>> +++ b/drivers/dma/idxd/idxd_pci.c
>> @@ -130,7 +130,7 @@ idxd_pci_dev_close(struct rte_dma_dev *dev)
>>   
>>   	/* free device memory */
>>   	IDXD_PMD_DEBUG("Freeing device driver memory");
>> -	rte_free(idxd->batch_idx_ring);
>> +	rte_free(idxd->batch_comp_ring);
>>   	rte_free(idxd->desc_ring);
>>   
> This is largely my fault, I expect, for being "smart" and allocating the
> memory for both arrays from the one allocation. To clarify things, we need
> to:
> 1) update the commit log message explaining why it's the wrong pointer,
> i.e. that the two are in the one memory reservation
> 2) similarly add a comment to the rte_free call noting that it frees the
> idx_ring too.
>
> Alternatively, we can also consider adjusting the allocation code so both
> arrays are allocated separately, and on free are freed similarly. We would,
> however, need to double check that doing so introduces no perf hit.
>
I'll add explanations/comments where needed in v2, thanks for the feedback.

Kevin

[PATCH 3/5] app/test: close dma devices during cleanup

[Kevin Laatz]

DMA devices are created during PCI probe of EAL init. These devices
need to be closed in order to perform necessary cleanup for those
devices. This patch adds the call to close() for all DMA devices.

Signed-off-by: Kevin Laatz <kevin.laatz@intel.com>
---
 app/test/test.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/app/test/test.c b/app/test/test.c
index e69cae3eea..cc986e5cc9 100644
--- a/app/test/test.c
+++ b/app/test/test.c
@@ -24,6 +24,7 @@ extern cmdline_parse_ctx_t main_ctx[];
 #include <rte_cycles.h>
 #include <rte_log.h>
 #include <rte_string_fns.h>
+#include <rte_dmadev.h>
 #ifdef RTE_LIB_TIMER
 #include <rte_timer.h>
 #endif
@@ -244,6 +245,11 @@ main(int argc, char **argv)
 #ifdef RTE_LIB_TIMER
 	rte_timer_subsystem_finalize();
 #endif
+
+	/* close all dmadevs */
+	RTE_DMA_FOREACH_DEV(i)
+		rte_dma_close(i);
+
 	rte_eal_cleanup();
 	return ret;
 }
-- 
2.31.1

Re: [PATCH 3/5] app/test: close dma devices during cleanup

[Bruce Richardson]

On Fri, Apr 08, 2022 at 03:15:02PM +0100, Kevin Laatz wrote:
> DMA devices are created during PCI probe of EAL init. These devices need
> to be closed in order to perform necessary cleanup for those devices.
> This patch adds the call to close() for all DMA devices.
> 
> Signed-off-by: Kevin Laatz <kevin.laatz@intel.com> --- app/test/test.c |
> 6 ++++++ 1 file changed, 6 insertions(+)
> 
Just to clarify the situation here - on EAL init, all buses are probed and
all devices initialized. On eal_cleanup/rte_exit the inverse does not
happen, then, i.e. all probed devices on all buses are not closed, right?
This would seem a better option than requiring each application to manually
close all devices even if it never used them. However, it is probably a
bigger and more complex change.

/Bruce

Re: [PATCH 3/5] app/test: close dma devices during cleanup

[Kevin Laatz]

On 08/04/2022 15:55, Bruce Richardson wrote:
> On Fri, Apr 08, 2022 at 03:15:02PM +0100, Kevin Laatz wrote:
>> DMA devices are created during PCI probe of EAL init. These devices need
>> to be closed in order to perform necessary cleanup for those devices.
>> This patch adds the call to close() for all DMA devices.
>>
>> Signed-off-by: Kevin Laatz <kevin.laatz@intel.com> --- app/test/test.c |
>> 6 ++++++ 1 file changed, 6 insertions(+)
>>
> Just to clarify the situation here - on EAL init, all buses are probed and
> all devices initialized. On eal_cleanup/rte_exit the inverse does not
> happen, then, i.e. all probed devices on all buses are not closed, right?
> This would seem a better option than requiring each application to manually
> close all devices even if it never used them. However, it is probably a
> bigger and more complex change.

+1, precisely.

I've prepared an RFC to explore option of adding bus cleanup to 
eal_cleanup() to start a discussion on that option.

https://patches.dpdk.org/project/dpdk/patch/20220419161438.1837860-1-kevin.laatz@intel.com/

BR,
Kevin

[PATCH 4/5] app/testpmd: stop and close dmadevs at exit

[Kevin Laatz]

DMA devices are created during PCI probe in EAL init. In order to
perform cleanup for those devices, they need to be stopped and closed.
This patch adds the necessary cleanup to ensure clean exit.

Signed-off-by: Kevin Laatz <kevin.laatz@intel.com>
---
 app/test-pmd/testpmd.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index fe2ce19f99..438749c5b8 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -45,6 +45,7 @@
 #include <rte_pci.h>
 #include <rte_ether.h>
 #include <rte_ethdev.h>
+#include <rte_dmadev.h>
 #include <rte_dev.h>
 #include <rte_string_fns.h>
 #ifdef RTE_NET_IXGBE
@@ -3402,6 +3403,14 @@ pmd_test_exit(void)
 		}
 	}
 
+	/* stop and close all dmadevs */
+	RTE_DMA_FOREACH_DEV(i) {
+		printf("\nStopping and closing dmadev %d...\n", i);
+		fflush(stdout);
+		rte_dma_stop(i);
+		rte_dma_close(i);
+	}
+
 	if (hot_plug) {
 		ret = rte_dev_event_monitor_stop();
 		if (ret) {
-- 
2.31.1

Re: [PATCH 4/5] app/testpmd: stop and close dmadevs at exit

[Bruce Richardson]

On Fri, Apr 08, 2022 at 03:15:03PM +0100, Kevin Laatz wrote:
> DMA devices are created during PCI probe in EAL init. In order to
> perform cleanup for those devices, they need to be stopped and closed.
> This patch adds the necessary cleanup to ensure clean exit.
> 
> Signed-off-by: Kevin Laatz <kevin.laatz@intel.com>
> ---
>  app/test-pmd/testpmd.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index fe2ce19f99..438749c5b8 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -45,6 +45,7 @@
>  #include <rte_pci.h>
>  #include <rte_ether.h>
>  #include <rte_ethdev.h>
> +#include <rte_dmadev.h>

This addition triggers a build error for me, as the header is not found.
Does dmadev need to be added as a testpmd dependency in this case?

[PATCH 5/5] examples/dma: fix missing dma close

[Kevin Laatz]

The application stops all dmadevs that it used but never closed any,
meaning device cleanup was not done.
This patch adds device cleanup for all dmadevs. All devices need to be
closed for completeness, since devices not used by the application may
also have been created during PCI probe of EAL init.

Fixes: d047310407a3 ("examples/ioat: port application to dmadev API")
Cc: stable@dpdk.org
Cc: bruce.richardson@intel.com

Signed-off-by: Kevin Laatz <kevin.laatz@intel.com>
---
 examples/dma/dmafwd.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/examples/dma/dmafwd.c b/examples/dma/dmafwd.c
index 608487e35c..4c612a0e0b 100644
--- a/examples/dma/dmafwd.c
+++ b/examples/dma/dmafwd.c
@@ -1097,6 +1097,12 @@ main(int argc, char **argv)
 			rte_ring_free(cfg.ports[i].rx_to_tx_ring);
 	}
 
+	/* close all dmadevs */
+	RTE_DMA_FOREACH_DEV(i) {
+		printf("Closing dmadev %d\n", i);
+		rte_dma_close(i);
+	}
+
 	/* clean up the EAL */
 	rte_eal_cleanup();
 
-- 
2.31.1

Re: [PATCH 0/5] Fix IDXD PCI device close

[Thomas Monjalon]

> Kevin Laatz (5):
>   dma/idxd: fix memory leak in pci close
>   dma/idxd: fix memory leak due to free on incorrect pointer
>   app/test: close dma devices during cleanup
>   app/testpmd: stop and close dmadevs at exit
>   examples/dma: fix missing dma close

Any news about the v2?

Re: [PATCH 0/5] Fix IDXD PCI device close

[Kevin Laatz]

On 31/05/2022 17:17, Thomas Monjalon wrote:
>> Kevin Laatz (5):
>>    dma/idxd: fix memory leak in pci close
>>    dma/idxd: fix memory leak due to free on incorrect pointer
>>    app/test: close dma devices during cleanup
>>    app/testpmd: stop and close dmadevs at exit
>>    examples/dma: fix missing dma close
> Any news about the v2?
>
Hi Thomas,

No v2 yet since we might be able to drop the application changes from 
this patchset - depending on the changes in the series adding bus 
cleanup to EAL cleanup [1].

Ideally I would like to rework this bug fix to utilize the changes 
proposed in [1], so there is a dependency there. That would allow us to 
fix this bug without adding dma_close() to all applications.

/Kevin

[1] https://patches.dpdk.org/project/dpdk/list/?series=23151

Re: [PATCH 0/5] Fix IDXD PCI device close

[Kevin Laatz]

On 31/05/2022 17:37, Kevin Laatz wrote:
> On 31/05/2022 17:17, Thomas Monjalon wrote:
>>> Kevin Laatz (5):
>>>    dma/idxd: fix memory leak in pci close
>>>    dma/idxd: fix memory leak due to free on incorrect pointer
>>>    app/test: close dma devices during cleanup
>>>    app/testpmd: stop and close dmadevs at exit
>>>    examples/dma: fix missing dma close
>> Any news about the v2?
>>
> Hi Thomas,
>
> No v2 yet since we might be able to drop the application changes from 
> this patchset - depending on the changes in the series adding bus 
> cleanup to EAL cleanup [1].
>
> Ideally I would like to rework this bug fix to utilize the changes 
> proposed in [1], so there is a dependency there. That would allow us 
> to fix this bug without adding dma_close() to all applications.
>
> /Kevin
>
> [1] https://patches.dpdk.org/project/dpdk/list/?series=23151

v6 of the EAL patch is up: 
https://patches.dpdk.org/project/dpdk/patch/20220601170234.11100-1-kevin.laatz@intel.com/

/Kevin

[PATCH v2 0/3] Fix IDXD PCI device close

[Kevin Laatz]

This patchset addresses the device close for IDXD PCI devices.
Initially, there was a memory leak reported by ASAN for the 'pci' member
of the 'idxd_dmadev' struct due to a missing free. In addition, this
patch set corrects the behaviour of the device close function to ensure
the cleanup is completed correctly.

Depends-on: patch-112376 ("eal: add device removal in rte cleanup")

---
v2:
* remove changes to applications (no longer needed with EAL changes)
* add fix for NULL pointer dereference in pci remove

Kevin Laatz (3):
  dma/idxd: fix memory leak in pci close
  dma/idxd: fix memory leak due to free on incorrect pointer
  dma/idxd: fix null pointer dereference during pci remove

 drivers/dma/idxd/idxd_common.c   |  2 ++
 drivers/dma/idxd/idxd_internal.h |  2 ++
 drivers/dma/idxd/idxd_pci.c      | 44 +++++++++++++++++++++++---------
 3 files changed, 36 insertions(+), 12 deletions(-)

-- 
2.31.1

[PATCH v2 1/3] dma/idxd: fix memory leak in pci close

[Kevin Laatz]

ASAN reports a memory leak for the 'pci' pointer in the 'idxd_dmadev'
struct.

This is fixed by free'ing the struct when the last queue on the PCI
device is being closed.

Fixes: 9449330a8458 ("dma/idxd: create dmadev instances on PCI probe")
Cc: stable@dpdk.org
Cc: bruce.richardson@intel.com

Reported-by: Xingguang He <xingguang.he@intel.com>
Signed-off-by: Kevin Laatz <kevin.laatz@intel.com>
---
 drivers/dma/idxd/idxd_common.c   |  2 ++
 drivers/dma/idxd/idxd_internal.h |  2 ++
 drivers/dma/idxd/idxd_pci.c      | 34 +++++++++++++++++++++++++-------
 3 files changed, 31 insertions(+), 7 deletions(-)

diff --git a/drivers/dma/idxd/idxd_common.c b/drivers/dma/idxd/idxd_common.c
index c77200a457..d347bbed21 100644
--- a/drivers/dma/idxd/idxd_common.c
+++ b/drivers/dma/idxd/idxd_common.c
@@ -620,6 +620,8 @@ idxd_dmadev_create(const char *name, struct rte_device *dev,
 	dmadev->fp_obj->dev_private = idxd;
 
 	idxd->dmadev->state = RTE_DMA_DEV_READY;
+	if (idxd->u.pci != NULL)
+		rte_atomic16_inc(&idxd->u.pci->ref_count);
 
 	return 0;
 
diff --git a/drivers/dma/idxd/idxd_internal.h b/drivers/dma/idxd/idxd_internal.h
index 3375600217..180a8587c6 100644
--- a/drivers/dma/idxd/idxd_internal.h
+++ b/drivers/dma/idxd/idxd_internal.h
@@ -7,6 +7,7 @@
 
 #include <rte_dmadev_pmd.h>
 #include <rte_spinlock.h>
+#include <rte_atomic.h>
 
 #include "idxd_hw_defs.h"
 
@@ -33,6 +34,7 @@ struct idxd_pci_common {
 	rte_spinlock_t lk;
 
 	uint8_t wq_cfg_sz;
+	rte_atomic16_t ref_count;
 	volatile struct rte_idxd_bar0 *regs;
 	volatile uint32_t *wq_regs_base;
 	volatile struct rte_idxd_grpcfg *grp_regs;
diff --git a/drivers/dma/idxd/idxd_pci.c b/drivers/dma/idxd/idxd_pci.c
index 65c6bbf4c1..918981f2ea 100644
--- a/drivers/dma/idxd/idxd_pci.c
+++ b/drivers/dma/idxd/idxd_pci.c
@@ -6,6 +6,7 @@
 #include <rte_devargs.h>
 #include <rte_dmadev_pmd.h>
 #include <rte_malloc.h>
+#include <rte_atomic.h>
 
 #include "idxd_internal.h"
 
@@ -115,20 +116,38 @@ idxd_pci_dev_close(struct rte_dma_dev *dev)
 {
 	struct idxd_dmadev *idxd = dev->fp_obj->dev_private;
 	uint8_t err_code;
+	int is_last_wq;
 
-	/* disable the device */
-	err_code = idxd_pci_dev_command(idxd, idxd_disable_dev);
-	if (err_code) {
-		IDXD_PMD_ERR("Error disabling device: code %#x", err_code);
-		return err_code;
+	if (idxd_is_wq_enabled(idxd)) {
+		/* disable the wq */
+		err_code = idxd_pci_dev_command(idxd, idxd_disable_wq);
+		if (err_code) {
+			IDXD_PMD_ERR("Error disabling wq: code %#x", err_code);
+			return err_code;
+		}
+		IDXD_PMD_DEBUG("IDXD WQ disabled OK");
 	}
-	IDXD_PMD_DEBUG("IDXD Device disabled OK");
 
 	/* free device memory */
 	IDXD_PMD_DEBUG("Freeing device driver memory");
 	rte_free(idxd->batch_idx_ring);
 	rte_free(idxd->desc_ring);
 
+	/* if this is the last WQ on the device, disable the device and free
+	 * the PCI struct
+	 */
+	is_last_wq = rte_atomic16_dec_and_test(&idxd->u.pci->ref_count);
+	if (is_last_wq) {
+		/* disable the device */
+		err_code = idxd_pci_dev_command(idxd, idxd_disable_dev);
+		if (err_code) {
+			IDXD_PMD_ERR("Error disabling device: code %#x", err_code);
+			return err_code;
+		}
+		IDXD_PMD_DEBUG("IDXD device disabled OK");
+		rte_free(idxd->u.pci);
+	}
+
 	return 0;
 }
 
@@ -159,12 +178,13 @@ init_pci_device(struct rte_pci_device *dev, struct idxd_dmadev *idxd,
 	uint8_t lg2_max_batch, lg2_max_copy_size;
 	unsigned int i, err_code;
 
-	pci = malloc(sizeof(*pci));
+	pci = rte_malloc(NULL, sizeof(*pci), 0);
 	if (pci == NULL) {
 		IDXD_PMD_ERR("%s: Can't allocate memory", __func__);
 		err_code = -1;
 		goto err;
 	}
+	memset(pci, 0, sizeof(*pci));
 	rte_spinlock_init(&pci->lk);
 
 	/* assign the bar registers, and then configure device */
-- 
2.31.1

Re: [PATCH v2 1/3] dma/idxd: fix memory leak in pci close

[Bruce Richardson]

On Sun, Jul 03, 2022 at 01:22:41PM +0100, Kevin Laatz wrote:
> ASAN reports a memory leak for the 'pci' pointer in the 'idxd_dmadev'
> struct.
> 
> This is fixed by free'ing the struct when the last queue on the PCI
> device is being closed.
> 
> Fixes: 9449330a8458 ("dma/idxd: create dmadev instances on PCI probe")
> Cc: stable@dpdk.org
> Cc: bruce.richardson@intel.com
> 
> Reported-by: Xingguang He <xingguang.he@intel.com>
> Signed-off-by: Kevin Laatz <kevin.laatz@intel.com>
> ---
>  drivers/dma/idxd/idxd_common.c   |  2 ++
>  drivers/dma/idxd/idxd_internal.h |  2 ++
>  drivers/dma/idxd/idxd_pci.c      | 34 +++++++++++++++++++++++++-------
>  3 files changed, 31 insertions(+), 7 deletions(-)
> 

Some comments inline below.

/Bruce

> diff --git a/drivers/dma/idxd/idxd_common.c b/drivers/dma/idxd/idxd_common.c
> index c77200a457..d347bbed21 100644
> --- a/drivers/dma/idxd/idxd_common.c
> +++ b/drivers/dma/idxd/idxd_common.c
> @@ -620,6 +620,8 @@ idxd_dmadev_create(const char *name, struct rte_device *dev,
>  	dmadev->fp_obj->dev_private = idxd;
>  
>  	idxd->dmadev->state = RTE_DMA_DEV_READY;
> +	if (idxd->u.pci != NULL)
> +		rte_atomic16_inc(&idxd->u.pci->ref_count);
>  
>  	return 0;
>  

I don't think this belongs in the common code. Can it be put somewhere in
the pci-specific driver code to avoid issues, e.g. after idxd_dmadev_create
returns in probe_pci() function.


> diff --git a/drivers/dma/idxd/idxd_internal.h b/drivers/dma/idxd/idxd_internal.h
> index 3375600217..180a8587c6 100644
> --- a/drivers/dma/idxd/idxd_internal.h
> +++ b/drivers/dma/idxd/idxd_internal.h
> @@ -7,6 +7,7 @@
>  
>  #include <rte_dmadev_pmd.h>
>  #include <rte_spinlock.h>
> +#include <rte_atomic.h>
>  
>  #include "idxd_hw_defs.h"
>  
> @@ -33,6 +34,7 @@ struct idxd_pci_common {
>  	rte_spinlock_t lk;
>  
>  	uint8_t wq_cfg_sz;
> +	rte_atomic16_t ref_count;
>  	volatile struct rte_idxd_bar0 *regs;
>  	volatile uint32_t *wq_regs_base;
>  	volatile struct rte_idxd_grpcfg *grp_regs;
> diff --git a/drivers/dma/idxd/idxd_pci.c b/drivers/dma/idxd/idxd_pci.c
> index 65c6bbf4c1..918981f2ea 100644
> --- a/drivers/dma/idxd/idxd_pci.c
> +++ b/drivers/dma/idxd/idxd_pci.c
> @@ -6,6 +6,7 @@
>  #include <rte_devargs.h>
>  #include <rte_dmadev_pmd.h>
>  #include <rte_malloc.h>
> +#include <rte_atomic.h>
>  
>  #include "idxd_internal.h"
>  
> @@ -115,20 +116,38 @@ idxd_pci_dev_close(struct rte_dma_dev *dev)
>  {
>  	struct idxd_dmadev *idxd = dev->fp_obj->dev_private;
>  	uint8_t err_code;
> +	int is_last_wq;
>  
> -	/* disable the device */
> -	err_code = idxd_pci_dev_command(idxd, idxd_disable_dev);
> -	if (err_code) {
> -		IDXD_PMD_ERR("Error disabling device: code %#x", err_code);
> -		return err_code;
> +	if (idxd_is_wq_enabled(idxd)) {
> +		/* disable the wq */
> +		err_code = idxd_pci_dev_command(idxd, idxd_disable_wq);
> +		if (err_code) {
> +			IDXD_PMD_ERR("Error disabling wq: code %#x", err_code);
> +			return err_code;
> +		}
> +		IDXD_PMD_DEBUG("IDXD WQ disabled OK");
>  	}
> -	IDXD_PMD_DEBUG("IDXD Device disabled OK");
>  
>  	/* free device memory */
>  	IDXD_PMD_DEBUG("Freeing device driver memory");
>  	rte_free(idxd->batch_idx_ring);
>  	rte_free(idxd->desc_ring);
>  
> +	/* if this is the last WQ on the device, disable the device and free
> +	 * the PCI struct
> +	 */
> +	is_last_wq = rte_atomic16_dec_and_test(&idxd->u.pci->ref_count);
> +	if (is_last_wq) {
> +		/* disable the device */
> +		err_code = idxd_pci_dev_command(idxd, idxd_disable_dev);
> +		if (err_code) {
> +			IDXD_PMD_ERR("Error disabling device: code %#x", err_code);
> +			return err_code;
> +		}
> +		IDXD_PMD_DEBUG("IDXD device disabled OK");
> +		rte_free(idxd->u.pci);
> +	}
> +
>  	return 0;
>  }
>  
> @@ -159,12 +178,13 @@ init_pci_device(struct rte_pci_device *dev, struct idxd_dmadev *idxd,
>  	uint8_t lg2_max_batch, lg2_max_copy_size;
>  	unsigned int i, err_code;
>  
> -	pci = malloc(sizeof(*pci));
> +	pci = rte_malloc(NULL, sizeof(*pci), 0);

Any particular reason for the change from regular malloc to rte_malloc?

>  	if (pci == NULL) {
>  		IDXD_PMD_ERR("%s: Can't allocate memory", __func__);
>  		err_code = -1;
>  		goto err;
>  	}
> +	memset(pci, 0, sizeof(*pci));
>  	rte_spinlock_init(&pci->lk);
>  
>  	/* assign the bar registers, and then configure device */
> -- 
> 2.31.1
> 

Re: [PATCH v2 1/3] dma/idxd: fix memory leak in pci close

[Kevin Laatz]

On 04/07/2022 14:19, Bruce Richardson wrote:
> On Sun, Jul 03, 2022 at 01:22:41PM +0100, Kevin Laatz wrote:
>> ASAN reports a memory leak for the 'pci' pointer in the 'idxd_dmadev'
>> struct.
>>
>> This is fixed by free'ing the struct when the last queue on the PCI
>> device is being closed.
>>
>> Fixes: 9449330a8458 ("dma/idxd: create dmadev instances on PCI probe")
>> Cc: stable@dpdk.org
>> Cc: bruce.richardson@intel.com
>>
>> Reported-by: Xingguang He <xingguang.he@intel.com>
>> Signed-off-by: Kevin Laatz <kevin.laatz@intel.com>
>> ---
>>   drivers/dma/idxd/idxd_common.c   |  2 ++
>>   drivers/dma/idxd/idxd_internal.h |  2 ++
>>   drivers/dma/idxd/idxd_pci.c      | 34 +++++++++++++++++++++++++-------
>>   3 files changed, 31 insertions(+), 7 deletions(-)
>>
> Some comments inline below.
>
> /Bruce
>
>> diff --git a/drivers/dma/idxd/idxd_common.c b/drivers/dma/idxd/idxd_common.c
>> index c77200a457..d347bbed21 100644
>> --- a/drivers/dma/idxd/idxd_common.c
>> +++ b/drivers/dma/idxd/idxd_common.c
>> @@ -620,6 +620,8 @@ idxd_dmadev_create(const char *name, struct rte_device *dev,
>>   	dmadev->fp_obj->dev_private = idxd;
>>   
>>   	idxd->dmadev->state = RTE_DMA_DEV_READY;
>> +	if (idxd->u.pci != NULL)
>> +		rte_atomic16_inc(&idxd->u.pci->ref_count);
>>   
>>   	return 0;
>>   
> I don't think this belongs in the common code. Can it be put somewhere in
> the pci-specific driver code to avoid issues, e.g. after idxd_dmadev_create
> returns in probe_pci() function.

Sure, I'll look to move it in v3.

[snip]
>   
> @@ -159,12 +178,13 @@ init_pci_device(struct rte_pci_device *dev, struct idxd_dmadev *idxd,
>   	uint8_t lg2_max_batch, lg2_max_copy_size;
>   	unsigned int i, err_code;
>   
> -	pci = malloc(sizeof(*pci));
> +	pci = rte_malloc(NULL, sizeof(*pci), 0);
> Any particular reason for the change from regular malloc to rte_malloc?

Mainly consistency, its the only place in the driver where malloc is 
used instead of rte_malloc.

I have no strong preference here - I can remove the change for v3.

/Kevin

Re: [PATCH v2 1/3] dma/idxd: fix memory leak in pci close

[Bruce Richardson]

On Mon, Jul 04, 2022 at 02:34:31PM +0100, Kevin Laatz wrote:
> On 04/07/2022 14:19, Bruce Richardson wrote:
> > On Sun, Jul 03, 2022 at 01:22:41PM +0100, Kevin Laatz wrote:
> > > ASAN reports a memory leak for the 'pci' pointer in the 'idxd_dmadev'
> > > struct.
> > > 
> > > This is fixed by free'ing the struct when the last queue on the PCI
> > > device is being closed.
> > > 
> > > Fixes: 9449330a8458 ("dma/idxd: create dmadev instances on PCI probe")
> > > Cc: stable@dpdk.org
> > > Cc: bruce.richardson@intel.com
> > > 
> > > Reported-by: Xingguang He <xingguang.he@intel.com>
> > > Signed-off-by: Kevin Laatz <kevin.laatz@intel.com>
> > > ---
> > >   drivers/dma/idxd/idxd_common.c   |  2 ++
> > >   drivers/dma/idxd/idxd_internal.h |  2 ++
> > >   drivers/dma/idxd/idxd_pci.c      | 34 +++++++++++++++++++++++++-------
> > >   3 files changed, 31 insertions(+), 7 deletions(-)
> > > 
> > Some comments inline below.
> > 
> > /Bruce
> > 
> > > diff --git a/drivers/dma/idxd/idxd_common.c b/drivers/dma/idxd/idxd_common.c
> > > index c77200a457..d347bbed21 100644
> > > --- a/drivers/dma/idxd/idxd_common.c
> > > +++ b/drivers/dma/idxd/idxd_common.c
> > > @@ -620,6 +620,8 @@ idxd_dmadev_create(const char *name, struct rte_device *dev,
> > >   	dmadev->fp_obj->dev_private = idxd;
> > >   	idxd->dmadev->state = RTE_DMA_DEV_READY;
> > > +	if (idxd->u.pci != NULL)
> > > +		rte_atomic16_inc(&idxd->u.pci->ref_count);
> > >   	return 0;
> > I don't think this belongs in the common code. Can it be put somewhere in
> > the pci-specific driver code to avoid issues, e.g. after idxd_dmadev_create
> > returns in probe_pci() function.
> 
> Sure, I'll look to move it in v3.
> 
> [snip]
> > @@ -159,12 +178,13 @@ init_pci_device(struct rte_pci_device *dev, struct idxd_dmadev *idxd,
> >   	uint8_t lg2_max_batch, lg2_max_copy_size;
> >   	unsigned int i, err_code;
> > -	pci = malloc(sizeof(*pci));
> > +	pci = rte_malloc(NULL, sizeof(*pci), 0);
> > Any particular reason for the change from regular malloc to rte_malloc?
> 
> Mainly consistency, its the only place in the driver where malloc is used
> instead of rte_malloc.
> 
> I have no strong preference here - I can remove the change for v3.
> 
If it's inconsistent with the rest, please do keep the change in v3.

[PATCH v2 2/3] dma/idxd: fix memory leak due to free on incorrect pointer

[Kevin Laatz]

During PCI device close, any allocated memory needs to be free'd.
Currently, one of the free's is being called on an incorrect idxd_dmadev
struct member, namely 'batch_idx_ring', causing a memleak from the
pointer that should have been free'd.
This patch fixes this memleak by calling free on the correct pointer.

Fixes: 9449330a8458 ("dma/idxd: create dmadev instances on PCI probe")
Cc: stable@dpdk.org
Cc: bruce.richardson@intel.com

Signed-off-by: Kevin Laatz <kevin.laatz@intel.com>
---
 drivers/dma/idxd/idxd_pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/dma/idxd/idxd_pci.c b/drivers/dma/idxd/idxd_pci.c
index 918981f2ea..9349c56b3f 100644
--- a/drivers/dma/idxd/idxd_pci.c
+++ b/drivers/dma/idxd/idxd_pci.c
@@ -130,7 +130,7 @@ idxd_pci_dev_close(struct rte_dma_dev *dev)
 
 	/* free device memory */
 	IDXD_PMD_DEBUG("Freeing device driver memory");
-	rte_free(idxd->batch_idx_ring);
+	rte_free(idxd->batch_comp_ring);
 	rte_free(idxd->desc_ring);
 
 	/* if this is the last WQ on the device, disable the device and free
-- 
2.31.1

Re: [PATCH v2 2/3] dma/idxd: fix memory leak due to free on incorrect pointer

[Bruce Richardson]

On Sun, Jul 03, 2022 at 01:22:42PM +0100, Kevin Laatz wrote:
> During PCI device close, any allocated memory needs to be free'd.
> Currently, one of the free's is being called on an incorrect idxd_dmadev
> struct member, namely 'batch_idx_ring', causing a memleak from the
> pointer that should have been free'd.

I think you need to explain that the two rings are beside each other in
memory and we need to free using the pointer to the start of the block,
rather than the pointer to the middle of it.

> This patch fixes this memleak by calling free on the correct pointer.
> 
> Fixes: 9449330a8458 ("dma/idxd: create dmadev instances on PCI probe")
> Cc: stable@dpdk.org
> Cc: bruce.richardson@intel.com
> 
> Signed-off-by: Kevin Laatz <kevin.laatz@intel.com>

With more explanation in the commit log

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>

> ---
>  drivers/dma/idxd/idxd_pci.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/dma/idxd/idxd_pci.c b/drivers/dma/idxd/idxd_pci.c
> index 918981f2ea..9349c56b3f 100644
> --- a/drivers/dma/idxd/idxd_pci.c
> +++ b/drivers/dma/idxd/idxd_pci.c
> @@ -130,7 +130,7 @@ idxd_pci_dev_close(struct rte_dma_dev *dev)
>  
>  	/* free device memory */
>  	IDXD_PMD_DEBUG("Freeing device driver memory");
> -	rte_free(idxd->batch_idx_ring);
> +	rte_free(idxd->batch_comp_ring);
>  	rte_free(idxd->desc_ring);
>  
>  	/* if this is the last WQ on the device, disable the device and free
> -- 
> 2.31.1
> 

Re: [PATCH v2 2/3] dma/idxd: fix memory leak due to free on incorrect pointer

[Bruce Richardson]

On Mon, Jul 04, 2022 at 02:23:38PM +0100, Bruce Richardson wrote:
> On Sun, Jul 03, 2022 at 01:22:42PM +0100, Kevin Laatz wrote:
> > During PCI device close, any allocated memory needs to be free'd.
> > Currently, one of the free's is being called on an incorrect idxd_dmadev
> > struct member, namely 'batch_idx_ring', causing a memleak from the
> > pointer that should have been free'd.
> 
> I think you need to explain that the two rings are beside each other in
> memory and we need to free using the pointer to the start of the block,
> rather than the pointer to the middle of it.
> 
> > This patch fixes this memleak by calling free on the correct pointer.
> > 
> > Fixes: 9449330a8458 ("dma/idxd: create dmadev instances on PCI probe")
> > Cc: stable@dpdk.org
> > Cc: bruce.richardson@intel.com
> > 
> > Signed-off-by: Kevin Laatz <kevin.laatz@intel.com>
> 
> With more explanation in the commit log
> 

Correction (obviously!)
Acked-by: Bruce Richardson <bruce.richardson@intel.com>

[PATCH v2 3/3] dma/idxd: fix null pointer dereference during pci remove

[Kevin Laatz]

The 'info' struct was being declared as a NULL pointer. If a NULL
pointer is passed to 'rte_dma_info_get', EINVAL is returned and the
struct is not populated. This subsequently causes a segfault when
dereferencing 'info'.

This patch fixes the issue by simply declaring 'info' as a variable and
passing its address to 'rte_dma_info_get'.

Fixes: 9449330a8458 ("dma/idxd: create dmadev instances on PCI probe")
Cc: stable@dpdk.org

Signed-off-by: Kevin Laatz <kevin.laatz@intel.com>
---
 drivers/dma/idxd/idxd_pci.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/dma/idxd/idxd_pci.c b/drivers/dma/idxd/idxd_pci.c
index 9349c56b3f..3c1effeb84 100644
--- a/drivers/dma/idxd/idxd_pci.c
+++ b/drivers/dma/idxd/idxd_pci.c
@@ -379,10 +379,10 @@ idxd_dmadev_remove_pci(struct rte_pci_device *dev)
 	IDXD_PMD_INFO("Closing %s on NUMA node %d", name, dev->device.numa_node);
 
 	RTE_DMA_FOREACH_DEV(i) {
-		struct rte_dma_info *info = {0};
-		rte_dma_info_get(i, info);
-		if (strncmp(name, info->dev_name, strlen(name)) == 0)
-			idxd_dmadev_destroy(info->dev_name);
+		struct rte_dma_info info;
+		rte_dma_info_get(i, &info);
+		if (strncmp(name, info.dev_name, strlen(name)) == 0)
+			idxd_dmadev_destroy(info.dev_name);
 	}
 
 	return 0;
-- 
2.31.1

Re: [PATCH v2 3/3] dma/idxd: fix null pointer dereference during pci remove

[Bruce Richardson]

On Sun, Jul 03, 2022 at 01:22:43PM +0100, Kevin Laatz wrote:
> The 'info' struct was being declared as a NULL pointer. If a NULL
> pointer is passed to 'rte_dma_info_get', EINVAL is returned and the
> struct is not populated. This subsequently causes a segfault when
> dereferencing 'info'.
> 
> This patch fixes the issue by simply declaring 'info' as a variable and

s/as a variable/on the stack/

> passing its address to 'rte_dma_info_get'.
> 
> Fixes: 9449330a8458 ("dma/idxd: create dmadev instances on PCI probe")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Kevin Laatz <kevin.laatz@intel.com>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>

> ---
>  drivers/dma/idxd/idxd_pci.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/dma/idxd/idxd_pci.c b/drivers/dma/idxd/idxd_pci.c
> index 9349c56b3f..3c1effeb84 100644
> --- a/drivers/dma/idxd/idxd_pci.c
> +++ b/drivers/dma/idxd/idxd_pci.c
> @@ -379,10 +379,10 @@ idxd_dmadev_remove_pci(struct rte_pci_device *dev)
>  	IDXD_PMD_INFO("Closing %s on NUMA node %d", name, dev->device.numa_node);
>  
>  	RTE_DMA_FOREACH_DEV(i) {
> -		struct rte_dma_info *info = {0};
> -		rte_dma_info_get(i, info);
> -		if (strncmp(name, info->dev_name, strlen(name)) == 0)
> -			idxd_dmadev_destroy(info->dev_name);
> +		struct rte_dma_info info;
> +		rte_dma_info_get(i, &info);
> +		if (strncmp(name, info.dev_name, strlen(name)) == 0)
> +			idxd_dmadev_destroy(info.dev_name);
>  	}
>  
>  	return 0;
> -- 
> 2.31.1
> 

[PATCH v3 0/3] Fix IDXD PCI device close

[Kevin Laatz]

This patchset addresses the device close for IDXD PCI devices.
Initially, there was a memory leak reported by ASAN for the 'pci' member
of the 'idxd_dmadev' struct due to a missing free. In addition, this
patch set corrects the behaviour of the device close function to ensure
the cleanup is completed correctly.

Depends-on: patch-112376 ("eal: add device removal in rte cleanup")

---
v3:
* move ref_count increment from common create to pci probe
* improve commit messages

v2:
* remove changes to applications (no longer needed with EAL changes)
* add fix for NULL pointer dereference in pci remove

Kevin Laatz (3):
  dma/idxd: fix memory leak in pci close
  dma/idxd: fix memory leak due to free on incorrect pointer
  dma/idxd: fix null pointer dereference during pci remove

 drivers/dma/idxd/idxd_internal.h |  2 ++
 drivers/dma/idxd/idxd_pci.c      | 45 +++++++++++++++++++++++---------
 2 files changed, 35 insertions(+), 12 deletions(-)

-- 
2.31.1

[PATCH v3 1/3] dma/idxd: fix memory leak in pci close

[Kevin Laatz]

ASAN reports a memory leak for the 'pci' pointer in the 'idxd_dmadev'
struct.

This is fixed by free'ing the struct when the last queue on the PCI
device is being closed.

Fixes: 9449330a8458 ("dma/idxd: create dmadev instances on PCI probe")
Cc: stable@dpdk.org
Cc: bruce.richardson@intel.com

Reported-by: Xingguang He <xingguang.he@intel.com>
Signed-off-by: Kevin Laatz <kevin.laatz@intel.com>

--
v3: move ref_count increment to pci probe
---
 drivers/dma/idxd/idxd_internal.h |  2 ++
 drivers/dma/idxd/idxd_pci.c      | 35 +++++++++++++++++++++++++-------
 2 files changed, 30 insertions(+), 7 deletions(-)

diff --git a/drivers/dma/idxd/idxd_internal.h b/drivers/dma/idxd/idxd_internal.h
index 3375600217..180a8587c6 100644
--- a/drivers/dma/idxd/idxd_internal.h
+++ b/drivers/dma/idxd/idxd_internal.h
@@ -7,6 +7,7 @@
 
 #include <rte_dmadev_pmd.h>
 #include <rte_spinlock.h>
+#include <rte_atomic.h>
 
 #include "idxd_hw_defs.h"
 
@@ -33,6 +34,7 @@ struct idxd_pci_common {
 	rte_spinlock_t lk;
 
 	uint8_t wq_cfg_sz;
+	rte_atomic16_t ref_count;
 	volatile struct rte_idxd_bar0 *regs;
 	volatile uint32_t *wq_regs_base;
 	volatile struct rte_idxd_grpcfg *grp_regs;
diff --git a/drivers/dma/idxd/idxd_pci.c b/drivers/dma/idxd/idxd_pci.c
index 65c6bbf4c1..fb618d34b6 100644
--- a/drivers/dma/idxd/idxd_pci.c
+++ b/drivers/dma/idxd/idxd_pci.c
@@ -6,6 +6,7 @@
 #include <rte_devargs.h>
 #include <rte_dmadev_pmd.h>
 #include <rte_malloc.h>
+#include <rte_atomic.h>
 
 #include "idxd_internal.h"
 
@@ -115,20 +116,38 @@ idxd_pci_dev_close(struct rte_dma_dev *dev)
 {
 	struct idxd_dmadev *idxd = dev->fp_obj->dev_private;
 	uint8_t err_code;
+	int is_last_wq;
 
-	/* disable the device */
-	err_code = idxd_pci_dev_command(idxd, idxd_disable_dev);
-	if (err_code) {
-		IDXD_PMD_ERR("Error disabling device: code %#x", err_code);
-		return err_code;
+	if (idxd_is_wq_enabled(idxd)) {
+		/* disable the wq */
+		err_code = idxd_pci_dev_command(idxd, idxd_disable_wq);
+		if (err_code) {
+			IDXD_PMD_ERR("Error disabling wq: code %#x", err_code);
+			return err_code;
+		}
+		IDXD_PMD_DEBUG("IDXD WQ disabled OK");
 	}
-	IDXD_PMD_DEBUG("IDXD Device disabled OK");
 
 	/* free device memory */
 	IDXD_PMD_DEBUG("Freeing device driver memory");
 	rte_free(idxd->batch_idx_ring);
 	rte_free(idxd->desc_ring);
 
+	/* if this is the last WQ on the device, disable the device and free
+	 * the PCI struct
+	 */
+	is_last_wq = rte_atomic16_dec_and_test(&idxd->u.pci->ref_count);
+	if (is_last_wq) {
+		/* disable the device */
+		err_code = idxd_pci_dev_command(idxd, idxd_disable_dev);
+		if (err_code) {
+			IDXD_PMD_ERR("Error disabling device: code %#x", err_code);
+			return err_code;
+		}
+		IDXD_PMD_DEBUG("IDXD device disabled OK");
+		rte_free(idxd->u.pci);
+	}
+
 	return 0;
 }
 
@@ -159,12 +178,13 @@ init_pci_device(struct rte_pci_device *dev, struct idxd_dmadev *idxd,
 	uint8_t lg2_max_batch, lg2_max_copy_size;
 	unsigned int i, err_code;
 
-	pci = malloc(sizeof(*pci));
+	pci = rte_malloc(NULL, sizeof(*pci), 0);
 	if (pci == NULL) {
 		IDXD_PMD_ERR("%s: Can't allocate memory", __func__);
 		err_code = -1;
 		goto err;
 	}
+	memset(pci, 0, sizeof(*pci));
 	rte_spinlock_init(&pci->lk);
 
 	/* assign the bar registers, and then configure device */
@@ -330,6 +350,7 @@ idxd_dmadev_probe_pci(struct rte_pci_driver *drv, struct rte_pci_device *dev)
 				free(idxd.u.pci);
 			return ret;
 		}
+		rte_atomic16_inc(&idxd.u.pci->ref_count);
 	}
 
 	return 0;
-- 
2.31.1

Re: [PATCH v3 1/3] dma/idxd: fix memory leak in pci close

[Bruce Richardson]

On Mon, Jul 04, 2022 at 04:27:49PM +0100, Kevin Laatz wrote:
> ASAN reports a memory leak for the 'pci' pointer in the 'idxd_dmadev'
> struct.
> 
> This is fixed by free'ing the struct when the last queue on the PCI
> device is being closed.
> 
> Fixes: 9449330a8458 ("dma/idxd: create dmadev instances on PCI probe")
> Cc: stable@dpdk.org
> Cc: bruce.richardson@intel.com
> 
> Reported-by: Xingguang He <xingguang.he@intel.com>
> Signed-off-by: Kevin Laatz <kevin.laatz@intel.com>
> 
> --
> v3: move ref_count increment to pci probe
> ---

Acked-by: Bruce Richardson <bruce.richardson@intel.com>

[PATCH v3 2/3] dma/idxd: fix memory leak due to free on incorrect pointer

[Kevin Laatz]

During PCI device close, any allocated memory needs to be free'd.
Currently, one of the free's is being called on an incorrect idxd_dmadev
struct member, namely 'batch_idx_ring'.

At device creation, memory is allocated for both 'batch_comp_ring' and
'batch_idx_ring' simultaeneously. Calling free only on 'batch_idx_ring'
meant the first half of this memory was not being free'd, leading to the
memleak.

This patch fixes this memleak by calling free on 'batch_comp_ring' which
will free the memory for both rings.

Fixes: 9449330a8458 ("dma/idxd: create dmadev instances on PCI probe")
Cc: stable@dpdk.org
Cc: bruce.richardson@intel.com

Signed-off-by: Kevin Laatz <kevin.laatz@intel.com>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
---
 drivers/dma/idxd/idxd_pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/dma/idxd/idxd_pci.c b/drivers/dma/idxd/idxd_pci.c
index fb618d34b6..2c3b01cd2b 100644
--- a/drivers/dma/idxd/idxd_pci.c
+++ b/drivers/dma/idxd/idxd_pci.c
@@ -130,7 +130,7 @@ idxd_pci_dev_close(struct rte_dma_dev *dev)
 
 	/* free device memory */
 	IDXD_PMD_DEBUG("Freeing device driver memory");
-	rte_free(idxd->batch_idx_ring);
+	rte_free(idxd->batch_comp_ring);
 	rte_free(idxd->desc_ring);
 
 	/* if this is the last WQ on the device, disable the device and free
-- 
2.31.1

[PATCH v3 3/3] dma/idxd: fix null pointer dereference during pci remove

[Kevin Laatz]

The 'info' struct was being declared as a NULL pointer. If a NULL
pointer is passed to 'rte_dma_info_get', EINVAL is returned and the
struct is not populated. This subsequently causes a segfault when
dereferencing 'info'.

This patch fixes the issue by simply declaring 'info' on the stack and
passing its address to 'rte_dma_info_get'.

Fixes: 9449330a8458 ("dma/idxd: create dmadev instances on PCI probe")
Cc: stable@dpdk.org

Signed-off-by: Kevin Laatz <kevin.laatz@intel.com>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
---
 drivers/dma/idxd/idxd_pci.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/dma/idxd/idxd_pci.c b/drivers/dma/idxd/idxd_pci.c
index 2c3b01cd2b..2f8ec06d9e 100644
--- a/drivers/dma/idxd/idxd_pci.c
+++ b/drivers/dma/idxd/idxd_pci.c
@@ -380,10 +380,10 @@ idxd_dmadev_remove_pci(struct rte_pci_device *dev)
 	IDXD_PMD_INFO("Closing %s on NUMA node %d", name, dev->device.numa_node);
 
 	RTE_DMA_FOREACH_DEV(i) {
-		struct rte_dma_info *info = {0};
-		rte_dma_info_get(i, info);
-		if (strncmp(name, info->dev_name, strlen(name)) == 0)
-			idxd_dmadev_destroy(info->dev_name);
+		struct rte_dma_info info;
+		rte_dma_info_get(i, &info);
+		if (strncmp(name, info.dev_name, strlen(name)) == 0)
+			idxd_dmadev_destroy(info.dev_name);
 	}
 
 	return 0;
-- 
2.31.1

Re: [PATCH v3 0/3] Fix IDXD PCI device close

[Thomas Monjalon]

04/07/2022 17:27, Kevin Laatz:
> This patchset addresses the device close for IDXD PCI devices.
> Initially, there was a memory leak reported by ASAN for the 'pci' member
> of the 'idxd_dmadev' struct due to a missing free. In addition, this
> patch set corrects the behaviour of the device close function to ensure
> the cleanup is completed correctly.
> 
> Depends-on: patch-112376 ("eal: add device removal in rte cleanup")

I assume it is not a real dependency.
We can fix closing without calling the close function in EAL cleanup.

Applied, thanks.