[v6] PCI: quirks: update Cavium ThunderX ACS quirk implementation

Commit Message

Vadim Lomovtsev Sept. 27, 2017, 6:20 p.m. UTC

This commit makes Cavium PCI ACS quirk applicable only to Cavium
ThunderX (CN8XXX) family PCIE Root Ports which has limited PCI capabilities
in terms of no ACS support advertisement. However, the RTL internally
implements similar protection as if ACS had completion/request redirection,
upstream forwarding and validation features enabled.

Current quirk implementation doesn't take into account PCIERCs which
also needs to be quirked. So the pci device id check mask is updated
and check of device ID moved into separate function.

Signed-off-by: Vadim Lomovtsev <[e-mail]>
---
	v5 -> v6: comment typo fix: change 0xa00 to 0xa000

 drivers/pci/quirks.c | 29 +++++++++++++++++++++--------
 1 file changed, 21 insertions(+), 8 deletions(-)

Comments

Vadim Lomovtsev Sept. 27, 2017, 8:03 p.m. UTC | #1

Hi guys,

I've found one typo (0xa00 instead of 0xa000 at code comment) and v6 has it fixed.
I bring my apologies for that, could you please review this patch once again.

WBR,
Vadim

On Wed, Sep 27, 2017 at 11:20:39AM -0700, Vadim Lomovtsev wrote:
> This commit makes Cavium PCI ACS quirk applicable only to Cavium
> ThunderX (CN8XXX) family PCIE Root Ports which has limited PCI capabilities
> in terms of no ACS support advertisement. However, the RTL internally
> implements similar protection as if ACS had completion/request redirection,
> upstream forwarding and validation features enabled.
> 
> Current quirk implementation doesn't take into account PCIERCs which
> also needs to be quirked. So the pci device id check mask is updated
> and check of device ID moved into separate function.
> 
> Signed-off-by: Vadim Lomovtsev <[e-mail]>
> ---
> 	v5 -> v6: comment typo fix: change 0xa00 to 0xa000
> 
>  drivers/pci/quirks.c | 29 +++++++++++++++++++++--------
>  1 file changed, 21 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index a4d3361..ed6c76d 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -4211,20 +4211,33 @@ static int pci_quirk_amd_sb_acs(struct pci_dev *dev, u16 acs_flags)
>  #endif
>  }
>  
> -static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags)
> +/*
> + * The Cavium downstream ports doesn't advertise their ACS capability registers.
> + * However, the RTL internally implements similar protection as if
> + * ACS had completion redirection, forwarding and validation features enabled.
> + * So by this flags we're asserting that the hardware implements and
> + * enables equivalent ACS functionality for these flags.
> + */
> +#define CAVIUM_CN8XXX_ACS_FLAGS (PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_SV | PCI_ACS_UF)
> +
> +static __inline__  bool pci_quirk_cavium_acs_match(struct pci_dev *dev)
>  {
>  	/*
> -	 * Cavium devices matching this quirk do not perform peer-to-peer
> -	 * with other functions, allowing masking out these bits as if they
> -	 * were unimplemented in the ACS capability.
> +	 * Effectively selects all downstream ports for whole ThunderX 1 family
> +	 * by 0xa000 mask (which represents 8 SoCs), while the lower bits of device ID
> +	 * are used to indicate which subdevice is used within the SoC.
>  	 */
> -	acs_flags &= ~(PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR |
> -		       PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_DT);
> +	return (pci_is_pcie(dev) &&
> +		(pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) &&
> +		((dev->device & 0xf800) == 0xa000));
> +}
>  
> -	if (!((dev->device >= 0xa000) && (dev->device <= 0xa0ff)))
> +static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags)
> +{
> +	if (!pci_quirk_cavium_acs_match(dev))
>  		return -ENOTTY;
>  
> -	return acs_flags ? 0 : 1;
> +	return acs_flags & ~(CAVIUM_CN8XXX_ACS_FLAGS) ? 0 : 1;
>  }
>  
>  static int pci_quirk_xgene_acs(struct pci_dev *dev, u16 acs_flags)
> -- 
> 2.4.11
>

