[PATCH 0/3] Cleanup resource_alignment parameter

[PATCH 0/3] Cleanup resource_alignment parameter

[Logan Gunthorpe]

This is a follow up to the cleanup I started last cycle on the
resource_alignment parameter after finding an improved way to do handle
the static buffer for the disable_acs_redir parameter. So this patchset
allows us to drop a significant chunk of static data.

Per the discussion last cycle, this version keeps the spin locks
(instead of the RCU implementation) and splits the change into a
couple different patches.

Thanks,

Logan

--

Logan Gunthorpe (3):
  PCI: Clean up resource_alignment parameter to not require static
    buffer
  PCI: Move pci_[get|set]_resource_alignment_param() into their callers
  PCI: Force trailing new line to resource_alignment_param in sysfs

 drivers/pci/pci.c | 65 +++++++++++++++++++++++++----------------------
 1 file changed, 35 insertions(+), 30 deletions(-)

--
2.20.1

[PATCH 1/3] PCI: Clean up resource_alignment parameter to not require static buffer

[Logan Gunthorpe]

Clean up the 'resource_alignment' parameter code to use kstrdup
in the initcall routine instead of a static buffer that wastes memory
regardless of whether the feature is used.  This allows us to drop
'COMMAND_LINE_SIZE' bytes (typically 256-4096 depending on architecture)
of static data.

This is similar to what has been done for the 'disable_acs_redir'
parameter.

Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pci.c | 41 +++++++++++++++++++++++------------------
 1 file changed, 23 insertions(+), 18 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 766f5779db92..bd7cdcfeb094 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -5896,8 +5896,7 @@ resource_size_t __weak pcibios_default_alignment(void)
 	return 0;
 }
 
-#define RESOURCE_ALIGNMENT_PARAM_SIZE COMMAND_LINE_SIZE
-static char resource_alignment_param[RESOURCE_ALIGNMENT_PARAM_SIZE] = {0};
+static char *resource_alignment_param;
 static DEFINE_SPINLOCK(resource_alignment_lock);
 
 /**
@@ -5918,7 +5917,7 @@ static resource_size_t pci_specified_resource_alignment(struct pci_dev *dev,
 
 	spin_lock(&resource_alignment_lock);
 	p = resource_alignment_param;
-	if (!*p && !align)
+	if (!p || !*p)
 		goto out;
 	if (pci_has_flag(PCI_PROBE_ONLY)) {
 		align = 0;
@@ -6084,21 +6083,25 @@ void pci_reassigndev_resource_alignment(struct pci_dev *dev)
 
 static ssize_t pci_set_resource_alignment_param(const char *buf, size_t count)
 {
-	if (count > RESOURCE_ALIGNMENT_PARAM_SIZE - 1)
-		count = RESOURCE_ALIGNMENT_PARAM_SIZE - 1;
 	spin_lock(&resource_alignment_lock);
-	strncpy(resource_alignment_param, buf, count);
-	resource_alignment_param[count] = '\0';
+
+	kfree(resource_alignment_param);
+	resource_alignment_param = kstrndup(buf, count, GFP_KERNEL);
+
 	spin_unlock(&resource_alignment_lock);
-	return count;
+
+	return resource_alignment_param ? count : -ENOMEM;
 }
 
 static ssize_t pci_get_resource_alignment_param(char *buf, size_t size)
 {
-	size_t count;
+	size_t count = 0;
+
 	spin_lock(&resource_alignment_lock);
-	count = snprintf(buf, size, "%s", resource_alignment_param);
+	if (resource_alignment_param)
+		count = snprintf(buf, size, "%s", resource_alignment_param);
 	spin_unlock(&resource_alignment_lock);
+
 	return count;
 }
 
@@ -6238,8 +6241,7 @@ static int __init pci_setup(char *str)
 			} else if (!strncmp(str, "cbmemsize=", 10)) {
 				pci_cardbus_mem_size = memparse(str + 10, &str);
 			} else if (!strncmp(str, "resource_alignment=", 19)) {
-				pci_set_resource_alignment_param(str + 19,
-							strlen(str + 19));
+				resource_alignment_param = str + 19;
 			} else if (!strncmp(str, "ecrc=", 5)) {
 				pcie_ecrc_get_policy(str + 5);
 			} else if (!strncmp(str, "hpiosize=", 9)) {
@@ -6275,15 +6277,18 @@ static int __init pci_setup(char *str)
 early_param("pci", pci_setup);
 
 /*
- * 'disable_acs_redir_param' is initialized in pci_setup(), above, to point
- * to data in the __initdata section which will be freed after the init
- * sequence is complete. We can't allocate memory in pci_setup() because some
- * architectures do not have any memory allocation service available during
- * an early_param() call. So we allocate memory and copy the variable here
- * before the init section is freed.
+ * 'resource_alignment_param' and 'disable_acs_redir_param' are initialized
+ * in pci_setup(), above, to point to data in the __initdata section which
+ * will be freed after the init sequence is complete. We can't allocate memory
+ * in pci_setup() because some architectures do not have any memory allocation
+ * service available during an early_param() call. So we allocate memory and
+ * copy the variable here before the init section is freed.
+ *
  */
 static int __init pci_realloc_setup_params(void)
 {
+	resource_alignment_param = kstrdup(resource_alignment_param,
+					   GFP_KERNEL);
 	disable_acs_redir_param = kstrdup(disable_acs_redir_param, GFP_KERNEL);
 
 	return 0;
-- 
2.20.1

[PATCH 2/3] PCI: Move pci_[get|set]_resource_alignment_param() into their callers

[Logan Gunthorpe]

Both the functions pci_get_resource_alignment_param() and
pci_set_resource_alignment_param() are now only called in one place:
resource_alignment_show() and resource_alignment_store() respectively.

There is no value in this extra set of functions so we move both into
their callers respectively.

Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pci.c | 30 ++++++++++--------------------
 1 file changed, 10 insertions(+), 20 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index bd7cdcfeb094..3e71e161f18b 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -6081,39 +6081,29 @@ void pci_reassigndev_resource_alignment(struct pci_dev *dev)
 	}
 }
 
