On Thu, 19 Mar 2026 01:14:57 +0000 Smita Koralahalli <[email protected]> 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 <[email protected]> > Signed-off-by: Smita Koralahalli <[email protected]> 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 <[email protected]> > --- > 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); >