Alex Williamson Sept. 27, 2017, 8:18 p.m. UTC | #2

On Wed, 27 Sep 2017 11:20:39 -0700
Vadim Lomovtsev <[e-mail]> wrote:

> This commit makes Cavium PCI ACS quirk applicable only to Cavium
> ThunderX (CN8XXX) family PCIE Root Ports which has limited PCI capabilities
> in terms of no ACS support advertisement. However, the RTL internally
> implements similar protection as if ACS had completion/request redirection,
> upstream forwarding and validation features enabled.
> 
> Current quirk implementation doesn't take into account PCIERCs which
> also needs to be quirked. So the pci device id check mask is updated
> and check of device ID moved into separate function.
> 
> Signed-off-by: Vadim Lomovtsev <[e-mail]>
> ---
> 	v5 -> v6: comment typo fix: change 0xa00 to 0xa000

Reviewed-by: Alex Williamson <[e-mail]>

 
>  drivers/pci/quirks.c | 29 +++++++++++++++++++++--------
>  1 file changed, 21 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index a4d3361..ed6c76d 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -4211,20 +4211,33 @@ static int pci_quirk_amd_sb_acs(struct pci_dev *dev, u16 acs_flags)
>  #endif
>  }
>  
> -static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags)
> +/*
> + * The Cavium downstream ports doesn't advertise their ACS capability registers.
> + * However, the RTL internally implements similar protection as if
> + * ACS had completion redirection, forwarding and validation features enabled.
> + * So by this flags we're asserting that the hardware implements and
> + * enables equivalent ACS functionality for these flags.
> + */
> +#define CAVIUM_CN8XXX_ACS_FLAGS (PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_SV | PCI_ACS_UF)
> +
> +static __inline__  bool pci_quirk_cavium_acs_match(struct pci_dev *dev)
>  {
>  	/*
> -	 * Cavium devices matching this quirk do not perform peer-to-peer
> -	 * with other functions, allowing masking out these bits as if they
> -	 * were unimplemented in the ACS capability.
> +	 * Effectively selects all downstream ports for whole ThunderX 1 family
> +	 * by 0xa000 mask (which represents 8 SoCs), while the lower bits of device ID
> +	 * are used to indicate which subdevice is used within the SoC.
>  	 */
> -	acs_flags &= ~(PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR |
> -		       PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_DT);
> +	return (pci_is_pcie(dev) &&
> +		(pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) &&
> +		((dev->device & 0xf800) == 0xa000));
> +}
>  
> -	if (!((dev->device >= 0xa000) && (dev->device <= 0xa0ff)))
> +static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags)
> +{
> +	if (!pci_quirk_cavium_acs_match(dev))
>  		return -ENOTTY;
>  
> -	return acs_flags ? 0 : 1;
> +	return acs_flags & ~(CAVIUM_CN8XXX_ACS_FLAGS) ? 0 : 1;
>  }
>  
>  static int pci_quirk_xgene_acs(struct pci_dev *dev, u16 acs_flags)

Vadim Lomovtsev Sept. 29, 2017, 12:22 p.m. UTC | #3

Hi Bjorn,

On Wed, Sep 27, 2017 at 02:18:54PM -0600, Alex Williamson wrote:
> On Wed, 27 Sep 2017 11:20:39 -0700
> Vadim Lomovtsev <[e-mail]> wrote:
> 
> > This commit makes Cavium PCI ACS quirk applicable only to Cavium
> > ThunderX (CN8XXX) family PCIE Root Ports which has limited PCI capabilities
> > in terms of no ACS support advertisement. However, the RTL internally
> > implements similar protection as if ACS had completion/request redirection,
> > upstream forwarding and validation features enabled.
> > 
> > Current quirk implementation doesn't take into account PCIERCs which
> > also needs to be quirked. So the pci device id check mask is updated
> > and check of device ID moved into separate function.
> > 
> > Signed-off-by: Vadim Lomovtsev <[e-mail]>
> > ---
> > 	v5 -> v6: comment typo fix: change 0xa00 to 0xa000
> 
> Reviewed-by: Alex Williamson <[e-mail]>
>

