On Wed, Jan 03, 2024 at 04:21:02PM -0800, 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. I see "scoped based put" follows a similar use in cleanup.h, but I think "scope-based X" reads better. > > 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? > > > > Does this change anything for callers of pci_dev_get() and > > pci_dev_put()? > > > > Does this avoid a common programming error? I just don't know what > > the benefit of this is yet. > > > > 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: > > --- > 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. > ...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 */ > } Thanks for this! I had skimmed cleanup.h previously, but it makes a lot more sense after your description here. I think a little introduction along these lines would be even more useful in cleanup.h since the concept is general and not PCI-specific. E.g., the motivation (avoid resource leaks with "goto error" pattern), a definition of "__free() based cleanup function" (IIUC, a function to be run when a variable goes out of scope), maybe something about ordering (it's important in the "goto error" pattern that the cleanups are done in a specific order; how does that translate to __free()?) But the commit log above is fine with me. (It does contain tabs, which get slightly mangled when "git log" indents it.) > 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. This part is also fine but doesn't seem strictly necessary to me. I think the part about error reports being fed back needs a lot more background to understand the connection, and probably only makes sense in the context of that patch. > 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. Thanks, Dan! Acked-by: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>