[PATCH 0/2] PCI: Unify ACS quirks

[PATCH 0/2] PCI: Unify ACS quirks

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

These are pretty trivial and not intended to fix anything, but just to
remove unnecessary differences of implementation from the ACS quirks.

Bjorn Helgaas (2):
  PCI: Make ACS quirk implementations more uniform
  PCI: Unify ACS quirk desired vs provided checking

 drivers/pci/quirks.c | 96 ++++++++++++++++++++++++++------------------
 1 file changed, 58 insertions(+), 38 deletions(-)

-- 
2.24.0.432.g9d3f5f5b63-goog

[PATCH 1/2] PCI: Make ACS quirk implementations more uniform

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

The ACS quirks differ in needless ways, which makes them look more
different than they really are.

Reorder the ACS flags in order of definitions in the spec:

  PCI_ACS_SV   Source Validation
  PCI_ACS_TB   Translation Blocking
  PCI_ACS_RR   P2P Request Redirect
  PCI_ACS_CR   P2P Completion Redirect
  PCI_ACS_UF   Upstream Forwarding
  PCI_ACS_EC   P2P Egress Control
  PCI_ACS_DT   Direct Translated P2P

(PCIe r5.0, sec 7.7.8.2) and use similar code structure in all.  No
functional change intended.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/quirks.c | 41 +++++++++++++++++++----------------------
 1 file changed, 19 insertions(+), 22 deletions(-)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 2544e210b984..59f73d084e1d 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -4366,18 +4366,18 @@ static bool pci_quirk_cavium_acs_match(struct pci_dev *dev)
 
 static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags)
 {
+	if (!pci_quirk_cavium_acs_match(dev))
+		return -ENOTTY;
+
 	/*
-	 * Cavium root ports don't advertise an ACS capability.  However,
+	 * Cavium Root Ports don't advertise an ACS capability.  However,
 	 * the RTL internally implements similar protection as if ACS had
-	 * Request Redirection, Completion Redirection, Source Validation,
+	 * Source Validation, Request Redirection, Completion Redirection,
 	 * and Upstream Forwarding features enabled.  Assert that the
 	 * hardware implements and enables equivalent ACS functionality for
 	 * these flags.
 	 */
-	acs_flags &= ~(PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_SV | PCI_ACS_UF);
-
-	if (!pci_quirk_cavium_acs_match(dev))
-		return -ENOTTY;
+	acs_flags &= ~(PCI_ACS_SV | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF);
 
 	return acs_flags ? 0 : 1;
 }
@@ -4395,7 +4395,7 @@ static int pci_quirk_xgene_acs(struct pci_dev *dev, u16 acs_flags)
 }
 
 /*
- * Many Intel PCH root ports do provide ACS-like features to disable peer
+ * Many Intel PCH Root Ports do provide ACS-like features to disable peer
  * transactions and validate bus numbers in requests, but do not provide an
  * actual PCIe ACS capability.  This is the list of device IDs known to fall
  * into that category as provided by Intel in Red Hat bugzilla 1037684.
@@ -4443,37 +4443,34 @@ static bool pci_quirk_intel_pch_acs_match(struct pci_dev *dev)
 	return false;
 }
 
-#define INTEL_PCH_ACS_FLAGS (PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_SV)
+#define INTEL_PCH_ACS_FLAGS (PCI_ACS_SV | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF)
 
 static int pci_quirk_intel_pch_acs(struct pci_dev *dev, u16 acs_flags)
 {
-	u16 flags = dev->dev_flags & PCI_DEV_FLAGS_ACS_ENABLED_QUIRK ?
-		    INTEL_PCH_ACS_FLAGS : 0;
-
 	if (!pci_quirk_intel_pch_acs_match(dev))
 		return -ENOTTY;
 
-	return acs_flags & ~flags ? 0 : 1;
+	if (dev->dev_flags & PCI_DEV_FLAGS_ACS_ENABLED_QUIRK)
+		acs_flags &= ~(INTEL_PCH_ACS_FLAGS);
+
+	return acs_flags ? 0 : 1;
 }
 
 /*
- * These QCOM root ports do provide ACS-like features to disable peer
+ * These QCOM Root Ports do provide ACS-like features to disable peer
  * transactions and validate bus numbers in requests, but do not provide an
  * actual PCIe ACS capability.  Hardware supports source validation but it
  * will report the issue as Completer Abort instead of ACS Violation.
- * Hardware doesn't support peer-to-peer and each root port is a root
- * complex with unique segment numbers.  It is not possible for one root
- * port to pass traffic to another root port.  All PCIe transactions are
- * terminated inside the root port.
+ * Hardware doesn't support peer-to-peer and each Root Port is a Root
+ * Complex with unique segment numbers.  It is not possible for one Root
+ * Port to pass traffic to another Root Port.  All PCIe transactions are
+ * terminated inside the Root Port.
  */
 static int pci_quirk_qcom_rp_acs(struct pci_dev *dev, u16 acs_flags)
 {
-	u16 flags = (PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_SV);
-	int ret = acs_flags & ~flags ? 0 : 1;
-
-	pci_info(dev, "Using QCOM ACS Quirk (%d)\n", ret);
+	acs_flags &= ~(PCI_ACS_SV | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF);
 
-	return ret;
+	return acs_flags ? 0 : 1;
 }
 
 static int pci_quirk_al_acs(struct pci_dev *dev, u16 acs_flags)
