diff mbox series

[v3,12/14] PCI: Fix trailing newline handling of resource_alignment_param

Message ID 20210518034109.158450-12-kw@linux.com (mailing list archive)
State Superseded
Delegated to: Bjorn Helgaas
Headers show
Series [v3,01/14] PCI: Use sysfs_emit() and sysfs_emit_at() in "show" functions | expand

Commit Message

Krzysztof Wilczyński May 18, 2021, 3:41 a.m. UTC
The value of the "resource_alignment" can be specified using a kernel
command-line argument (using the "pci=resource_alignment=") or through
the corresponding sysfs object under the /sys/bus/pci path.

Currently, when the value is set via the kernel command-line argument,
and then subsequently accessed through sysfs object, the value read back
will not be correct, as per:

  # grep -oE 'pci=resource_alignment.+' /proc/cmdline
  pci=resource_alignment=20@00:1f.2
  # cat /sys/bus/pci/resource_alignment
  20@00:1f.

This is also true when the value is set through the sysfs object, but
the trailing newline has not been included, as per:

  # echo -n 20@00:1f.2 > /sys/bus/pci/resource_alignment
  # cat /sys/bus/pci/resource_alignment
  20@00:1f.

When the value set through the sysfs object includes the trailing
newline, then reading it back will work as intended, as per:

  # echo 20@00:1f.2 > /sys/bus/pci/resource_alignment
  # cat /sys/bus/pci/resource_alignment
  20@00:1f.2

To fix this inconsistency, append a trailing newline in the show()
function and strip the trailing line in the store() function if one is
present.

Also, allow for the value previously set using either a command-line
argument or through the sysfs object to be cleared at run-time.

Fixes: e499081da1a2 ("PCI: Force trailing new line to resource_alignment_param in sysfs")
Signed-off-by: Krzysztof Wilczyński <kw@linux.com>
Reviewed-by: Logan Gunthorpe <logang@deltatee.com>
---
Changes in v2:
  None.
Changes in v3:
  Added Logan Gunthorpe's "Reviewed-by".

 drivers/pci/pci.c | 31 +++++++++++++++++--------------
 1 file changed, 17 insertions(+), 14 deletions(-)

Comments

Krzysztof Wilczyński May 21, 2021, 3:14 p.m. UTC | #1
[+cc Greg and Sasha for visibility]

Hi Bjorn and Logan,

[...]
> Fixes: e499081da1a2 ("PCI: Force trailing new line to resource_alignment_param in sysfs")
[...]

This probably would be a candidate for a back-port to stable and
long-term releases.  But, since the move to sysfs_emit()/sysfs_emit_at()
would be then irrelevant, I can split this patch so that it fixes the
issue first, and then other patch will move it to sysfs_emit(), so that
it would be easier to apply when back-porting.

Would this be fine with you Logan?  Especially since you already offered
your review.  I think Bjorn wouldn't mind the split either.

Do let me know folks, and I will change things accordingly.

	Krzysztof
Logan Gunthorpe May 21, 2021, 3:34 p.m. UTC | #2
On 2021-05-21 9:14 a.m., Krzysztof Wilczyński wrote:
> [+cc Greg and Sasha for visibility]
> 
> Hi Bjorn and Logan,
> 
> [...]
>> Fixes: e499081da1a2 ("PCI: Force trailing new line to resource_alignment_param in sysfs")
> [...]
> 
> This probably would be a candidate for a back-port to stable and
> long-term releases.  But, since the move to sysfs_emit()/sysfs_emit_at()
> would be then irrelevant, I can split this patch so that it fixes the
> issue first, and then other patch will move it to sysfs_emit(), so that
> it would be easier to apply when back-porting.
> 
> Would this be fine with you Logan?  Especially since you already offered
> your review.  I think Bjorn wouldn't mind the split either.

Totally fine by me. I can re-review the two patches if you like.

Thanks,

Logan
Greg KH May 21, 2021, 5:19 p.m. UTC | #3
On Fri, May 21, 2021 at 05:14:43PM +0200, Krzysztof Wilczyński wrote:
> [+cc Greg and Sasha for visibility]
> 
> Hi Bjorn and Logan,
> 
> [...]
> > Fixes: e499081da1a2 ("PCI: Force trailing new line to resource_alignment_param in sysfs")
> [...]
> 
> This probably would be a candidate for a back-port to stable and
> long-term releases.  But, since the move to sysfs_emit()/sysfs_emit_at()
> would be then irrelevant, I can split this patch so that it fixes the
> issue first, and then other patch will move it to sysfs_emit(), so that
> it would be easier to apply when back-porting.

sysfs_emit() and sysfs_emit_at() are in all supported stable and
long-term kernel trees at this time, so there's no need to shy away from
using them.

thanks,

greg k-h
Krzysztof Wilczyński May 25, 2021, 10:29 a.m. UTC | #4
Hi Greg,

[...]
> > This probably would be a candidate for a back-port to stable and
> > long-term releases.  But, since the move to sysfs_emit()/sysfs_emit_at()
> > would be then irrelevant, I can split this patch so that it fixes the
> > issue first, and then other patch will move it to sysfs_emit(), so that
> > it would be easier to apply when back-porting.
> 
> sysfs_emit() and sysfs_emit_at() are in all supported stable and
> long-term kernel trees at this time, so there's no need to shy away from
> using them.

Excellent!  With some changes requested by Bjorn this will then be
easier to apply to current stable and long-term kernels.

	Krzysztof
diff mbox series

Patch

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 5ed316ea5831..7cde86bdcc8e 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -6439,34 +6439,37 @@  static ssize_t resource_alignment_show(struct bus_type *bus, char *buf)
 
 	spin_lock(&resource_alignment_lock);
 	if (resource_alignment_param)
-		count = sysfs_emit(buf, "%s", resource_alignment_param);
+		count = sysfs_emit(buf, "%s\n", 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;
 }
 
 static ssize_t resource_alignment_store(struct bus_type *bus,
 					const char *buf, size_t count)
 {
-	char *param = kstrndup(buf, count, GFP_KERNEL);
+	char *param, *old, *end;
 
+	param = kstrndup(buf, count, GFP_KERNEL);
 	if (!param)
 		return -ENOMEM;
 
+	end = strchr(param, '\n');
+	if (end)
+		*end = '\0';
+
 	spin_lock(&resource_alignment_lock);
-	kfree(resource_alignment_param);
-	resource_alignment_param = param;
+	old = resource_alignment_param;
+	if (strlen(param)) {
+		resource_alignment_param = param;
+	} else {
+		kfree(resource_alignment_param);
+		resource_alignment_param = NULL;
+	}
 	spin_unlock(&resource_alignment_lock);
+
+	kfree(old);
+
 	return count;
 }