Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] pci/p2pdma: pure clients cannot set ats_per_mapping because pdev->p2pdma is NULL --
commit f332086fc4a4273d099c81fafb33298021d47d1a Author: Leon Romanovsky <[email protected]> PCI/P2PDMA: Let a client declare that it selects ATS per mapping The PCIe ATS Enable bit covers the whole device, so P2PDMA cannot tell from it whether a client will translate a given address. Most devices translate any address once ATS is enabled, but some choose ATS per DMA mapping and can still use bus addresses for the rest. Add pcim_p2pdma_set_ats_per_mapping() so the driver of such a device can declare that before P2PDMA starts treating clients with ATS enabled as translating everything. > diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c > index cf190bd572d4a..6a63901c65fb6 100644 > --- a/drivers/pci/p2pdma.c > +++ b/drivers/pci/p2pdma.c [ ... ] > @@ -1562,6 +1563,32 @@ void pci_p2pmem_publish(struct pci_dev *pdev, bool > publish) > } > EXPORT_SYMBOL_GPL(pci_p2pmem_publish); > > +/** > + * pcim_p2pdma_set_ats_per_mapping - Declare per-mapping ATS for a client > + * @pdev: PCI device that initiates peer-to-peer DMA > + * > + * Declare that @pdev issues Translated Requests only for the DMA mappings > its > + * driver sets up to use ATS, rather than for any address once ATS is > enabled. > + * P2PDMA then routes this client's Requests with the Address Type each query > + * asks about, rather than the one its ATS Enable bit implies. > + */ > +void pcim_p2pdma_set_ats_per_mapping(struct pci_dev *pdev) > +{ > + struct pci_p2pdma *p2p; > + > + p2p = rcu_dereference_protected(pdev->p2pdma, 1); > + if (!p2p) > + /* > + * ats_per_mapping is a performance optimization, > + * if pcim_p2pdma_init() didn't set pdev->p2pdma pointer > + * for some reason, let's simply use ATS global settings. > + */ > + return; [Severity: Medium] Will this silently fail for pure P2PDMA clients? It looks like pdev->p2pdma is only allocated when a device registers as a P2P memory provider via pcim_p2pdma_init(): drivers/pci/p2pdma.c:pcim_p2pdma_init() { ... p2p = devm_kzalloc(&pdev->dev, sizeof(*p2p), GFP_KERNEL); ... } If a driver for a pure P2PDMA client (e.g., an NVMe initiator without CMB) calls pcim_p2pdma_set_ats_per_mapping() to declare its ATS behavior, won't pdev->p2pdma be NULL since it never called pcim_p2pdma_init()? If so, wouldn't this cause the client to silently fall back to suboptimal global ATS routing, as it can never successfully set the ats_per_mapping flag? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=16
