Re: [PATCH v9 16/18] PCI/P2PDMA: Let a client declare that it selects ATS per mapping

[sashiko-bot@kernel.org]

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 <leon@kernel.org>

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/20261001-fix-p2p-acs-v4-0-v9-0-1a8e0f50ddd9@nvidia.com?part=16