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 <log...@deltatee.com>
> Tested-by: Andrew Maier <andrew.ma...@eideticom.com>
> Cc: Bjorn Helgaas <bhelg...@google.com>
> ---
>
> 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
