On Tue, Jul 28, 2026 at 04:33:43PM -0600, Alex Williamson wrote:
Hi Leon,
Please see below...
On Wed, 15 Jul 2026 18:47:24 +0100 Matt Evans matt@ozlabs.org wrote:
Preparing for a refactor in a subsequent patch, split the pool-related release code into a new pci_p2pdma_release_pool() function.
This allows future compile-time selection of a null implementation for pci_p2pdma_release_pool(), when p2pdma.c is refactored into core- and P2P-related files.
Signed-off-by: Matt Evans matt@ozlabs.org Acked-by: Bjorn Helgaas bhelgaas@google.com Reviewed-by: Logan Gunthorpe logang@deltatee.com
drivers/pci/p2pdma.c | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-)
diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c index b2d5266f8653..498bca257419 100644 --- a/drivers/pci/p2pdma.c +++ b/drivers/pci/p2pdma.c @@ -226,6 +226,17 @@ static const struct dev_pagemap_ops p2pdma_pgmap_ops = { .folio_free = p2pdma_folio_free, }; +static void pci_p2pdma_release_pool(struct pci_dev *pdev,
struct pci_p2pdma *p2pdma)+{
- if (!p2pdma->pool)
return;- synchronize_rcu();
Sashiko notes[1] a high, preexisting issue here that looks like it was introduced via 372d6d1b8ae3 ("PCI/P2PDMA: Refactor to separate core P2P functionality from memory allocation"). That commit creates a two-tier scheme where the optional memory allocation capabilities, such as the pool, live above a core layer. Prior to that commit, this synchronize RCU call was unconditional. Making it conditional on the pool suggests it was only considered relevant to the optional layer.
However, map_types, which remains in the core layer, is RCU referenced. Was the synchronize_rcu() call here miscategorized into the optional tier? Should it instead have remained unconditional?
I remember this concern when I wrote the patches and wanted to remove RCU entirely. I revisited the Sashiko report, but reached the same conclusion again.
The "bug" reported by Sashiko does not exist. The two-layer split ensures that p2p is bound to the driver's lifecycle. As a result, pdev->p2pdma is assigned and cleared only once during the lifetime of pdev.
In this case, the RCU primitives are effectively NOPs, since nothing will ever update that pointer.
I still needed to keep rcu_dereference() in pci_p2pdma_map_type() to satisfy static analyzers, which would otherwise complain about accessing an RCU-protected pointer without the proper annotations.
RCU is used only in the sysfs flow.
Thanks
We might need a precursor Fixes: patch to this series that makes it unconditional in pci_p2pdma_release(), so that it remains there with this refactor rather than becoming part of this new pool release function. Thanks,
Alex
[1]https://lore.kernel.org/all/20260715180521.3CDD61F00A3A@smtp.kernel.org/
- gen_pool_destroy(p2pdma->pool);
- sysfs_remove_group(&pdev->dev.kobj, &p2pmem_group);
+}
static void pci_p2pdma_release(void *data) { struct pci_dev *pdev = data; @@ -237,15 +248,8 @@ static void pci_p2pdma_release(void *data) /* Flush and disable pci_alloc_p2p_mem() */ pdev->p2pdma = NULL;
- if (p2pdma->pool)
synchronize_rcu();
- pci_p2pdma_release_pool(pdev, p2pdma); xa_destroy(&p2pdma->map_types);
- if (!p2pdma->pool)
return;- gen_pool_destroy(p2pdma->pool);
- sysfs_remove_group(&pdev->dev.kobj, &p2pmem_group);
} /** @@ -946,8 +950,8 @@ void *pci_alloc_p2pmem(struct pci_dev *pdev, size_t size) struct pci_p2pdma *p2pdma; /*
* Pairs with synchronize_rcu() in pci_p2pdma_release() to* ensure pdev->p2pdma is non-NULL for the duration of the
* Pairs with synchronize_rcu() in pci_p2pdma_release_pool()* to ensure pdev->p2pdma is non-NULL for the duration of the*/ rcu_read_lock();
- read-lock.