If there is no any objections, could you please take this patch ?

WBR,
Vadim

>  
> >  drivers/pci/quirks.c | 29 +++++++++++++++++++++--------
> >  1 file changed, 21 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> > index a4d3361..ed6c76d 100644
> > --- a/drivers/pci/quirks.c
> > +++ b/drivers/pci/quirks.c
> > @@ -4211,20 +4211,33 @@ static int pci_quirk_amd_sb_acs(struct pci_dev *dev, u16 acs_flags)
> >  #endif
> >  }
> >  
> > -static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags)
> > +/*
> > + * The Cavium downstream ports doesn't advertise their ACS capability registers.
> > + * However, the RTL internally implements similar protection as if
> > + * ACS had completion redirection, forwarding and validation features enabled.
> > + * So by this flags we're asserting that the hardware implements and
> > + * enables equivalent ACS functionality for these flags.
> > + */
> > +#define CAVIUM_CN8XXX_ACS_FLAGS (PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_SV | PCI_ACS_UF)
> > +
> > +static __inline__  bool pci_quirk_cavium_acs_match(struct pci_dev *dev)
> >  {
> >  	/*
> > -	 * Cavium devices matching this quirk do not perform peer-to-peer
> > -	 * with other functions, allowing masking out these bits as if they
> > -	 * were unimplemented in the ACS capability.
> > +	 * Effectively selects all downstream ports for whole ThunderX 1 family
> > +	 * by 0xa000 mask (which represents 8 SoCs), while the lower bits of device ID
> > +	 * are used to indicate which subdevice is used within the SoC.
> >  	 */
> > -	acs_flags &= ~(PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR |
> > -		       PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_DT);
> > +	return (pci_is_pcie(dev) &&
> > +		(pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) &&
> > +		((dev->device & 0xf800) == 0xa000));
> > +}
> >  
> > -	if (!((dev->device >= 0xa000) && (dev->device <= 0xa0ff)))
> > +static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags)
> > +{
> > +	if (!pci_quirk_cavium_acs_match(dev))
> >  		return -ENOTTY;
> >  
> > -	return acs_flags ? 0 : 1;
> > +	return acs_flags & ~(CAVIUM_CN8XXX_ACS_FLAGS) ? 0 : 1;
> >  }
> >  
> >  static int pci_quirk_xgene_acs(struct pci_dev *dev, u16 acs_flags)
>

Vadim Lomovtsev Oct. 9, 2017, 4:14 p.m. UTC | #4

Hi Bjorn,

Would you please consider to take this patch ?
It is required to provide correct ACS mask for CN8xxx PCI root ports.

WBR,
Vadim

