Re: [PATCH v3 21/25] dax/region: Create resources on sparse DAX regions

[Ira Weiny]

Dave Jiang wrote:
> 
> 
> On 8/16/24 7:44 AM, ira.we...@intel.com wrote:
> > From: Navneet Singh <navneet.si...@intel.com>
> > 

[snip]

> > diff --git a/drivers/cxl/core/extent.c b/drivers/cxl/core/extent.c
> > index d7d526a51e2b..103b0bec3a4a 100644
> > --- a/drivers/cxl/core/extent.c
> > +++ b/drivers/cxl/core/extent.c
> > @@ -271,20 +271,67 @@ static void calc_hpa_range(struct 
> > cxl_endpoint_decoder *cxled,
> >     hpa_range->end = hpa_range->start + range_len(dpa_range) - 1;
> >  }
> >  
> > +static int cxlr_notify_extent(struct cxl_region *cxlr, enum dc_event event,
> > +                         struct region_extent *region_extent)
> > +{
> > +   struct cxl_dax_region *cxlr_dax;
> > +   struct device *dev;
> > +   int rc = 0;
> > +
> > +   cxlr_dax = cxlr->cxlr_dax;
> > +   dev = &cxlr_dax->dev;
> > +   dev_dbg(dev, "Trying notify: type %d HPA %par\n",
> > +           event, &region_extent->hpa_range);
> > +
> > +   /*
> > +    * NOTE the lack of a driver indicates a notification has failed.  No
> > +    * user space coordiantion was possible.
> > +    */
> > +   device_lock(dev);
> > +   if (dev->driver) {
> > +           struct cxl_driver *driver = to_cxl_drv(dev->driver);
> > +           struct cxl_notify_data notify_data = (struct cxl_notify_data) {
> > +                   .event = event,
> > +                   .region_extent = region_extent,
> > +           };
> > +
> > +           if (driver->notify) {
> > +                   dev_dbg(dev, "Notify: type %d HPA %par\n",
> > +                           event, &region_extent->hpa_range);
> > +                   rc = driver->notify(dev, &notify_data);
> > +           }
> > +   }
> > +   device_unlock(dev);
> 
> Maybe a cleaner version:
>       guard(device)(dev);
>       if (!dev->driver || !dev->driver->notify)
>               return 0;

There is no dev->driver->notify.  But this works.

        if (!dev->driver)
                return 0;
        driver = to_cxl_drv(dev->driver);
        if (!driver->notify)
                return 0;

Not quite as clean but I did miss the use of guard.

> 
>       dev_dbg(...);
>       return driver->notify(dev, &notify_data);
>

I've cleaned it up.

[snip]

> > +
> > +int dax_region_add_resource(struct dax_region *dax_region,
> > +                       struct device *device,
> > +                       resource_size_t start, resource_size_t length)
> >
> kdoc header?

Because dax_region_add_resource() is part of the DAX private interfaces
and not intended to be used outside the DAX subsystem I skipped the kdoc
here even though the function must be exported.  Same for
dax_region_rm_resource() and dax_avail_size().

This is similar to run_dax() in that it is designed as a 'generic
operation' within the dax subsystem but not generally useful to the
kernel.

For now I'll move their declarations in dax-private.h and make a similar
comment.

Ira

> 
>  +{
> > +   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 */
> > +   if (rc)
> > +           dev_set_drvdata(device, NULL);
> > +   return rc;
> > +}
> > +EXPORT_SYMBOL_GPL(dax_region_add_resource);
> > +
> > +int dax_region_rm_resource(struct dax_region *dax_region,
> > +                      struct device *dev)
> 
> kdoc header
> > +{
> > +   struct dax_resource *dax_resource;
> > +
> > +   guard(rwsem_write)(&dax_region_rwsem);
> > +
> > +   dax_resource = dev_get_drvdata(dev);
> > +   if (!dax_resource)
> > +           return 0;
> > +
> > +   if (dax_resource->use_cnt)
> > +           return -EBUSY;
> > +
> > +   /* avoid races with users trying to use the extent */
> > +   __dax_release_resource(dax_resource);
> > +   return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(dax_region_rm_resource);
> > +
> >  bool static_dev_dax(struct dev_dax *dev_dax)
> >  {
> >     return is_static(dev_dax->region);
> > @@ -296,19 +373,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)
> > +
> > +resource_size_t
> > +dax_avail_size(struct resource *dax_resource)
> kdoc header
> 
> DJ
> 

[snip]