[PATCH v2 0/7] dmaengine: idxd: fix memory leak in error handling path

[PATCH v2 0/7] dmaengine: idxd: fix memory leak in error handling path

[Shuai Xue]

changes since v1:
- add Reviewed-by tag for patch 1-5 from Dave Jiang
- add fixes tag
- add patch 6 and 7 to fix memory leak in remove call per Vinicius

Shuai Xue (7):
  dmaengine: idxd: fix memory leak in error handling path of
    idxd_setup_wqs
  dmaengine: idxd: fix memory leak in error handling path of
    idxd_setup_engines
  dmaengine: idxd: fix memory leak in error handling path of
    idxd_setup_groups
  dmaengine: idxd: fix memory leak in error handling path of idxd_alloc
  dmaengine: idxd: fix memory leak in error handling path of
    idxd_pci_probe
  dmaengine: idxd: Add missing idxd cleanup to fix memory leak in remove
    call
  dmaengine: idxd: Refactor remove call with idxd_cleanup() helper

 drivers/dma/idxd/init.c | 75 ++++++++++++++++++++++++++++-------------
 1 file changed, 51 insertions(+), 24 deletions(-)

-- 
2.39.3

[PATCH v2 1/7] dmaengine: idxd: fix memory leak in error handling path of idxd_setup_wqs

[Shuai Xue]

Memory allocated for wqs is not freed if an error occurs during
idxd_setup_wqs(). To fix it, free the allocated memory in the reverse
order of allocation before exiting the function in case of an error.