On Wed, Sep 27, 2017 at 02:18:54PM -0600, Alex Williamson wrote:
> On Wed, 27 Sep 2017 11:20:39 -0700
> Vadim Lomovtsev <[e-mail]> wrote:
> 
> > This commit makes Cavium PCI ACS quirk applicable only to Cavium
> > ThunderX (CN8XXX) family PCIE Root Ports which has limited PCI capabilities
> > in terms of no ACS support advertisement. However, the RTL internally
> > implements similar protection as if ACS had completion/request redirection,
> > upstream forwarding and validation features enabled.
> > 
> > Current quirk implementation doesn't take into account PCIERCs which
> > also needs to be quirked. So the pci device id check mask is updated
> > and check of device ID moved into separate function.
> > 
> > Signed-off-by: Vadim Lomovtsev <[e-mail]>
> > ---
> > 	v5 -> v6: comment typo fix: change 0xa00 to 0xa000
> 
> Reviewed-by: Alex Williamson <[e-mail]>
> 
>  
> >  drivers/pci/quirks.c | 29 +++++++++++++++++++++--------
> >  1 file changed, 21 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> > index a4d3361..ed6c76d 100644
> > --- a/drivers/pci/quirks.c
> > +++ b/drivers/pci/quirks.c
> > @@ -4211,20 +4211,33 @@ static int pci_quirk_amd_sb_acs(struct pci_dev *dev, u16 acs_flags)
> >  #endif
> >  }
> >  
> > -static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags)
> > +/*
> > + * The Cavium downstream ports doesn't advertise their ACS capability registers.
> > + * However, the RTL internally implements similar protection as if
> > + * ACS had completion redirection, forwarding and validation features enabled.
> > + * So by this flags we're asserting that the hardware implements and
> > + * enables equivalent ACS functionality for these flags.
> > + */
> > +#define CAVIUM_CN8XXX_ACS_FLAGS (PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_SV | PCI_ACS_UF)
> > +
> > +static __inline__  bool pci_quirk_cavium_acs_match(struct pci_dev *dev)
> >  {
> >  	/*
> > -	 * Cavium devices matching this quirk do not perform peer-to-peer
> > -	 * with other functions, allowing masking out these bits as if they
> > -	 * were unimplemented in the ACS capability.
> > +	 * Effectively selects all downstream ports for whole ThunderX 1 family
> > +	 * by 0xa000 mask (which represents 8 SoCs), while the lower bits of device ID
> > +	 * are used to indicate which subdevice is used within the SoC.
> >  	 */
> > -	acs_flags &= ~(PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR |
> > -		       PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_DT);
> > +	return (pci_is_pcie(dev) &&
> > +		(pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) &&
> > +		((dev->device & 0xf800) == 0xa000));
> > +}
> >  
> > -	if (!((dev->device >= 0xa000) && (dev->device <= 0xa0ff)))
> > +static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags)
> > +{
> > +	if (!pci_quirk_cavium_acs_match(dev))
> >  		return -ENOTTY;
> >  
> > -	return acs_flags ? 0 : 1;
> > +	return acs_flags & ~(CAVIUM_CN8XXX_ACS_FLAGS) ? 0 : 1;
> >  }
> >  
> >  static int pci_quirk_xgene_acs(struct pci_dev *dev, u16 acs_flags)
>

Richter, Robert Oct. 12, 2017, 1:27 p.m. UTC | #5

Bjorn,

On 27.09.17 11:20:39, Vadim Lomovtsev wrote:
> This commit makes Cavium PCI ACS quirk applicable only to Cavium
> ThunderX (CN8XXX) family PCIE Root Ports which has limited PCI capabilities
> in terms of no ACS support advertisement. However, the RTL internally
> implements similar protection as if ACS had completion/request redirection,
> upstream forwarding and validation features enabled.
> 
> Current quirk implementation doesn't take into account PCIERCs which
> also needs to be quirked. So the pci device id check mask is updated
> and check of device ID moved into separate function.
> 
> Signed-off-by: Vadim Lomovtsev <[e-mail]>

ping on this patch too for pci/host-thunder. Do you think we can mark
this stable for 4.11? Since there is a dependent patch not yet in
stable (b77d537d00, should be added to stable too) there might be
conflicts. But maybe this could be figured out once the patch is
considered for inclusion into stable.

Thanks,

-Robert

> ---
> 	v5 -> v6: comment typo fix: change 0xa00 to 0xa000
> 
>  drivers/pci/quirks.c | 29 +++++++++++++++++++++--------
>  1 file changed, 21 insertions(+), 8 deletions(-)

Bjorn Helgaas Oct. 16, 2017, 9:23 p.m. UTC | #6

[+cc David, Manish]

Please use a subject line that tells more about what's going on.
"Update quirk" doesn't really convey any useful information.
Something like "Apply Cavium ThunderX ACS quirk only to Root Ports".

