[v2,3/5] PCI/ATS: Skip VF ATS initialization if PF does not implement it

From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

If PF does not implement ATS and VF implements/uses it, it might lead to
runtime issues. Also, as per spec r4.0, sec 9.3.7.8, PF should implement
ATS if VF implements it. So add additional check to confirm given device
aligns with the spec.

Cc: Ashok Raj <ashok.raj@intel.com>
Cc: Keith Busch <keith.busch@intel.com>
Suggested-by: Ashok Raj <ashok.raj@intel.com>
Reviewed-by: Keith Busch <keith.busch@intel.com>
Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 drivers/pci/ats.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

> -----Original Message-----
> From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-owner@vger.kernel.org] On Behalf Of
> sathyanarayanan.kuppuswamy@linux.intel.com
> Sent: Monday, May 6, 2019 12:20 PM
> To: bhelgaas@google.com
> Cc: linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org; ashok.raj@intel.com;
> keith.busch@intel.com; sathyanarayanan.kuppuswamy@linux.intel.com
> Subject: [PATCH v2 3/5] PCI/ATS: Skip VF ATS initialization if PF does not implement it
> 
> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> 
> If PF does not implement ATS and VF implements/uses it, it might lead to
> runtime issues. Also, as per spec r4.0, sec 9.3.7.8, PF should implement
> ATS if VF implements it. So add additional check to confirm given device
> aligns with the spec.
...
> +	/*
> +	 * Per PCIe r4.0, sec 9.3.7.8, if VF implements Address Translation
> +	 * Services (ATS) Extended Capability then corresponding PF should
> +	 * also implement it.
> +	 */
...

In standardese, "should" means recommended, not required. The PCIe spec uses
"must" for this rule; the comments should match.

On Mon, May 06, 2019 at 10:20:05AM -0700, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> 
> If PF does not implement ATS and VF implements/uses it, it might lead to
> runtime issues. Also, as per spec r4.0, sec 9.3.7.8, PF should implement
> ATS if VF implements it. So add additional check to confirm given device
> aligns with the spec.

"might lead to runtime issues" is pretty wishy-washy.  I really don't
want to prevent some device from working merely because it has
something in config space that doesn't follow the spec exactly but we
never touch.

> Cc: Ashok Raj <ashok.raj@intel.com>
> Cc: Keith Busch <keith.busch@intel.com>
> Suggested-by: Ashok Raj <ashok.raj@intel.com>
> Reviewed-by: Keith Busch <keith.busch@intel.com>
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> ---
>  drivers/pci/ats.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
> index e7a904e347c3..718e6f414680 100644
> --- a/drivers/pci/ats.c
> +++ b/drivers/pci/ats.c
> @@ -19,6 +19,7 @@
>  void pci_ats_init(struct pci_dev *dev)
>  {
>  	int pos;
> +	struct pci_dev *pdev;
>  
>  	if (pci_ats_disabled())
>  		return;
> @@ -27,6 +28,17 @@ void pci_ats_init(struct pci_dev *dev)
>  	if (!pos)
>  		return;
>  
> +	/*
> +	 * Per PCIe r4.0, sec 9.3.7.8, if VF implements Address Translation
> +	 * Services (ATS) Extended Capability then corresponding PF should
> +	 * also implement it.
> +	 */
> +	if (dev->is_virtfn) {
> +		pdev = pci_physfn(dev);
> +		if (!pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_ATS))
> +			return;
> +	}
> +
>  	dev->ats_cap = pos;
>  }
>  
> -- 
> 2.20.1
>