-- 
2.24.0.432.g9d3f5f5b63-goog

[PATCH 2/2] PCI: Unify ACS quirk desired vs provided checking

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

Most of the ACS quirks have a similar pattern of:

  acs_flags &= ~( <controls provided by this device> );
  return acs_flags ? 0 : 1;

Pull this out into a helper function to simplify the quirks slightly.  The
helper function is also a convenient place for comments about what the list
of ACS controls means.  No functional change intended.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/quirks.c | 67 +++++++++++++++++++++++++++++---------------
 1 file changed, 45 insertions(+), 22 deletions(-)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 59f73d084e1d..9a1051071a81 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -4296,6 +4296,24 @@ static void quirk_chelsio_T5_disable_root_port_attributes(struct pci_dev *pdev)
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_CHELSIO, PCI_ANY_ID,
 			 quirk_chelsio_T5_disable_root_port_attributes);
 
+/*
+ * pci_acs_ctrl_enabled - compare desired ACS controls with those provided
+ *			  by a device
+ * @acs_ctrl_req: Bitmask of desired ACS controls
+ * @acs_ctrl_ena: Bitmask of ACS controls enabled or provided implicitly by
+ *		  the hardware design
+ *
+ * Return 1 if all ACS controls in the @acs_ctrl_req bitmask are included
+ * in @acs_ctrl_ena, i.e., the device provides all the access controls the
+ * caller desires.  Return 0 otherwise.
+ */
+static int pci_acs_ctrl_enabled(u16 acs_ctrl_req, u16 acs_ctrl_ena)
+{
+	if ((acs_ctrl_req & acs_ctrl_ena) == acs_ctrl_req)
+		return 1;
+	return 0;
+}
+
 /*
  * AMD has indicated that the devices below do not support peer-to-peer
  * in any system where they are found in the southbridge with an AMD
@@ -4339,7 +4357,7 @@ static int pci_quirk_amd_sb_acs(struct pci_dev *dev, u16 acs_flags)
 	/* Filter out flags not applicable to multifunction */
 	acs_flags &= (PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_EC | PCI_ACS_DT);
 
-	return acs_flags & ~(PCI_ACS_RR | PCI_ACS_CR) ? 0 : 1;
+	return pci_acs_ctrl_enabled(acs_flags, PCI_ACS_RR | PCI_ACS_CR);
 #else
 	return -ENODEV;
 #endif
@@ -4377,9 +4395,8 @@ static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags)
 	 * hardware implements and enables equivalent ACS functionality for
 	 * these flags.
 	 */
-	acs_flags &= ~(PCI_ACS_SV | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF);
-
-	return acs_flags ? 0 : 1;
+	return pci_acs_ctrl_enabled(acs_flags,
+		PCI_ACS_SV | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF);
 }
 
 static int pci_quirk_xgene_acs(struct pci_dev *dev, u16 acs_flags)
