* [PATCH 1/3] cxl/port: Use __free() to drop put_device() for cxl_port
@ 2024-08-13 7:05 Li Ming
2024-08-13 7:05 ` [PATCH 2/3] cxl/port: Use scoped_guard() to drop device_lock()/unlock pair Li Ming
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Li Ming @ 2024-08-13 7:05 UTC (permalink / raw)
To: dave, jonathan.cameron, dave.jiang, alison.schofield,
vishal.l.verma, ira.weiny, dan.j.williams
Cc: linux-cxl, Li Ming
Using scope-based resource management __free() marco with a new helper
called put_cxl_port() to drop open coded the put_device() used to
dereference the 'struct device' of a cxl_port.
Suggested-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Li Ming <ming4.li@intel.com>
---
drivers/cxl/core/pci.c | 6 ++----
drivers/cxl/core/port.c | 25 +++++++++----------------
drivers/cxl/cxl.h | 2 ++
drivers/cxl/mem.c | 5 ++---
drivers/cxl/pci.c | 7 ++-----
5 files changed, 17 insertions(+), 28 deletions(-)
diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index 51132a575b27..4725e37d90fb 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -915,15 +915,13 @@ static void cxl_handle_rdport_errors(struct cxl_dev_state *cxlds)
struct pci_dev *pdev = to_pci_dev(cxlds->dev);
struct aer_capability_regs aer_regs;
struct cxl_dport *dport;
- struct cxl_port *port;
int severity;
- port = cxl_pci_find_port(pdev, &dport);
+ struct cxl_port *port __free(put_cxl_port) =
+ cxl_pci_find_port(pdev, &dport);
if (!port)
return;
- put_device(&port->dev);
-
if (!cxl_rch_get_aer_info(dport->regs.dport_aer, &aer_regs))
return;
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index 1d5007e3795a..6119cb3ad25c 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -1472,7 +1472,7 @@ static void cxl_detach_ep(void *data)
struct cxl_memdev *cxlmd = data;
for (int i = cxlmd->depth - 1; i >= 1; i--) {
- struct cxl_port *port, *parent_port;
+ struct cxl_port *parent_port;
struct detach_ctx ctx = {
.cxlmd = cxlmd,
.depth = i,
@@ -1485,7 +1485,7 @@ static void cxl_detach_ep(void *data)
port_has_memdev);
if (!dev)
continue;
- port = to_cxl_port(dev);
+ struct cxl_port *port __free(put_cxl_port) = to_cxl_port(dev);
parent_port = to_cxl_port(port->dev.parent);
device_lock(&parent_port->dev);
@@ -1512,7 +1512,6 @@ static void cxl_detach_ep(void *data)
dev_name(&port->dev));
delete_switch_port(port);
}
- put_device(&port->dev);
device_unlock(&parent_port->dev);
}
}
@@ -1539,8 +1538,8 @@ static int add_port_attach_ep(struct cxl_memdev *cxlmd,
struct device *uport_dev,
struct device *dport_dev)
{
+ struct cxl_port *port __free(put_cxl_port) = NULL;
struct device *dparent = grandparent(dport_dev);
- struct cxl_port *port, *parent_port = NULL;
struct cxl_dport *dport, *parent_dport;
resource_size_t component_reg_phys;
int rc;
@@ -1556,7 +1555,8 @@ static int add_port_attach_ep(struct cxl_memdev *cxlmd,
return -ENXIO;
}
- parent_port = find_cxl_port(dparent, &parent_dport);
+ struct cxl_port *parent_port __free(put_cxl_port) =
+ find_cxl_port(dparent, &parent_dport);
if (!parent_port) {
/* iterate to create this parent_port */
return -EAGAIN;
@@ -1596,10 +1596,8 @@ static int add_port_attach_ep(struct cxl_memdev *cxlmd,
*/
rc = -ENXIO;
}
- put_device(&port->dev);
}
- put_device(&parent_port->dev);
return rc;
}
@@ -1630,7 +1628,6 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd)
struct device *dport_dev = grandparent(iter);
struct device *uport_dev;
struct cxl_dport *dport;
- struct cxl_port *port;
/*
* The terminal "grandparent" in PCI is NULL and @platform_bus
@@ -1649,7 +1646,8 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd)
dev_dbg(dev, "scan: iter: %s dport_dev: %s parent: %s\n",
dev_name(iter), dev_name(dport_dev),
dev_name(uport_dev));
- port = find_cxl_port(dport_dev, &dport);
+ struct cxl_port *port __free(put_cxl_port) =
+ find_cxl_port(dport_dev, &dport);
if (port) {
dev_dbg(&cxlmd->dev,
"found already registered port %s:%s\n",
@@ -1664,18 +1662,13 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd)
* the parent_port lock as the current port may be being
* reaped.
*/
- if (rc && rc != -EBUSY) {
- put_device(&port->dev);
+ if (rc && rc != -EBUSY)
return rc;
- }
/* Any more ports to add between this one and the root? */
- if (!dev_is_cxl_root_child(&port->dev)) {
- put_device(&port->dev);
+ if (!dev_is_cxl_root_child(&port->dev))
continue;
- }
- put_device(&port->dev);
return 0;
}
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 9afb407d438f..cad297fba700 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -744,6 +744,8 @@ struct cxl_root *find_cxl_root(struct cxl_port *port);
void put_cxl_root(struct cxl_root *cxl_root);
DEFINE_FREE(put_cxl_root, struct cxl_root *, if (_T) put_cxl_root(_T))
+DEFINE_FREE(put_cxl_port, struct cxl_port *,
+ if (!IS_ERR_OR_NULL(_T)) put_device(&_T->dev))
int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd);
void cxl_bus_rescan(void);
void cxl_bus_drain(void);
diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
index 7de232eaeb17..ab9b8ab8df44 100644
--- a/drivers/cxl/mem.c
+++ b/drivers/cxl/mem.c
@@ -109,7 +109,6 @@ static int cxl_mem_probe(struct device *dev)
struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlmd->cxlds);
struct cxl_dev_state *cxlds = cxlmd->cxlds;
struct device *endpoint_parent;
- struct cxl_port *parent_port;
struct cxl_dport *dport;
struct dentry *dentry;
int rc;
@@ -146,7 +145,8 @@ static int cxl_mem_probe(struct device *dev)
if (rc)
return rc;
- parent_port = cxl_mem_find_port(cxlmd, &dport);
+ struct cxl_port *parent_port __free(put_cxl_port) =
+ cxl_mem_find_port(cxlmd, &dport);
if (!parent_port) {
dev_err(dev, "CXL port topology not found\n");
return -ENXIO;
@@ -179,7 +179,6 @@ static int cxl_mem_probe(struct device *dev)
rc = devm_cxl_add_endpoint(endpoint_parent, cxlmd, dport);
unlock:
device_unlock(endpoint_parent);
- put_device(&parent_port->dev);
if (rc)
return rc;
diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 4be35dc22202..26e75499abdd 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -473,7 +473,6 @@ static bool is_cxl_restricted(struct pci_dev *pdev)
static int cxl_rcrb_get_comp_regs(struct pci_dev *pdev,
struct cxl_register_map *map)
{
- struct cxl_port *port;
struct cxl_dport *dport;
resource_size_t component_reg_phys;
@@ -482,14 +481,12 @@ static int cxl_rcrb_get_comp_regs(struct pci_dev *pdev,
.resource = CXL_RESOURCE_NONE,
};
- port = cxl_pci_find_port(pdev, &dport);
+ struct cxl_port *port __free(put_cxl_port) =
+ cxl_pci_find_port(pdev, &dport);
if (!port)
return -EPROBE_DEFER;
component_reg_phys = cxl_rcd_component_reg_phys(&pdev->dev, dport);
-
- put_device(&port->dev);
-
if (component_reg_phys == CXL_RESOURCE_NONE)
return -ENXIO;
--
2.40.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/3] cxl/port: Use scoped_guard() to drop device_lock()/unlock pair
2024-08-13 7:05 [PATCH 1/3] cxl/port: Use __free() to drop put_device() for cxl_port Li Ming
@ 2024-08-13 7:05 ` Li Ming
2024-08-21 21:49 ` Ira Weiny
2024-08-13 7:05 ` [PATCH 3/3] cxl/port: Refactor __devm_cxl_add_port() to drop goto pattern Li Ming
2024-08-21 21:37 ` [PATCH 1/3] cxl/port: Use __free() to drop put_device() for cxl_port Ira Weiny
2 siblings, 1 reply; 9+ messages in thread
From: Li Ming @ 2024-08-13 7:05 UTC (permalink / raw)
To: dave, jonathan.cameron, dave.jiang, alison.schofield,
vishal.l.verma, ira.weiny, dan.j.williams
Cc: linux-cxl, Li Ming
A device_lock() and device_unlock() pair can be replaced by a
scope-based resource management function scoped_guard() which can make
the code more readable and safer. Some device_lock() and device_unlock()
pairs in CXL subsystem can be simply replaced by a scoped_guard().
Suggested-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Li Ming <ming4.li@intel.com>
---
drivers/cxl/core/mbox.c | 26 ++++++------
drivers/cxl/core/port.c | 87 ++++++++++++++++++---------------------
drivers/cxl/core/region.c | 25 ++++++-----
drivers/cxl/mem.c | 22 +++++-----
drivers/cxl/pmem.c | 16 ++++---
5 files changed, 82 insertions(+), 94 deletions(-)
diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index e5cdeafdf76e..a8ab5f2697be 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -1214,19 +1214,19 @@ int cxl_mem_sanitize(struct cxl_memdev *cxlmd, u16 cmd)
int rc;
/* synchronize with cxl_mem_probe() and decoder write operations */
- device_lock(&cxlmd->dev);
- endpoint = cxlmd->endpoint;
- down_read(&cxl_region_rwsem);
- /*
- * Require an endpoint to be safe otherwise the driver can not
- * be sure that the device is unmapped.
- */
- if (endpoint && cxl_num_decoders_committed(endpoint) == 0)
- rc = __cxl_mem_sanitize(mds, cmd);
- else
- rc = -EBUSY;
- up_read(&cxl_region_rwsem);
- device_unlock(&cxlmd->dev);
+ scoped_guard(device, &cxlmd->dev) {
+ endpoint = cxlmd->endpoint;
+ down_read(&cxl_region_rwsem);
+ /*
+ * Require an endpoint to be safe otherwise the driver can not
+ * be sure that the device is unmapped.
+ */
+ if (endpoint && cxl_num_decoders_committed(endpoint) == 0)
+ rc = __cxl_mem_sanitize(mds, cmd);
+ else
+ rc = -EBUSY;
+ up_read(&cxl_region_rwsem);
+ }
return rc;
}
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index 6119cb3ad25c..53e2593daa95 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -1260,14 +1260,12 @@ static int add_ep(struct cxl_ep *new)
struct cxl_port *port = new->dport->port;
int rc;
- device_lock(&port->dev);
- if (port->dead) {
- device_unlock(&port->dev);
- return -ENXIO;
+ scoped_guard(device, &port->dev) {
+ if (port->dead)
+ return -ENXIO;
+ rc = xa_insert(&port->endpoints, (unsigned long)new->ep, new,
+ GFP_KERNEL);
}
- rc = xa_insert(&port->endpoints, (unsigned long)new->ep, new,
- GFP_KERNEL);
- device_unlock(&port->dev);
return rc;
}
@@ -1393,14 +1391,14 @@ static void delete_endpoint(void *data)
struct cxl_port *endpoint = cxlmd->endpoint;
struct device *host = endpoint_host(endpoint);
- device_lock(host);
- if (host->driver && !endpoint->dead) {
- devm_release_action(host, cxl_unlink_parent_dport, endpoint);
- devm_release_action(host, cxl_unlink_uport, endpoint);
- devm_release_action(host, unregister_port, endpoint);
+ scoped_guard(device, host) {
+ if (host->driver && !endpoint->dead) {
+ devm_release_action(host, cxl_unlink_parent_dport, endpoint);
+ devm_release_action(host, cxl_unlink_uport, endpoint);
+ devm_release_action(host, unregister_port, endpoint);
+ }
+ cxlmd->endpoint = NULL;
}
- cxlmd->endpoint = NULL;
- device_unlock(host);
put_device(&endpoint->dev);
put_device(host);
}
@@ -1562,40 +1560,36 @@ static int add_port_attach_ep(struct cxl_memdev *cxlmd,
return -EAGAIN;
}
- device_lock(&parent_port->dev);
- if (!parent_port->dev.driver) {
- dev_warn(&cxlmd->dev,
- "port %s:%s disabled, failed to enumerate CXL.mem\n",
- dev_name(&parent_port->dev), dev_name(uport_dev));
- port = ERR_PTR(-ENXIO);
- goto out;
- }
+ scoped_guard(device, &parent_port->dev) {
+ if (!parent_port->dev.driver) {
+ dev_warn(&cxlmd->dev,
+ "port %s:%s disabled, failed to enumerate CXL.mem\n",
+ dev_name(&parent_port->dev), dev_name(uport_dev));
+ return -ENXIO;
+ }
- port = find_cxl_port_at(parent_port, dport_dev, &dport);
- if (!port) {
- component_reg_phys = find_component_registers(uport_dev);
- port = devm_cxl_add_port(&parent_port->dev, uport_dev,
- component_reg_phys, parent_dport);
- /* retry find to pick up the new dport information */
- if (!IS_ERR(port))
+ port = find_cxl_port_at(parent_port, dport_dev, &dport);
+ if (!port) {
+ component_reg_phys = find_component_registers(uport_dev);
+ port = devm_cxl_add_port(&parent_port->dev, uport_dev,
+ component_reg_phys, parent_dport);
+ if (IS_ERR(port))
+ return PTR_ERR(port);
+
+ /* retry find to pick up the new dport information */
port = find_cxl_port_at(parent_port, dport_dev, &dport);
+ }
}
-out:
- device_unlock(&parent_port->dev);
- if (IS_ERR(port))
- rc = PTR_ERR(port);
- else {
- dev_dbg(&cxlmd->dev, "add to new port %s:%s\n",
- dev_name(&port->dev), dev_name(port->uport_dev));
- rc = cxl_add_ep(dport, &cxlmd->dev);
- if (rc == -EBUSY) {
- /*
- * "can't" happen, but this error code means
- * something to the caller, so translate it.
- */
- rc = -ENXIO;
- }
+ dev_dbg(&cxlmd->dev, "add to new port %s:%s\n",
+ dev_name(&port->dev), dev_name(port->uport_dev));
+ rc = cxl_add_ep(dport, &cxlmd->dev);
+ if (rc == -EBUSY) {
+ /*
+ * "can't" happen, but this error code means
+ * something to the caller, so translate it.
+ */
+ rc = -ENXIO;
}
return rc;
@@ -1986,9 +1980,8 @@ int cxl_decoder_add(struct cxl_decoder *cxld, int *target_map)
port = to_cxl_port(cxld->dev.parent);
- device_lock(&port->dev);
- rc = cxl_decoder_add_locked(cxld, target_map);
- device_unlock(&port->dev);
+ scoped_guard(device, &port->dev)
+ rc = cxl_decoder_add_locked(cxld, target_map);
return rc;
}
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 21ad5f242875..a3ce7cbe5c6c 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -3094,11 +3094,11 @@ 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);
+ scoped_guard(device, &cxl_nvb->dev) {
+ if (cxlr->cxlr_pmem)
+ devm_release_action(&cxl_nvb->dev, cxlr_pmem_unregister,
+ cxlr->cxlr_pmem);
+ }
cxlr->cxl_nvb = NULL;
put_device(&cxl_nvb->dev);
}
@@ -3134,14 +3134,13 @@ static int devm_cxl_add_pmem_region(struct cxl_region *cxlr)
dev_dbg(&cxlr->dev, "%s: register %s\n", dev_name(dev->parent),
dev_name(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);
-
+ scoped_guard(device, &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;
+ }
if (rc)
goto err_bridge;
diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
index ab9b8ab8df44..ae94018a01bd 100644
--- a/drivers/cxl/mem.c
+++ b/drivers/cxl/mem.c
@@ -168,19 +168,17 @@ static int cxl_mem_probe(struct device *dev)
cxl_setup_parent_dport(dev, dport);
- device_lock(endpoint_parent);
- if (!endpoint_parent->driver) {
- dev_err(dev, "CXL port topology %s not enabled\n",
- dev_name(endpoint_parent));
- rc = -ENXIO;
- goto unlock;
- }
+ scoped_guard(device, endpoint_parent) {
+ if (!endpoint_parent->driver) {
+ dev_err(dev, "CXL port topology %s not enabled\n",
+ dev_name(endpoint_parent));
+ return -ENXIO;
+ }
- rc = devm_cxl_add_endpoint(endpoint_parent, cxlmd, dport);
-unlock:
- device_unlock(endpoint_parent);
- if (rc)
- return rc;
+ rc = devm_cxl_add_endpoint(endpoint_parent, cxlmd, dport);
+ if (rc)
+ return rc;
+ }
/*
* The kernel may be operating out of CXL memory on this device,
diff --git a/drivers/cxl/pmem.c b/drivers/cxl/pmem.c
index 4ef93da22335..647c7e25ef3a 100644
--- a/drivers/cxl/pmem.c
+++ b/drivers/cxl/pmem.c
@@ -233,15 +233,13 @@ static int detach_nvdimm(struct device *dev, void *data)
if (!is_cxl_nvdimm(dev))
return 0;
- device_lock(dev);
- if (!dev->driver)
- goto out;
-
- cxl_nvd = to_cxl_nvdimm(dev);
- if (cxl_nvd->cxlmd && cxl_nvd->cxlmd->cxl_nvb == data)
- release = true;
-out:
- device_unlock(dev);
+ scoped_guard(device, dev) {
+ if (dev->driver) {
+ cxl_nvd = to_cxl_nvdimm(dev);
+ if (cxl_nvd->cxlmd && cxl_nvd->cxlmd->cxl_nvb == data)
+ release = true;
+ }
+ }
if (release)
device_release_driver(dev);
return 0;
--
2.40.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 3/3] cxl/port: Refactor __devm_cxl_add_port() to drop goto pattern
2024-08-13 7:05 [PATCH 1/3] cxl/port: Use __free() to drop put_device() for cxl_port Li Ming
2024-08-13 7:05 ` [PATCH 2/3] cxl/port: Use scoped_guard() to drop device_lock()/unlock pair Li Ming
@ 2024-08-13 7:05 ` Li Ming
2024-08-21 22:01 ` Ira Weiny
2024-08-21 21:37 ` [PATCH 1/3] cxl/port: Use __free() to drop put_device() for cxl_port Ira Weiny
2 siblings, 1 reply; 9+ messages in thread
From: Li Ming @ 2024-08-13 7:05 UTC (permalink / raw)
To: dave, jonathan.cameron, dave.jiang, alison.schofield,
vishal.l.verma, ira.weiny, dan.j.williams
Cc: linux-cxl, Li Ming
The "goto error" pattern is not recommended, it can be removed via
refactoring. In __devm_cxl_add_port(), there is a 'goto' to call
put_device() for the error cases between device_initialize() and
device_add() to dereference the 'struct device' of a new cxl_port.
The refactoring is introducing a new function called cxl_port_add()
which is used to add the 'struct device' of a new cxl_port to
device hierarchy, moving the functions needing the help of above
'goto' into cxl_port_add(), and using a scope-based resource management
__free() to drop the open coded put_device() and 'goto' for the error
cases.
Suggested-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Li Ming <ming4.li@intel.com>
---
drivers/cxl/core/port.c | 59 ++++++++++++++++++++++++-----------------
1 file changed, 35 insertions(+), 24 deletions(-)
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index 53e2593daa95..a886b16b2610 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -828,27 +828,20 @@ static void cxl_debugfs_create_dport_dir(struct cxl_dport *dport)
&cxl_einj_inject_fops);
}
-static struct cxl_port *__devm_cxl_add_port(struct device *host,
- struct device *uport_dev,
- resource_size_t component_reg_phys,
- struct cxl_dport *parent_dport)
+static int cxl_port_add(struct cxl_port *port,
+ resource_size_t component_reg_phys,
+ struct cxl_dport *parent_dport)
{
- struct cxl_port *port;
- struct device *dev;
+ struct device *dev __free(put_device) = &port->dev;
int rc;
- port = cxl_port_alloc(uport_dev, parent_dport);
- if (IS_ERR(port))
- return port;
-
- dev = &port->dev;
- if (is_cxl_memdev(uport_dev)) {
- struct cxl_memdev *cxlmd = to_cxl_memdev(uport_dev);
+ if (is_cxl_memdev(port->uport_dev)) {
+ struct cxl_memdev *cxlmd = to_cxl_memdev(port->uport_dev);
struct cxl_dev_state *cxlds = cxlmd->cxlds;
rc = dev_set_name(dev, "endpoint%d", port->id);
if (rc)
- goto err;
+ return rc;
/*
* The endpoint driver already enumerated the component and RAS
@@ -861,19 +854,41 @@ static struct cxl_port *__devm_cxl_add_port(struct device *host,
} else if (parent_dport) {
rc = dev_set_name(dev, "port%d", port->id);
if (rc)
- goto err;
+ return rc;
rc = cxl_port_setup_regs(port, component_reg_phys);
if (rc)
- goto err;
- } else
+ return rc;
+ } else {
rc = dev_set_name(dev, "root%d", port->id);
- if (rc)
- goto err;
+ if (rc)
+ return rc;
+ }
rc = device_add(dev);
if (rc)
- goto err;
+ return rc;
+
+ /* Inhibit the cleanup function invoked */
+ dev = NULL;
+ return 0;
+}
+
+static struct cxl_port *__devm_cxl_add_port(struct device *host,
+ struct device *uport_dev,
+ resource_size_t component_reg_phys,
+ struct cxl_dport *parent_dport)
+{
+ struct cxl_port *port;
+ int rc;
+
+ port = cxl_port_alloc(uport_dev, parent_dport);
+ if (IS_ERR(port))
+ return port;
+
+ rc = cxl_port_add(port, component_reg_phys, parent_dport);
+ if (rc)
+ return ERR_PTR(rc);
rc = devm_add_action_or_reset(host, unregister_port, port);
if (rc)
@@ -891,10 +906,6 @@ static struct cxl_port *__devm_cxl_add_port(struct device *host,
port->pci_latency = cxl_pci_get_latency(to_pci_dev(uport_dev));
return port;
-
-err:
- put_device(dev);
- return ERR_PTR(rc);
}
/**
--
2.40.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] cxl/port: Use __free() to drop put_device() for cxl_port
2024-08-13 7:05 [PATCH 1/3] cxl/port: Use __free() to drop put_device() for cxl_port Li Ming
2024-08-13 7:05 ` [PATCH 2/3] cxl/port: Use scoped_guard() to drop device_lock()/unlock pair Li Ming
2024-08-13 7:05 ` [PATCH 3/3] cxl/port: Refactor __devm_cxl_add_port() to drop goto pattern Li Ming
@ 2024-08-21 21:37 ` Ira Weiny
2024-08-22 1:26 ` Li, Ming4
2 siblings, 1 reply; 9+ messages in thread
From: Ira Weiny @ 2024-08-21 21:37 UTC (permalink / raw)
To: Li Ming, dave, jonathan.cameron, dave.jiang, alison.schofield,
vishal.l.verma, ira.weiny, dan.j.williams
Cc: linux-cxl, Li Ming
Li Ming wrote:
> Using scope-based resource management __free() marco with a new helper
> called put_cxl_port() to drop open coded the put_device() used to
> dereference the 'struct device' of a cxl_port.
>
> Suggested-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Li Ming <ming4.li@intel.com>
> ---
> drivers/cxl/core/pci.c | 6 ++----
> drivers/cxl/core/port.c | 25 +++++++++----------------
> drivers/cxl/cxl.h | 2 ++
> drivers/cxl/mem.c | 5 ++---
> drivers/cxl/pci.c | 7 ++-----
> 5 files changed, 17 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index 51132a575b27..4725e37d90fb 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -915,15 +915,13 @@ static void cxl_handle_rdport_errors(struct cxl_dev_state *cxlds)
> struct pci_dev *pdev = to_pci_dev(cxlds->dev);
> struct aer_capability_regs aer_regs;
> struct cxl_dport *dport;
> - struct cxl_port *port;
> int severity;
>
> - port = cxl_pci_find_port(pdev, &dport);
> + struct cxl_port *port __free(put_cxl_port) =
> + cxl_pci_find_port(pdev, &dport);
> if (!port)
> return;
>
> - put_device(&port->dev);
I don't think this is wrong but we are not holding the lock for the
duration of the function where before we were not.
It seems like this is somewhat an abuse of the cxl_pci_find_port() call in
that we don't really need the cxl_port reference but rather the dport... :-/
> -
> if (!cxl_rch_get_aer_info(dport->regs.dport_aer, &aer_regs))
> return;
>
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index 1d5007e3795a..6119cb3ad25c 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -1472,7 +1472,7 @@ static void cxl_detach_ep(void *data)
> struct cxl_memdev *cxlmd = data;
>
> for (int i = cxlmd->depth - 1; i >= 1; i--) {
> - struct cxl_port *port, *parent_port;
> + struct cxl_port *parent_port;
> struct detach_ctx ctx = {
> .cxlmd = cxlmd,
> .depth = i,
> @@ -1485,7 +1485,7 @@ static void cxl_detach_ep(void *data)
> port_has_memdev);
> if (!dev)
> continue;
> - port = to_cxl_port(dev);
> + struct cxl_port *port __free(put_cxl_port) = to_cxl_port(dev);
This threw me because 'to_cxl_port' does not take a reference...
Seems ok though.
>
> parent_port = to_cxl_port(port->dev.parent);
> device_lock(&parent_port->dev);
> @@ -1512,7 +1512,6 @@ static void cxl_detach_ep(void *data)
> dev_name(&port->dev));
> delete_switch_port(port);
> }
> - put_device(&port->dev);
> device_unlock(&parent_port->dev);
> }
> }
> @@ -1539,8 +1538,8 @@ static int add_port_attach_ep(struct cxl_memdev *cxlmd,
> struct device *uport_dev,
> struct device *dport_dev)
> {
> + struct cxl_port *port __free(put_cxl_port) = NULL;
> struct device *dparent = grandparent(dport_dev);
> - struct cxl_port *port, *parent_port = NULL;
> struct cxl_dport *dport, *parent_dport;
> resource_size_t component_reg_phys;
> int rc;
> @@ -1556,7 +1555,8 @@ static int add_port_attach_ep(struct cxl_memdev *cxlmd,
> return -ENXIO;
> }
>
> - parent_port = find_cxl_port(dparent, &parent_dport);
> + struct cxl_port *parent_port __free(put_cxl_port) =
> + find_cxl_port(dparent, &parent_dport);
> if (!parent_port) {
> /* iterate to create this parent_port */
> return -EAGAIN;
> @@ -1596,10 +1596,8 @@ static int add_port_attach_ep(struct cxl_memdev *cxlmd,
> */
> rc = -ENXIO;
> }
> - put_device(&port->dev);
> }
>
> - put_device(&parent_port->dev);
> return rc;
> }
>
> @@ -1630,7 +1628,6 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd)
> struct device *dport_dev = grandparent(iter);
> struct device *uport_dev;
> struct cxl_dport *dport;
> - struct cxl_port *port;
>
> /*
> * The terminal "grandparent" in PCI is NULL and @platform_bus
> @@ -1649,7 +1646,8 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd)
> dev_dbg(dev, "scan: iter: %s dport_dev: %s parent: %s\n",
> dev_name(iter), dev_name(dport_dev),
> dev_name(uport_dev));
> - port = find_cxl_port(dport_dev, &dport);
> + struct cxl_port *port __free(put_cxl_port) =
Does __free() get called before the next iteration of the loop? I guess
it does because it would be out of scope outside the loop?
Ira
> + find_cxl_port(dport_dev, &dport);
> if (port) {
> dev_dbg(&cxlmd->dev,
> "found already registered port %s:%s\n",
> @@ -1664,18 +1662,13 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd)
> * the parent_port lock as the current port may be being
> * reaped.
> */
> - if (rc && rc != -EBUSY) {
> - put_device(&port->dev);
> + if (rc && rc != -EBUSY)
> return rc;
> - }
>
> /* Any more ports to add between this one and the root? */
> - if (!dev_is_cxl_root_child(&port->dev)) {
> - put_device(&port->dev);
> + if (!dev_is_cxl_root_child(&port->dev))
> continue;
> - }
>
> - put_device(&port->dev);
> return 0;
> }
>
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 9afb407d438f..cad297fba700 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -744,6 +744,8 @@ struct cxl_root *find_cxl_root(struct cxl_port *port);
> void put_cxl_root(struct cxl_root *cxl_root);
> DEFINE_FREE(put_cxl_root, struct cxl_root *, if (_T) put_cxl_root(_T))
>
> +DEFINE_FREE(put_cxl_port, struct cxl_port *,
> + if (!IS_ERR_OR_NULL(_T)) put_device(&_T->dev))
> int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd);
> void cxl_bus_rescan(void);
> void cxl_bus_drain(void);
> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> index 7de232eaeb17..ab9b8ab8df44 100644
> --- a/drivers/cxl/mem.c
> +++ b/drivers/cxl/mem.c
> @@ -109,7 +109,6 @@ static int cxl_mem_probe(struct device *dev)
> struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlmd->cxlds);
> struct cxl_dev_state *cxlds = cxlmd->cxlds;
> struct device *endpoint_parent;
> - struct cxl_port *parent_port;
> struct cxl_dport *dport;
> struct dentry *dentry;
> int rc;
> @@ -146,7 +145,8 @@ static int cxl_mem_probe(struct device *dev)
> if (rc)
> return rc;
>
> - parent_port = cxl_mem_find_port(cxlmd, &dport);
> + struct cxl_port *parent_port __free(put_cxl_port) =
> + cxl_mem_find_port(cxlmd, &dport);
> if (!parent_port) {
> dev_err(dev, "CXL port topology not found\n");
> return -ENXIO;
> @@ -179,7 +179,6 @@ static int cxl_mem_probe(struct device *dev)
> rc = devm_cxl_add_endpoint(endpoint_parent, cxlmd, dport);
> unlock:
> device_unlock(endpoint_parent);
> - put_device(&parent_port->dev);
> if (rc)
> return rc;
>
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 4be35dc22202..26e75499abdd 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -473,7 +473,6 @@ static bool is_cxl_restricted(struct pci_dev *pdev)
> static int cxl_rcrb_get_comp_regs(struct pci_dev *pdev,
> struct cxl_register_map *map)
> {
> - struct cxl_port *port;
> struct cxl_dport *dport;
> resource_size_t component_reg_phys;
>
> @@ -482,14 +481,12 @@ static int cxl_rcrb_get_comp_regs(struct pci_dev *pdev,
> .resource = CXL_RESOURCE_NONE,
> };
>
> - port = cxl_pci_find_port(pdev, &dport);
> + struct cxl_port *port __free(put_cxl_port) =
> + cxl_pci_find_port(pdev, &dport);
> if (!port)
> return -EPROBE_DEFER;
>
> component_reg_phys = cxl_rcd_component_reg_phys(&pdev->dev, dport);
> -
> - put_device(&port->dev);
> -
> if (component_reg_phys == CXL_RESOURCE_NONE)
> return -ENXIO;
>
> --
> 2.40.1
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/3] cxl/port: Use scoped_guard() to drop device_lock()/unlock pair
2024-08-13 7:05 ` [PATCH 2/3] cxl/port: Use scoped_guard() to drop device_lock()/unlock pair Li Ming
@ 2024-08-21 21:49 ` Ira Weiny
2024-08-22 1:47 ` Li, Ming4
0 siblings, 1 reply; 9+ messages in thread
From: Ira Weiny @ 2024-08-21 21:49 UTC (permalink / raw)
To: Li Ming, dave, jonathan.cameron, dave.jiang, alison.schofield,
vishal.l.verma, ira.weiny, dan.j.williams
Cc: linux-cxl, Li Ming
Li Ming wrote:
> A device_lock() and device_unlock() pair can be replaced by a
> scope-based resource management function scoped_guard() which can make
> the code more readable and safer. Some device_lock() and device_unlock()
> pairs in CXL subsystem can be simply replaced by a scoped_guard().
>
> Suggested-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Li Ming <ming4.li@intel.com>
> ---
> drivers/cxl/core/mbox.c | 26 ++++++------
> drivers/cxl/core/port.c | 87 ++++++++++++++++++---------------------
> drivers/cxl/core/region.c | 25 ++++++-----
> drivers/cxl/mem.c | 22 +++++-----
> drivers/cxl/pmem.c | 16 ++++---
> 5 files changed, 82 insertions(+), 94 deletions(-)
>
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index e5cdeafdf76e..a8ab5f2697be 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -1214,19 +1214,19 @@ int cxl_mem_sanitize(struct cxl_memdev *cxlmd, u16 cmd)
> int rc;
>
> /* synchronize with cxl_mem_probe() and decoder write operations */
> - device_lock(&cxlmd->dev);
> - endpoint = cxlmd->endpoint;
> - down_read(&cxl_region_rwsem);
> - /*
> - * Require an endpoint to be safe otherwise the driver can not
> - * be sure that the device is unmapped.
> - */
> - if (endpoint && cxl_num_decoders_committed(endpoint) == 0)
> - rc = __cxl_mem_sanitize(mds, cmd);
> - else
> - rc = -EBUSY;
> - up_read(&cxl_region_rwsem);
> - device_unlock(&cxlmd->dev);
> + scoped_guard(device, &cxlmd->dev) {
> + endpoint = cxlmd->endpoint;
> + down_read(&cxl_region_rwsem);
> + /*
> + * Require an endpoint to be safe otherwise the driver can not
> + * be sure that the device is unmapped.
> + */
> + if (endpoint && cxl_num_decoders_committed(endpoint) == 0)
> + rc = __cxl_mem_sanitize(mds, cmd);
> + else
> + rc = -EBUSY;
> + up_read(&cxl_region_rwsem);
> + }
>
> return rc;
> }
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index 6119cb3ad25c..53e2593daa95 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -1260,14 +1260,12 @@ static int add_ep(struct cxl_ep *new)
> struct cxl_port *port = new->dport->port;
> int rc;
>
> - device_lock(&port->dev);
> - if (port->dead) {
> - device_unlock(&port->dev);
> - return -ENXIO;
> + scoped_guard(device, &port->dev) {
> + if (port->dead)
> + return -ENXIO;
> + rc = xa_insert(&port->endpoints, (unsigned long)new->ep, new,
> + GFP_KERNEL);
> }
> - rc = xa_insert(&port->endpoints, (unsigned long)new->ep, new,
> - GFP_KERNEL);
> - device_unlock(&port->dev);
>
> return rc;
> }
> @@ -1393,14 +1391,14 @@ static void delete_endpoint(void *data)
> struct cxl_port *endpoint = cxlmd->endpoint;
> struct device *host = endpoint_host(endpoint);
>
> - device_lock(host);
> - if (host->driver && !endpoint->dead) {
> - devm_release_action(host, cxl_unlink_parent_dport, endpoint);
> - devm_release_action(host, cxl_unlink_uport, endpoint);
> - devm_release_action(host, unregister_port, endpoint);
> + scoped_guard(device, host) {
> + if (host->driver && !endpoint->dead) {
> + devm_release_action(host, cxl_unlink_parent_dport, endpoint);
> + devm_release_action(host, cxl_unlink_uport, endpoint);
> + devm_release_action(host, unregister_port, endpoint);
> + }
> + cxlmd->endpoint = NULL;
> }
> - cxlmd->endpoint = NULL;
> - device_unlock(host);
> put_device(&endpoint->dev);
> put_device(host);
> }
> @@ -1562,40 +1560,36 @@ static int add_port_attach_ep(struct cxl_memdev *cxlmd,
> return -EAGAIN;
> }
>
> - device_lock(&parent_port->dev);
> - if (!parent_port->dev.driver) {
> - dev_warn(&cxlmd->dev,
> - "port %s:%s disabled, failed to enumerate CXL.mem\n",
> - dev_name(&parent_port->dev), dev_name(uport_dev));
> - port = ERR_PTR(-ENXIO);
> - goto out;
> - }
> + scoped_guard(device, &parent_port->dev) {
> + if (!parent_port->dev.driver) {
> + dev_warn(&cxlmd->dev,
> + "port %s:%s disabled, failed to enumerate CXL.mem\n",
> + dev_name(&parent_port->dev), dev_name(uport_dev));
> + return -ENXIO;
> + }
>
> - port = find_cxl_port_at(parent_port, dport_dev, &dport);
> - if (!port) {
> - component_reg_phys = find_component_registers(uport_dev);
> - port = devm_cxl_add_port(&parent_port->dev, uport_dev,
> - component_reg_phys, parent_dport);
> - /* retry find to pick up the new dport information */
> - if (!IS_ERR(port))
> + port = find_cxl_port_at(parent_port, dport_dev, &dport);
> + if (!port) {
> + component_reg_phys = find_component_registers(uport_dev);
> + port = devm_cxl_add_port(&parent_port->dev, uport_dev,
> + component_reg_phys, parent_dport);
> + if (IS_ERR(port))
> + return PTR_ERR(port);
> +
> + /* retry find to pick up the new dport information */
> port = find_cxl_port_at(parent_port, dport_dev, &dport);
Can this call to find_cxl_port_at() fail?
> + }
> }
> -out:
> - device_unlock(&parent_port->dev);
>
> - if (IS_ERR(port))
^^^^^^^^^^^^
Does this need to be checked above?
It seems like the logic is changing for the call to cxl_add_ep() but it
might be ok?
Ira
> - rc = PTR_ERR(port);
> - else {
> - dev_dbg(&cxlmd->dev, "add to new port %s:%s\n",
> - dev_name(&port->dev), dev_name(port->uport_dev));
> - rc = cxl_add_ep(dport, &cxlmd->dev);
> - if (rc == -EBUSY) {
> - /*
> - * "can't" happen, but this error code means
> - * something to the caller, so translate it.
> - */
> - rc = -ENXIO;
> - }
> + dev_dbg(&cxlmd->dev, "add to new port %s:%s\n",
> + dev_name(&port->dev), dev_name(port->uport_dev));
> + rc = cxl_add_ep(dport, &cxlmd->dev);
> + if (rc == -EBUSY) {
> + /*
> + * "can't" happen, but this error code means
> + * something to the caller, so translate it.
> + */
> + rc = -ENXIO;
> }
>
> return rc;
> @@ -1986,9 +1980,8 @@ int cxl_decoder_add(struct cxl_decoder *cxld, int *target_map)
>
> port = to_cxl_port(cxld->dev.parent);
>
> - device_lock(&port->dev);
> - rc = cxl_decoder_add_locked(cxld, target_map);
> - device_unlock(&port->dev);
> + scoped_guard(device, &port->dev)
> + rc = cxl_decoder_add_locked(cxld, target_map);
>
> return rc;
> }
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 21ad5f242875..a3ce7cbe5c6c 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -3094,11 +3094,11 @@ 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);
> + scoped_guard(device, &cxl_nvb->dev) {
> + if (cxlr->cxlr_pmem)
> + devm_release_action(&cxl_nvb->dev, cxlr_pmem_unregister,
> + cxlr->cxlr_pmem);
> + }
> cxlr->cxl_nvb = NULL;
> put_device(&cxl_nvb->dev);
> }
> @@ -3134,14 +3134,13 @@ static int devm_cxl_add_pmem_region(struct cxl_region *cxlr)
> dev_dbg(&cxlr->dev, "%s: register %s\n", dev_name(dev->parent),
> dev_name(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);
> -
> + scoped_guard(device, &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;
> + }
> if (rc)
> goto err_bridge;
>
> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> index ab9b8ab8df44..ae94018a01bd 100644
> --- a/drivers/cxl/mem.c
> +++ b/drivers/cxl/mem.c
> @@ -168,19 +168,17 @@ static int cxl_mem_probe(struct device *dev)
>
> cxl_setup_parent_dport(dev, dport);
>
> - device_lock(endpoint_parent);
> - if (!endpoint_parent->driver) {
> - dev_err(dev, "CXL port topology %s not enabled\n",
> - dev_name(endpoint_parent));
> - rc = -ENXIO;
> - goto unlock;
> - }
> + scoped_guard(device, endpoint_parent) {
> + if (!endpoint_parent->driver) {
> + dev_err(dev, "CXL port topology %s not enabled\n",
> + dev_name(endpoint_parent));
> + return -ENXIO;
> + }
>
> - rc = devm_cxl_add_endpoint(endpoint_parent, cxlmd, dport);
> -unlock:
> - device_unlock(endpoint_parent);
> - if (rc)
> - return rc;
> + rc = devm_cxl_add_endpoint(endpoint_parent, cxlmd, dport);
> + if (rc)
> + return rc;
> + }
>
> /*
> * The kernel may be operating out of CXL memory on this device,
> diff --git a/drivers/cxl/pmem.c b/drivers/cxl/pmem.c
> index 4ef93da22335..647c7e25ef3a 100644
> --- a/drivers/cxl/pmem.c
> +++ b/drivers/cxl/pmem.c
> @@ -233,15 +233,13 @@ static int detach_nvdimm(struct device *dev, void *data)
> if (!is_cxl_nvdimm(dev))
> return 0;
>
> - device_lock(dev);
> - if (!dev->driver)
> - goto out;
> -
> - cxl_nvd = to_cxl_nvdimm(dev);
> - if (cxl_nvd->cxlmd && cxl_nvd->cxlmd->cxl_nvb == data)
> - release = true;
> -out:
> - device_unlock(dev);
> + scoped_guard(device, dev) {
> + if (dev->driver) {
> + cxl_nvd = to_cxl_nvdimm(dev);
> + if (cxl_nvd->cxlmd && cxl_nvd->cxlmd->cxl_nvb == data)
> + release = true;
> + }
> + }
> if (release)
> device_release_driver(dev);
> return 0;
> --
> 2.40.1
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] cxl/port: Refactor __devm_cxl_add_port() to drop goto pattern
2024-08-13 7:05 ` [PATCH 3/3] cxl/port: Refactor __devm_cxl_add_port() to drop goto pattern Li Ming
@ 2024-08-21 22:01 ` Ira Weiny
2024-08-22 2:07 ` Li, Ming4
0 siblings, 1 reply; 9+ messages in thread
From: Ira Weiny @ 2024-08-21 22:01 UTC (permalink / raw)
To: Li Ming, dave, jonathan.cameron, dave.jiang, alison.schofield,
vishal.l.verma, ira.weiny, dan.j.williams
Cc: linux-cxl, Li Ming
Li Ming wrote:
> The "goto error" pattern is not recommended, it can be removed via
> refactoring. In __devm_cxl_add_port(), there is a 'goto' to call
> put_device() for the error cases between device_initialize() and
> device_add() to dereference the 'struct device' of a new cxl_port.
> The refactoring is introducing a new function called cxl_port_add()
> which is used to add the 'struct device' of a new cxl_port to
> device hierarchy, moving the functions needing the help of above
> 'goto' into cxl_port_add(), and using a scope-based resource management
> __free() to drop the open coded put_device() and 'goto' for the error
> cases.
>
> Suggested-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Li Ming <ming4.li@intel.com>
> ---
> drivers/cxl/core/port.c | 59 ++++++++++++++++++++++++-----------------
> 1 file changed, 35 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index 53e2593daa95..a886b16b2610 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -828,27 +828,20 @@ static void cxl_debugfs_create_dport_dir(struct cxl_dport *dport)
> &cxl_einj_inject_fops);
> }
>
> -static struct cxl_port *__devm_cxl_add_port(struct device *host,
> - struct device *uport_dev,
> - resource_size_t component_reg_phys,
> - struct cxl_dport *parent_dport)
> +static int cxl_port_add(struct cxl_port *port,
> + resource_size_t component_reg_phys,
> + struct cxl_dport *parent_dport)
> {
> - struct cxl_port *port;
> - struct device *dev;
> + struct device *dev __free(put_device) = &port->dev;
> int rc;
>
> - port = cxl_port_alloc(uport_dev, parent_dport);
> - if (IS_ERR(port))
> - return port;
> -
> - dev = &port->dev;
> - if (is_cxl_memdev(uport_dev)) {
> - struct cxl_memdev *cxlmd = to_cxl_memdev(uport_dev);
> + if (is_cxl_memdev(port->uport_dev)) {
> + struct cxl_memdev *cxlmd = to_cxl_memdev(port->uport_dev);
> struct cxl_dev_state *cxlds = cxlmd->cxlds;
>
> rc = dev_set_name(dev, "endpoint%d", port->id);
> if (rc)
> - goto err;
> + return rc;
>
> /*
> * The endpoint driver already enumerated the component and RAS
> @@ -861,19 +854,41 @@ static struct cxl_port *__devm_cxl_add_port(struct device *host,
> } else if (parent_dport) {
> rc = dev_set_name(dev, "port%d", port->id);
> if (rc)
> - goto err;
> + return rc;
>
> rc = cxl_port_setup_regs(port, component_reg_phys);
> if (rc)
> - goto err;
> - } else
> + return rc;
> + } else {
> rc = dev_set_name(dev, "root%d", port->id);
> - if (rc)
> - goto err;
> + if (rc)
> + return rc;
> + }
>
> rc = device_add(dev);
> if (rc)
> - goto err;
> + return rc;
> +
> + /* Inhibit the cleanup function invoked */
> + dev = NULL;
I'm tempted to say we should use no_free_ptr() here. But I don't think
there is any magic we need in there.
So.
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
> + return 0;
> +}
> +
> +static struct cxl_port *__devm_cxl_add_port(struct device *host,
> + struct device *uport_dev,
> + resource_size_t component_reg_phys,
> + struct cxl_dport *parent_dport)
> +{
> + struct cxl_port *port;
> + int rc;
> +
> + port = cxl_port_alloc(uport_dev, parent_dport);
> + if (IS_ERR(port))
> + return port;
> +
> + rc = cxl_port_add(port, component_reg_phys, parent_dport);
> + if (rc)
> + return ERR_PTR(rc);
>
> rc = devm_add_action_or_reset(host, unregister_port, port);
> if (rc)
> @@ -891,10 +906,6 @@ static struct cxl_port *__devm_cxl_add_port(struct device *host,
> port->pci_latency = cxl_pci_get_latency(to_pci_dev(uport_dev));
>
> return port;
> -
> -err:
> - put_device(dev);
> - return ERR_PTR(rc);
> }
>
> /**
> --
> 2.40.1
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] cxl/port: Use __free() to drop put_device() for cxl_port
2024-08-21 21:37 ` [PATCH 1/3] cxl/port: Use __free() to drop put_device() for cxl_port Ira Weiny
@ 2024-08-22 1:26 ` Li, Ming4
0 siblings, 0 replies; 9+ messages in thread
From: Li, Ming4 @ 2024-08-22 1:26 UTC (permalink / raw)
To: Ira Weiny, dave, jonathan.cameron, dave.jiang, alison.schofield,
vishal.l.verma, dan.j.williams
Cc: linux-cxl
On 8/22/2024 5:37 AM, Ira Weiny wrote:
> Li Ming wrote:
>> Using scope-based resource management __free() marco with a new helper
>> called put_cxl_port() to drop open coded the put_device() used to
>> dereference the 'struct device' of a cxl_port.
>>
>> Suggested-by: Dan Williams <dan.j.williams@intel.com>
>> Signed-off-by: Li Ming <ming4.li@intel.com>
>> ---
>> drivers/cxl/core/pci.c | 6 ++----
>> drivers/cxl/core/port.c | 25 +++++++++----------------
>> drivers/cxl/cxl.h | 2 ++
>> drivers/cxl/mem.c | 5 ++---
>> drivers/cxl/pci.c | 7 ++-----
>> 5 files changed, 17 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
>> index 51132a575b27..4725e37d90fb 100644
>> --- a/drivers/cxl/core/pci.c
>> +++ b/drivers/cxl/core/pci.c
>> @@ -915,15 +915,13 @@ static void cxl_handle_rdport_errors(struct cxl_dev_state *cxlds)
>> struct pci_dev *pdev = to_pci_dev(cxlds->dev);
>> struct aer_capability_regs aer_regs;
>> struct cxl_dport *dport;
>> - struct cxl_port *port;
>> int severity;
>>
>> - port = cxl_pci_find_port(pdev, &dport);
>> + struct cxl_port *port __free(put_cxl_port) =
>> + cxl_pci_find_port(pdev, &dport);
>> if (!port)
>> return;
>>
>> - put_device(&port->dev);
> I don't think this is wrong but we are not holding the lock for the
> duration of the function where before we were not.
>
> It seems like this is somewhat an abuse of the cxl_pci_find_port() call in
> that we don't really need the cxl_port reference but rather the dport... :-/
Thank you for the review.
Yes, seems like that, but CXL core does not provide a helper function to find a dport via pci device without getting the cxl_port reference yet.
>> -
>> if (!cxl_rch_get_aer_info(dport->regs.dport_aer, &aer_regs))
>> return;
>>
>> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
>> index 1d5007e3795a..6119cb3ad25c 100644
>> --- a/drivers/cxl/core/port.c
>> +++ b/drivers/cxl/core/port.c
>> @@ -1472,7 +1472,7 @@ static void cxl_detach_ep(void *data)
>> struct cxl_memdev *cxlmd = data;
>>
>> for (int i = cxlmd->depth - 1; i >= 1; i--) {
>> - struct cxl_port *port, *parent_port;
>> + struct cxl_port *parent_port;
>> struct detach_ctx ctx = {
>> .cxlmd = cxlmd,
>> .depth = i,
>> @@ -1485,7 +1485,7 @@ static void cxl_detach_ep(void *data)
>> port_has_memdev);
>> if (!dev)
>> continue;
>> - port = to_cxl_port(dev);
>> + struct cxl_port *port __free(put_cxl_port) = to_cxl_port(dev);
> This threw me because 'to_cxl_port' does not take a reference...
>
> Seems ok though.
Yes, to_cxl_port() does not take a reference here, but the 'dev' is provided by a bus_find_device() which will take a reference for the 'dev'.
maybe I should use __free() for the 'dev' like below? I think it is clearer.
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index 1d5007e3795a..e4bff611e8fa 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -1481,8 +1481,8 @@ static void cxl_detach_ep(void *data)
struct cxl_ep *ep;
bool died = false;
- dev = bus_find_device(&cxl_bus_type, NULL, &ctx,
- port_has_memdev);
+ struct device *dev __free(put_device) = bus_find_device(&cxl_bus_type, NULL,
+ &ctx, port_has_memdev);
if (!dev)
continue;
port = to_cxl_port(dev);
@@ -1512,7 +1512,6 @@ static void cxl_detach_ep(void *data)
dev_name(&port->dev));
delete_switch_port(port);
}
- put_device(&port->dev);
device_unlock(&parent_port->dev);
}
}
>
>>
>> parent_port = to_cxl_port(port->dev.parent);
>> device_lock(&parent_port->dev);
>> @@ -1512,7 +1512,6 @@ static void cxl_detach_ep(void *data)
>> dev_name(&port->dev));
>> delete_switch_port(port);
>> }
>> - put_device(&port->dev);
>> device_unlock(&parent_port->dev);
>> }
>> }
>> @@ -1539,8 +1538,8 @@ static int add_port_attach_ep(struct cxl_memdev *cxlmd,
>> struct device *uport_dev,
>> struct device *dport_dev)
>> {
>> + struct cxl_port *port __free(put_cxl_port) = NULL;
>> struct device *dparent = grandparent(dport_dev);
>> - struct cxl_port *port, *parent_port = NULL;
>> struct cxl_dport *dport, *parent_dport;
>> resource_size_t component_reg_phys;
>> int rc;
>> @@ -1556,7 +1555,8 @@ static int add_port_attach_ep(struct cxl_memdev *cxlmd,
>> return -ENXIO;
>> }
>>
>> - parent_port = find_cxl_port(dparent, &parent_dport);
>> + struct cxl_port *parent_port __free(put_cxl_port) =
>> + find_cxl_port(dparent, &parent_dport);
>> if (!parent_port) {
>> /* iterate to create this parent_port */
>> return -EAGAIN;
>> @@ -1596,10 +1596,8 @@ static int add_port_attach_ep(struct cxl_memdev *cxlmd,
>> */
>> rc = -ENXIO;
>> }
>> - put_device(&port->dev);
>> }
>>
>> - put_device(&parent_port->dev);
>> return rc;
>> }
>>
>> @@ -1630,7 +1628,6 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd)
>> struct device *dport_dev = grandparent(iter);
>> struct device *uport_dev;
>> struct cxl_dport *dport;
>> - struct cxl_port *port;
>>
>> /*
>> * The terminal "grandparent" in PCI is NULL and @platform_bus
>> @@ -1649,7 +1646,8 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd)
>> dev_dbg(dev, "scan: iter: %s dport_dev: %s parent: %s\n",
>> dev_name(iter), dev_name(dport_dev),
>> dev_name(uport_dev));
>> - port = find_cxl_port(dport_dev, &dport);
>> + struct cxl_port *port __free(put_cxl_port) =
> Does __free() get called before the next iteration of the loop? I guess
> it does because it would be out of scope outside the loop?
>
> Ira
Yes, It will get called before the next iteration of the loop. I have validated it.
>
>> + find_cxl_port(dport_dev, &dport);
>> if (port) {
>> dev_dbg(&cxlmd->dev,
>> "found already registered port %s:%s\n",
>> @@ -1664,18 +1662,13 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd)
>> * the parent_port lock as the current port may be being
>> * reaped.
>> */
>> - if (rc && rc != -EBUSY) {
>> - put_device(&port->dev);
>> + if (rc && rc != -EBUSY)
>> return rc;
>> - }
>>
>> /* Any more ports to add between this one and the root? */
>> - if (!dev_is_cxl_root_child(&port->dev)) {
>> - put_device(&port->dev);
>> + if (!dev_is_cxl_root_child(&port->dev))
>> continue;
>> - }
>>
>> - put_device(&port->dev);
>> return 0;
>> }
>>
>> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
>> index 9afb407d438f..cad297fba700 100644
>> --- a/drivers/cxl/cxl.h
>> +++ b/drivers/cxl/cxl.h
>> @@ -744,6 +744,8 @@ struct cxl_root *find_cxl_root(struct cxl_port *port);
>> void put_cxl_root(struct cxl_root *cxl_root);
>> DEFINE_FREE(put_cxl_root, struct cxl_root *, if (_T) put_cxl_root(_T))
>>
>> +DEFINE_FREE(put_cxl_port, struct cxl_port *,
>> + if (!IS_ERR_OR_NULL(_T)) put_device(&_T->dev))
>> int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd);
>> void cxl_bus_rescan(void);
>> void cxl_bus_drain(void);
>> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
>> index 7de232eaeb17..ab9b8ab8df44 100644
>> --- a/drivers/cxl/mem.c
>> +++ b/drivers/cxl/mem.c
>> @@ -109,7 +109,6 @@ static int cxl_mem_probe(struct device *dev)
>> struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlmd->cxlds);
>> struct cxl_dev_state *cxlds = cxlmd->cxlds;
>> struct device *endpoint_parent;
>> - struct cxl_port *parent_port;
>> struct cxl_dport *dport;
>> struct dentry *dentry;
>> int rc;
>> @@ -146,7 +145,8 @@ static int cxl_mem_probe(struct device *dev)
>> if (rc)
>> return rc;
>>
>> - parent_port = cxl_mem_find_port(cxlmd, &dport);
>> + struct cxl_port *parent_port __free(put_cxl_port) =
>> + cxl_mem_find_port(cxlmd, &dport);
>> if (!parent_port) {
>> dev_err(dev, "CXL port topology not found\n");
>> return -ENXIO;
>> @@ -179,7 +179,6 @@ static int cxl_mem_probe(struct device *dev)
>> rc = devm_cxl_add_endpoint(endpoint_parent, cxlmd, dport);
>> unlock:
>> device_unlock(endpoint_parent);
>> - put_device(&parent_port->dev);
>> if (rc)
>> return rc;
>>
>> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
>> index 4be35dc22202..26e75499abdd 100644
>> --- a/drivers/cxl/pci.c
>> +++ b/drivers/cxl/pci.c
>> @@ -473,7 +473,6 @@ static bool is_cxl_restricted(struct pci_dev *pdev)
>> static int cxl_rcrb_get_comp_regs(struct pci_dev *pdev,
>> struct cxl_register_map *map)
>> {
>> - struct cxl_port *port;
>> struct cxl_dport *dport;
>> resource_size_t component_reg_phys;
>>
>> @@ -482,14 +481,12 @@ static int cxl_rcrb_get_comp_regs(struct pci_dev *pdev,
>> .resource = CXL_RESOURCE_NONE,
>> };
>>
>> - port = cxl_pci_find_port(pdev, &dport);
>> + struct cxl_port *port __free(put_cxl_port) =
>> + cxl_pci_find_port(pdev, &dport);
>> if (!port)
>> return -EPROBE_DEFER;
>>
>> component_reg_phys = cxl_rcd_component_reg_phys(&pdev->dev, dport);
>> -
>> - put_device(&port->dev);
>> -
>> if (component_reg_phys == CXL_RESOURCE_NONE)
>> return -ENXIO;
>>
>> --
>> 2.40.1
>>
>
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/3] cxl/port: Use scoped_guard() to drop device_lock()/unlock pair
2024-08-21 21:49 ` Ira Weiny
@ 2024-08-22 1:47 ` Li, Ming4
0 siblings, 0 replies; 9+ messages in thread
From: Li, Ming4 @ 2024-08-22 1:47 UTC (permalink / raw)
To: Ira Weiny, dave, jonathan.cameron, dave.jiang, alison.schofield,
vishal.l.verma, dan.j.williams
Cc: linux-cxl
On 8/22/2024 5:49 AM, Ira Weiny wrote:
> Li Ming wrote:
>> A device_lock() and device_unlock() pair can be replaced by a
>> scope-based resource management function scoped_guard() which can make
>> the code more readable and safer. Some device_lock() and device_unlock()
>> pairs in CXL subsystem can be simply replaced by a scoped_guard().
>>
>> Suggested-by: Dan Williams <dan.j.williams@intel.com>
>> Signed-off-by: Li Ming <ming4.li@intel.com>
>> ---
>> drivers/cxl/core/mbox.c | 26 ++++++------
>> drivers/cxl/core/port.c | 87 ++++++++++++++++++---------------------
>> drivers/cxl/core/region.c | 25 ++++++-----
>> drivers/cxl/mem.c | 22 +++++-----
>> drivers/cxl/pmem.c | 16 ++++---
>> 5 files changed, 82 insertions(+), 94 deletions(-)
>>
>> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
>> index e5cdeafdf76e..a8ab5f2697be 100644
>> --- a/drivers/cxl/core/mbox.c
>> +++ b/drivers/cxl/core/mbox.c
>> @@ -1214,19 +1214,19 @@ int cxl_mem_sanitize(struct cxl_memdev *cxlmd, u16 cmd)
>> int rc;
>>
>> /* synchronize with cxl_mem_probe() and decoder write operations */
>> - device_lock(&cxlmd->dev);
>> - endpoint = cxlmd->endpoint;
>> - down_read(&cxl_region_rwsem);
>> - /*
>> - * Require an endpoint to be safe otherwise the driver can not
>> - * be sure that the device is unmapped.
>> - */
>> - if (endpoint && cxl_num_decoders_committed(endpoint) == 0)
>> - rc = __cxl_mem_sanitize(mds, cmd);
>> - else
>> - rc = -EBUSY;
>> - up_read(&cxl_region_rwsem);
>> - device_unlock(&cxlmd->dev);
>> + scoped_guard(device, &cxlmd->dev) {
>> + endpoint = cxlmd->endpoint;
>> + down_read(&cxl_region_rwsem);
>> + /*
>> + * Require an endpoint to be safe otherwise the driver can not
>> + * be sure that the device is unmapped.
>> + */
>> + if (endpoint && cxl_num_decoders_committed(endpoint) == 0)
>> + rc = __cxl_mem_sanitize(mds, cmd);
>> + else
>> + rc = -EBUSY;
>> + up_read(&cxl_region_rwsem);
>> + }
>>
>> return rc;
>> }
>> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
>> index 6119cb3ad25c..53e2593daa95 100644
>> --- a/drivers/cxl/core/port.c
>> +++ b/drivers/cxl/core/port.c
>> @@ -1260,14 +1260,12 @@ static int add_ep(struct cxl_ep *new)
>> struct cxl_port *port = new->dport->port;
>> int rc;
>>
>> - device_lock(&port->dev);
>> - if (port->dead) {
>> - device_unlock(&port->dev);
>> - return -ENXIO;
>> + scoped_guard(device, &port->dev) {
>> + if (port->dead)
>> + return -ENXIO;
>> + rc = xa_insert(&port->endpoints, (unsigned long)new->ep, new,
>> + GFP_KERNEL);
>> }
>> - rc = xa_insert(&port->endpoints, (unsigned long)new->ep, new,
>> - GFP_KERNEL);
>> - device_unlock(&port->dev);
>>
>> return rc;
>> }
>> @@ -1393,14 +1391,14 @@ static void delete_endpoint(void *data)
>> struct cxl_port *endpoint = cxlmd->endpoint;
>> struct device *host = endpoint_host(endpoint);
>>
>> - device_lock(host);
>> - if (host->driver && !endpoint->dead) {
>> - devm_release_action(host, cxl_unlink_parent_dport, endpoint);
>> - devm_release_action(host, cxl_unlink_uport, endpoint);
>> - devm_release_action(host, unregister_port, endpoint);
>> + scoped_guard(device, host) {
>> + if (host->driver && !endpoint->dead) {
>> + devm_release_action(host, cxl_unlink_parent_dport, endpoint);
>> + devm_release_action(host, cxl_unlink_uport, endpoint);
>> + devm_release_action(host, unregister_port, endpoint);
>> + }
>> + cxlmd->endpoint = NULL;
>> }
>> - cxlmd->endpoint = NULL;
>> - device_unlock(host);
>> put_device(&endpoint->dev);
>> put_device(host);
>> }
>> @@ -1562,40 +1560,36 @@ static int add_port_attach_ep(struct cxl_memdev *cxlmd,
>> return -EAGAIN;
>> }
>>
>> - device_lock(&parent_port->dev);
>> - if (!parent_port->dev.driver) {
>> - dev_warn(&cxlmd->dev,
>> - "port %s:%s disabled, failed to enumerate CXL.mem\n",
>> - dev_name(&parent_port->dev), dev_name(uport_dev));
>> - port = ERR_PTR(-ENXIO);
>> - goto out;
>> - }
>> + scoped_guard(device, &parent_port->dev) {
>> + if (!parent_port->dev.driver) {
>> + dev_warn(&cxlmd->dev,
>> + "port %s:%s disabled, failed to enumerate CXL.mem\n",
>> + dev_name(&parent_port->dev), dev_name(uport_dev));
>> + return -ENXIO;
>> + }
>>
>> - port = find_cxl_port_at(parent_port, dport_dev, &dport);
>> - if (!port) {
>> - component_reg_phys = find_component_registers(uport_dev);
>> - port = devm_cxl_add_port(&parent_port->dev, uport_dev,
>> - component_reg_phys, parent_dport);
>> - /* retry find to pick up the new dport information */
>> - if (!IS_ERR(port))
>> + port = find_cxl_port_at(parent_port, dport_dev, &dport);
>> + if (!port) {
>> + component_reg_phys = find_component_registers(uport_dev);
>> + port = devm_cxl_add_port(&parent_port->dev, uport_dev,
>> + component_reg_phys, parent_dport);
>> + if (IS_ERR(port))
>> + return PTR_ERR(port);
>> +
>> + /* retry find to pick up the new dport information */
>> port = find_cxl_port_at(parent_port, dport_dev, &dport);
> Can this call to find_cxl_port_at() fail?
I have thought about it during making this changes.
I was also confused if the find_cxl_port_at() could fail. my understanding is that this new cxl port is just added before calling find_cxl_port_at() with holding the lock of parent_port, the find_cxl_port_at() will always get the cxl_port in this case. other threads have no chance to release the new cxl_port in this case.
>> + }
>> }
>> -out:
>> - device_unlock(&parent_port->dev);
>>
>> - if (IS_ERR(port))
> ^^^^^^^^^^^^
> Does this need to be checked above?
>
> It seems like the logic is changing for the call to cxl_add_ep() but it
> might be ok?
>
> Ira
If my understanding is correct, you meat that above 'IS_ERR(port)' can help to check the result of the above 'port = find_cxl_port_at(parent_port, dport_dev, &dport)', but actually It cannot, because the return of find_cxl_port_at() is a correct cxl port or a NULL. It can help if it is a 'IS_ERR_OR_NULL()'.
>
>> - rc = PTR_ERR(port);
>> - else {
>> - dev_dbg(&cxlmd->dev, "add to new port %s:%s\n",
>> - dev_name(&port->dev), dev_name(port->uport_dev));
>> - rc = cxl_add_ep(dport, &cxlmd->dev);
>> - if (rc == -EBUSY) {
>> - /*
>> - * "can't" happen, but this error code means
>> - * something to the caller, so translate it.
>> - */
>> - rc = -ENXIO;
>> - }
>> + dev_dbg(&cxlmd->dev, "add to new port %s:%s\n",
>> + dev_name(&port->dev), dev_name(port->uport_dev));
>> + rc = cxl_add_ep(dport, &cxlmd->dev);
>> + if (rc == -EBUSY) {
>> + /*
>> + * "can't" happen, but this error code means
>> + * something to the caller, so translate it.
>> + */
>> + rc = -ENXIO;
>> }
>>
>> return rc;
>> @@ -1986,9 +1980,8 @@ int cxl_decoder_add(struct cxl_decoder *cxld, int *target_map)
>>
>> port = to_cxl_port(cxld->dev.parent);
>>
>> - device_lock(&port->dev);
>> - rc = cxl_decoder_add_locked(cxld, target_map);
>> - device_unlock(&port->dev);
>> + scoped_guard(device, &port->dev)
>> + rc = cxl_decoder_add_locked(cxld, target_map);
>>
>> return rc;
>> }
>> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
>> index 21ad5f242875..a3ce7cbe5c6c 100644
>> --- a/drivers/cxl/core/region.c
>> +++ b/drivers/cxl/core/region.c
>> @@ -3094,11 +3094,11 @@ 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);
>> + scoped_guard(device, &cxl_nvb->dev) {
>> + if (cxlr->cxlr_pmem)
>> + devm_release_action(&cxl_nvb->dev, cxlr_pmem_unregister,
>> + cxlr->cxlr_pmem);
>> + }
>> cxlr->cxl_nvb = NULL;
>> put_device(&cxl_nvb->dev);
>> }
>> @@ -3134,14 +3134,13 @@ static int devm_cxl_add_pmem_region(struct cxl_region *cxlr)
>> dev_dbg(&cxlr->dev, "%s: register %s\n", dev_name(dev->parent),
>> dev_name(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);
>> -
>> + scoped_guard(device, &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;
>> + }
>> if (rc)
>> goto err_bridge;
>>
>> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
>> index ab9b8ab8df44..ae94018a01bd 100644
>> --- a/drivers/cxl/mem.c
>> +++ b/drivers/cxl/mem.c
>> @@ -168,19 +168,17 @@ static int cxl_mem_probe(struct device *dev)
>>
>> cxl_setup_parent_dport(dev, dport);
>>
>> - device_lock(endpoint_parent);
>> - if (!endpoint_parent->driver) {
>> - dev_err(dev, "CXL port topology %s not enabled\n",
>> - dev_name(endpoint_parent));
>> - rc = -ENXIO;
>> - goto unlock;
>> - }
>> + scoped_guard(device, endpoint_parent) {
>> + if (!endpoint_parent->driver) {
>> + dev_err(dev, "CXL port topology %s not enabled\n",
>> + dev_name(endpoint_parent));
>> + return -ENXIO;
>> + }
>>
>> - rc = devm_cxl_add_endpoint(endpoint_parent, cxlmd, dport);
>> -unlock:
>> - device_unlock(endpoint_parent);
>> - if (rc)
>> - return rc;
>> + rc = devm_cxl_add_endpoint(endpoint_parent, cxlmd, dport);
>> + if (rc)
>> + return rc;
>> + }
>>
>> /*
>> * The kernel may be operating out of CXL memory on this device,
>> diff --git a/drivers/cxl/pmem.c b/drivers/cxl/pmem.c
>> index 4ef93da22335..647c7e25ef3a 100644
>> --- a/drivers/cxl/pmem.c
>> +++ b/drivers/cxl/pmem.c
>> @@ -233,15 +233,13 @@ static int detach_nvdimm(struct device *dev, void *data)
>> if (!is_cxl_nvdimm(dev))
>> return 0;
>>
>> - device_lock(dev);
>> - if (!dev->driver)
>> - goto out;
>> -
>> - cxl_nvd = to_cxl_nvdimm(dev);
>> - if (cxl_nvd->cxlmd && cxl_nvd->cxlmd->cxl_nvb == data)
>> - release = true;
>> -out:
>> - device_unlock(dev);
>> + scoped_guard(device, dev) {
>> + if (dev->driver) {
>> + cxl_nvd = to_cxl_nvdimm(dev);
>> + if (cxl_nvd->cxlmd && cxl_nvd->cxlmd->cxl_nvb == data)
>> + release = true;
>> + }
>> + }
>> if (release)
>> device_release_driver(dev);
>> return 0;
>> --
>> 2.40.1
>>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] cxl/port: Refactor __devm_cxl_add_port() to drop goto pattern
2024-08-21 22:01 ` Ira Weiny
@ 2024-08-22 2:07 ` Li, Ming4
0 siblings, 0 replies; 9+ messages in thread
From: Li, Ming4 @ 2024-08-22 2:07 UTC (permalink / raw)
To: Ira Weiny, dave, jonathan.cameron, dave.jiang, alison.schofield,
vishal.l.verma, dan.j.williams
Cc: linux-cxl
On 8/22/2024 6:01 AM, Ira Weiny wrote:
> Li Ming wrote:
>> The "goto error" pattern is not recommended, it can be removed via
>> refactoring. In __devm_cxl_add_port(), there is a 'goto' to call
>> put_device() for the error cases between device_initialize() and
>> device_add() to dereference the 'struct device' of a new cxl_port.
>> The refactoring is introducing a new function called cxl_port_add()
>> which is used to add the 'struct device' of a new cxl_port to
>> device hierarchy, moving the functions needing the help of above
>> 'goto' into cxl_port_add(), and using a scope-based resource management
>> __free() to drop the open coded put_device() and 'goto' for the error
>> cases.
>>
>> Suggested-by: Dan Williams <dan.j.williams@intel.com>
>> Signed-off-by: Li Ming <ming4.li@intel.com>
>> ---
>> drivers/cxl/core/port.c | 59 ++++++++++++++++++++++++-----------------
>> 1 file changed, 35 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
>> index 53e2593daa95..a886b16b2610 100644
>> --- a/drivers/cxl/core/port.c
>> +++ b/drivers/cxl/core/port.c
>> @@ -828,27 +828,20 @@ static void cxl_debugfs_create_dport_dir(struct cxl_dport *dport)
>> &cxl_einj_inject_fops);
>> }
>>
>> -static struct cxl_port *__devm_cxl_add_port(struct device *host,
>> - struct device *uport_dev,
>> - resource_size_t component_reg_phys,
>> - struct cxl_dport *parent_dport)
>> +static int cxl_port_add(struct cxl_port *port,
>> + resource_size_t component_reg_phys,
>> + struct cxl_dport *parent_dport)
>> {
>> - struct cxl_port *port;
>> - struct device *dev;
>> + struct device *dev __free(put_device) = &port->dev;
>> int rc;
>>
>> - port = cxl_port_alloc(uport_dev, parent_dport);
>> - if (IS_ERR(port))
>> - return port;
>> -
>> - dev = &port->dev;
>> - if (is_cxl_memdev(uport_dev)) {
>> - struct cxl_memdev *cxlmd = to_cxl_memdev(uport_dev);
>> + if (is_cxl_memdev(port->uport_dev)) {
>> + struct cxl_memdev *cxlmd = to_cxl_memdev(port->uport_dev);
>> struct cxl_dev_state *cxlds = cxlmd->cxlds;
>>
>> rc = dev_set_name(dev, "endpoint%d", port->id);
>> if (rc)
>> - goto err;
>> + return rc;
>>
>> /*
>> * The endpoint driver already enumerated the component and RAS
>> @@ -861,19 +854,41 @@ static struct cxl_port *__devm_cxl_add_port(struct device *host,
>> } else if (parent_dport) {
>> rc = dev_set_name(dev, "port%d", port->id);
>> if (rc)
>> - goto err;
>> + return rc;
>>
>> rc = cxl_port_setup_regs(port, component_reg_phys);
>> if (rc)
>> - goto err;
>> - } else
>> + return rc;
>> + } else {
>> rc = dev_set_name(dev, "root%d", port->id);
>> - if (rc)
>> - goto err;
>> + if (rc)
>> + return rc;
>> + }
>>
>> rc = device_add(dev);
>> if (rc)
>> - goto err;
>> + return rc;
>> +
>> + /* Inhibit the cleanup function invoked */
>> + dev = NULL;
> I'm tempted to say we should use no_free_ptr() here. But I don't think
> there is any magic we need in there.
Yes, I also think using no_free_ptr() is better. but I have to add a extra local variable to store the return value of no_free_ptr(), otherwise there should be a compilation warning.
@@ -870,7 +870,7 @@ static int cxl_port_add(struct cxl_port *port,
return rc;
/* Inhibit the cleanup function invoked */
- dev = NULL;
+ no_free_ptr(dev);
return 0;
}
compilation warning:
drivers/cxl/core/port.c: In function ‘cxl_port_add’:
./include/linux/cleanup.h:76:22: warning: ignoring return value of ‘__must_check_fn’ declared with attribute ‘warn_unused_result’ [-Wunused-result]
76 | ((typeof(p)) __must_check_fn(__get_and_null_ptr(p)))
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/cxl/core/port.c:873:9: note: in expansion of macro ‘no_free_ptr’
873 | no_free_ptr(dev);
| ^~~~~~~~~~~
LD [M] drivers/cxl/core/cxl_core.o
MODPOST Module.symvers
> So.
>
> Reviewed-by: Ira Weiny <ira.weiny@intel.com>
>
>> + return 0;
>> +}
>> +
>> +static struct cxl_port *__devm_cxl_add_port(struct device *host,
>> + struct device *uport_dev,
>> + resource_size_t component_reg_phys,
>> + struct cxl_dport *parent_dport)
>> +{
>> + struct cxl_port *port;
>> + int rc;
>> +
>> + port = cxl_port_alloc(uport_dev, parent_dport);
>> + if (IS_ERR(port))
>> + return port;
>> +
>> + rc = cxl_port_add(port, component_reg_phys, parent_dport);
>> + if (rc)
>> + return ERR_PTR(rc);
>>
>> rc = devm_add_action_or_reset(host, unregister_port, port);
>> if (rc)
>> @@ -891,10 +906,6 @@ static struct cxl_port *__devm_cxl_add_port(struct device *host,
>> port->pci_latency = cxl_pci_get_latency(to_pci_dev(uport_dev));
>>
>> return port;
>> -
>> -err:
>> - put_device(dev);
>> - return ERR_PTR(rc);
>> }
>>
>> /**
>> --
>> 2.40.1
>>
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2024-08-22 2:07 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-08-13 7:05 [PATCH 1/3] cxl/port: Use __free() to drop put_device() for cxl_port Li Ming
2024-08-13 7:05 ` [PATCH 2/3] cxl/port: Use scoped_guard() to drop device_lock()/unlock pair Li Ming
2024-08-21 21:49 ` Ira Weiny
2024-08-22 1:47 ` Li, Ming4
2024-08-13 7:05 ` [PATCH 3/3] cxl/port: Refactor __devm_cxl_add_port() to drop goto pattern Li Ming
2024-08-21 22:01 ` Ira Weiny
2024-08-22 2:07 ` Li, Ming4
2024-08-21 21:37 ` [PATCH 1/3] cxl/port: Use __free() to drop put_device() for cxl_port Ira Weiny
2024-08-22 1:26 ` Li, Ming4
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox