| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <b15f966b-1ffd-43e0-9483-0b1fa9c90ac3@intel.com>
Date: Fri, 15 Mar 2024 12:05:24 +0800
From: "Li, Ming" <ming4.li@...el.com>
To: Dan Williams <dan.j.williams@...el.com>, <rrichter@....com>,
<terry.bowman@....com>
CC: <linux-cxl@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH 5/6] cxl: Use __free() for cxl_pci/mem_find_port() to
drop put_device()
On 3/15/2024 10:24 AM, Dan Williams wrote:
> 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@...el.com>
>> Signed-off-by: Li Ming <ming4.li@...el.com>
>> ---
>> 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.
Thanks, I will do that.
Powered by blists - more mailing lists