Fixes: a8563a33a5e2 ("dmanegine: idxd: reformat opcap output to match bitmap_parse() input")
Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com>
Reviewed-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/dma/idxd/init.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c
index b946f78f85e1..b85736fd25bd 100644
--- a/drivers/dma/idxd/init.c
+++ b/drivers/dma/idxd/init.c
@@ -169,8 +169,8 @@ static int idxd_setup_wqs(struct idxd_device *idxd)
 
 	idxd->wq_enable_map = bitmap_zalloc_node(idxd->max_wqs, GFP_KERNEL, dev_to_node(dev));
 	if (!idxd->wq_enable_map) {
-		kfree(idxd->wqs);
-		return -ENOMEM;
+		rc = -ENOMEM;
+		goto err_bitmap;
 	}
 
 	for (i = 0; i < idxd->max_wqs; i++) {
@@ -191,6 +191,7 @@ static int idxd_setup_wqs(struct idxd_device *idxd)
 		rc = dev_set_name(conf_dev, "wq%d.%d", idxd->id, wq->id);
 		if (rc < 0) {
 			put_device(conf_dev);
+			kfree(wq);
 			goto err;
 		}
 
@@ -204,6 +205,7 @@ static int idxd_setup_wqs(struct idxd_device *idxd)
 		wq->wqcfg = kzalloc_node(idxd->wqcfg_size, GFP_KERNEL, dev_to_node(dev));
 		if (!wq->wqcfg) {
 			put_device(conf_dev);
+			kfree(wq);
 			rc = -ENOMEM;
 			goto err;
 		}
@@ -211,7 +213,9 @@ static int idxd_setup_wqs(struct idxd_device *idxd)
 		if (idxd->hw.wq_cap.op_config) {
 			wq->opcap_bmap = bitmap_zalloc(IDXD_MAX_OPCAP_BITS, GFP_KERNEL);
 			if (!wq->opcap_bmap) {
+				kfree(wq->wqcfg);
 				put_device(conf_dev);
+				kfree(wq);
 				rc = -ENOMEM;
 				goto err;
 			}
@@ -225,11 +229,21 @@ static int idxd_setup_wqs(struct idxd_device *idxd)
 	return 0;
 
  err:
-	while (--i >= 0) {
+	while (i-- > 0) {
 		wq = idxd->wqs[i];
+		if (idxd->hw.wq_cap.op_config)
+			bitmap_free(wq->opcap_bmap);
+		kfree(wq->wqcfg);
 		conf_dev = wq_confdev(wq);
 		put_device(conf_dev);
+		kfree(wq);
+
 	}
+	bitmap_free(idxd->wq_enable_map);
+
+err_bitmap:
+	kfree(idxd->wqs);
+
 	return rc;
 }
 
-- 
2.39.3

Re: [PATCH v2 1/7] dmaengine: idxd: fix memory leak in error handling path of idxd_setup_wqs()

[Markus Elfring]

> Memory allocated for wqs is not freed if an error occurs during
> idxd_setup_wqs(). To fix it, free the allocated memory in the reverse
> order of allocation before exiting the function in case of an error.
>
> Fixes: a8563a33a5e2 ("dmanegine: idxd: reformat opcap output to match bitmap_parse() input")
…

Will a “stable tag” become relevant also for this patch series?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/stable-kernel-rules.rst?h=v6.14-rc2#n3


> +++ b/drivers/dma/idxd/init.c
> @@ -169,8 +169,8 @@ static int idxd_setup_wqs(struct idxd_device *idxd)
…
> @@ -204,6 +205,7 @@ static int idxd_setup_wqs(struct idxd_device *idxd)
>  		wq->wqcfg = kzalloc_node(idxd->wqcfg_size, GFP_KERNEL, dev_to_node(dev));
>  		if (!wq->wqcfg) {
>  			put_device(conf_dev);
> +			kfree(wq);
>  			rc = -ENOMEM;
>  			goto err;
>  		}
…

I got the impression that more common exception handling code could be moved
to additional jump targets at the end of such function implementations.
Will further adjustment opportunities be taken into account for
the affected resource management?

Regards,
Markus

Re: [PATCH v2 1/7] dmaengine: idxd: fix memory leak in error handling path of idxd_setup_wqs()

[Shuai Xue]

在 2025/2/15 19:00, Markus Elfring 写道:
>> Memory allocated for wqs is not freed if an error occurs during
>> idxd_setup_wqs(). To fix it, free the allocated memory in the reverse
>> order of allocation before exiting the function in case of an error.
>>
>> Fixes: a8563a33a5e2 ("dmanegine: idxd: reformat opcap output to match bitmap_parse() input")
> …
> 
> Will a “stable tag” become relevant also for this patch series?
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/stable-kernel-rules.rst?h=v6.14-rc2#n3
> 

I don't know if this is a real serious issue for stable kernel.
But I would like to add stable tag if the mantainer @Vinicius and @Dave ask.

> 
>> +++ b/drivers/dma/idxd/init.c
>> @@ -169,8 +169,8 @@ static int idxd_setup_wqs(struct idxd_device *idxd)
> …
>> @@ -204,6 +205,7 @@ static int idxd_setup_wqs(struct idxd_device *idxd)
>>   		wq->wqcfg = kzalloc_node(idxd->wqcfg_size, GFP_KERNEL, dev_to_node(dev));
>>   		if (!wq->wqcfg) {
>>   			put_device(conf_dev);
>> +			kfree(wq);
>>   			rc = -ENOMEM;
>>   			goto err;
>>   		}
> …
> 
> I got the impression that more common exception handling code could be moved
> to additional jump targets at the end of such function implementations.
> Will further adjustment opportunities be taken into account for
> the affected resource management?

Sure, I will move them to the jump targets.

> 
> Regards,
> Markus

Thanks for valuable comments.
Shuai

Re: [v2 1/7] dmaengine: idxd: fix memory leak in error handling path of idxd_setup_wqs()

[Markus Elfring]

>> Will a “stable tag” become relevant also for this patch series?
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/stable-kernel-rules.rst?h=v6.14-rc2#n3
>
> I don't know if this is a real serious issue for stable kernel.
How many resource leaks would you like to avoid with your contributions?

Regards,
Markus

Re: [v2 1/7] dmaengine: idxd: fix memory leak in error handling path of idxd_setup_wqs()

[Shuai Xue]

在 2025/2/16 17:34, Markus Elfring 写道:
>>> Will a “stable tag” become relevant also for this patch series?
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/stable-kernel-rules.rst?h=v6.14-rc2#n3
>>
>> I don't know if this is a real serious issue for stable kernel.
> How many resource leaks would you like to avoid with your contributions?

For this patch:

     - wqs = 8 * 8 bytes = 64 bytes
     - wq = 8 * 2400 bytes =  19200 bytes
     - opcap_bmap = 256 bit / 8 = 32 bytes

Around 18 KB.


> 
> Regards,
> Markus

Thanks.
Shuai

Re: [PATCH v2 1/7] dmaengine: idxd: fix memory leak in error handling path of idxd_setup_wqs

[Markus Elfring]

…
> Fixes: a8563a33a5e2 ("dmanegine: idxd: reformat opcap output to match bitmap_parse() input")
…

Would other commits be more appropriate for the desired reference
(according to the affected function implementation)?

Examples:
2022-09-29: de5819b994893197c71c86d21af10f85f50d6499 ("dmaengine: idxd: track enabled workqueues in bitmap")
2021-04-21: 700af3a0a26cbac87e4a0ae1dfa79597d0056d5f ("dmaengine: idxd: add 'struct idxd_dev' as wrapper for conf_dev")
2021-04-20: 7c5dd23e57c14cf7177b8a5e0fd08916e0c60005 ("dmaengine: idxd: fix wq conf_dev 'struct device' lifetime")


Regards,
Markus

Re: [PATCH v2 1/7] dmaengine: idxd: fix memory leak in error handling path of idxd_setup_wqs

[Shuai Xue]

在 2025/2/15 21:34, Markus Elfring 写道:
> …
>> Fixes: a8563a33a5e2 ("dmanegine: idxd: reformat opcap output to match bitmap_parse() input")
> …
> 
> Would other commits be more appropriate for the desired reference
> (according to the affected function implementation)?
> 
> Examples:
> 2022-09-29: de5819b994893197c71c86d21af10f85f50d6499 ("dmaengine: idxd: track enabled workqueues in bitmap")
> 2021-04-21: 700af3a0a26cbac87e4a0ae1dfa79597d0056d5f ("dmaengine: idxd: add 'struct idxd_dev' as wrapper for conf_dev")
> 2021-04-20: 7c5dd23e57c14cf7177b8a5e0fd08916e0c60005 ("dmaengine: idxd: fix wq conf_dev 'struct device' lifetime")
> 

Ok, I will add the fixes.

> 
> Regards,
> Markus

Thanks.

Re: [PATCH v2 1/7] dmaengine: idxd: fix memory leak in error handling path of idxd_setup_wqs

[Fenghua Yu]

Hi, Shuai,

On 2/14/25 21:44, Shuai Xue wrote:
> Memory allocated for wqs is not freed if an error occurs during
> idxd_setup_wqs(). To fix it, free the allocated memory in the reverse
> order of allocation before exiting the function in case of an error.
>
> Fixes: a8563a33a5e2 ("dmanegine: idxd: reformat opcap output to match bitmap_parse() input")
> Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com>
> Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> ---
>   drivers/dma/idxd/init.c | 20 +++++++++++++++++---
>   1 file changed, 17 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c
> index b946f78f85e1..b85736fd25bd 100644
> --- a/drivers/dma/idxd/init.c
> +++ b/drivers/dma/idxd/init.c
> @@ -169,8 +169,8 @@ static int idxd_setup_wqs(struct idxd_device *idxd)
>   
>   	idxd->wq_enable_map = bitmap_zalloc_node(idxd->max_wqs, GFP_KERNEL, dev_to_node(dev));
>   	if (!idxd->wq_enable_map) {
> -		kfree(idxd->wqs);
> -		return -ENOMEM;
> +		rc = -ENOMEM;
> +		goto err_bitmap;
>   	}
>   
>   	for (i = 0; i < idxd->max_wqs; i++) {
> @@ -191,6 +191,7 @@ static int idxd_setup_wqs(struct idxd_device *idxd)
>   		rc = dev_set_name(conf_dev, "wq%d.%d", idxd->id, wq->id);
>   		if (rc < 0) {
>   			put_device(conf_dev);
> +			kfree(wq);
>   			goto err;
>   		}
>   
> @@ -204,6 +205,7 @@ static int idxd_setup_wqs(struct idxd_device *idxd)
>   		wq->wqcfg = kzalloc_node(idxd->wqcfg_size, GFP_KERNEL, dev_to_node(dev));
>   		if (!wq->wqcfg) {
>   			put_device(conf_dev);
> +			kfree(wq);
>   			rc = -ENOMEM;
>   			goto err;
>   		}
> @@ -211,7 +213,9 @@ static int idxd_setup_wqs(struct idxd_device *idxd)
>   		if (idxd->hw.wq_cap.op_config) {
>   			wq->opcap_bmap = bitmap_zalloc(IDXD_MAX_OPCAP_BITS, GFP_KERNEL);
>   			if (!wq->opcap_bmap) {
> +				kfree(wq->wqcfg);
>   				put_device(conf_dev);
> +				kfree(wq);
>   				rc = -ENOMEM;
>   				goto err;
>   			}
> @@ -225,11 +229,21 @@ static int idxd_setup_wqs(struct idxd_device *idxd)
>   	return 0;
>   
>    err:
> -	while (--i >= 0) {
> +	while (i-- > 0) {

Why changed to "i-- > 0" here? Before coming to here, the mem areas 
allocated for wqs[i] are freed already and there is not need to free 
them again here, right? And if i>1, mem areas for wqs[0] won't be freed 
and will leak, right?

>   		wq = idxd->wqs[i];
> +		if (idxd->hw.wq_cap.op_config)
> +			bitmap_free(wq->opcap_bmap);
> +		kfree(wq->wqcfg);
>   		conf_dev = wq_confdev(wq);
>   		put_device(conf_dev);
> +		kfree(wq);
> +
>   	}
> +	bitmap_free(idxd->wq_enable_map);
> +
> +err_bitmap:
> +	kfree(idxd->wqs);
> +
>   	return rc;
>   }
>   

Thanks.


-Fenghua

Re: [PATCH v2 1/7] dmaengine: idxd: fix memory leak in error handling path of idxd_setup_wqs

[Shuai Xue]

在 2025/2/19 00:32, Fenghua Yu 写道:
> Hi, Shuai,
> 
> On 2/14/25 21:44, Shuai Xue wrote:
>> Memory allocated for wqs is not freed if an error occurs during
>> idxd_setup_wqs(). To fix it, free the allocated memory in the reverse
>> order of allocation before exiting the function in case of an error.
>>
>> Fixes: a8563a33a5e2 ("dmanegine: idxd: reformat opcap output to match bitmap_parse() input")
>> Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com>
>> Reviewed-by: Dave Jiang <dave.jiang@intel.com>
>> ---
>>   drivers/dma/idxd/init.c | 20 +++++++++++++++++---
>>   1 file changed, 17 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c
>> index b946f78f85e1..b85736fd25bd 100644
>> --- a/drivers/dma/idxd/init.c
>> +++ b/drivers/dma/idxd/init.c
>> @@ -169,8 +169,8 @@ static int idxd_setup_wqs(struct idxd_device *idxd)
>>       idxd->wq_enable_map = bitmap_zalloc_node(idxd->max_wqs, GFP_KERNEL, dev_to_node(dev));
>>       if (!idxd->wq_enable_map) {
>> -        kfree(idxd->wqs);
>> -        return -ENOMEM;
>> +        rc = -ENOMEM;
>> +        goto err_bitmap;
>>       }
>>       for (i = 0; i < idxd->max_wqs; i++) {
>> @@ -191,6 +191,7 @@ static int idxd_setup_wqs(struct idxd_device *idxd)
>>           rc = dev_set_name(conf_dev, "wq%d.%d", idxd->id, wq->id);
>>           if (rc < 0) {
>>               put_device(conf_dev);
>> +            kfree(wq);
>>               goto err;
>>           }
>> @@ -204,6 +205,7 @@ static int idxd_setup_wqs(struct idxd_device *idxd)
>>           wq->wqcfg = kzalloc_node(idxd->wqcfg_size, GFP_KERNEL, dev_to_node(dev));
>>           if (!wq->wqcfg) {
>>               put_device(conf_dev);
>> +            kfree(wq);
>>               rc = -ENOMEM;
>>               goto err;
>>           }
>> @@ -211,7 +213,9 @@ static int idxd_setup_wqs(struct idxd_device *idxd)
>>           if (idxd->hw.wq_cap.op_config) {
>>               wq->opcap_bmap = bitmap_zalloc(IDXD_MAX_OPCAP_BITS, GFP_KERNEL);
>>               if (!wq->opcap_bmap) {
>> +                kfree(wq->wqcfg);
>>                   put_device(conf_dev);
>> +                kfree(wq);
>>                   rc = -ENOMEM;
>>                   goto err;
>>               }
>> @@ -225,11 +229,21 @@ static int idxd_setup_wqs(struct idxd_device *idxd)
>>       return 0;
>>    err:
>> -    while (--i >= 0) {
>> +    while (i-- > 0) {
> 
> Why changed to "i-- > 0" here? Before coming to here, the mem areas allocated for wqs[i] are freed already and there is not need to free them again here, right? 

Yes.

> And if i>1, mem areas for wqs[0] won't be freed and will leak, right?

No, the two ways of writing are equivalent.

#include <stdio.h>

int main()
{
     int i = 1;
     while (i-- > 0)
         printf("freeing i %d\n", i);

     return 0;
}

// console output
// freeing i 0

I will drop this line to avoid confusion.

Thanks.
Shuai

[PATCH v2 2/7] dmaengine: idxd: fix memory leak in error handling path of idxd_setup_engines

[Shuai Xue]

Memory allocated for engines is not freed if an error occurs during
idxd_setup_engines(). To fix it, free the allocated memory in the
reverse order of allocation before exiting the function in case of an
error.

Fixes: 75b911309060 ("dmaengine: idxd: fix engine conf_dev lifetime")
Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com>
Reviewed-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/dma/idxd/init.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c
index b85736fd25bd..4e47075c5bef 100644
--- a/drivers/dma/idxd/init.c
+++ b/drivers/dma/idxd/init.c
@@ -277,6 +277,7 @@ static int idxd_setup_engines(struct idxd_device *idxd)
 		rc = dev_set_name(conf_dev, "engine%d.%d", idxd->id, engine->id);
 		if (rc < 0) {
 			put_device(conf_dev);
+			kfree(engine);
 			goto err;
 		}
 
@@ -290,7 +291,10 @@ static int idxd_setup_engines(struct idxd_device *idxd)
 		engine = idxd->engines[i];
 		conf_dev = engine_confdev(engine);
 		put_device(conf_dev);
+		kfree(engine);
 	}
+	kfree(idxd->engines);
+
 	return rc;
 }
 
-- 
2.39.3

Re: [PATCH v2 2/7] dmaengine: idxd: fix memory leak in error handling path of idxd_setup_engines

[Fenghua Yu]

On 2/14/25 21:44, Shuai Xue wrote:
> Memory allocated for engines is not freed if an error occurs during
> idxd_setup_engines(). To fix it, free the allocated memory in the
> reverse order of allocation before exiting the function in case of an
> error.
>
> Fixes: 75b911309060 ("dmaengine: idxd: fix engine conf_dev lifetime")
> Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com>
> Reviewed-by: Dave Jiang <dave.jiang@intel.com>

Reviewed-by: Fenghua Yu <fenghuay@nvidia.com>

> ---
>   drivers/dma/idxd/init.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c
> index b85736fd25bd..4e47075c5bef 100644
> --- a/drivers/dma/idxd/init.c
> +++ b/drivers/dma/idxd/init.c
> @@ -277,6 +277,7 @@ static int idxd_setup_engines(struct idxd_device *idxd)
>   		rc = dev_set_name(conf_dev, "engine%d.%d", idxd->id, engine->id);
>   		if (rc < 0) {
>   			put_device(conf_dev);
> +			kfree(engine);
>   			goto err;
>   		}
>   
> @@ -290,7 +291,10 @@ static int idxd_setup_engines(struct idxd_device *idxd)
>   		engine = idxd->engines[i];
>   		conf_dev = engine_confdev(engine);
>   		put_device(conf_dev);
> +		kfree(engine);
>   	}
> +	kfree(idxd->engines);
> +
>   	return rc;
>   }
>   


Thanks.


-Fenghua

[PATCH v2 3/7] dmaengine: idxd: fix memory leak in error handling path of idxd_setup_groups

[Shuai Xue]

Memory allocated for groups is not freed if an error occurs during
idxd_setup_groups(). To fix it, free the allocated memory in the reverse
order of allocation before exiting the function in case of an error.

Fixes: defe49f96012 ("dmaengine: idxd: fix group conf_dev lifetime")
Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com>
Reviewed-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/dma/idxd/init.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c
index 4e47075c5bef..a2da68e6144d 100644
--- a/drivers/dma/idxd/init.c
+++ b/drivers/dma/idxd/init.c
@@ -328,6 +328,7 @@ static int idxd_setup_groups(struct idxd_device *idxd)
 		rc = dev_set_name(conf_dev, "group%d.%d", idxd->id, group->id);
 		if (rc < 0) {
 			put_device(conf_dev);
+			kfree(group);
 			goto err;
 		}
 
@@ -352,7 +353,10 @@ static int idxd_setup_groups(struct idxd_device *idxd)
 	while (--i >= 0) {
 		group = idxd->groups[i];
 		put_device(group_confdev(group));
+		kfree(group);
 	}
+	kfree(idxd->groups);
+
 	return rc;
 }
 
-- 
2.39.3

Re: [PATCH v2 3/7] dmaengine: idxd: fix memory leak in error handling path of idxd_setup_groups

[Fenghua Yu]

Hi, Shuai,

On 2/14/25 21:44, Shuai Xue wrote:
> Memory allocated for groups is not freed if an error occurs during
> idxd_setup_groups(). To fix it, free the allocated memory in the reverse
> order of allocation before exiting the function in case of an error.
>
> Fixes: defe49f96012 ("dmaengine: idxd: fix group conf_dev lifetime")
> Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com>
> Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> ---
>   drivers/dma/idxd/init.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c
> index 4e47075c5bef..a2da68e6144d 100644
> --- a/drivers/dma/idxd/init.c
> +++ b/drivers/dma/idxd/init.c
> @@ -328,6 +328,7 @@ static int idxd_setup_groups(struct idxd_device *idxd)
>   		rc = dev_set_name(conf_dev, "group%d.%d", idxd->id, group->id);
>   		if (rc < 0) {
>   			put_device(conf_dev);
> +			kfree(group);
>   			goto err;
>   		}
>   
> @@ -352,7 +353,10 @@ static int idxd_setup_groups(struct idxd_device *idxd)
>   	while (--i >= 0) {
>   		group = idxd->groups[i];
>   		put_device(group_confdev(group));
> +		kfree(group);
>   	}
> +	kfree(idxd->groups);
> +

What happens to the memory areas previously allocated for wqs and 
engines after idxd_setup_groups() fails? They need to be freed as well, 
but currently they are not.

Maybe a separate patch cleans up the previously allocated mem areas for 
wqs/engines/groups if there is any failure after the allocations?



>   	return rc;
>   }
>   

Thanks.


-Fenghua

Re: [PATCH v2 3/7] dmaengine: idxd: fix memory leak in error handling path of idxd_setup_groups

[Shuai Xue]

在 2025/2/19 04:20, Fenghua Yu 写道:
> Hi, Shuai,
> 
> On 2/14/25 21:44, Shuai Xue wrote:
>> Memory allocated for groups is not freed if an error occurs during
>> idxd_setup_groups(). To fix it, free the allocated memory in the reverse
>> order of allocation before exiting the function in case of an error.
>>
>> Fixes: defe49f96012 ("dmaengine: idxd: fix group conf_dev lifetime")
>> Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com>
>> Reviewed-by: Dave Jiang <dave.jiang@intel.com>
>> ---
>>   drivers/dma/idxd/init.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c
>> index 4e47075c5bef..a2da68e6144d 100644
>> --- a/drivers/dma/idxd/init.c
>> +++ b/drivers/dma/idxd/init.c
>> @@ -328,6 +328,7 @@ static int idxd_setup_groups(struct idxd_device *idxd)
>>           rc = dev_set_name(conf_dev, "group%d.%d", idxd->id, group->id);
>>           if (rc < 0) {
>>               put_device(conf_dev);
>> +            kfree(group);
>>               goto err;
>>           }
>> @@ -352,7 +353,10 @@ static int idxd_setup_groups(struct idxd_device *idxd)
>>       while (--i >= 0) {
>>           group = idxd->groups[i];
>>           put_device(group_confdev(group));
>> +        kfree(group);
>>       }
>> +    kfree(idxd->groups);
>> +
> 
> What happens to the memory areas previously allocated for wqs and engines after idxd_setup_groups() fails? They need to be freed as well, but currently they are not.
> 
> Maybe a separate patch cleans up the previously allocated mem areas for wqs/engines/groups if there is any failure after the allocations?
> 

Agreed, will add a new patch to fix it.

Thanks
Shuai

[PATCH v2 4/7] dmaengine: idxd: fix memory leak in error handling path of idxd_alloc

[Shuai Xue]

Memory allocated for idxd is not freed if an error occurs during
idxd_alloc(). To fix it, free the allocated memory in the reverse order
of allocation before exiting the function in case of an error.

Fixes: a8563a33a5e2 ("dmanegine: idxd: reformat opcap output to match bitmap_parse() input")
Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com>
Reviewed-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/dma/idxd/init.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c
index a2da68e6144d..dc34830fe7c3 100644
--- a/drivers/dma/idxd/init.c
+++ b/drivers/dma/idxd/init.c
@@ -567,28 +567,34 @@ static struct idxd_device *idxd_alloc(struct pci_dev *pdev, struct idxd_driver_d
 	idxd_dev_set_type(&idxd->idxd_dev, idxd->data->type);
 	idxd->id = ida_alloc(&idxd_ida, GFP_KERNEL);
 	if (idxd->id < 0)
-		return NULL;
+		goto err_ida;
 
 	idxd->opcap_bmap = bitmap_zalloc_node(IDXD_MAX_OPCAP_BITS, GFP_KERNEL, dev_to_node(dev));
-	if (!idxd->opcap_bmap) {
-		ida_free(&idxd_ida, idxd->id);
-		return NULL;
-	}
+	if (!idxd->opcap_bmap)
+		goto err_opcap;
 
 	device_initialize(conf_dev);
 	conf_dev->parent = dev;
 	conf_dev->bus = &dsa_bus_type;
 	conf_dev->type = idxd->data->dev_type;
 	rc = dev_set_name(conf_dev, "%s%d", idxd->data->name_prefix, idxd->id);
-	if (rc < 0) {
-		put_device(conf_dev);
-		return NULL;
-	}
+	if (rc < 0)
+		goto err_name;
 
 	spin_lock_init(&idxd->dev_lock);
 	spin_lock_init(&idxd->cmd_lock);
 
 	return idxd;
+
+err_name:
+	put_device(conf_dev);
+	bitmap_free(idxd->opcap_bmap);
+err_opcap:
+	ida_free(&idxd_ida, idxd->id);
+err_ida:
+	kfree(idxd);
+
+	return NULL;
 }
 
 static int idxd_enable_system_pasid(struct idxd_device *idxd)
-- 
2.39.3

[PATCH v2 5/7] dmaengine: idxd: fix memory leak in error handling path of idxd_pci_probe

[Shuai Xue]

Memory allocated for idxd is not freed if an error occurs during
idxd_pci_probe(). To fix it, free the allocated memory in the reverse
order of allocation before exiting the function in case of an error.

Fixes: bfe1d56091c1 ("dmaengine: idxd: Init and probe for Intel data accelerators")
Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com>
Reviewed-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/dma/idxd/init.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c
index dc34830fe7c3..ac1cdc1d82bf 100644
--- a/drivers/dma/idxd/init.c
+++ b/drivers/dma/idxd/init.c
@@ -550,6 +550,14 @@ static void idxd_read_caps(struct idxd_device *idxd)
 		idxd->hw.iaa_cap.bits = ioread64(idxd->reg_base + IDXD_IAACAP_OFFSET);
 }
 
+static void idxd_free(struct idxd_device *idxd)
+{
+	put_device(idxd_confdev(idxd));
+	bitmap_free(idxd->opcap_bmap);
+	ida_free(&idxd_ida, idxd->id);
+	kfree(idxd);
+}
+
 static struct idxd_device *idxd_alloc(struct pci_dev *pdev, struct idxd_driver_data *data)
 {
 	struct device *dev = &pdev->dev;
@@ -1219,7 +1227,7 @@ int idxd_pci_probe_alloc(struct idxd_device *idxd, struct pci_dev *pdev,
  err:
 	pci_iounmap(pdev, idxd->reg_base);
  err_iomap:
-	put_device(idxd_confdev(idxd));
+	idxd_free(idxd);
  err_idxd_alloc:
 	pci_disable_device(pdev);
 	return rc;
-- 
2.39.3

Re: [PATCH v2 5/7] dmaengine: idxd: fix memory leak in error handling path of idxd_pci_probe

[Fenghua Yu]

Hi, Shuai,

On 2/14/25 21:44, Shuai Xue wrote:
> Memory allocated for idxd is not freed if an error occurs during
> idxd_pci_probe(). To fix it, free the allocated memory in the reverse
> order of allocation before exiting the function in case of an error.
>
> Fixes: bfe1d56091c1 ("dmaengine: idxd: Init and probe for Intel data accelerators")
> Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com>
> Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> ---
>   drivers/dma/idxd/init.c | 10 +++++++++-
>   1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c
> index dc34830fe7c3..ac1cdc1d82bf 100644
> --- a/drivers/dma/idxd/init.c
> +++ b/drivers/dma/idxd/init.c
> @@ -550,6 +550,14 @@ static void idxd_read_caps(struct idxd_device *idxd)
>   		idxd->hw.iaa_cap.bits = ioread64(idxd->reg_base + IDXD_IAACAP_OFFSET);
>   }
>   
> +static void idxd_free(struct idxd_device *idxd)
> +{
> +	put_device(idxd_confdev(idxd));
> +	bitmap_free(idxd->opcap_bmap);
> +	ida_free(&idxd_ida, idxd->id);
> +	kfree(idxd);

opcap_bmap, idxd_ida, idxd are NOT allocated during FLR re-init idxd 
device. In the FLR case, they should not be freed.

> +}
> +
>   static struct idxd_device *idxd_alloc(struct pci_dev *pdev, struct idxd_driver_data *data)
>   {
>   	struct device *dev = &pdev->dev;
> @@ -1219,7 +1227,7 @@ int idxd_pci_probe_alloc(struct idxd_device *idxd, struct pci_dev *pdev,
>    err:
>   	pci_iounmap(pdev, idxd->reg_base);
>    err_iomap:
> -	put_device(idxd_confdev(idxd));
> +	idxd_free(idxd);
>    err_idxd_alloc:
>   	pci_disable_device(pdev);
>   	return rc;


Thanks.


-Fenghua

Re: [PATCH v2 5/7] dmaengine: idxd: fix memory leak in error handling path of idxd_pci_probe

[Shuai Xue]

在 2025/2/19 04:21, Fenghua Yu 写道:
> Hi, Shuai,
> 
> On 2/14/25 21:44, Shuai Xue wrote:
>> Memory allocated for idxd is not freed if an error occurs during
>> idxd_pci_probe(). To fix it, free the allocated memory in the reverse
>> order of allocation before exiting the function in case of an error.
>>
>> Fixes: bfe1d56091c1 ("dmaengine: idxd: Init and probe for Intel data accelerators")
>> Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com>
>> Reviewed-by: Dave Jiang <dave.jiang@intel.com>
>> ---
>>   drivers/dma/idxd/init.c | 10 +++++++++-
>>   1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c
>> index dc34830fe7c3..ac1cdc1d82bf 100644
>> --- a/drivers/dma/idxd/init.c
>> +++ b/drivers/dma/idxd/init.c
>> @@ -550,6 +550,14 @@ static void idxd_read_caps(struct idxd_device *idxd)
>>           idxd->hw.iaa_cap.bits = ioread64(idxd->reg_base + IDXD_IAACAP_OFFSET);
>>   }
>> +static void idxd_free(struct idxd_device *idxd)
>> +{
>> +    put_device(idxd_confdev(idxd));
>> +    bitmap_free(idxd->opcap_bmap);
>> +    ida_free(&idxd_ida, idxd->id);
>> +    kfree(idxd);
> 
> opcap_bmap, idxd_ida, idxd are NOT allocated during FLR re-init idxd device. In the FLR case, they should not be freed.

Great catch, thanks.

Will fix it in next version.

Shuai

Re: [PATCH v2 5/7] dmaengine: idxd: fix memory leak in error handling path of idxd_pci_probe

[Shuai Xue]

在 2025/2/19 04:21, Fenghua Yu 写道:
> Hi, Shuai,
> 
> On 2/14/25 21:44, Shuai Xue wrote:
>> Memory allocated for idxd is not freed if an error occurs during
>> idxd_pci_probe(). To fix it, free the allocated memory in the reverse
>> order of allocation before exiting the function in case of an error.
>>
>> Fixes: bfe1d56091c1 ("dmaengine: idxd: Init and probe for Intel data accelerators")
>> Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com>
>> Reviewed-by: Dave Jiang <dave.jiang@intel.com>
>> ---
>>   drivers/dma/idxd/init.c | 10 +++++++++-
>>   1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c
>> index dc34830fe7c3..ac1cdc1d82bf 100644
>> --- a/drivers/dma/idxd/init.c
>> +++ b/drivers/dma/idxd/init.c
>> @@ -550,6 +550,14 @@ static void idxd_read_caps(struct idxd_device *idxd)
>>           idxd->hw.iaa_cap.bits = ioread64(idxd->reg_base + IDXD_IAACAP_OFFSET);
>>   }
>> +static void idxd_free(struct idxd_device *idxd)
>> +{
>> +    put_device(idxd_confdev(idxd));
>> +    bitmap_free(idxd->opcap_bmap);
>> +    ida_free(&idxd_ida, idxd->id);
>> +    kfree(idxd);
> 
> opcap_bmap, idxd_ida, idxd are NOT allocated during FLR re-init idxd device. In the FLR case, they should not be freed.
> 

After relook the code, idxd_free() will not be called in FLR case,
because all error lable in idxd_pci_probe_alloc() is called from
alloc_idxd=true, not the FLR case.

Anyway, I will add a protection in idxd_free().

Thanks.
Shuai

[PATCH v2 6/7] dmaengine: idxd: Add missing idxd cleanup to fix memory leak in remove call

[Shuai Xue]

The remove call stack is missing idxd cleanup to free bitmap, ida and
the idxd_device. Call idxd_free() helper routines to make sure we exit
gracefully.

Fixes: bfe1d56091c1 ("dmaengine: idxd: Init and probe for Intel data accelerators")
Suggested-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com>
---
 drivers/dma/idxd/init.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c
index ac1cdc1d82bf..f40f1c44a302 100644
--- a/drivers/dma/idxd/init.c
+++ b/drivers/dma/idxd/init.c
@@ -1295,7 +1295,7 @@ static void idxd_remove(struct pci_dev *pdev)
 	pci_disable_device(pdev);
 	destroy_workqueue(idxd->wq);
 	perfmon_pmu_remove(idxd);
-	put_device(idxd_confdev(idxd));
+	idxd_free(idxd);
 }
 
 static struct pci_driver idxd_pci_driver = {
-- 
2.39.3

[PATCH v2 7/7] dmaengine: idxd: Refactor remove call with idxd_cleanup() helper

[Shuai Xue]

The idxd_cleanup() helper clean up perfmon, interrupts, internals and so
on. Refactor remove call with idxd_cleanup() helper to avoid code
duplication. Note, this also fixes the missing put_device() for idxd
groups, enginces and wqs.

Fixes: bfe1d56091c1 ("dmaengine: idxd: Init and probe for Intel data accelerators")
Suggested-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com>
---
 drivers/dma/idxd/init.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c
index f40f1c44a302..0fbfbe024c29 100644
--- a/drivers/dma/idxd/init.c
+++ b/drivers/dma/idxd/init.c
@@ -1282,20 +1282,11 @@ static void idxd_remove(struct pci_dev *pdev)
 	get_device(idxd_confdev(idxd));
 	device_unregister(idxd_confdev(idxd));
 	idxd_shutdown(pdev);
-	if (device_pasid_enabled(idxd))
-		idxd_disable_system_pasid(idxd);
 	idxd_device_remove_debugfs(idxd);
-
-	irq_entry = idxd_get_ie(idxd, 0);
-	free_irq(irq_entry->vector, irq_entry);
-	pci_free_irq_vectors(pdev);
+	idxd_cleanup(idxd);
 	pci_iounmap(pdev, idxd->reg_base);
-	if (device_user_pasid_enabled(idxd))
-		idxd_disable_sva(pdev);
-	pci_disable_device(pdev);
-	destroy_workqueue(idxd->wq);
-	perfmon_pmu_remove(idxd);
 	idxd_free(idxd);
+	pci_disable_device(pdev);
 }
 
 static struct pci_driver idxd_pci_driver = {
-- 
2.39.3

Re: [PATCH v2 7/7] dmaengine: idxd: Refactor remove call with idxd_cleanup() helper

[Fenghua Yu]

Hi, Shuai,

On 2/14/25 21:44, Shuai Xue wrote:
> The idxd_cleanup() helper clean up perfmon, interrupts, internals and so

s/clean/cleans/


> on. Refactor remove call with idxd_cleanup() helper to avoid code
s/idxd_cleanup()/the idxd_cleanup()/
> duplication. Note, this also fixes the missing put_device() for idxd
> groups, enginces and wqs.
>
> Fixes: bfe1d56091c1 ("dmaengine: idxd: Init and probe for Intel data accelerators")
> Suggested-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
> Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com>
> ---
>   drivers/dma/idxd/init.c | 13 ++-----------
>   1 file changed, 2 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c
> index f40f1c44a302..0fbfbe024c29 100644
> --- a/drivers/dma/idxd/init.c
> +++ b/drivers/dma/idxd/init.c
> @@ -1282,20 +1282,11 @@ static void idxd_remove(struct pci_dev *pdev)
>   	get_device(idxd_confdev(idxd));
get_device() is called here.
>   	device_unregister(idxd_confdev(idxd));
>   	idxd_shutdown(pdev);
> -	if (device_pasid_enabled(idxd))
> -		idxd_disable_system_pasid(idxd);
>   	idxd_device_remove_debugfs(idxd);
> -
> -	irq_entry = idxd_get_ie(idxd, 0);
> -	free_irq(irq_entry->vector, irq_entry);
> -	pci_free_irq_vectors(pdev);
> +	idxd_cleanup(idxd);
>   	pci_iounmap(pdev, idxd->reg_base);
> -	if (device_user_pasid_enabled(idxd))
> -		idxd_disable_sva(pdev);
> -	pci_disable_device(pdev);
> -	destroy_workqueue(idxd->wq);
> -	perfmon_pmu_remove(idxd);
>   	idxd_free(idxd);

put_device() is called inside idxd_free(). Seems not easy to read code 
to match the pair.

Plus idxd_free() is called only in non FLR case.

Maybe it's better to change this code to:

1. call put_device() outside idxd_free() so that it's easy to match the 
get_device() and put_deivce in the same level of function.

2. idxd_free() called here is OK because this is not in FLR handler. But 
only call it in non FLR path in idxd_pci_probe_alloc()

?

> +	pci_disable_device(pdev);
>   }
>   
>   static struct pci_driver idxd_pci_driver = {

Thanks.


-Fenghua

Re: [PATCH v2 7/7] dmaengine: idxd: Refactor remove call with idxd_cleanup() helper

[Shuai Xue]

在 2025/2/19 05:01, Fenghua Yu 写道:
> Hi, Shuai,
> 
> On 2/14/25 21:44, Shuai Xue wrote:
>> The idxd_cleanup() helper clean up perfmon, interrupts, internals and so
> 
> s/clean/cleans/
> 
> 
>> on. Refactor remove call with idxd_cleanup() helper to avoid code
> s/idxd_cleanup()/the idxd_cleanup()/
>> duplication. Note, this also fixes the missing put_device() for idxd
>> groups, enginces and wqs.
>>
>> Fixes: bfe1d56091c1 ("dmaengine: idxd: Init and probe for Intel data accelerators")
>> Suggested-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
>> Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com>
>> ---
>>   drivers/dma/idxd/init.c | 13 ++-----------
>>   1 file changed, 2 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c
>> index f40f1c44a302..0fbfbe024c29 100644
>> --- a/drivers/dma/idxd/init.c
>> +++ b/drivers/dma/idxd/init.c
>> @@ -1282,20 +1282,11 @@ static void idxd_remove(struct pci_dev *pdev)
>>       get_device(idxd_confdev(idxd));
> get_device() is called here.
>>       device_unregister(idxd_confdev(idxd));
>>       idxd_shutdown(pdev);
>> -    if (device_pasid_enabled(idxd))
>> -        idxd_disable_system_pasid(idxd);
>>       idxd_device_remove_debugfs(idxd);
>> -
>> -    irq_entry = idxd_get_ie(idxd, 0);
>> -    free_irq(irq_entry->vector, irq_entry);
>> -    pci_free_irq_vectors(pdev);
>> +    idxd_cleanup(idxd);
>>       pci_iounmap(pdev, idxd->reg_base);
>> -    if (device_user_pasid_enabled(idxd))
>> -        idxd_disable_sva(pdev);
>> -    pci_disable_device(pdev);
>> -    destroy_workqueue(idxd->wq);
>> -    perfmon_pmu_remove(idxd);
>>       idxd_free(idxd);
> 
> put_device() is called inside idxd_free(). Seems not easy to read code to match the pair.

IMHO, idxd_free() is paired with idxd_alloc() which grap a reference count by
device_initialize(). So, we should match that right pair.

> * When ->release() is called for the idxd->conf_dev, it frees all the memory related
> * to the idxd context.

I did not figure out why you explictly grab reference count of
idxd_confdev(idxd).

idxd_unregister_devices() is paired with idxd_register_devices(), it only
decrease reference through wqs, engines, and groups. So a refcnt of
idxd->conf_dev is still hold by idxd_alloc().

Please correct me, if I missed anything.

> 
> Plus idxd_free() is called only in non FLR case.
> 
> Maybe it's better to change this code to:
> 
> 1. call put_device() outside idxd_free() so that it's easy to match the get_device() and put_deivce in the same level of function.

See my comments above.
> 
> 2. idxd_free() called here is OK because this is not in FLR handler. But only call it in non FLR path in idxd_pci_probe_alloc()
> 

Exactly, so, shoud I add a protection in idxd_free() in a fact that non FLR case will not call it.

Thanks.
Shuai

Re: [PATCH v2 7/7] dmaengine: idxd: Refactor remove call with idxd_cleanup() helper

[Shuai Xue]

在 2025/2/19 21:28, Shuai Xue 写道:
> 
> 
> 在 2025/2/19 05:01, Fenghua Yu 写道:
>> Hi, Shuai,
>>
>> On 2/14/25 21:44, Shuai Xue wrote:
>>> The idxd_cleanup() helper clean up perfmon, interrupts, internals and so
>>
>> s/clean/cleans/
>>
>>
>>> on. Refactor remove call with idxd_cleanup() helper to avoid code
>> s/idxd_cleanup()/the idxd_cleanup()/
>>> duplication. Note, this also fixes the missing put_device() for idxd
>>> groups, enginces and wqs.
>>>
>>> Fixes: bfe1d56091c1 ("dmaengine: idxd: Init and probe for Intel data accelerators")
>>> Suggested-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
>>> Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com>
>>> ---
>>>   drivers/dma/idxd/init.c | 13 ++-----------
>>>   1 file changed, 2 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c
>>> index f40f1c44a302..0fbfbe024c29 100644
>>> --- a/drivers/dma/idxd/init.c
>>> +++ b/drivers/dma/idxd/init.c
>>> @@ -1282,20 +1282,11 @@ static void idxd_remove(struct pci_dev *pdev)
>>>       get_device(idxd_confdev(idxd));
>> get_device() is called here.
>>>       device_unregister(idxd_confdev(idxd));
>>>       idxd_shutdown(pdev);
>>> -    if (device_pasid_enabled(idxd))
>>> -        idxd_disable_system_pasid(idxd);
>>>       idxd_device_remove_debugfs(idxd);
>>> -
>>> -    irq_entry = idxd_get_ie(idxd, 0);
>>> -    free_irq(irq_entry->vector, irq_entry);
>>> -    pci_free_irq_vectors(pdev);
>>> +    idxd_cleanup(idxd);
>>>       pci_iounmap(pdev, idxd->reg_base);
>>> -    if (device_user_pasid_enabled(idxd))
>>> -        idxd_disable_sva(pdev);
>>> -    pci_disable_device(pdev);
>>> -    destroy_workqueue(idxd->wq);
>>> -    perfmon_pmu_remove(idxd);
>>>       idxd_free(idxd);
>>
>> put_device() is called inside idxd_free(). Seems not easy to read code to match the pair.
> 
> IMHO, idxd_free() is paired with idxd_alloc() which grap a reference count by
> device_initialize(). So, we should match that right pair.
> 
>> * When ->release() is called for the idxd->conf_dev, it frees all the memory related
>> * to the idxd context.
> 
> I did not figure out why you explictly grab reference count of
> idxd_confdev(idxd).
> 
> idxd_unregister_devices() is paired with idxd_register_devices(), it only
> decrease reference through wqs, engines, and groups. So a refcnt of
> idxd->conf_dev is still hold by idxd_alloc().
> 
> Please correct me, if I missed anything.
> 
>>
>> Plus idxd_free() is called only in non FLR case.
>>
>> Maybe it's better to change this code to:
>>
>> 1. call put_device() outside idxd_free() so that it's easy to match the get_device() and put_deivce in the same level of function.
> 
> See my comments above.
>>
>> 2. idxd_free() called here is OK because this is not in FLR handler. But only call it in non FLR path in idxd_pci_probe_alloc()
>>
> 
> Exactly, so, shoud I add a protection in idxd_free() in a fact that non FLR case will not call it.
> 
> Thanks.
> Shuai

Hi, all，

Any feedback?

Thanks.
Shuai