From: Ira Weiny <ira.weiny@intel.com>
To: Li Ming <ming4.li@intel.com>, <dave@stgolabs.net>,
<jonathan.cameron@huawei.com>, <dave.jiang@intel.com>,
<alison.schofield@intel.com>, <vishal.l.verma@intel.com>,
<ira.weiny@intel.com>, <dan.j.williams@intel.com>
Cc: <linux-cxl@vger.kernel.org>, Li Ming <ming4.li@intel.com>
Subject: Re: [PATCH 1/3] cxl/port: Use __free() to drop put_device() for cxl_port
Date: Wed, 21 Aug 2024 16:37:45 -0500 [thread overview]
Message-ID: <66c65e2917d00_1719d294ed@iweiny-mobl.notmuch> (raw)
In-Reply-To: <20240813070552.3353530-1-ming4.li@intel.com>
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
>
next prev parent reply other threads:[~2024-08-21 21:37 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
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 ` Ira Weiny [this message]
2024-08-22 1:26 ` [PATCH 1/3] cxl/port: Use __free() to drop put_device() for cxl_port Li, Ming4
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=66c65e2917d00_1719d294ed@iweiny-mobl.notmuch \
--to=ira.weiny@intel.com \
--cc=alison.schofield@intel.com \
--cc=dan.j.williams@intel.com \
--cc=dave.jiang@intel.com \
--cc=dave@stgolabs.net \
--cc=jonathan.cameron@huawei.com \
--cc=linux-cxl@vger.kernel.org \
--cc=ming4.li@intel.com \
--cc=vishal.l.verma@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox