The three objects 'struct cxl_nvdimm_bridge', 'struct cxl_nvdimm', and
'struct cxl_pmem_region' manage CXL persistent memory resources. The
bridge represents base platform resources, the nvdimm represents one or
more endpoints, and the region is a collection of nvdimms that
contribute to an assembled address range.
Their relationship is such that a region is torn down if any component
endpoints are removed. All regions and endpoints are torn down if the
foundational bridge device goes down.
A workqueue was deployed to manage these interdependencies, but it is
difficult to reason about, and fragile. A recent attempt to take the CXL
root device lock in the cxl_mem driver was reported by lockdep as
colliding with the flush_work() in the cxl_pmem flows.
Instead of the workqueue, arrange for all pmem/nvdimm devices to be torn
down immediately and hierarchically. A similar change is made to both
the 'cxl_nvdimm' and 'cxl_pmem_region' objects. For bisect-ability both
changes are made in the same patch which unfortunately makes the patch
bigger than desired.
Arrange for cxl_memdev and cxl_region to register a cxl_nvdimm and
cxl_pmem_region as a devres release action of the bridge device.
Additionally, include a devres release action of the cxl_memdev or
cxl_region device that triggers the bridge's release action if an endpoint
exits before the bridge. I.e. this allows either unplugging the bridge,
or unplugging and endpoint to result in the same cleanup actions.
To keep the patch smaller the cleanup of the now defunct workqueue
infrastructure is saved for a follow-on patch.
Tested-by: Robert Richter <rrichter@amd.com>
Link: https://lore.kernel.org/r/166993041773.1882361.16444301376147207609.stgit@dwillia2-xfh.jf.intel.com
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
static struct lock_class_key cxl_nvdimm_key;
-static struct cxl_nvdimm *cxl_nvdimm_alloc(struct cxl_memdev *cxlmd)
+static struct cxl_nvdimm *cxl_nvdimm_alloc(struct cxl_nvdimm_bridge *cxl_nvb,
+ struct cxl_memdev *cxlmd)
{
struct cxl_nvdimm *cxl_nvd;
struct device *dev;
dev = &cxl_nvd->dev;
cxl_nvd->cxlmd = cxlmd;
+ cxlmd->cxl_nvd = cxl_nvd;
device_initialize(dev);
lockdep_set_class(&dev->mutex, &cxl_nvdimm_key);
device_set_pm_not_required(dev);
return cxl_nvd;
}
-static void cxl_nvd_unregister(void *dev)
+static void cxl_nvd_unregister(void *_cxl_nvd)
{
- device_unregister(dev);
+ struct cxl_nvdimm *cxl_nvd = _cxl_nvd;
+ struct cxl_memdev *cxlmd = cxl_nvd->cxlmd;
+ struct cxl_nvdimm_bridge *cxl_nvb = cxlmd->cxl_nvb;
+
+ /*
+ * Either the bridge is in ->remove() context under the device_lock(),
+ * or cxlmd_release_nvdimm() is cancelling the bridge's release action
+ * for @cxl_nvd and doing it itself (while manually holding the bridge
+ * lock).
+ */
+ device_lock_assert(&cxl_nvb->dev);
+ cxl_nvd->cxlmd = NULL;
+ cxlmd->cxl_nvd = NULL;
+ device_unregister(&cxl_nvd->dev);
+}
+
+static void cxlmd_release_nvdimm(void *_cxlmd)
+{
+ struct cxl_memdev *cxlmd = _cxlmd;
+ struct cxl_nvdimm_bridge *cxl_nvb = cxlmd->cxl_nvb;
+
+ device_lock(&cxl_nvb->dev);
+ if (cxlmd->cxl_nvd)
+ devm_release_action(&cxl_nvb->dev, cxl_nvd_unregister,
+ cxlmd->cxl_nvd);
+ device_unlock(&cxl_nvb->dev);
+ put_device(&cxl_nvb->dev);
}
/**
* devm_cxl_add_nvdimm() - add a bridge between a cxl_memdev and an nvdimm
- * @host: same host as @cxlmd
* @cxlmd: cxl_memdev instance that will perform LIBNVDIMM operations
*
* Return: 0 on success negative error code on failure.
*/
-int devm_cxl_add_nvdimm(struct device *host, struct cxl_memdev *cxlmd)
+int devm_cxl_add_nvdimm(struct cxl_memdev *cxlmd)
{
+ struct cxl_nvdimm_bridge *cxl_nvb;
struct cxl_nvdimm *cxl_nvd;
struct device *dev;
int rc;
- cxl_nvd = cxl_nvdimm_alloc(cxlmd);
- if (IS_ERR(cxl_nvd))
- return PTR_ERR(cxl_nvd);
+ cxl_nvb = cxl_find_nvdimm_bridge(&cxlmd->dev);
+ if (!cxl_nvb)
+ return -ENODEV;
+
+ cxl_nvd = cxl_nvdimm_alloc(cxl_nvb, cxlmd);
+ if (IS_ERR(cxl_nvd)) {
+ rc = PTR_ERR(cxl_nvd);
+ goto err_alloc;
+ }
+ cxlmd->cxl_nvb = cxl_nvb;
dev = &cxl_nvd->dev;
rc = dev_set_name(dev, "pmem%d", cxlmd->id);
if (rc)
goto err;
- dev_dbg(host, "%s: register %s\n", dev_name(dev->parent),
- dev_name(dev));
+ dev_dbg(&cxlmd->dev, "register %s\n", dev_name(dev));
+
+ /*
+ * The two actions below arrange for @cxl_nvd to be deleted when either
+ * the top-level PMEM bridge goes down, or the endpoint device goes
+ * through ->remove().
+ */
+ device_lock(&cxl_nvb->dev);
+ if (cxl_nvb->dev.driver)
+ rc = devm_add_action_or_reset(&cxl_nvb->dev, cxl_nvd_unregister,
+ cxl_nvd);
+ else
+ rc = -ENXIO;
+ device_unlock(&cxl_nvb->dev);
+
+ if (rc)
+ goto err_alloc;
- return devm_add_action_or_reset(host, cxl_nvd_unregister, dev);
+ /* @cxlmd carries a reference on @cxl_nvb until cxlmd_release_nvdimm */
+ return devm_add_action_or_reset(&cxlmd->dev, cxlmd_release_nvdimm, cxlmd);
err:
put_device(dev);
+err_alloc:
+ cxlmd->cxl_nvb = NULL;
+ cxlmd->cxl_nvd = NULL;
+ put_device(&cxl_nvb->dev);
+
return rc;
}
EXPORT_SYMBOL_NS_GPL(devm_cxl_add_nvdimm, CXL);
static struct cxl_pmem_region *cxl_pmem_region_alloc(struct cxl_region *cxlr)
{
struct cxl_region_params *p = &cxlr->params;
+ struct cxl_nvdimm_bridge *cxl_nvb;
struct cxl_pmem_region *cxlr_pmem;
struct device *dev;
int i;
struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
struct cxl_pmem_region_mapping *m = &cxlr_pmem->mapping[i];
+ /*
+ * Regions never span CXL root devices, so by definition the
+ * bridge for one device is the same for all.
+ */
+ if (i == 0) {
+ cxl_nvb = cxl_find_nvdimm_bridge(&cxlmd->dev);
+ if (!cxl_nvb) {
+ cxlr_pmem = ERR_PTR(-ENODEV);
+ goto out;
+ }
+ cxlr->cxl_nvb = cxl_nvb;
+ }
m->cxlmd = cxlmd;
get_device(&cxlmd->dev);
m->start = cxled->dpa_res->start;
dev = &cxlr_pmem->dev;
cxlr_pmem->cxlr = cxlr;
+ cxlr->cxlr_pmem = cxlr_pmem;
device_initialize(dev);
lockdep_set_class(&dev->mutex, &cxl_pmem_region_key);
device_set_pm_not_required(dev);
return cxlr_pmem;
}
-static void cxlr_pmem_unregister(void *dev)
+static void cxlr_pmem_unregister(void *_cxlr_pmem)
{
- device_unregister(dev);
+ struct cxl_pmem_region *cxlr_pmem = _cxlr_pmem;
+ struct cxl_region *cxlr = cxlr_pmem->cxlr;
+ struct cxl_nvdimm_bridge *cxl_nvb = cxlr->cxl_nvb;
+
+ /*
+ * Either the bridge is in ->remove() context under the device_lock(),
+ * or cxlr_release_nvdimm() is cancelling the bridge's release action
+ * for @cxlr_pmem and doing it itself (while manually holding the bridge
+ * lock).
+ */
+ device_lock_assert(&cxl_nvb->dev);
+ cxlr->cxlr_pmem = NULL;
+ cxlr_pmem->cxlr = NULL;
+ device_unregister(&cxlr_pmem->dev);
+}
+
+static void cxlr_release_nvdimm(void *_cxlr)
+{
+ struct cxl_region *cxlr = _cxlr;
+ struct cxl_nvdimm_bridge *cxl_nvb = cxlr->cxl_nvb;
+
+ device_lock(&cxl_nvb->dev);
+ if (cxlr->cxlr_pmem)
+ devm_release_action(&cxl_nvb->dev, cxlr_pmem_unregister,
+ cxlr->cxlr_pmem);
+ device_unlock(&cxl_nvb->dev);
+ cxlr->cxl_nvb = NULL;
+ put_device(&cxl_nvb->dev);
}
/**
static int devm_cxl_add_pmem_region(struct cxl_region *cxlr)
{
struct cxl_pmem_region *cxlr_pmem;
+ struct cxl_nvdimm_bridge *cxl_nvb;
struct device *dev;
int rc;
cxlr_pmem = cxl_pmem_region_alloc(cxlr);
if (IS_ERR(cxlr_pmem))
return PTR_ERR(cxlr_pmem);
+ cxl_nvb = cxlr->cxl_nvb;
dev = &cxlr_pmem->dev;
rc = dev_set_name(dev, "pmem_region%d", cxlr->id);
dev_dbg(&cxlr->dev, "%s: register %s\n", dev_name(dev->parent),
dev_name(dev));
- return devm_add_action_or_reset(&cxlr->dev, cxlr_pmem_unregister, dev);
+ device_lock(&cxl_nvb->dev);
+ if (cxl_nvb->dev.driver)
+ rc = devm_add_action_or_reset(&cxl_nvb->dev,
+ cxlr_pmem_unregister, cxlr_pmem);
+ else
+ rc = -ENXIO;
+ device_unlock(&cxl_nvb->dev);
+
+ if (rc)
+ goto err_bridge;
+
+ /* @cxlr carries a reference on @cxl_nvb until cxlr_release_nvdimm */
+ return devm_add_action_or_reset(&cxlr->dev, cxlr_release_nvdimm, cxlr);
err:
put_device(dev);
+err_bridge:
+ put_device(&cxl_nvb->dev);
+ cxlr->cxl_nvb = NULL;
return rc;
}
* @id: This region's id. Id is globally unique across all regions
* @mode: Endpoint decoder allocation / access mode
* @type: Endpoint decoder target type
+ * @cxl_nvb: nvdimm bridge for coordinating @cxlr_pmem setup / shutdown
+ * @cxlr_pmem: (for pmem regions) cached copy of the nvdimm bridge
* @params: active + config params for the region
*/
struct cxl_region {
int id;
enum cxl_decoder_mode mode;
enum cxl_decoder_type type;
+ struct cxl_nvdimm_bridge *cxl_nvb;
+ struct cxl_pmem_region *cxlr_pmem;
struct cxl_region_params params;
};
struct device dev;
struct cxl_region *cxlr;
struct nd_region *nd_region;
- struct cxl_nvdimm_bridge *bridge;
struct range hpa_range;
int nr_mappings;
struct cxl_pmem_region_mapping mapping[];
struct cxl_nvdimm *to_cxl_nvdimm(struct device *dev);
bool is_cxl_nvdimm(struct device *dev);
bool is_cxl_nvdimm_bridge(struct device *dev);
-int devm_cxl_add_nvdimm(struct device *host, struct cxl_memdev *cxlmd);
+int devm_cxl_add_nvdimm(struct cxl_memdev *cxlmd);
struct cxl_nvdimm_bridge *cxl_find_nvdimm_bridge(struct device *dev);
#ifdef CONFIG_CXL_REGION
* @cdev: char dev core object for ioctl operations
* @cxlds: The device state backing this device
* @detach_work: active memdev lost a port in its ancestry
+ * @cxl_nvb: coordinate removal of @cxl_nvd if present
+ * @cxl_nvd: optional bridge to an nvdimm if the device supports pmem
* @id: id number of this memdev instance.
*/
struct cxl_memdev {
struct cdev cdev;
struct cxl_dev_state *cxlds;
struct work_struct detach_work;
+ struct cxl_nvdimm_bridge *cxl_nvb;
+ struct cxl_nvdimm *cxl_nvd;
int id;
};
static int cxl_mem_probe(struct device *dev)
{
struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
+ struct cxl_dev_state *cxlds = cxlmd->cxlds;
struct cxl_port *parent_port;
struct cxl_dport *dport;
struct dentry *dentry;
if (rc)
return rc;
+ if (resource_size(&cxlds->pmem_res) && IS_ENABLED(CONFIG_CXL_PMEM)) {
+ rc = devm_cxl_add_nvdimm(cxlmd);
+ if (rc == -ENODEV)
+ dev_info(dev, "PMEM disabled by platform\n");
+ else
+ return rc;
+ }
+
/*
* The kernel may be operating out of CXL memory on this device,
* there is no spec defined way to determine whether this device
if (IS_ERR(cxlmd))
return PTR_ERR(cxlmd);
- if (resource_size(&cxlds->pmem_res) && IS_ENABLED(CONFIG_CXL_PMEM))
- rc = devm_cxl_add_nvdimm(&pdev->dev, cxlmd);
-
return rc;
}
{
struct cxl_nvdimm *cxl_nvd = to_cxl_nvdimm(dev);
struct cxl_memdev *cxlmd = cxl_nvd->cxlmd;
+ struct cxl_nvdimm_bridge *cxl_nvb = cxlmd->cxl_nvb;
unsigned long flags = 0, cmd_mask = 0;
struct cxl_dev_state *cxlds = cxlmd->cxlds;
- struct cxl_nvdimm_bridge *cxl_nvb;
struct nvdimm *nvdimm;
int rc;
- cxl_nvb = cxl_find_nvdimm_bridge(dev);
- if (!cxl_nvb)
- return -ENXIO;
-
- device_lock(&cxl_nvb->dev);
- if (!cxl_nvb->nvdimm_bus) {
- rc = -ENXIO;
- goto out;
- }
-
set_exclusive_cxl_commands(cxlds, exclusive_cmds);
rc = devm_add_action_or_reset(dev, clear_exclusive, cxlds);
if (rc)
- goto out;
+ return rc;
set_bit(NDD_LABELING, &flags);
set_bit(ND_CMD_GET_CONFIG_SIZE, &cmd_mask);
set_bit(ND_CMD_SET_CONFIG_DATA, &cmd_mask);
nvdimm = nvdimm_create(cxl_nvb->nvdimm_bus, cxl_nvd, NULL, flags,
cmd_mask, 0, NULL);
- if (!nvdimm) {
- rc = -ENOMEM;
- goto out;
- }
+ if (!nvdimm)
+ return -ENOMEM;
dev_set_drvdata(dev, nvdimm);
- cxl_nvd->bridge = cxl_nvb;
- rc = devm_add_action_or_reset(dev, unregister_nvdimm, nvdimm);
-out:
- device_unlock(&cxl_nvb->dev);
- put_device(&cxl_nvb->dev);
-
- return rc;
+ return devm_add_action_or_reset(dev, unregister_nvdimm, nvdimm);
}
static struct cxl_driver cxl_nvdimm_driver = {
return cxl_pmem_nvdimm_ctl(nvdimm, cmd, buf, buf_len);
}
+static void unregister_nvdimm_bus(void *_cxl_nvb)
+{
+ struct cxl_nvdimm_bridge *cxl_nvb = _cxl_nvb;
+ struct nvdimm_bus *nvdimm_bus = cxl_nvb->nvdimm_bus;
+
+ cxl_nvb->nvdimm_bus = NULL;
+ nvdimm_bus_unregister(nvdimm_bus);
+}
+
static bool online_nvdimm_bus(struct cxl_nvdimm_bridge *cxl_nvb)
{
if (cxl_nvb->nvdimm_bus)
{
struct cxl_nvdimm_bridge *cxl_nvb = to_cxl_nvdimm_bridge(dev);
- if (cxl_nvb->state == CXL_NVB_DEAD)
- return -ENXIO;
+ cxl_nvb->nd_desc = (struct nvdimm_bus_descriptor) {
+ .provider_name = "CXL",
+ .module = THIS_MODULE,
+ .ndctl = cxl_pmem_ctl,
+ };
- if (cxl_nvb->state == CXL_NVB_NEW) {
- cxl_nvb->nd_desc = (struct nvdimm_bus_descriptor) {
- .provider_name = "CXL",
- .module = THIS_MODULE,
- .ndctl = cxl_pmem_ctl,
- };
+ cxl_nvb->nvdimm_bus =
+ nvdimm_bus_register(&cxl_nvb->dev, &cxl_nvb->nd_desc);
- INIT_WORK(&cxl_nvb->state_work, cxl_nvb_update_state);
- }
+ if (!cxl_nvb->nvdimm_bus)
+ return -ENOMEM;
- cxl_nvb->state = CXL_NVB_ONLINE;
- cxl_nvdimm_bridge_state_work(cxl_nvb);
+ INIT_WORK(&cxl_nvb->state_work, cxl_nvb_update_state);
- return 0;
+ return devm_add_action_or_reset(dev, unregister_nvdimm_bus, cxl_nvb);
}
static struct cxl_driver cxl_nvdimm_bridge_driver = {
},
};
-static int match_cxl_nvdimm(struct device *dev, void *data)
-{
- return is_cxl_nvdimm(dev);
-}
-
static void unregister_nvdimm_region(void *nd_region)
{
nvdimm_region_delete(nd_region);
struct nd_mapping_desc mappings[CXL_DECODER_MAX_INTERLEAVE];
struct cxl_pmem_region *cxlr_pmem = to_cxl_pmem_region(dev);
struct cxl_region *cxlr = cxlr_pmem->cxlr;
+ struct cxl_nvdimm_bridge *cxl_nvb = cxlr->cxl_nvb;
struct cxl_pmem_region_info *info = NULL;
- struct cxl_nvdimm_bridge *cxl_nvb;
struct nd_interleave_set *nd_set;
struct nd_region_desc ndr_desc;
struct cxl_nvdimm *cxl_nvd;
struct resource *res;
int rc, i = 0;
- cxl_nvb = cxl_find_nvdimm_bridge(&cxlr_pmem->mapping[0].cxlmd->dev);
- if (!cxl_nvb) {
- dev_dbg(dev, "bridge not found\n");
- return -ENXIO;
- }
- cxlr_pmem->bridge = cxl_nvb;
-
- device_lock(&cxl_nvb->dev);
- if (!cxl_nvb->nvdimm_bus) {
- dev_dbg(dev, "nvdimm bus not found\n");
- rc = -ENXIO;
- goto out_nvb;
- }
-
memset(&mappings, 0, sizeof(mappings));
memset(&ndr_desc, 0, sizeof(ndr_desc));
res = devm_kzalloc(dev, sizeof(*res), GFP_KERNEL);
- if (!res) {
- rc = -ENOMEM;
- goto out_nvb;
- }
+ if (!res)
+ return -ENOMEM;
res->name = "Persistent Memory";
res->start = cxlr_pmem->hpa_range.start;
rc = insert_resource(&iomem_resource, res);
if (rc)
- goto out_nvb;
+ return rc;
rc = devm_add_action_or_reset(dev, cxlr_pmem_remove_resource, res);
if (rc)
- goto out_nvb;
+ return rc;
ndr_desc.res = res;
ndr_desc.provider_data = cxlr_pmem;
}
nd_set = devm_kzalloc(dev, sizeof(*nd_set), GFP_KERNEL);
- if (!nd_set) {
- rc = -ENOMEM;
- goto out_nvb;
- }
+ if (!nd_set)
+ return -ENOMEM;
ndr_desc.memregion = cxlr->id;
set_bit(ND_REGION_CXL, &ndr_desc.flags);
set_bit(ND_REGION_PERSIST_MEMCTRL, &ndr_desc.flags);
info = kmalloc_array(cxlr_pmem->nr_mappings, sizeof(*info), GFP_KERNEL);
- if (!info) {
- rc = -ENOMEM;
- goto out_nvb;
- }
+ if (!info)
+ return -ENOMEM;
for (i = 0; i < cxlr_pmem->nr_mappings; i++) {
struct cxl_pmem_region_mapping *m = &cxlr_pmem->mapping[i];
struct cxl_memdev *cxlmd = m->cxlmd;
struct cxl_dev_state *cxlds = cxlmd->cxlds;
- struct device *d;
- d = device_find_child(&cxlmd->dev, NULL, match_cxl_nvdimm);
- if (!d) {
- dev_dbg(dev, "[%d]: %s: no cxl_nvdimm found\n", i,
- dev_name(&cxlmd->dev));
- rc = -ENODEV;
- goto out_nvd;
- }
-
- /* safe to drop ref now with bridge lock held */
- put_device(d);
-
- cxl_nvd = to_cxl_nvdimm(d);
+ cxl_nvd = cxlmd->cxl_nvd;
nvdimm = dev_get_drvdata(&cxl_nvd->dev);
if (!nvdimm) {
dev_dbg(dev, "[%d]: %s: no nvdimm found\n", i,
cxlr_pmem->nd_region);
out_nvd:
kfree(info);
-out_nvb:
- device_unlock(&cxl_nvb->dev);
- put_device(&cxl_nvb->dev);
return rc;
}
if (IS_ERR(cxlmd))
return PTR_ERR(cxlmd);
- if (resource_size(&cxlds->pmem_res) && IS_ENABLED(CONFIG_CXL_PMEM))
- rc = devm_cxl_add_nvdimm(dev, cxlmd);
-
return 0;
}
OSDN Git Service
