Received-SPF: pass (google.com: domain of linux-kernel+bounces-17253-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Thu, 4 Jan 2024 13:46:56 -0800
From: Ira Weiny <ira.weiny@intel.com>
To: Dan Williams <dan.j.williams@intel.com>, Bjorn Helgaas
	<helgaas@kernel.org>
CC: Bjorn Helgaas <bhelgaas@google.com>, Ira Weiny <ira.weiny@intel.com>,
	Jonathan Cameron <jonathan.cameron@huawei.com>, Smita Koralahalli
	<Smita.KoralahalliChannabasappa@amd.com>, Shiju Jose <shiju.jose@huawei.com>,
	Yazen Ghannam <yazen.ghannam@amd.com>, Davidlohr Bueso <dave@stgolabs.net>,
	Dave Jiang <dave.jiang@intel.com>, Alison Schofield
	<alison.schofield@intel.com>, Vishal Verma <vishal.l.verma@intel.com>, "Ard
 Biesheuvel" <ardb@kernel.org>, <linux-efi@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>, <linux-cxl@vger.kernel.org>,
	<linux-pci@vger.kernel.org>
Subject: Re: [PATCH v5 8/9] PCI: Define scoped based management functions
Message-ID: <6597275045fbf_267e82949d@iweiny-mobl.notmuch>
References: <6595f9eec5986_be70229443@dwillia2-xfh.jf.intel.com.notmuch>
 <20240104183218.GA1820872@bhelgaas>
 <65970003ee9ef_8dc6829412@dwillia2-xfh.jf.intel.com.notmuch>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <65970003ee9ef_8dc6829412@dwillia2-xfh.jf.intel.com.notmuch>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?U09usRBCrjSlY88cx1Eok1C/f8thM+tT1w1AmgfcYzAEacDCF6TyvIwRRr+8?=
 =?us-ascii?Q?X8OWO2ws3t7/MmLvKvs7h7MuBQiUwoRIt8h4XDHsT3zy+2lmBIZNn6gEF/SL?=
 =?us-ascii?Q?mNPfCKpelD0u3vKuL0/pLYMRzwnE1H4+hfqaDmO2tVJspoypiypXOOUI/2gk?=
 =?us-ascii?Q?u/2fNoM7T2m9lroKDV8zjzpwWdqhFUNTz0pBnE8KJNsJdwO2XRfdJD6pK8x9?=
 =?us-ascii?Q?VsaqzhmLMp40iiud6R21lQoBvxDDJ1UvjNBCULeppG75H2YhRKtTqOmNIGPz?=
 =?us-ascii?Q?UdIud/e/Htb8/8JAoMwQH1PXCEkv7SA0a64Tdbmn8HO4GZKmiMQPSf8manNR?=
 =?us-ascii?Q?g6CTqJH1FbT0d5v63aYeKQB7HNW2/c8o6nEy3rCc/pu2fEzR14ciwoR3vb/K?=
 =?us-ascii?Q?IpyEcqaGgWcCV53ZUvhYnGnlKOBmLpeO3aZ9+LkH9W//GLZ+J0KfKD1b0Pr/?=
 =?us-ascii?Q?+nlLMjPbp2AV+b2cWRSZ0flaN1epX/DwFlpZAr3IKD7Q1MGc2Zg4GSGQ3ihK?=
 =?us-ascii?Q?Ha9bPBZURfj5O/YRxYTi3ytSJjoJCImTCfZEmEcPpBDQAokAH+pMfCSaGkDz?=
 =?us-ascii?Q?AVd7JEiUHMsR7kowMxq8BjVrIXmV2JUXHbbh3FoGQAf+/aMBbfhS/AvTuQg5?=
 =?us-ascii?Q?u/c2s2IOs/6jH6U6ZyCQhSDLQia+VY4kW+QqucmoigFmyWM58tuONd7EXqMV?=
 =?us-ascii?Q?ZeLAOdoamYAuoIchDMj+A8LrdZQ9ocNoARYUlQzCiRG9+GeTlYxq/a0YY6xS?=
 =?us-ascii?Q?q+YAElPJ4fD7X37qHspmjvrn2Dmowgv3yfNIeBWp0zDmY9sMUqgvxZbfPVpm?=
 =?us-ascii?Q?guAJmb7mNvBrCVetm0Up7r2Ki/d70Xh85YAf/Lel5WgYkMY1YcNXnW71a61u?=
 =?us-ascii?Q?KwnZH1xEwmhixVNbUacyO/6tT0l7+L7lF1OedNkMVHHwpaOjm5pB4igIoN+n?=
 =?us-ascii?Q?fEcv+IY/bC8V5Mjwgkf5Qx4s2P/k9XptGqPYSob+ffDBs0Zftad4xersFJuX?=
 =?us-ascii?Q?THzYoagnUIvtjQOj26hvnWgA/eGiy7wq2INvfQzLv3ekOqsMgHnSoQelChqe?=
 =?us-ascii?Q?DFbMeuxGAzGXdFKcO5xHXH2AShIGdjlOjRYQ/e6pmOprDFkw5HvchtrMgNGe?=
 =?us-ascii?Q?eT+pJeZ7ZaQHQjCzRXb8JAjR/SXlsSRvPd9tWl2rNknd5zy7eDvEVRlPL3tu?=
 =?us-ascii?Q?3tx/rek9o6nGIiON2F7ljl2D2s47oDcmlnsOnOIp4GX3vgnEPqBA8/zkaYPf?=
 =?us-ascii?Q?6zc3VTiO4M35GvQqCqmhViFi3qYYXlmziWxPdCYtf+1M9lFbfP7VvxzbCjv+?=
 =?us-ascii?Q?AMTEkG6NOty/79CwTOemNkBN3KjQsVgwL/Ntm5aJexZ7RsQ54SyqaLdCbNQ9?=
 =?us-ascii?Q?OYUEmiF7d81pz1yyHXCuC4oLSqkVOBrURJgQbYpKSMZWf83Ys6E2zY9E4KTt?=
 =?us-ascii?Q?zArhHQ8wzWMqoMOsRdAcDeEyIsJXagP70uxdRHGBKlgecMeT8y6VH4GtVk8s?=
 =?us-ascii?Q?6xqhznXePSiNjS4dAlaadJTVaa3Zv7EZL41gR8dGLadrI80wf91N+VzqSuH3?=
 =?us-ascii?Q?yl0pdrzX1XHFiaBtVBnHexdg9hH00Z4N6cp6FSf6?=
X-MS-Exchange-CrossTenant-Network-Message-Id: f8e3c414-222f-4433-8f2f-08dc0d6eae2c
X-MS-Exchange-CrossTenant-AuthSource: SA1PR11MB6733.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 04 Jan 2024 21:47:00.1633
 (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: 04b30ARXeNb/I22sEcrwwFrRH93YNuLxXybOp+mBJ6wEE9AQu0IM//X/M/6pRjPhLQe+Zq0aR56R3nEcpnd7oA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ2PR11MB8497
X-OriginatorOrg: intel.com

Dan Williams wrote:
> Bjorn Helgaas wrote:
> [..]
> > > ---
> > > PCI: Introduce cleanup helpers for device reference counts and locks
> > > 
> > > The "goto error" pattern is notorious for introducing subtle resource
> > > leaks. Use the new cleanup.h helpers for PCI device reference counts and
> > > locks.
> > > 
> > > Similar to the new put_device() and device_lock() cleanup helpers,
> > > __free(put_device) and guard(device), define the same for PCI devices,
> > > __free(pci_dev_put) and guard(pci_dev).  These helpers eliminate the
> > > need for "goto free;" and "goto unlock;" patterns. For example, A
> > > 'struct pci_dev *' instance declared as:
> > > 
> > > 	struct pci_dev *pdev __free(pci_dev_put) = NULL;
> > 
> > I see several similar __free() uses with NULL initializations in gpio,
> > but I think this idiom would be slightly improved if the __free()
> > function were more closely associated with the actual pci_dev_get():
> > 
> >   struct pci_dev *pdev __free(pci_dev_put) = pci_get_device(...);
> > 
> > Not always possible, I know, but easier to analyze when it is.
> 
> I tend to agree, but it does lead to some long lines, for example:
> 
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 4fd1f207c84e..549ba4b8294e 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -975,15 +975,14 @@ static void cxl_cper_event_call(enum cxl_event_type ev_type,
>  				struct cxl_cper_event_rec *rec)
>  {
>  	struct cper_cxl_event_devid *device_id = &rec->hdr.device_id;
> -	struct pci_dev *pdev __free(pci_dev_put) = NULL;
>  	enum cxl_event_log_type log_type;
>  	struct cxl_dev_state *cxlds;
>  	unsigned int devfn;
>  	u32 hdr_flags;
>  
>  	devfn = PCI_DEVFN(device_id->device_num, device_id->func_num);
> -	pdev = pci_get_domain_bus_and_slot(device_id->segment_num,
> -					   device_id->bus_num, devfn);
> +	struct pci_dev *pdev __free(pci_dev_put) = pci_get_domain_bus_and_slot(
> +		device_id->segment_num, device_id->bus_num, devfn);
>  	if (!pdev)
>  		return;
>  
> ...so I think people are choosing the "... __free(x) = NULL;" style for
> code density readability.
> 

Also in this case we need devfn assigned first.

Is the above patch compliant with current style guidelines?

Or would it be better to do?

diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index b14237f824cf..8a180c6abb67 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -975,15 +975,14 @@ static void cxl_cper_event_call(enum cxl_event_type ev_type,
                                struct cxl_cper_event_rec *rec)
 {
        struct cper_cxl_event_devid *device_id = &rec->hdr.device_id;
-       struct pci_dev *pdev __free(pci_dev_put) = NULL;
        enum cxl_event_log_type log_type;
        struct cxl_dev_state *cxlds;
-       unsigned int devfn;
+       unsigned int devfn = PCI_DEVFN(device_id->device_num, device_id->func_num);
+       struct pci_dev *pdev __free(pci_dev_put) = pci_get_domain_bus_and_slot(
+                                                       device_id->segment_num,
+                                                       device_id->bus_num, devfn);
        u32 hdr_flags;
 
-       devfn = PCI_DEVFN(device_id->device_num, device_id->func_num);
-       pdev = pci_get_domain_bus_and_slot(device_id->segment_num,
-                                          device_id->bus_num, devfn);
        if (!pdev)
                return;
 

Ira