Re: [PATCH v4 24/28] dax/region: Create resources on sparse DAX regions

[Ira Weiny <ira.weiny@intel.com>]

Jonathan Cameron wrote:
> On Mon, 07 Oct 2024 18:16:30 -0500
> ira.weiny@intel.com wrote:
> 
> > From: Navneet Singh <navneet.singh@intel.com>

[snip]

> > +static int cxlr_notify_extent(struct cxl_region *cxlr, enum dc_event event,
> > +			      struct region_extent *region_extent)
> > +{
> > +	struct device *dev = &cxlr->cxlr_dax->dev;
> > +	struct cxl_notify_data notify_data;
> > +	struct cxl_driver *driver;
> > +
> > +	dev_dbg(dev, "Trying notify: type %d HPA %pra\n",
> > +		event, &region_extent->hpa_range);
> > +
> > +	guard(device)(dev);
> > +
> > +	/*
> > +	 * The lack of a driver indicates a notification has failed.  No user
> > +	 * space coordiantion was possible.
> spell check.
> coordination

Done.


[snip]

> > +
> > +int dax_region_add_resource(struct dax_region *dax_region,
> > +			    struct device *device,
> > +			    resource_size_t start, resource_size_t length)
> > +{
> > +	struct resource *new_resource;
> > +	int rc;
> > +
> > +	struct dax_resource *dax_resource __free(kfree) =
> > +				kzalloc(sizeof(*dax_resource), GFP_KERNEL);
> > +	if (!dax_resource)
> > +		return -ENOMEM;
> > +
> > +	guard(rwsem_write)(&dax_region_rwsem);
> > +
> > +	dev_dbg(dax_region->dev, "DAX region resource %pr\n", &dax_region->res);
> > +	new_resource = __request_region(&dax_region->res, start, length, "extent", 0);
> > +	if (!new_resource) {
> > +		dev_err(dax_region->dev, "Failed to add region s:%pa l:%pa\n",
> > +			&start, &length);
> > +		return -ENOSPC;
> > +	}
> > +
> > +	dev_dbg(dax_region->dev, "add resource %pr\n", new_resource);
> > +	dax_resource->region = dax_region;
> > +	dax_resource->res = new_resource;
> > +	dev_set_drvdata(device, dax_resource);
> > +	rc = devm_add_action_or_reset(device, dax_release_resource,
> > +				      no_free_ptr(dax_resource));
> > +	/*  On error; ensure driver data is cleared under semaphore */
> 
> It's not used in the dax_release_resource callback (that I can
> immediately spot) so could you just not set it until after
> this has succeeded?
> 
> > +	if (rc)
> > +		dev_set_drvdata(device, NULL);
> i.e. move
> 	dev_set_drvdata(device, dax_resource);
> to here.

Oh boy...  I probably needed a better comment on this one.  No we can't do that
as written because no_free_ptr() was used to flag that the pointer was handed
off.  Thus at this point dax_resource is always NULL.

That said, I realize now this code has an issue with using
devm_add_action_or_reset() because on error dax_region_rwsem will be taken for
write recursively.

So I have to re-write this using devm_add_action() with a manual reset using
__dax_release_resource()...  in that case no_free_ptr() can be moved to a
better place.

All that results in something much nicer:

...
        /*
         * open code devm_add_action_or_reset() to avoid recursive write lock
         * of dax_region_rwsem in the error case.
         */
        rc = devm_add_action(device, dax_release_resource, dax_resource);
        if (rc) {
                __dax_release_resource(dax_resource);
                return rc;
        }

        dev_set_drvdata(device, no_free_ptr(dax_resource));
        return 0;
}

> 
> > +	return rc;
> > +}
> > +EXPORT_SYMBOL_GPL(dax_region_add_resource);
> Adding quite a few exports. Is it time to namespace DAX exports?
> Perhaps a follow up series.

Perhaps.  The calls have a dax_ prefix.  In addition, I thought use of the
export namespaces were out of favor?