@@ -4389,9 +4406,8 @@ static int pci_quirk_xgene_acs(struct pci_dev *dev, u16 acs_flags)
 	 * transactions with others, allowing masking out these bits as if they
 	 * were unimplemented in the ACS capability.
 	 */
-	acs_flags &= ~(PCI_ACS_SV | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF);
-
-	return acs_flags ? 0 : 1;
+	return pci_acs_ctrl_enabled(acs_flags,
+		PCI_ACS_SV | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF);
 }
 
 /*
@@ -4443,17 +4459,16 @@ static bool pci_quirk_intel_pch_acs_match(struct pci_dev *dev)
 	return false;
 }
 
-#define INTEL_PCH_ACS_FLAGS (PCI_ACS_SV | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF)
-
 static int pci_quirk_intel_pch_acs(struct pci_dev *dev, u16 acs_flags)
 {
 	if (!pci_quirk_intel_pch_acs_match(dev))
 		return -ENOTTY;
 
 	if (dev->dev_flags & PCI_DEV_FLAGS_ACS_ENABLED_QUIRK)
-		acs_flags &= ~(INTEL_PCH_ACS_FLAGS);
+		return pci_acs_ctrl_enabled(acs_flags,
+			PCI_ACS_SV | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF);
 
-	return acs_flags ? 0 : 1;
+	return pci_acs_ctrl_enabled(acs_flags, 0);
 }
 
 /*
@@ -4468,9 +4483,8 @@ static int pci_quirk_intel_pch_acs(struct pci_dev *dev, u16 acs_flags)
  */
 static int pci_quirk_qcom_rp_acs(struct pci_dev *dev, u16 acs_flags)
 {
-	acs_flags &= ~(PCI_ACS_SV | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF);
-
-	return acs_flags ? 0 : 1;
+	return pci_acs_ctrl_enabled(acs_flags,
+		PCI_ACS_SV | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF);
 }
 
 static int pci_quirk_al_acs(struct pci_dev *dev, u16 acs_flags)
@@ -4571,7 +4585,7 @@ static int pci_quirk_intel_spt_pch_acs(struct pci_dev *dev, u16 acs_flags)
 
 	pci_read_config_dword(dev, pos + INTEL_SPT_ACS_CTRL, &ctrl);
 
-	return acs_flags & ~ctrl ? 0 : 1;
+	return pci_acs_ctrl_enabled(acs_flags, ctrl);
 }
 
 static int pci_quirk_mf_endpoint_acs(struct pci_dev *dev, u16 acs_flags)
@@ -4585,10 +4599,9 @@ static int pci_quirk_mf_endpoint_acs(struct pci_dev *dev, u16 acs_flags)
 	 * perform peer-to-peer with other functions, allowing us to mask out
 	 * these bits as if they were unimplemented in the ACS capability.
 	 */
-	acs_flags &= ~(PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR |
-		       PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_DT);
-
-	return acs_flags ? 0 : 1;
+	return pci_acs_ctrl_enabled(acs_flags,
+		PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR |
+		PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_DT);
 }
 
 static int pci_quirk_brcm_acs(struct pci_dev *dev, u16 acs_flags)
@@ -4599,9 +4612,8 @@ static int pci_quirk_brcm_acs(struct pci_dev *dev, u16 acs_flags)
 	 * Allow each Root Port to be in a separate IOMMU group by masking
 	 * SV/RR/CR/UF bits.
 	 */
