* [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).