Received-SPF: pass (google.com: domain of linux-kernel+bounces-17029-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 09:17:42 -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: <6596e836a783a_1587c29412@iweiny-mobl.notmuch>
References: <6595e201beb4_be7022944d@dwillia2-xfh.jf.intel.com.notmuch>
 <20240103230147.GA1800245@bhelgaas>
 <6595f9eec5986_be70229443@dwillia2-xfh.jf.intel.com.notmuch>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <6595f9eec5986_be70229443@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?9I1KpEd932XKXqhx0bGXat+no66milJv2KINa0YSZ3v7gwJBaF585WONy7u2?=
 =?us-ascii?Q?vXvh4He+EM41ZuyWsCzxDfkk94hNmVtcCMJR0ZVy/ewSrOE2D7cWSKDUGz1E?=
 =?us-ascii?Q?NwoECXwqBNn3+njeJNb+fHXxiTlhKZJzYNbQVMtnWccpxGsVr5K3HNEeTpde?=
 =?us-ascii?Q?fzhh7OuiuXDzjU51UXS5zgMqOTCXkabziTNzNKq2cC4rGLX/M4E0eym6RgQ3?=
 =?us-ascii?Q?5cgVUuHH2manIiV3z+t0U9aXgn1k4+EaDT7FTo/MQL4H1dzIuXJ1ZmdWIFKb?=
 =?us-ascii?Q?RZ+tmFSgeRTVgH3A4uFxlK9UR7ssA+5OUdY0RYk+Sw54OaqrLGxwTFfPBfdi?=
 =?us-ascii?Q?KOMmYIsYf09ISHeXMqxP9R5QQt0C+nyEAmmGNFDgl15dlZj+/2lW9Z9hNNkQ?=
 =?us-ascii?Q?jCW1nXL+Y1G+Gj6neoz1asymU8XQ0ogPtw1j2SWU4eUpxejuR/+ZyBo3jo7f?=
 =?us-ascii?Q?dP4pZ29tJstSFiTOOwSg6qkjBkjODV1NpRHXjUz2BZrfKc5YcYLUd5bhbj/S?=
 =?us-ascii?Q?e6WxGr5uBLJpbmbzK5iYqoKGR01I/I32OVZ4FA0cu/6GwN3ehE8H/pG6kJcr?=
 =?us-ascii?Q?mPWKTDuZ6j76oCkFuwmCDKboBdNLbQ/p3Ss4eHhHesZIFNVfTCd4SbhJLdvE?=
 =?us-ascii?Q?r69VB3BC7/TVaMGfwswGLemEBx9LwsoYiOfZtbVPXuauhIKbRTn6klkCuonT?=
 =?us-ascii?Q?QXCrkduOVoxZMeVcXXRbLlsxTulR2zSDPdnxlbmkl05jXk4JhiYXg6gbECDI?=
 =?us-ascii?Q?MIf1AobLZGONxpCGJ5jCnuxZpKHL9gsMQ8QEDv56tsjNsMXRuA/ioIrQKmyn?=
 =?us-ascii?Q?aKQ9jBTGybpPSqX5RKm7FsjQPXmxdZDlnwSWKgDQDruwfPlSxNeQrIuCTr/+?=
 =?us-ascii?Q?GV8LjprWgiQ6pI/3MPUTYYgkqrckKqrjvLOIOxY5Ah5/NRCuIcDzFVPLg3kH?=
 =?us-ascii?Q?+QWDqskUG2NCSNrml+FqItX3AFEavOfW0A5AvmTmxCGsaVhgvNEUIS/aIr3O?=
 =?us-ascii?Q?hAykkG6PqPCUE7kWUZrwA1C9Z9fpBknfRunmCobv2JIESBiITyzrCIPwGMVp?=
 =?us-ascii?Q?NBUIN8R6cAcmnAzGlSJg32J0mF1UgkYFUjjmDae/IHJ08qwJ4EY+zeM2vzIZ?=
 =?us-ascii?Q?v4ClJRkO1/Y+AM2J2zkICdU08ypW9NqBLkML485OlQeepdIfoLkCD1ZFnwxH?=
 =?us-ascii?Q?ORocYqUucV/b5i0EOfgKZtS4yoFD7IETD57jq2wmCzX9UWRfN9lqDv1VTKB2?=
 =?us-ascii?Q?VJR+4RcyIBnhz+xgY3zeuOdIJuVod48T4gpVOYWQ0ONDE0d2Rz5pQ8u0T0aq?=
 =?us-ascii?Q?pEQ8J6omm93ogBJtTV//Jh6OsiI0yal+OHBe9+eItoEeqwGSAQ26/8t6VbS8?=
 =?us-ascii?Q?khXzaF64WZ4P/sdVxIchUy5YMHRsguUdeT2myFK9zgQ1PZPWTueE2YK08B18?=
 =?us-ascii?Q?QTlC6naiG0NvDYsDAQS8i9LZqNb5bnVoaA3d1MZ3bs/6t/bE8+kT9MDlKs2S?=
 =?us-ascii?Q?9r+waCw/uj6+UiGR6FfwAsvl4N5X1ga4oG6CD55Y+GW8+9UnA55CLcNTekqi?=
 =?us-ascii?Q?zqCC3sz3HUPbfImYirI8RK3/h/LtNvBNsPBddtTO?=
X-MS-Exchange-CrossTenant-Network-Message-Id: b61efcf1-e6d0-4a84-0b7a-08dc0d4916e4
X-MS-Exchange-CrossTenant-AuthSource: SA1PR11MB6733.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 04 Jan 2024 17:17:54.9808
 (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: 4B+MZoMvsWq4mQKTWIh49leJ8CtDyt4HbyCGhJXMsofRDbSxMgJ4TrJeLUAl8FAJF0Su7pbTPga9M64eYDsWkw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR11MB6215
X-OriginatorOrg: intel.com

Dan Williams wrote:
> Bjorn Helgaas wrote:
> > On Wed, Jan 03, 2024 at 02:38:57PM -0800, Dan Williams wrote:
> > > Ira Weiny wrote:
> > > > Users of pci_dev_get() can benefit from a scoped based put.  Also,
> > > > locking a PCI device is often done within a single scope.
> > > > 
> > > > Define a pci_dev_put() free function and a PCI device lock guard.  These
> > > > will initially be used in new CXL event processing code but is defined
> > > > in a separate patch for others to pickup and use/backport easier.
> > > 
> > > Any heartburn if I take this through cxl.git with the rest in this
> > > series? Patch 9 has a dependency on this one.
> > 
> > No real heartburn.  I was trying to figure out what this does
> > since I'm not familiar with the "scoped based put" idea and
> > 'git grep -i "scope.*base"' wasn't much help.
> > 
> > I would kind of like the commit log to say a little more about what
> > the "scoped based put" (does that have too many past tenses in it?)
> > means and how users of pci_dev_get() will benefit.
> 
> That is completely fair, and I should have checked to make sure that the
> changelog clarified the impact of the change.

Agreed.  Apologies for the brevity.

> 
> > I don't know what "locking a PCI device is often done within a single
> > scope" is trying to tell me either.  What if it's *not* done within a
> > single scope?

I was not trying to fix that but Dan covers it below indicating that the
pointer can be returned outside the scope if needed with no_free_ptr().

> > 
> > Does this change anything for callers of pci_dev_get() and
> > pci_dev_put()?

Current callers don't need to use this.

> > 
> > Does this avoid a common programming error?  I just don't know what
> > the benefit of this is yet.

Dan covered it well below.

> > 
> > I'm sure this is really cool stuff, but there's little documentation,
> > few existing users, and I don't know what I need to look for when
> > reviewing things.
> 
> Here a is a re-write of the changelog:

FWIW

Signed-off-by: Ira Weiny <ira.weiny@intel.com>

> 
> ---
> 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;
> 
> ...will automatically call pci_dev_put() if @pdev is non-NULL when @pdev
> goes out of scope (automatic variable scope). If a function wants to
> invoke pci_dev_put() on error, but return @pdev on success, it can do:
> 
> 	return no_free_ptr(pdev);
> 
> ...or:
> 
> 	return_ptr(pdev);
> 
> For potential cleanup opportunity there are 587 open-coded calls to
> pci_dev_put() in the kernel with 65 instances within 10 lines of a goto
> statement with the CXL driver threatening to add another one.
> 
> The guard() helper holds the associated lock for the remainder of the
> current scope in which it was invoked. So, for example:
> 
> 	func(...)
> 	{
> 		if (...) {
> 			...
> 			guard(pci_dev); /* pci_dev_lock() invoked here */
> 			...
> 		} /* <- implied pci_dev_unlock() triggered here */
> 	}
> 
> There are 15 invocations of pci_dev_unlock() in the kernel with 5
> instances within 10 lines of a goto statement. Again, the CXL driver is
> threatening to add another.
> 
> Introduce these helpers to preclude the addition of new more error prone
> goto put; / goto unlock; sequences. For now, these helpers are used in
> drivers/cxl/pci.c to allow ACPI error reports to be fed back into the
> CXL driver associated with the PCI device identified in the report.
> 
> ---
> 
> As for reviewing conversions that use these new helpers, one of the
> gotchas I have found is that it is easy to make a mistake if a goto
> still exists in the function after the conversion. So unless and until
> all of the resources a function acquires, that currently need a goto to
> unwind them on error, can be converted to cleanup.h based helpers it is
> best not to mix styles.
> 
> I think the function documentation in include/linux/cleanup.h does a
> decent job of explaining how to use the helpers. However, I am happy to
> suggest some updates if you think it would help.