On Thu, 14 Jul 2022 17:03:21 -0700
Dan Williams <dan.j.williams@xxxxxxxxx> wrote:
> The LIBNVDIMM subsystem is a platform agnostic representation of system
> NVDIMM / persistent memory resources. To date, the CXL subsystem's
> interaction with LIBNVDIMM has been to register an nvdimm-bridge device
> and cxl_nvdimm objects to proxy CXL capabilities into existing LIBNVDIMM
> subsystem mechanics.
>
> With regions the approach is the same. Create a new cxl_pmem_region
> object to proxy CXL region details into a LIBNVDIMM definition. With
> this enabling LIBNVDIMM can partition CXL persistent memory regions with
> legacy namespace labels. A follow-on patch will add CXL region label and
> CXL namespace label support to persist region configurations across
> driver reload / system-reset events.
>
> Co-developed-by: Ben Widawsky <bwidawsk@xxxxxxxxxx>
> Signed-off-by: Ben Widawsky <bwidawsk@xxxxxxxxxx>
> Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx>
On trivial query below. Either way I think this looks good.
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> +
> +static void unregister_nvdimm_region(void *nd_region)
> +{
> + struct cxl_nvdimm_bridge *cxl_nvb;
> + struct cxl_pmem_region *cxlr_pmem;
> + int i;
> +
> + cxlr_pmem = nd_region_provider_data(nd_region);
> + cxl_nvb = cxlr_pmem->bridge;
> + device_lock(&cxl_nvb->dev);
> + for (i = 0; i < cxlr_pmem->nr_mappings; i++) {
> + struct cxl_pmem_region_mapping *m = &cxlr_pmem->mapping[i];
> + struct cxl_nvdimm *cxl_nvd = m->cxl_nvd;
> +
> + if (cxl_nvd->region) {
> + put_device(&cxlr_pmem->dev);
> + cxl_nvd->region = NULL;
> + }
> + }
> + device_unlock(&cxl_nvb->dev);
> +
> + nvdimm_region_delete(nd_region);
I'm not convinced by the ordering in here. Can we do the nvdimm_region_delete() before
taking the device_lock()? That would make this a nice mirror image of what is
going on in probe(). I may well be missing a reason that doesn't work though.
> +}
> +
