1. Patchew
  2. linux
  3. [PATCH v7 0/7] dax/hmem, cxl: Coordinate Soft Reserved handling with CXL and HMEM
  4. View patch

[PATCH v7 4/7] dax: Track all dax_region allocations under a global resource tree

Smita Koralahalli posted 7 patches 2 weeks, 4 days ago
There is a newer version of this series
[PATCH v7 4/7] dax: Track all dax_region allocations under a global resource tree
Posted by Smita Koralahalli 2 weeks, 4 days ago 
Introduce a global "DAX Regions" resource root and register each
dax_region->res under it via request_resource(). Release the resource on
dax_region teardown.

By enforcing a single global namespace for dax_region allocations, this
ensures only one of dax_hmem or dax_cxl can successfully register a
dax_region for a given range.

Suggested-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>
---
 drivers/dax/bus.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/drivers/dax/bus.c b/drivers/dax/bus.c
index c94c09622516..448e2bc285c3 100644
--- a/drivers/dax/bus.c
+++ b/drivers/dax/bus.c
@@ -10,6 +10,7 @@
 #include "dax-private.h"
 #include "bus.h"
 
+static struct resource dax_regions = DEFINE_RES_MEM_NAMED(0, -1, "DAX Regions");
 static DEFINE_MUTEX(dax_bus_lock);
 
 /*
@@ -625,6 +626,7 @@ static void dax_region_unregister(void *region)
 {
 	struct dax_region *dax_region = region;
 
+	release_resource(&dax_region->res);
 	sysfs_remove_groups(&dax_region->dev->kobj,
 			dax_region_attribute_groups);
 	dax_region_put(dax_region);
@@ -635,6 +637,7 @@ struct dax_region *alloc_dax_region(struct device *parent, int region_id,
 		unsigned long flags)
 {
 	struct dax_region *dax_region;
+	int rc;
 
 	/*
 	 * The DAX core assumes that it can store its private data in
@@ -667,14 +670,25 @@ struct dax_region *alloc_dax_region(struct device *parent, int region_id,
 		.flags = IORESOURCE_MEM | flags,
 	};
 
-	if (sysfs_create_groups(&parent->kobj, dax_region_attribute_groups)) {
-		kfree(dax_region);
-		return NULL;
+	rc = request_resource(&dax_regions, &dax_region->res);
+	if (rc) {
+		dev_dbg(parent, "dax_region resource conflict for %pR\n",
+			&dax_region->res);
+		goto err_res;
 	}
 
+	if (sysfs_create_groups(&parent->kobj, dax_region_attribute_groups))
+		goto err_sysfs;
+
 	if (devm_add_action_or_reset(parent, dax_region_unregister, dax_region))
 		return NULL;
 	return dax_region;
+
+err_sysfs:
+	release_resource(&dax_region->res);
+err_res:
+	kfree(dax_region);
+	return NULL;
 }
 EXPORT_SYMBOL_GPL(alloc_dax_region);
 
-- 
2.17.1
Re: [PATCH v7 4/7] dax: Track all dax_region allocations under a global resource tree
Posted by Jonathan Cameron 2 weeks, 4 days ago 
On Thu, 19 Mar 2026 01:14:57 +0000
Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com> wrote:

> Introduce a global "DAX Regions" resource root and register each
> dax_region->res under it via request_resource(). Release the resource on
> dax_region teardown.
> 
> By enforcing a single global namespace for dax_region allocations, this
> ensures only one of dax_hmem or dax_cxl can successfully register a
> dax_region for a given range.
> 
> Suggested-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>

The comment below is about the existing code.  If we decide not to tidy that
up for now and you swap the ordering of release_resource() and sysfs_remove_groups()
in unregister.

Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>

> ---
>  drivers/dax/bus.c | 20 +++++++++++++++++---
>  1 file changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/dax/bus.c b/drivers/dax/bus.c
> index c94c09622516..448e2bc285c3 100644
> --- a/drivers/dax/bus.c
> +++ b/drivers/dax/bus.c
> @@ -10,6 +10,7 @@
>  #include "dax-private.h"
>  #include "bus.h"
>  
> +static struct resource dax_regions = DEFINE_RES_MEM_NAMED(0, -1, "DAX Regions");
>  static DEFINE_MUTEX(dax_bus_lock);
>  
>  /*
> @@ -625,6 +626,7 @@ static void dax_region_unregister(void *region)
>  {
>  	struct dax_region *dax_region = region;
>  
> +	release_resource(&dax_region->res);

Should reverse the line above and the line below so we unwind in reverse of
setup.  I doubt it matters in practice today but keeping ordering like that
makes it much easier to see if a future patch messes things up.

>  	sysfs_remove_groups(&dax_region->dev->kobj,
>  			dax_region_attribute_groups);
>  	dax_region_put(dax_region);
> @@ -635,6 +637,7 @@ struct dax_region *alloc_dax_region(struct device *parent, int region_id,
>  		unsigned long flags)
>  {
>  	struct dax_region *dax_region;
> +	int rc;
>  
>  	/*
>  	 * The DAX core assumes that it can store its private data in
> @@ -667,14 +670,25 @@ struct dax_region *alloc_dax_region(struct device *parent, int region_id,
>  		.flags = IORESOURCE_MEM | flags,
>  	};
>  
> -	if (sysfs_create_groups(&parent->kobj, dax_region_attribute_groups)) {
> -		kfree(dax_region);
> -		return NULL;
> +	rc = request_resource(&dax_regions, &dax_region->res);
> +	if (rc) {
> +		dev_dbg(parent, "dax_region resource conflict for %pR\n",
> +			&dax_region->res);
> +		goto err_res;
>  	}
>  
> +	if (sysfs_create_groups(&parent->kobj, dax_region_attribute_groups))
> +		goto err_sysfs;
> +
>  	if (devm_add_action_or_reset(parent, dax_region_unregister, dax_region))

This is curious. The code flips over to a kref_put() based release but we didn't
do anything with the kref in the previous call. So whilst not 'buggy' as such
it's definitely inconsistent and we should clean it up.

This should really have been doing the release via dax_region_put() from the
kref_init().  In practice that means never calling kfree(dax_regions) error paths
because the kref_init() is just after the allocation. Instead call dax_region_put()
in all those error paths.

 

>  		return NULL;
>  	return dax_region;
> +
> +err_sysfs:
> +	release_resource(&dax_region->res);
> +err_res:
> +	kfree(dax_region);

From above I think this should be
	dax_region_put(dax_region);

> +	return NULL;
>  }
>  EXPORT_SYMBOL_GPL(alloc_dax_region);
>
Re: [PATCH v7 4/7] dax: Track all dax_region allocations under a global resource tree
Posted by Koralahalli Channabasappa, Smita 2 weeks, 2 days ago 
On 3/19/2026 6:59 AM, Jonathan Cameron wrote:
> On Thu, 19 Mar 2026 01:14:57 +0000
> Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com> wrote:
> 
>> Introduce a global "DAX Regions" resource root and register each
>> dax_region->res under it via request_resource(). Release the resource on
>> dax_region teardown.
>>
>> By enforcing a single global namespace for dax_region allocations, this
>> ensures only one of dax_hmem or dax_cxl can successfully register a
>> dax_region for a given range.
>>
>> Suggested-by: Dan Williams <dan.j.williams@intel.com>
>> Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>
> 
> The comment below is about the existing code.  If we decide not to tidy that
> up for now and you swap the ordering of release_resource() and sysfs_remove_groups()
> in unregister.

Okay I think I can do both.

> 
> Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
> 
>> ---
>>   drivers/dax/bus.c | 20 +++++++++++++++++---
>>   1 file changed, 17 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/dax/bus.c b/drivers/dax/bus.c
>> index c94c09622516..448e2bc285c3 100644
>> --- a/drivers/dax/bus.c
>> +++ b/drivers/dax/bus.c
>> @@ -10,6 +10,7 @@
>>   #include "dax-private.h"
>>   #include "bus.h"
>>   
>> +static struct resource dax_regions = DEFINE_RES_MEM_NAMED(0, -1, "DAX Regions");
>>   static DEFINE_MUTEX(dax_bus_lock);
>>   
>>   /*
>> @@ -625,6 +626,7 @@ static void dax_region_unregister(void *region)
>>   {
>>   	struct dax_region *dax_region = region;
>>   
>> +	release_resource(&dax_region->res);
> 
> Should reverse the line above and the line below so we unwind in reverse of
> setup.  I doubt it matters in practice today but keeping ordering like that
> makes it much easier to see if a future patch messes things up.

Okay.

> 
>>   	sysfs_remove_groups(&dax_region->dev->kobj,
>>   			dax_region_attribute_groups);
>>   	dax_region_put(dax_region);
>> @@ -635,6 +637,7 @@ struct dax_region *alloc_dax_region(struct device *parent, int region_id,
>>   		unsigned long flags)
>>   {
>>   	struct dax_region *dax_region;
>> +	int rc;
>>   
>>   	/*
>>   	 * The DAX core assumes that it can store its private data in
>> @@ -667,14 +670,25 @@ struct dax_region *alloc_dax_region(struct device *parent, int region_id,
>>   		.flags = IORESOURCE_MEM | flags,
>>   	};
>>   
>> -	if (sysfs_create_groups(&parent->kobj, dax_region_attribute_groups)) {
>> -		kfree(dax_region);
>> -		return NULL;
>> +	rc = request_resource(&dax_regions, &dax_region->res);
>> +	if (rc) {
>> +		dev_dbg(parent, "dax_region resource conflict for %pR\n",
>> +			&dax_region->res);
>> +		goto err_res;
>>   	}
>>   
>> +	if (sysfs_create_groups(&parent->kobj, dax_region_attribute_groups))
>> +		goto err_sysfs;
>> +
>>   	if (devm_add_action_or_reset(parent, dax_region_unregister, dax_region))
> 
> This is curious. The code flips over to a kref_put() based release but we didn't
> do anything with the kref in the previous call. So whilst not 'buggy' as such
> it's definitely inconsistent and we should clean it up.
> 
> This should really have been doing the release via dax_region_put() from the
> kref_init().  In practice that means never calling kfree(dax_regions) error paths
> because the kref_init() is just after the allocation. Instead call dax_region_put()
> in all those error paths.
> 
>   
> 
>>   		return NULL;
>>   	return dax_region;
>> +
>> +err_sysfs:
>> +	release_resource(&dax_region->res);
>> +err_res:
>> +	kfree(dax_region);
> 
>  From above I think this should be
> 	dax_region_put(dax_region);

Thank you for pointing this out. I will have a separate patch for this 
change first in the series.

Thanks
Smita

> 
>> +	return NULL;
>>   }
>>   EXPORT_SYMBOL_GPL(alloc_dax_region);
>>   
>

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.