Received-SPF: pass (google.com: domain of linux-kernel+bounces-103985-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Thu, 14 Mar 2024 19:24:24 -0700
From: Dan Williams <dan.j.williams@intel.com>
To: Li Ming <ming4.li@intel.com>, <dan.j.williams@intel.com>,
	<rrichter@amd.com>, <terry.bowman@amd.com>
CC: <linux-cxl@vger.kernel.org>, <linux-kernel@vger.kernel.org>, Li Ming
	<ming4.li@intel.com>
Subject: Re: [RFC PATCH 5/6] cxl: Use __free() for cxl_pci/mem_find_port() to
 drop put_device()
Message-ID: <65f3b158b9674_aa222947@dwillia2-mobl3.amr.corp.intel.com.notmuch>
References: <20240313083602.239201-1-ming4.li@intel.com>
 <20240313083602.239201-6-ming4.li@intel.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20240313083602.239201-6-ming4.li@intel.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?vKO3FZEJNVCj4qM1Cnt32ieV/rIdK4cFEYdJw4QJd6T0c1anozWDtokIjUkr?=
 =?us-ascii?Q?8n/lYYUfxVeIBHFN4U4MA5I40oaEX7YiGdyiXxaI/cZ7NrrAxIAJDQvY4JzN?=
 =?us-ascii?Q?yx6ZpIirMXZX+OdAvjg9aHJ7J+Yqtwm+XOP7Xb9va81nVSfkTle4TDGmymni?=
 =?us-ascii?Q?qT3vXFudOzSd2WWjG6YbqekS1lfWyuqycWkU9TRyyhmQ/5IEcdCnRPpHXjSY?=
 =?us-ascii?Q?qN5O1dkQWC4fScUnzRYQeMLMFr32qYldItmOe9kusAvZeYwXni9TlKhCnpVG?=
 =?us-ascii?Q?7bzwV4hGEqhqIuuNtHSMZP425BNPyKT4Zs9Y0emnrPDySHr4t0F+pLHZF/21?=
 =?us-ascii?Q?8K8UlOag8/3tTbjAhXdYoEeCwUrdoMMnyP7yodQm+j8CLE2e2YbvtDmeFeS7?=
 =?us-ascii?Q?MqQy4cDSMUVBGY+YE8xZy8DZoa7jvSE73TF28oJoRknjZzG/U6ITAWTb5Rey?=
 =?us-ascii?Q?pM+94M6ac0u6RXaiDweg7zQTSLMZSwYOLogjLTG581iNIP9St5sizRr7lUCj?=
 =?us-ascii?Q?Wbg7sdh/g1Kw2RnObYyLqdazErF/gCTrKyQ445hd/9LdS1GTpaU1t7wMPKF9?=
 =?us-ascii?Q?Lyvk7ZXRcQQv/3F+NUiVQlTGkE8ne9aNuIvRHaL2czo5CnVBc8cUKan4+d4Z?=
 =?us-ascii?Q?L2vDHW8GCvFjPkySWK8iAh08acsDwlJY2CcguIbOY6MtZ9NLmTDlx17j3DWe?=
 =?us-ascii?Q?8cmEEQdet3bBDc268hCci3rdaW9X3IkkTHCgiTYmV9k+rQlBQhOcKZl7vcFd?=
 =?us-ascii?Q?71bXgccTMAHMP/X8GOxJl+RBoJxPUfC3rt32z6JPGQZFDHPJC/q6vrYq0VSE?=
 =?us-ascii?Q?xHsUjfOg297Qmf70XzYgwBGNOq06SfqxtRS9ALVmpIY2kwKQdYsLwl86w+qK?=
 =?us-ascii?Q?Yibd+qrxnP6d0zDydoPpFq52hW31UQvi+cnAYpcm3sDoOgRYQuinDpr8WyGk?=
 =?us-ascii?Q?srpS76Ybe7/2qJC/NcF4n7rJstGQnnXCiITZQjGE2jdPD7NXGmcS68SphFJ8?=
 =?us-ascii?Q?pK2+Tt/IO2mrn8IkQelCoRICb9PIoZjaMit5Oqk4U/6chC6gkWqmyf5ejZSp?=
 =?us-ascii?Q?SnG2f5U+4B8EJHewi85UBe0b4TKYiktALaxf7Upmw0rZaiKnnN2TzbPtmyjl?=
 =?us-ascii?Q?0M5vj08PhIFBSxGXIXQa4GVIdiuaf2veJ8laeywt3uE1Mqls56no48d9ofVE?=
 =?us-ascii?Q?oltrBrvFCe3TCJD1wYSJ6ylypX1TAJf7fuvDynwgwLJyRssybEhuMpSeK+DD?=
 =?us-ascii?Q?HfwPycHvfrEmA/9+ncnkIgIOwQnBlg+RQ+8Jd7n6XqFg68PFc03utXYhnD55?=
 =?us-ascii?Q?ctYZ67IAT6tsNUuiDO/8xVZJMAd6kgBdPRlLCkt4pb37uL54G36gOSYOY18i?=
 =?us-ascii?Q?OReM7DzBG5GllAX15ieayBglbVVDIyBElboWanPxLoacd/jCWJR1PBu5qWYA?=
 =?us-ascii?Q?i6qrBC2gRfipXCIDjB8oRzITA5mZZJekDgPZaYvDJhf1Ln33yDbD/DFWM/VE?=
 =?us-ascii?Q?e8V5H3MrB/c7o8+MnZROdntM8kmMj514Up+JJgTpyDGEr0XlMwNF0xU7zSu4?=
 =?us-ascii?Q?01RZuh/zKy6ZRadHUIloFu89ycOATl5vlYWO4epBWvvnVezqiDX8jUDNK+uR?=
 =?us-ascii?Q?sQ=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 6bfa34dc-887b-4967-ef59-08dc4497095e
X-MS-Exchange-CrossTenant-AuthSource: PH8PR11MB8107.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 15 Mar 2024 02:24:27.0331
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 2qixCBEc6pJSmBze55yuZSiiIuqaZmTFufh2MwUnpmsEL5Yg25Rjw2BvHnc7vbbJQrMC1pj8GuGtpMWL/2eqnWwPUR4ENedOaivjRfJcZk0=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN0PR11MB5961
X-OriginatorOrg: intel.com

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@intel.com>
> Signed-off-by: Li Ming <ming4.li@intel.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.