On Wed, Sep 27, 2017 at 11:20:39AM -0700, Vadim Lomovtsev wrote:
> This commit makes Cavium PCI ACS quirk applicable only to Cavium
> ThunderX (CN8XXX) family PCIE Root Ports which has limited PCI capabilities
> in terms of no ACS support advertisement. However, the RTL internally
> implements similar protection as if ACS had completion/request redirection,
> upstream forwarding and validation features enabled.
> 
> Current quirk implementation doesn't take into account PCIERCs which
> also needs to be quirked. So the pci device id check mask is updated
> and check of device ID moved into separate function.

s/PCIE/PCIe/ above

s/PCIERCs/PCIe Root Ports/ (I assume, since usually a Root Complex
itself doesn't appear as a pci_dev)

> Signed-off-by: Vadim Lomovtsev <[e-mail]>
> ---
> 	v5 -> v6: comment typo fix: change 0xa00 to 0xa000
> 
>  drivers/pci/quirks.c | 29 +++++++++++++++++++++--------
>  1 file changed, 21 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index a4d3361..ed6c76d 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -4211,20 +4211,33 @@ static int pci_quirk_amd_sb_acs(struct pci_dev *dev, u16 acs_flags)
>  #endif
>  }
>  
> -static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags)
> +/*
> + * The Cavium downstream ports doesn't advertise their ACS capability registers.
> + * However, the RTL internally implements similar protection as if
> + * ACS had completion redirection, forwarding and validation features enabled.
> + * So by this flags we're asserting that the hardware implements and
> + * enables equivalent ACS functionality for these flags.
> + */
> +#define CAVIUM_CN8XXX_ACS_FLAGS (PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_SV | PCI_ACS_UF)

You are changing the set of flags, which isn't mentioned in the changelog.
I think the best thing would be to have two patches: one that changes the
set of flags and a second that changes the set of affected devices.

This set of flags was used twice in the original patch, so a #define made
sense.  But now you only use it once, so there's no benefit in adding the
#define, and adding it makes the change in the set of flags harder to see.

> +static __inline__  bool pci_quirk_cavium_acs_match(struct pci_dev *dev)

No need to use "__inline__" here.  This isn't performance critical,
and the compiler will probably inline it anyway because there's only
one use.

>  {
>  	/*
> -	 * Cavium devices matching this quirk do not perform peer-to-peer
> -	 * with other functions, allowing masking out these bits as if they
> -	 * were unimplemented in the ACS capability.
> +	 * Effectively selects all downstream ports for whole ThunderX 1 family
> +	 * by 0xa000 mask (which represents 8 SoCs), while the lower bits of device ID
> +	 * are used to indicate which subdevice is used within the SoC.

Please wrap your comments so they fit in 80 columns.

>  	 */
> -	acs_flags &= ~(PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR |
> -		       PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_DT);
> +	return (pci_is_pcie(dev) &&
> +		(pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) &&
> +		((dev->device & 0xf800) == 0xa000));

I'm just a little confused by the 0xa000 mask you refer to in the
comment, because the mask in the code is 0xf800.

Previously the quirk applied to all Cavium device IDs in the range
0xa000-0xa0ff.  Now it applies to device IDs in the range
0xa000-0xa7ff, but only if they are PCIe Root Ports.  Right?

> +}
>  
> -	if (!((dev->device >= 0xa000) && (dev->device <= 0xa0ff)))
> +static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags)
> +{
> +	if (!pci_quirk_cavium_acs_match(dev))
>  		return -ENOTTY;
>  
> -	return acs_flags ? 0 : 1;
> +	return acs_flags & ~(CAVIUM_CN8XXX_ACS_FLAGS) ? 0 : 1;
>  }
>  
>  static int pci_quirk_xgene_acs(struct pci_dev *dev, u16 acs_flags)
> -- 
> 2.4.11
>

Vadim Lomovtsev Oct. 17, 2017, 11:29 a.m. UTC | #7

On Mon, Oct 16, 2017 at 04:23:20PM -0500, Bjorn Helgaas wrote:
Hi Bjorn,

