* [PATCH 0/2] PCI: Unify ACS quirks
@ 2019-11-14 22:05 Bjorn Helgaas
2019-11-14 22:06 ` [PATCH 1/2] PCI: Make ACS quirk implementations more uniform Bjorn Helgaas
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Bjorn Helgaas @ 2019-11-14 22:05 UTC (permalink / raw)
To: Alex Williamson, linux-pci
Cc: George Cherian, Robert Richter, Feng Kan, Logan Gunthorpe,
Abhinav Ratna, 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] PCI: Make ACS quirk implementations more uniform
2019-11-14 22:05 [PATCH 0/2] PCI: Unify ACS quirks Bjorn Helgaas
@ 2019-11-14 22:06 ` Bjorn Helgaas
2019-11-14 22:06 ` [PATCH 2/2] PCI: Unify ACS quirk desired vs provided checking Bjorn Helgaas
2019-11-20 13:23 ` [PATCH 0/2] PCI: Unify ACS quirks Bjorn Helgaas
2 siblings, 0 replies; 6+ messages in thread
From: Bjorn Helgaas @ 2019-11-14 22:06 UTC (permalink / raw)
To: Alex Williamson, linux-pci
Cc: George Cherian, Robert Richter, Feng Kan, Logan Gunthorpe,
Abhinav Ratna, 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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] PCI: Unify ACS quirk desired vs provided checking
2019-11-14 22:05 [PATCH 0/2] PCI: Unify ACS quirks Bjorn Helgaas
2019-11-14 22:06 ` [PATCH 1/2] PCI: Make ACS quirk implementations more uniform Bjorn Helgaas
@ 2019-11-14 22:06 ` Bjorn Helgaas
2019-11-14 22:17 ` Logan Gunthorpe
2019-11-14 22:25 ` Alex Williamson
2019-11-20 13:23 ` [PATCH 0/2] PCI: Unify ACS quirks Bjorn Helgaas
2 siblings, 2 replies; 6+ messages in thread
From: Bjorn Helgaas @ 2019-11-14 22:06 UTC (permalink / raw)
To: Alex Williamson, linux-pci
Cc: George Cherian, Robert Richter, Feng Kan, Logan Gunthorpe,
Abhinav Ratna, 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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] PCI: Unify ACS quirk desired vs provided checking
2019-11-14 22:06 ` [PATCH 2/2] PCI: Unify ACS quirk desired vs provided checking Bjorn Helgaas
@ 2019-11-14 22:17 ` Logan Gunthorpe
2019-11-14 22:25 ` Alex Williamson
1 sibling, 0 replies; 6+ messages in thread
From: Logan Gunthorpe @ 2019-11-14 22:17 UTC (permalink / raw)
To: Bjorn Helgaas, Alex Williamson, linux-pci
Cc: George Cherian, Robert Richter, Feng Kan, Abhinav Ratna, Bjorn Helgaas
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;
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] PCI: Unify ACS quirk desired vs provided checking
2019-11-14 22:06 ` [PATCH 2/2] PCI: Unify ACS quirk desired vs provided checking Bjorn Helgaas
2019-11-14 22:17 ` Logan Gunthorpe
@ 2019-11-14 22:25 ` Alex Williamson
1 sibling, 0 replies; 6+ messages in thread
From: Alex Williamson @ 2019-11-14 22:25 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: linux-pci, George Cherian, Robert Richter, Feng Kan,
Logan Gunthorpe, Abhinav Ratna, Bjorn Helgaas
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;
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 0/2] PCI: Unify ACS quirks
2019-11-14 22:05 [PATCH 0/2] PCI: Unify ACS quirks Bjorn Helgaas
2019-11-14 22:06 ` [PATCH 1/2] PCI: Make ACS quirk implementations more uniform Bjorn Helgaas
2019-11-14 22:06 ` [PATCH 2/2] PCI: Unify ACS quirk desired vs provided checking Bjorn Helgaas
@ 2019-11-20 13:23 ` Bjorn Helgaas
2 siblings, 0 replies; 6+ messages in thread
From: Bjorn Helgaas @ 2019-11-20 13:23 UTC (permalink / raw)
To: Alex Williamson, linux-pci
Cc: George Cherian, Robert Richter, Feng Kan, Logan Gunthorpe, Abhinav Ratna
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-11-20 13:23 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-14 22:05 [PATCH 0/2] PCI: Unify ACS quirks Bjorn Helgaas
2019-11-14 22:06 ` [PATCH 1/2] PCI: Make ACS quirk implementations more uniform Bjorn Helgaas
2019-11-14 22:06 ` [PATCH 2/2] PCI: Unify ACS quirk desired vs provided checking Bjorn Helgaas
2019-11-14 22:17 ` Logan Gunthorpe
2019-11-14 22:25 ` Alex Williamson
2019-11-20 13:23 ` [PATCH 0/2] PCI: Unify ACS quirks Bjorn Helgaas
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).