Re: [RFC PATCH 5/6] cxl: Use __free() for cxl_pci/mem_find_port() to drop put_device()
From: Dan Williams
Date: Thu Mar 14 2024 - 22:24:51 EST
Li Ming wrote:
> Introduce a new helper function called put_cxl_port() to instead of the
> put_device() in order to release the device reference of struct cxl_port
> got via get_device() in cxl_pci/mem_find_port().
>
> Besides, use scope-based resource management __free() to drop the open
> coded put_device() for each cxl_pci/mem_find_port().
>
> Suggested-by: Dan Williams <dan.j.williams@xxxxxxxxx>
> Signed-off-by: Li Ming <ming4.li@xxxxxxxxx>
> ---
> drivers/cxl/core/pci.c | 6 ++----
> drivers/cxl/core/port.c | 9 +++++++++
> drivers/cxl/cxl.h | 2 ++
> drivers/cxl/mem.c | 5 ++---
> drivers/cxl/pci.c | 12 +++++-------
> 5 files changed, 20 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index 6c9c8d92f8f7..7254484330d2 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -902,15 +902,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;
> + struct cxl_port *port __free(put_cxl_port) =
> + cxl_pci_find_port(pdev, &dport);
>
> - port = cxl_pci_find_port(pdev, &dport);
> if (!port)
Keep the declaration separated from the rest of the variable
declarations like this:
diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index 0df09bd79408..2d1ef35fe99c 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -916,12 +916,11 @@ static void cxl_handle_rdport_errors(struct cxl_dev_state *cxlds)
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;
> 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 e59d9d37aa65..6e2fc2fce7c9 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -1671,6 +1671,15 @@ struct cxl_port *cxl_mem_find_port(struct cxl_memdev *cxlmd,
> }
> EXPORT_SYMBOL_NS_GPL(cxl_mem_find_port, CXL);
>
> +void put_cxl_port(struct cxl_port *port)
> +{
> + if (!port)
> + return;
> +
> + put_device(&port->dev);
> +}
> +EXPORT_SYMBOL_NS_GPL(put_cxl_port, CXL);
> +
> static int decoder_populate_targets(struct cxl_switch_decoder *cxlsd,
> struct cxl_port *port, int *target_map)
> {
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index b6017c0c57b4..476158782e3e 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -743,6 +743,8 @@ struct cxl_port *cxl_pci_find_port(struct pci_dev *pdev,
> struct cxl_dport **dport);
> struct cxl_port *cxl_mem_find_port(struct cxl_memdev *cxlmd,
> struct cxl_dport **dport);
> +void put_cxl_port(struct cxl_port *port);
> +DEFINE_FREE(put_cxl_port, struct cxl_port *, if (_T) put_cxl_port(_T))
> bool schedule_cxl_memdev_detach(struct cxl_memdev *cxlmd);
>
> struct cxl_dport *devm_cxl_add_dport(struct cxl_port *port,
> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> index c5c9d8e0d88d..5aaa8ee2a46d 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;
> @@ -170,7 +170,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;
When converting a function to use cleanup helpers, it should try to
remove all gotos, something like:
@@ -159,21 +159,18 @@ 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);
+ if (rc)
+ return rc;
}
- rc = devm_cxl_add_endpoint(endpoint_parent, cxlmd, dport);
-unlock:
- device_unlock(endpoint_parent);
- put_device(&parent_port->dev);
- if (rc)
- return rc;
-
if (resource_size(&cxlds->pmem_res) && IS_ENABLED(CONFIG_CXL_PMEM)) {
rc = devm_cxl_add_nvdimm(cxlmd);
if (rc == -ENODEV)
>
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 4fd1f207c84e..d0ec8c5b1e99 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -473,23 +473,21 @@ 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;
> + struct cxl_port *port __free(put_cxl_port) =
> + cxl_pci_find_port(pdev, &dport);
> +
> + if (!port)
> + return -EPROBE_DEFER;
>
> *map = (struct cxl_register_map) {
> .host = &pdev->dev,
> .resource = CXL_RESOURCE_NONE,
> };
>
> - port = cxl_pci_find_port(pdev, &dport);
> - if (!port)
> - return -EPROBE_DEFER;
> -
Here again don't move the cxl_pci_find_port() earlier in the function,
keep it after the assignment of @map.