diff mbox series

PCI: Fix issue with "pci=disable_acs_redir" parameter being ignored

Message ID 20190401193302.12813-1-logang@deltatee.com (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show
Series PCI: Fix issue with "pci=disable_acs_redir" parameter being ignored | expand

Commit Message

Logan Gunthorpe April 1, 2019, 7:33 p.m. UTC 
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@deltatee.com>
Tested-by: Andrew Maier <andrew.maier@eideticom.com>
Cc: Bjorn Helgaas <bhelgaas@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(-)

--
2.20.1

Comments

Bjorn Helgaas April 5, 2019, 9:39 p.m. UTC | #1 
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@deltatee.com>
> Tested-by: Andrew Maier <andrew.maier@eideticom.com>
> Cc: Bjorn Helgaas <bhelgaas@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
Logan Gunthorpe April 5, 2019, 10:11 p.m. UTC | #2 
On 2019-04-05 3:39 p.m., Bjorn Helgaas wrote:
>> -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.

Yeah, I was looking for a better solution and eventually decided to just
copy what was done in the past.

> 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.  

That sounds like a good idea that I didn't think of. If I can't find
something better, I'll do that.

> In fact, I bet there's already a static
> copy lying around somewhere for /proc/cmdline.

There are lots of copies of cmdline around, but none that are available
for early param. I did a sampling of some of the other early params but
none seem to be anywhere near as complex as the PCI one and I haven't
yet found one that needs to keep a copy of any or all of the command line.

In any case, I'll keep digging and try to get a better patch for you
shortly.

Logan
diff mbox series

Patch

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];
+
+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);