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]
