Jonathan Cameron wrote: > On Fri, 16 Aug 2024 09:44:29 -0500 > 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); > > I'd use guard() for this as then can just return the notify result > and drop local variable rc Yep Dave already mentioned this and it is done. > > > > + 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); > > + return rc; > > +} > > > > @@ -338,8 +390,20 @@ static int cxlr_add_extent(struct cxl_dax_region > > *cxlr_dax, > > return rc; > > } > > > > - /* device model handles freeing region_extent */ > > - return online_region_extent(region_extent); > > + rc = online_region_extent(region_extent); > > + /* device model handled freeing region_extent */ > > + if (rc) > > + return rc; > > + > > + rc = cxlr_notify_extent(cxlr_dax->cxlr, DCD_ADD_CAPACITY, > > region_extent); > > + /* > > + * The region device was breifly live but DAX layer ensures it was not > > briefly Fixed Thanks > > > + * used > > + */ > > + if (rc) > > + region_rm_extent(region_extent); > > + > > + return rc; > > } > > > diff --git a/drivers/dax/bus.c b/drivers/dax/bus.c > > index 975860371d9f..f14b0cfa7edd 100644 > > --- a/drivers/dax/bus.c > > +++ b/drivers/dax/bus.c > > > +EXPORT_SYMBOL_GPL(dax_region_add_resource); > > + > > +int dax_region_rm_resource(struct dax_region *dax_region, > > + struct device *dev) > > +{ > > + 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 */ > > Not obvious to me from local code, why does releasing the resource > here avoid a race? Perhaps the comment needs expanding. We are under the dax_region_rwsem here. So that avoids the users from seeing the extent while the lower level code is going to remove it. How about? /* * release the resource under dax_region_rwsem to avoid races with * users trying to use the extent */ > > > + __dax_release_resource(dax_resource); > > + return 0; > > +} > > +EXPORT_SYMBOL_GPL(dax_region_rm_resource); > > + > > > > +static ssize_t dev_dax_resize_sparse(struct dax_region *dax_region, > > + struct dev_dax *dev_dax, > > + resource_size_t to_alloc) > > +{ > > + struct dax_resource *dax_resource; > > + resource_size_t available_size; > > + struct device *extent_dev; > > + ssize_t alloc; > > + > > + extent_dev = device_find_child(dax_region->dev, dax_region, > > + find_free_extent); > > There is a __free for put device and it will tidy this up a tiny bit. And fix the bug... :-/ Thanks. > > > + if (!extent_dev) > > + return 0; > > + > > + dax_resource = dev_get_drvdata(extent_dev); > > + if (!dax_resource) > > + return 0; > > + > > + available_size = dax_avail_size(dax_resource->res); > > + to_alloc = min(available_size, to_alloc); > I'd put those two inline and skip the local variables unless > they have more use in later patches. Nope just made the line shorter and I tend to not embed calls like that but it is fine your way too. I'll change it. > > alloc = __dev_dax_resize(dax_resources->res, dev_dax, > min(dax_avail_size(dax_resources->res), > to_alloc), > dax_resource); > > > > + alloc = __dev_dax_resize(dax_resource->res, dev_dax, to_alloc, > > dax_resource); > > + if (alloc > 0) > > + dax_resource->use_cnt++; > > + put_device(extent_dev); > > + return alloc; > > +} > > + > > > @@ -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 are created > > initially with 0 size"); > must be created initially with 0 size. > > Otherwise this error message says that they are, so why is it an error? Indeed! > > > + rc = -EINVAL; > > + goto err_id; > > + } > > + > > 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/cxl.c b/drivers/dax/cxl.c > > index 367e86b1c22a..bf3b82b0120d 100644 > > --- a/drivers/dax/cxl.c > > +++ b/drivers/dax/cxl.c > > @@ -5,6 +5,60 @@ > > ... > > > +static int cxl_dax_region_notify(struct device *dev, > > + struct cxl_notify_data *notify_data) > > +{ > > + struct cxl_dax_region *cxlr_dax = to_cxl_dax_region(dev); > > + struct dax_region *dax_region = dev_get_drvdata(dev); > > + struct region_extent *region_extent = notify_data->region_extent; > > + > > + switch (notify_data->event) { > > + case DCD_ADD_CAPACITY: > > + return __cxl_dax_add_resource(dax_region, region_extent); > > + case DCD_RELEASE_CAPACITY: > > + return dax_region_rm_resource(dax_region, ®ion_extent->dev); > > + case DCD_FORCED_CAPACITY_RELEASE: > > + default: > > + dev_err(&cxlr_dax->dev, "Unknown DC event %d\n", > > + notify_data->event); > > + break; > Might as well return here and not below. > Makes it really really obvious this is the error path and currently the only > one that hits the return statement. > > + } ok > > + > > + return -ENXIO; > > +} > > > static int cxl_dax_region_probe(struct device *dev) > > { > > @@ -24,14 +78,16 @@ static int cxl_dax_region_probe(struct device *dev) > > flags |= IORESOURCE_DAX_SPARSE_CAP; > > > > dax_region = alloc_dax_region(dev, cxlr->id, &cxlr_dax->hpa_range, nid, > > - PMD_SIZE, flags); > > + PMD_SIZE, flags, &sparse_ops); > > if (!dax_region) > > return -ENOMEM; > > > > - if (cxlr->mode == CXL_REGION_DC) > > + if (cxlr->mode == CXL_REGION_DC) { > > + device_for_each_child(&cxlr_dax->dev, dax_region, > > + cxl_dax_add_resource); > > /* Add empty seed dax device */ > > dev_size = 0; > > - else > > + } else > > Coding style says that you need brackets for all branches if > one needs them (as multiline). Just above: > https://www.kernel.org/doc/html/v4.10/process/coding-style.html#spaces > Yep missed it. thanks for the review! Ira [snip]