> 
> 
> 
> >  bool static_dev_dax(struct dev_dax *dev_dax)
> >  {
> >  	return is_static(dev_dax->region);
> > @@ -296,19 +376,44 @@ static ssize_t region_align_show(struct device *dev,
> >  static struct device_attribute dev_attr_region_align =
> >  		__ATTR(align, 0400, region_align_show, NULL);
> >  
> > +#define for_each_child_resource(extent, res) \
> > +	for (res = (extent)->child; res; res = res->sibling)
> > +
> Extent naming in here is a little off for a general sounding macro.
> Maybe for_each_child_resource(parent, res) or something like that?
> 
> Seem generally useful. Maybe move to resource.h?

I could (with the name change).

I guess the self review process ended up with something generic except for the
'extent' name.

> 
> > @@ -1494,8 +1679,14 @@ static struct dev_dax *__devm_create_dev_dax(struct dev_dax_data *data)
> >  	device_initialize(dev);
> >  	dev_set_name(dev, "dax%d.%d", dax_region->id, dev_dax->id);
> >  
> > +	if (is_sparse(dax_region) && data->size) {
> > +		dev_err(parent, "Sparse DAX region devices must be created initially with 0 size");
> > +		rc = -EINVAL;
> > +		goto err_id;
> 
> Right label?  This code doesn't have side effects and the next error path is goto err_range
> Looks like you fail to reverse the alloc_dev_dax_id() in this error path.

Yea.

Worse yet I think this check could be done much earlier before dev_dax
allocation.

Let me work on that.

> 
> > +	}
> > +
> >  	rc = alloc_dev_dax_range(&dax_region->res, dev_dax, dax_region->res.start,
> > -				 data->size);
> > +				 data->size, NULL);
> >  	if (rc)
> >  		goto err_range;
> >  
> > diff --git a/drivers/dax/bus.h b/drivers/dax/bus.h
> > index 783bfeef42cc..ae5029ea6047 100644
> > --- a/drivers/dax/bus.h
> > +++ b/drivers/dax/bus.h
> > @@ -9,6 +9,7 @@ struct dev_dax;
> >  struct resource;
> >  struct dax_device;
> >  struct dax_region;
> > +struct dax_sparse_ops;
> >  
> >  /* dax bus specific ioresource flags */
> >  #define IORESOURCE_DAX_STATIC BIT(0)
> > @@ -17,7 +18,7 @@ struct dax_region;
> >  
> >  struct dax_region *alloc_dax_region(struct device *parent, int region_id,
> >  		struct range *range, int target_node, unsigned int align,
> > -		unsigned long flags);
> > +		unsigned long flags, struct dax_sparse_ops *sparse_ops);
> >  
> >  struct dev_dax_data {
> >  	struct dax_region *dax_region;
> > diff --git a/drivers/dax/cxl.c b/drivers/dax/cxl.c
> > index 367e86b1c22a..df979ea2cb59 100644
> > --- a/drivers/dax/cxl.c
> > +++ b/drivers/dax/cxl.c
> > @@ -5,6 +5,58 @@
> >  
> >  #include "../cxl/cxl.h"
> >  #include "bus.h"
> > +#include "dax-private.h"
> > +
> > +static int __cxl_dax_add_resource(struct dax_region *dax_region,
> > +				  struct region_extent *region_extent)
> > +{
> > +	resource_size_t start, length;
> > +	struct device *dev;
> > +
> > +	dev = &region_extent->dev;
> Might as well do
> 	struct device *dev = &region_extent->dev;

sure.

> 
> 
> > +	start = dax_region->res.start + region_extent->hpa_range.start;
> > +	length = range_len(&region_extent->hpa_range);
> > +	return dax_region_add_resource(dax_region, dev, start, length);
> > +}
> 
> 
> > diff --git a/drivers/dax/dax-private.h b/drivers/dax/dax-private.h
> > index ccde98c3d4e2..e3866115243e 100644
> > --- a/drivers/dax/dax-private.h
> > +++ b/drivers/dax/dax-private.h
> ...
> 
> > +/*
> > + * Similar to run_dax() dax_region_{add,rm}_resource() and dax_avail_size() are
> > + * exported but are not intended to be generic operations outside the dax
> > + * subsystem.  They are only generic between the dax layer and the dax drivers.
> > + */
> > +int dax_region_add_resource(struct dax_region *dax_region, struct device *dev,
> > +			    resource_size_t start, resource_size_t length);
> > +int dax_region_rm_resource(struct dax_region *dax_region,
> > +			   struct device *dev);
> > +resource_size_t dax_avail_size(struct resource *dax_resource);
> > +
> > +typedef int (*match_cb)(struct device *dev, resource_size_t *size_avail);
> Why is this here?
> 

Left over from a bygone implementation...  :-/

Deleted

Ira