-static ssize_t pci_set_resource_alignment_param(const char *buf, size_t count)
-{
-	spin_lock(&resource_alignment_lock);
-
-	kfree(resource_alignment_param);
-	resource_alignment_param = kstrndup(buf, count, GFP_KERNEL);
-
-	spin_unlock(&resource_alignment_lock);
-
-	return resource_alignment_param ? count : -ENOMEM;
-}
-
-static ssize_t pci_get_resource_alignment_param(char *buf, size_t size)
+static ssize_t resource_alignment_show(struct bus_type *bus, char *buf)
 {
 	size_t count = 0;
 
 	spin_lock(&resource_alignment_lock);
 	if (resource_alignment_param)
-		count = snprintf(buf, size, "%s", resource_alignment_param);
+		count = snprintf(buf, PAGE_SIZE, "%s", resource_alignment_param);
 	spin_unlock(&resource_alignment_lock);
 
 	return count;
 }
 
-static ssize_t resource_alignment_show(struct bus_type *bus, char *buf)
-{
-	return pci_get_resource_alignment_param(buf, PAGE_SIZE);
-}
-
 static ssize_t resource_alignment_store(struct bus_type *bus,
 					const char *buf, size_t count)
 {
-	return pci_set_resource_alignment_param(buf, count);
+	spin_lock(&resource_alignment_lock);
+
+	kfree(resource_alignment_param);
+	resource_alignment_param = kstrndup(buf, count, GFP_KERNEL);
+
+	spin_unlock(&resource_alignment_lock);
+
+	return resource_alignment_param ? count : -ENOMEM;
 }
 
 static BUS_ATTR_RW(resource_alignment);
-- 
2.20.1

[PATCH 3/3] PCI: Force trailing new line to resource_alignment_param in sysfs

[Logan Gunthorpe]

When 'pci=resource_alignment=' is specified on the command line, there
is no trailing new line.  Then, when it's read through the corresponding
sysfs attribute, there will be no newline and a cat command will not
show correctly in a shell. If the parameter is set through sysfs
a new line will be stored and it will 'cat' correctly.

To solve this, append a new line character in the show function if
one does not already exist.

Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pci.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 3e71e161f18b..99d130ac6b96 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -6090,6 +6090,16 @@ static ssize_t resource_alignment_show(struct bus_type *bus, char *buf)
 		count = snprintf(buf, PAGE_SIZE, "%s", resource_alignment_param);
 	spin_unlock(&resource_alignment_lock);
 
+	/*
+	 * When set by the command line, resource_alignment_param will not
+	 * have a trailing line feed, which is ugly. So conditionally add
+	 * it here.
+	 */
+	if (count >= 2 && buf[count - 2] != '\n' && count < PAGE_SIZE - 1) {
+		buf[count - 1] = '\n';
+		buf[count++] = 0;
+	}
+
 	return count;
 }
 
-- 
2.20.1

Re: [PATCH 0/3] Cleanup resource_alignment parameter

[Bjorn Helgaas]

On Fri, May 24, 2019 at 02:16:07PM -0600, Logan Gunthorpe wrote:
> This is a follow up to the cleanup I started last cycle on the
> resource_alignment parameter after finding an improved way to do handle
> the static buffer for the disable_acs_redir parameter. So this patchset
> allows us to drop a significant chunk of static data.
> 
> Per the discussion last cycle, this version keeps the spin locks
> (instead of the RCU implementation) and splits the change into a
> couple different patches.

Hi Logan,

This series doesn't apply cleanly to either v5.3-rc1 or v5.2-rc1.
Would you mind refreshing it?

> Logan Gunthorpe (3):
>   PCI: Clean up resource_alignment parameter to not require static
>     buffer
>   PCI: Move pci_[get|set]_resource_alignment_param() into their callers
>   PCI: Force trailing new line to resource_alignment_param in sysfs
> 
>  drivers/pci/pci.c | 65 +++++++++++++++++++++++++----------------------
>  1 file changed, 35 insertions(+), 30 deletions(-)
> 
> --
> 2.20.1