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, ®ion_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, ®ion_extent->hpa_range); > > + rc = driver->notify(dev, ¬ify_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, ¬ify_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]