-	acs_flags &= ~(PCI_ACS_SV | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF);
-
-	return acs_flags ? 0 : 1;
+	return pci_acs_ctrl_enabled(acs_flags,
+		PCI_ACS_SV | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF);
 }
 
 static const struct pci_dev_acs_enabled {
@@ -4703,6 +4715,17 @@ static const struct pci_dev_acs_enabled {
 	{ 0 }
 };
 
+/*
+ * pci_dev_specific_acs_enabled - check whether device provides ACS controls
+ * @dev:	PCI device
+ * @acs_flags:	Bitmask of desired ACS controls
+ *
+ * Returns:
+ *   -ENOTTY:	No quirk applies to this device; we can't tell whether the
+ *		device provides the desired controls
+ *   0:		Device does not provide all the desired controls
+ *   >0:	Device provides all the controls in @acs_flags
+ */
 int pci_dev_specific_acs_enabled(struct pci_dev *dev, u16 acs_flags)
 {
 	const struct pci_dev_acs_enabled *i;
-- 
2.24.0.432.g9d3f5f5b63-goog

Re: [PATCH 2/2] PCI: Unify ACS quirk desired vs provided checking

[Logan Gunthorpe]

On 2019-11-14 3:06 p.m., Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> Most of the ACS quirks have a similar pattern of:
> 
>   acs_flags &= ~( <controls provided by this device> );
>   return acs_flags ? 0 : 1;
> 
> Pull this out into a helper function to simplify the quirks slightly.  The
> helper function is also a convenient place for comments about what the list
> of ACS controls means.  No functional change intended.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

Looks correct to me. For both patches:

Reviewed-by: Logan Gunthorpe <logang@deltatee.com>

> ---
>  drivers/pci/quirks.c | 67 +++++++++++++++++++++++++++++---------------
>  1 file changed, 45 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 59f73d084e1d..9a1051071a81 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -4296,6 +4296,24 @@ static void quirk_chelsio_T5_disable_root_port_attributes(struct pci_dev *pdev)
>  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_CHELSIO, PCI_ANY_ID,
>  			 quirk_chelsio_T5_disable_root_port_attributes);
>  
> +/*
> + * pci_acs_ctrl_enabled - compare desired ACS controls with those provided
> + *			  by a device
> + * @acs_ctrl_req: Bitmask of desired ACS controls
> + * @acs_ctrl_ena: Bitmask of ACS controls enabled or provided implicitly by
> + *		  the hardware design
> + *
> + * Return 1 if all ACS controls in the @acs_ctrl_req bitmask are included
> + * in @acs_ctrl_ena, i.e., the device provides all the access controls the
> + * caller desires.  Return 0 otherwise.
> + */
> +static int pci_acs_ctrl_enabled(u16 acs_ctrl_req, u16 acs_ctrl_ena)
> +{
> +	if ((acs_ctrl_req & acs_ctrl_ena) == acs_ctrl_req)
> +		return 1;
> +	return 0;
> +}
> +
>  /*
>   * AMD has indicated that the devices below do not support peer-to-peer
>   * in any system where they are found in the southbridge with an AMD
> @@ -4339,7 +4357,7 @@ static int pci_quirk_amd_sb_acs(struct pci_dev *dev, u16 acs_flags)
>  	/* Filter out flags not applicable to multifunction */
>  	acs_flags &= (PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_EC | PCI_ACS_DT);
>  
> -	return acs_flags & ~(PCI_ACS_RR | PCI_ACS_CR) ? 0 : 1;
> +	return pci_acs_ctrl_enabled(acs_flags, PCI_ACS_RR | PCI_ACS_CR);
>  #else
>  	return -ENODEV;
>  #endif
> @@ -4377,9 +4395,8 @@ static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags)
>  	 * hardware implements and enables equivalent ACS functionality for
>  	 * these flags.
>  	 */
> -	acs_flags &= ~(PCI_ACS_SV | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF);
> -
> -	return acs_flags ? 0 : 1;
> +	return pci_acs_ctrl_enabled(acs_flags,
> +		PCI_ACS_SV | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF);
>  }
>  
>  static int pci_quirk_xgene_acs(struct pci_dev *dev, u16 acs_flags)
> @@ -4389,9 +4406,8 @@ static int pci_quirk_xgene_acs(struct pci_dev *dev, u16 acs_flags)
>  	 * transactions with others, allowing masking out these bits as if they
>  	 * were unimplemented in the ACS capability.
>  	 */
> -	acs_flags &= ~(PCI_ACS_SV | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF);
> -
> -	return acs_flags ? 0 : 1;
> +	return pci_acs_ctrl_enabled(acs_flags,
> +		PCI_ACS_SV | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF);
>  }
>  
>  /*
> @@ -4443,17 +4459,16 @@ static bool pci_quirk_intel_pch_acs_match(struct pci_dev *dev)
>  	return false;
>  }
>  
> -#define INTEL_PCH_ACS_FLAGS (PCI_ACS_SV | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF)
> -
>  static int pci_quirk_intel_pch_acs(struct pci_dev *dev, u16 acs_flags)
>  {
>  	if (!pci_quirk_intel_pch_acs_match(dev))
>  		return -ENOTTY;
>  
>  	if (dev->dev_flags & PCI_DEV_FLAGS_ACS_ENABLED_QUIRK)
> -		acs_flags &= ~(INTEL_PCH_ACS_FLAGS);
> +		return pci_acs_ctrl_enabled(acs_flags,
> +			PCI_ACS_SV | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF);
>  
> -	return acs_flags ? 0 : 1;
> +	return pci_acs_ctrl_enabled(acs_flags, 0);
>  }
>  
>  /*
> @@ -4468,9 +4483,8 @@ static int pci_quirk_intel_pch_acs(struct pci_dev *dev, u16 acs_flags)
>   */
>  static int pci_quirk_qcom_rp_acs(struct pci_dev *dev, u16 acs_flags)
>  {
> -	acs_flags &= ~(PCI_ACS_SV | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF);
> -
> -	return acs_flags ? 0 : 1;
> +	return pci_acs_ctrl_enabled(acs_flags,
> +		PCI_ACS_SV | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF);
>  }
>  
>  static int pci_quirk_al_acs(struct pci_dev *dev, u16 acs_flags)
> @@ -4571,7 +4585,7 @@ static int pci_quirk_intel_spt_pch_acs(struct pci_dev *dev, u16 acs_flags)
>  
>  	pci_read_config_dword(dev, pos + INTEL_SPT_ACS_CTRL, &ctrl);
>  
> -	return acs_flags & ~ctrl ? 0 : 1;
> +	return pci_acs_ctrl_enabled(acs_flags, ctrl);
>  }
>  
>  static int pci_quirk_mf_endpoint_acs(struct pci_dev *dev, u16 acs_flags)
> @@ -4585,10 +4599,9 @@ static int pci_quirk_mf_endpoint_acs(struct pci_dev *dev, u16 acs_flags)
>  	 * perform peer-to-peer with other functions, allowing us to mask out
>  	 * these bits as if they were unimplemented in the ACS capability.
>  	 */
> -	acs_flags &= ~(PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR |
> -		       PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_DT);
> -
> -	return acs_flags ? 0 : 1;
> +	return pci_acs_ctrl_enabled(acs_flags,
> +		PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR |
> +		PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_DT);
>  }
>  
>  static int pci_quirk_brcm_acs(struct pci_dev *dev, u16 acs_flags)
> @@ -4599,9 +4612,8 @@ static int pci_quirk_brcm_acs(struct pci_dev *dev, u16 acs_flags)
>  	 * Allow each Root Port to be in a separate IOMMU group by masking
>  	 * SV/RR/CR/UF bits.
>  	 */
> -	acs_flags &= ~(PCI_ACS_SV | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF);
> -
> -	return acs_flags ? 0 : 1;
> +	return pci_acs_ctrl_enabled(acs_flags,
> +		PCI_ACS_SV | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF);
>  }
>  
>  static const struct pci_dev_acs_enabled {
> @@ -4703,6 +4715,17 @@ static const struct pci_dev_acs_enabled {
>  	{ 0 }
>  };
>  
> +/*
> + * pci_dev_specific_acs_enabled - check whether device provides ACS controls
> + * @dev:	PCI device
> + * @acs_flags:	Bitmask of desired ACS controls
> + *
> + * Returns:
> + *   -ENOTTY:	No quirk applies to this device; we can't tell whether the
> + *		device provides the desired controls
> + *   0:		Device does not provide all the desired controls
> + *   >0:	Device provides all the controls in @acs_flags
> + */
>  int pci_dev_specific_acs_enabled(struct pci_dev *dev, u16 acs_flags)
>  {
>  	const struct pci_dev_acs_enabled *i;
> 

Re: [PATCH 2/2] PCI: Unify ACS quirk desired vs provided checking

[Alex Williamson]

On Thu, 14 Nov 2019 16:06:01 -0600
Bjorn Helgaas <helgaas@kernel.org> wrote:

> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> Most of the ACS quirks have a similar pattern of:
> 
>   acs_flags &= ~( <controls provided by this device> );
>   return acs_flags ? 0 : 1;
> 
> Pull this out into a helper function to simplify the quirks slightly.  The
> helper function is also a convenient place for comments about what the list
> of ACS controls means.  No functional change intended.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  drivers/pci/quirks.c | 67 +++++++++++++++++++++++++++++---------------
>  1 file changed, 45 insertions(+), 22 deletions(-)

Much cleaner, and looks equivalent to me.  For both:

Reviewed-by: Alex Williamson <alex.williamson@redhat.com>

Thanks!

> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 59f73d084e1d..9a1051071a81 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -4296,6 +4296,24 @@ static void quirk_chelsio_T5_disable_root_port_attributes(struct pci_dev *pdev)
>  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_CHELSIO, PCI_ANY_ID,
>  			 quirk_chelsio_T5_disable_root_port_attributes);
>  
> +/*
> + * pci_acs_ctrl_enabled - compare desired ACS controls with those provided
> + *			  by a device
> + * @acs_ctrl_req: Bitmask of desired ACS controls
> + * @acs_ctrl_ena: Bitmask of ACS controls enabled or provided implicitly by
> + *		  the hardware design
> + *
> + * Return 1 if all ACS controls in the @acs_ctrl_req bitmask are included
> + * in @acs_ctrl_ena, i.e., the device provides all the access controls the
> + * caller desires.  Return 0 otherwise.
> + */
> +static int pci_acs_ctrl_enabled(u16 acs_ctrl_req, u16 acs_ctrl_ena)
> +{
> +	if ((acs_ctrl_req & acs_ctrl_ena) == acs_ctrl_req)
> +		return 1;
> +	return 0;
> +}
> +
>  /*
>   * AMD has indicated that the devices below do not support peer-to-peer
>   * in any system where they are found in the southbridge with an AMD
> @@ -4339,7 +4357,7 @@ static int pci_quirk_amd_sb_acs(struct pci_dev *dev, u16 acs_flags)
>  	/* Filter out flags not applicable to multifunction */
>  	acs_flags &= (PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_EC | PCI_ACS_DT);
>  
> -	return acs_flags & ~(PCI_ACS_RR | PCI_ACS_CR) ? 0 : 1;
> +	return pci_acs_ctrl_enabled(acs_flags, PCI_ACS_RR | PCI_ACS_CR);
>  #else
>  	return -ENODEV;
>  #endif
> @@ -4377,9 +4395,8 @@ static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags)
>  	 * hardware implements and enables equivalent ACS functionality for
>  	 * these flags.
>  	 */
> -	acs_flags &= ~(PCI_ACS_SV | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF);
> -
> -	return acs_flags ? 0 : 1;
> +	return pci_acs_ctrl_enabled(acs_flags,
> +		PCI_ACS_SV | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF);
>  }
>  
>  static int pci_quirk_xgene_acs(struct pci_dev *dev, u16 acs_flags)
> @@ -4389,9 +4406,8 @@ static int pci_quirk_xgene_acs(struct pci_dev *dev, u16 acs_flags)
>  	 * transactions with others, allowing masking out these bits as if they
>  	 * were unimplemented in the ACS capability.
>  	 */
> -	acs_flags &= ~(PCI_ACS_SV | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF);
> -
> -	return acs_flags ? 0 : 1;
> +	return pci_acs_ctrl_enabled(acs_flags,
> +		PCI_ACS_SV | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF);
>  }
>  
>  /*
> @@ -4443,17 +4459,16 @@ static bool pci_quirk_intel_pch_acs_match(struct pci_dev *dev)
>  	return false;
>  }
>  
> -#define INTEL_PCH_ACS_FLAGS (PCI_ACS_SV | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF)
> -
>  static int pci_quirk_intel_pch_acs(struct pci_dev *dev, u16 acs_flags)
>  {
>  	if (!pci_quirk_intel_pch_acs_match(dev))
>  		return -ENOTTY;
>  
>  	if (dev->dev_flags & PCI_DEV_FLAGS_ACS_ENABLED_QUIRK)
> -		acs_flags &= ~(INTEL_PCH_ACS_FLAGS);
> +		return pci_acs_ctrl_enabled(acs_flags,
> +			PCI_ACS_SV | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF);
>  
> -	return acs_flags ? 0 : 1;
> +	return pci_acs_ctrl_enabled(acs_flags, 0);
>  }
>  
>  /*
> @@ -4468,9 +4483,8 @@ static int pci_quirk_intel_pch_acs(struct pci_dev *dev, u16 acs_flags)
>   */
>  static int pci_quirk_qcom_rp_acs(struct pci_dev *dev, u16 acs_flags)
>  {
> -	acs_flags &= ~(PCI_ACS_SV | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF);
> -
> -	return acs_flags ? 0 : 1;
> +	return pci_acs_ctrl_enabled(acs_flags,
> +		PCI_ACS_SV | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF);
>  }
>  
>  static int pci_quirk_al_acs(struct pci_dev *dev, u16 acs_flags)
> @@ -4571,7 +4585,7 @@ static int pci_quirk_intel_spt_pch_acs(struct pci_dev *dev, u16 acs_flags)
>  
>  	pci_read_config_dword(dev, pos + INTEL_SPT_ACS_CTRL, &ctrl);
>  
> -	return acs_flags & ~ctrl ? 0 : 1;
> +	return pci_acs_ctrl_enabled(acs_flags, ctrl);
>  }
>  
>  static int pci_quirk_mf_endpoint_acs(struct pci_dev *dev, u16 acs_flags)
> @@ -4585,10 +4599,9 @@ static int pci_quirk_mf_endpoint_acs(struct pci_dev *dev, u16 acs_flags)
>  	 * perform peer-to-peer with other functions, allowing us to mask out
>  	 * these bits as if they were unimplemented in the ACS capability.
>  	 */
> -	acs_flags &= ~(PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR |
> -		       PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_DT);
> -
> -	return acs_flags ? 0 : 1;
> +	return pci_acs_ctrl_enabled(acs_flags,
> +		PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR |
> +		PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_DT);
>  }
>  
>  static int pci_quirk_brcm_acs(struct pci_dev *dev, u16 acs_flags)
> @@ -4599,9 +4612,8 @@ static int pci_quirk_brcm_acs(struct pci_dev *dev, u16 acs_flags)
>  	 * Allow each Root Port to be in a separate IOMMU group by masking
>  	 * SV/RR/CR/UF bits.
>  	 */
> -	acs_flags &= ~(PCI_ACS_SV | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF);
> -
> -	return acs_flags ? 0 : 1;
> +	return pci_acs_ctrl_enabled(acs_flags,
> +		PCI_ACS_SV | PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF);
>  }
>  
>  static const struct pci_dev_acs_enabled {
> @@ -4703,6 +4715,17 @@ static const struct pci_dev_acs_enabled {
>  	{ 0 }
>  };
>  
> +/*
> + * pci_dev_specific_acs_enabled - check whether device provides ACS controls
> + * @dev:	PCI device
> + * @acs_flags:	Bitmask of desired ACS controls
> + *
> + * Returns:
> + *   -ENOTTY:	No quirk applies to this device; we can't tell whether the
> + *		device provides the desired controls
> + *   0:		Device does not provide all the desired controls
> + *   >0:	Device provides all the controls in @acs_flags
> + */
>  int pci_dev_specific_acs_enabled(struct pci_dev *dev, u16 acs_flags)
>  {
>  	const struct pci_dev_acs_enabled *i;

Re: [PATCH 0/2] PCI: Unify ACS quirks

[Bjorn Helgaas]

On Thu, Nov 14, 2019 at 04:05:59PM -0600, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> These are pretty trivial and not intended to fix anything, but just to
> remove unnecessary differences of implementation from the ACS quirks.
> 
> Bjorn Helgaas (2):
>   PCI: Make ACS quirk implementations more uniform
>   PCI: Unify ACS quirk desired vs provided checking
> 
>  drivers/pci/quirks.c | 96 ++++++++++++++++++++++++++------------------
>  1 file changed, 58 insertions(+), 38 deletions(-)

Applied with reviewed-by from Alex and Logan to pci/virtualization for
v5.5.