> [+cc David, Manish]
> 
> Please use a subject line that tells more about what's going on.
> "Update quirk" doesn't really convey any useful information.
> Something like "Apply Cavium ThunderX ACS quirk only to Root Ports".
> 
> On Wed, Sep 27, 2017 at 11:20:39AM -0700, Vadim Lomovtsev wrote:
> > This commit makes Cavium PCI ACS quirk applicable only to Cavium
> > ThunderX (CN8XXX) family PCIE Root Ports which has limited PCI capabilities
> > in terms of no ACS support advertisement. However, the RTL internally
> > implements similar protection as if ACS had completion/request redirection,
> > upstream forwarding and validation features enabled.
> > 
> > Current quirk implementation doesn't take into account PCIERCs which
> > also needs to be quirked. So the pci device id check mask is updated
> > and check of device ID moved into separate function.
> 
> s/PCIE/PCIe/ above
> 
> s/PCIERCs/PCIe Root Ports/ (I assume, since usually a Root Complex
> itself doesn't appear as a pci_dev)
> 
> > Signed-off-by: Vadim Lomovtsev <[e-mail]>
> > ---
> > 	v5 -> v6: comment typo fix: change 0xa00 to 0xa000
> > 
> >  drivers/pci/quirks.c | 29 +++++++++++++++++++++--------
> >  1 file changed, 21 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> > index a4d3361..ed6c76d 100644
> > --- a/drivers/pci/quirks.c
> > +++ b/drivers/pci/quirks.c
> > @@ -4211,20 +4211,33 @@ static int pci_quirk_amd_sb_acs(struct pci_dev *dev, u16 acs_flags)
> >  #endif
> >  }
> >  
> > -static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags)
> > +/*
> > + * The Cavium downstream ports doesn't advertise their ACS capability registers.
> > + * However, the RTL internally implements similar protection as if
> > + * ACS had completion redirection, forwarding and validation features enabled.
> > + * So by this flags we're asserting that the hardware implements and
> > + * enables equivalent ACS functionality for these flags.
> > + */
> > +#define CAVIUM_CN8XXX_ACS_FLAGS (PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_SV | PCI_ACS_UF)
> 
> You are changing the set of flags, which isn't mentioned in the changelog.
> I think the best thing would be to have two patches: one that changes the
> set of flags and a second that changes the set of affected devices.
> 
> This set of flags was used twice in the original patch, so a #define made
> sense.  But now you only use it once, so there's no benefit in adding the
> #define, and adding it makes the change in the set of flags harder to see.
> 
> > +static __inline__  bool pci_quirk_cavium_acs_match(struct pci_dev *dev)
> 
> No need to use "__inline__" here.  This isn't performance critical,
> and the compiler will probably inline it anyway because there's only
> one use.
>
> >  {
> >  	/*
> > -	 * Cavium devices matching this quirk do not perform peer-to-peer
> > -	 * with other functions, allowing masking out these bits as if they
> > -	 * were unimplemented in the ACS capability.
> > +	 * Effectively selects all downstream ports for whole ThunderX 1 family
> > +	 * by 0xa000 mask (which represents 8 SoCs), while the lower bits of device ID
> > +	 * are used to indicate which subdevice is used within the SoC.
> 
> Please wrap your comments so they fit in 80 columns.
> 
> >  	 */
> > -	acs_flags &= ~(PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR |
> > -		       PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_DT);
> > +	return (pci_is_pcie(dev) &&
> > +		(pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) &&
> > +		((dev->device & 0xf800) == 0xa000));
> 
> I'm just a little confused by the 0xa000 mask you refer to in the
> comment, because the mask in the code is 0xf800.

Sorry for confusion, will correct it to 0xf800 for comment.

> 
> Previously the quirk applied to all Cavium device IDs in the range
> 0xa000-0xa0ff.  Now it applies to device IDs in the range
> 0xa000-0xa7ff, but only if they are PCIe Root Ports.  Right?

Correct. Since this quirk is necessary for Root Ports only.

Thanks for reply, I'll re-work this one accordingly to your comments and re-send v7.

WBR,
Vadim

> 
> > +}
> >  
> > -	if (!((dev->device >= 0xa000) && (dev->device <= 0xa0ff)))
> > +static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags)
> > +{
> > +	if (!pci_quirk_cavium_acs_match(dev))
> >  		return -ENOTTY;
> >  
> > -	return acs_flags ? 0 : 1;
> > +	return acs_flags & ~(CAVIUM_CN8XXX_ACS_FLAGS) ? 0 : 1;
> >  }
> >  
> >  static int pci_quirk_xgene_acs(struct pci_dev *dev, u16 acs_flags)
> > -- 
> > 2.4.11
> >

