Re: [PATCH] PCI: Fix issue with "pci=disable_acs_redir" parameter being ignored

[Bjorn Helgaas]

On Mon, Apr 01, 2019 at 01:33:02PM -0600, Logan Gunthorpe wrote:
> In most cases, kmalloc will not be available early in boot when
> pci_setup() is called. Thus, the kstrdup call that was added to fix the
> __initdata bug with the disable_acs_redir parameter usually returns
> NULL. Thus the parameter is discarded and it does not take into effect.
> 
> To fix this we have to do what we originally did with the
> resource_alignment parameter: allocate a static buffer and copy the
> string from the command line to it.
> 
> Fixes: d2fd6e81912a ("PCI: Fix __initdata issue with "pci=disable_acs_redir" parameter")
> Signed-off-by: Logan Gunthorpe <logang@xxxxxxxxxxxx>
> Tested-by: Andrew Maier <andrew.maier@xxxxxxxxxxxxx>
> Cc: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
> ---
> 
> Sorry, it seems I didn't test the previous patch in this area properly
> and I actually made disable_acs_redir ineffective in most cases.
> 
> This change should fix it and I got a colleague to test it fully
> on his system as well.
> 
> drivers/pci/pci.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 7c1b362f599a..537395dfd0df 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -3125,7 +3125,17 @@ void pci_request_acs(void)
>  	pci_acs_enable = 1;
>  }
> 
> -static const char *disable_acs_redir_param;
> +static char disable_acs_redir_param[COMMAND_LINE_SIZE];

I certainly acknowledge the problem, but I'm a little hesitant to add
a static buffer of 256-4096 bytes (2048 on x86, arm64, powerpc, sparc)
for a relatively low-usage situation.  The memory usage doesn't seem
in proportion to the value-add.

Ugh, and we allocate another similar buffer for
resource_alignment_param[], which I would guess is also rarely used.

Since disable_acs_redir_param[] and resource_alignment_param[] are
both read-only and big enough to hold the entire command line, we
should be able to share a single buffer between them if we made the
parsers a little smarter.  In fact, I bet there's already a static
copy lying around somewhere for /proc/cmdline.

> +static void pci_set_disable_acs_redir_param(const char *param)
> +{
> +	if (strlen(param) >= sizeof(disable_acs_redir_param)) {
> +		pr_err("PCI: disable_acs_redir parameter is too long and has been ignored!\n");
> +		return;
> +	}
> +
> +	strcpy(disable_acs_redir_param, param);
> +}
> 
>  /**
>   * pci_disable_acs_redir - disable ACS redirect capabilities
> @@ -3140,7 +3150,7 @@ static void pci_disable_acs_redir(struct pci_dev *dev)
>  	int pos;
>  	u16 ctrl;
> 
> -	if (!disable_acs_redir_param)
> +	if (!disable_acs_redir_param[0])
>  		return;
> 
>  	p = disable_acs_redir_param;
> @@ -6262,8 +6272,7 @@ static int __init pci_setup(char *str)
>  			} else if (!strncmp(str, "pcie_scan_all", 13)) {
>  				pci_add_flags(PCI_SCAN_ALL_PCIE_DEVS);
>  			} else if (!strncmp(str, "disable_acs_redir=", 18)) {
> -				disable_acs_redir_param =
> -					kstrdup(str + 18, GFP_KERNEL);
> +				pci_set_disable_acs_redir_param(str + 18);
>  			} else {
>  				printk(KERN_ERR "PCI: Unknown option `%s'\n",
>  						str);
> --
> 2.20.1