Re: [PATCH 1/1] PCI: Fix Extend ACS configurability

[Jason Gunthorpe <jgg@nvidia.com>]

On Fri, Dec 13, 2024 at 12:29:42PM -0800, Tushar Dave wrote:

> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index dc663c0ca670..fc1c37910d1c 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -4654,11 +4654,10 @@
>  				Format:
>  				<ACS flags>@<pci_dev>[; ...]
>  				Specify one or more PCI devices (in the format
> -				specified above) optionally prepended with flags
> -				and separated by semicolons. The respective
> -				capabilities will be enabled, disabled or
> -				unchanged based on what is specified in
> -				flags.
> +				specified above) prepended with flags and separated
> +				by semicolons. The respective capabilities will be
> +				enabled, disabled or unchanged based on what is
> +				specified in flags.
>  
>  				ACS Flags is defined as follows:
>  				  bit-0 : ACS Source Validation
> @@ -4673,7 +4672,7 @@
>  				  '1' – force enabled
>  				  'x' – unchanged
>  				For example,
> -				  pci=config_acs=10x
> +				  pci=config_acs=10x@pci:0:0
>  				would configure all devices that support
>  				ACS to enable P2P Request Redirect, disable
>  				Translation Blocking, and leave Source

Is this an unrelated change? The format of the command line shouldn't
be changed to fix the described bug, why is the documentation changed?

>  static void __pci_config_acs(struct pci_dev *dev, struct pci_acs *caps,
> -			     const char *p, u16 mask, u16 flags)
> +			     const char *p, const u16 acs_mask, const u16 acs_flags)
>  {
> +	u16 flags = acs_flags;
> +	u16 mask = acs_mask;
>  	char *delimit;
>  	int ret = 0;
>  
> @@ -964,7 +965,7 @@ static void __pci_config_acs(struct pci_dev *dev, struct pci_acs *caps,
>  		return;
>  
>  	while (*p) {
> -		if (!mask) {
> +		if (!acs_mask) {
>  			/* Check for ACS flags */
>  			delimit = strstr(p, "@");
>  			if (delimit) {
> @@ -972,6 +973,8 @@ static void __pci_config_acs(struct pci_dev *dev, struct pci_acs *caps,
>  				u32 shift = 0;
>  
>  				end = delimit - p - 1;
> +				mask = 0;
> +				flags = 0;
>  
>  				while (end > -1) {
>  					if (*(p + end) == '0') {

This function the entire fix, right? Because the routine was
clobbering acs_mask as it processed the earlier devices?

> @@ -1028,10 +1031,10 @@ static void __pci_config_acs(struct pci_dev *dev, struct pci_acs *caps,
>  
>  	pci_dbg(dev, "ACS mask  = %#06x\n", mask);
>  	pci_dbg(dev, "ACS flags = %#06x\n", flags);
> +	pci_dbg(dev, "ACS control = %#06x\n", caps->ctrl);
>  
> -	/* If mask is 0 then we copy the bit from the firmware setting. */
> -	caps->ctrl = (caps->ctrl & ~mask) | (caps->fw_ctrl & mask);
> -	caps->ctrl |= flags;
> +	caps->ctrl &= ~mask;
> +	caps->ctrl |= (flags & mask);

And why delete fw_ctrl? Doesn't that break the unchanged
functionality?

Jason