Vadim Lomovtsev Oct. 17, 2017, 12:47 p.m. UTC | #8

From: Vadim Lomovtsev <[e-mail]>

version 7:
- update patch description accordingly to review comments;
- split for two patches: for ACS mask change and device id match change;
- remove macro #define of ACS flags, put it back to quirk function;
- remove '__inline_' from device id matching function;
- wrap code comments to fit into 80 columns;
- comment fix: change 0xa00 to 0xf800 for matching function description;

version 6:
- comment typo fix: change 0xa00 to 0xa000;

version 5:
- update code comments accordingly to review comments;

version 4:
- update ACS mask (remove TB and TD bits), update commit message (remove
  ACS register printout);

version 3:
- update subject: remove CN8XXX from subject line, replace it with ThunderX;

version 2:
- update match function in order to filter only ThunderX devices by device
  ids to properly filter CN8XXX devices, update subject & description with
  ACS register info (v2 was rejected by maillist due to triple X in subject);
  
Vadim Lomovtsev (2):
  PCI: quirks: Set Cavium ACS capability quirk flags to assert
    RR/CR/SV/UF.
  PCI: quirks: Apply Cavium ThunderX ACS quirk only to Root Ports

 drivers/pci/quirks.c | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

Patch

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index a4d3361..ed6c76d 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -4211,20 +4211,33 @@  static int pci_quirk_amd_sb_acs(struct pci_dev *dev, u16 acs_flags)
 #endif
 }
 
-static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags)
+/*
+ * The Cavium downstream ports doesn't advertise their ACS capability registers.
+ * However, the RTL internally implements similar protection as if
+ * ACS had completion redirection, forwarding and validation features enabled.
+ * So by this flags we're asserting that the hardware implements and
+ * enables equivalent ACS functionality for these flags.
+ */
+#define CAVIUM_CN8XXX_ACS_FLAGS (PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_SV | PCI_ACS_UF)
+
+static __inline__  bool pci_quirk_cavium_acs_match(struct pci_dev *dev)
 {
 	/*
-	 * Cavium devices matching this quirk do not perform peer-to-peer
-	 * with other functions, allowing masking out these bits as if they
-	 * were unimplemented in the ACS capability.
+	 * Effectively selects all downstream ports for whole ThunderX 1 family
+	 * by 0xa000 mask (which represents 8 SoCs), while the lower bits of device ID
+	 * are used to indicate which subdevice is used within the SoC.
 	 */
-	acs_flags &= ~(PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR |
-		       PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_DT);
+	return (pci_is_pcie(dev) &&
+		(pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) &&
+		((dev->device & 0xf800) == 0xa000));
+}
 
-	if (!((dev->device >= 0xa000) && (dev->device <= 0xa0ff)))
+static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags)
+{
+	if (!pci_quirk_cavium_acs_match(dev))
 		return -ENOTTY;
 
-	return acs_flags ? 0 : 1;
+	return acs_flags & ~(CAVIUM_CN8XXX_ACS_FLAGS) ? 0 : 1;
 }
 
 static int pci_quirk_xgene_acs(struct pci_dev *dev, u16 acs_flags)