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

["Koralahalli Channabasappa, Smita" <skoralah@amd.com>]

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);
>>   
>