| Message ID | 20210603000112.703037-4-kw@linux.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | PCI/sysfs: Use sysfs_emit() and sysfs_emit_at() in "show" functions | expand |
On Thu, Jun 03, 2021 at 12:01:09AM +0000, Krzysztof Wilczyński wrote: > 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> > --- > drivers/pci/pci.c | 34 ++++++++++++++++++++-------------- > 1 file changed, 20 insertions(+), 14 deletions(-) > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index 5ed316ea5831..b46445a49543 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -6439,34 +6439,40 @@ 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; > + > + if (count >= (PAGE_SIZE - 1)) > + return -EINVAL; > > + 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); When "strlen(param) == 0", don't we kfree resource_alignment_param twice? Once here, > + resource_alignment_param = NULL; > + } > spin_unlock(&resource_alignment_lock); > + > + kfree(old); and again here? > return count; > } > > -- > 2.31.1 >
Hi Bjorn, [...] > > + old = resource_alignment_param; > > + if (strlen(param)) { > > + resource_alignment_param = param; > > + } else { > > + kfree(resource_alignment_param); > > When "strlen(param) == 0", don't we kfree resource_alignment_param > twice? Once here, Yes, we should be freeing the allocation made using kstrndup() here, this is my bad, I completely missed that the wrong variable name was used. > > + resource_alignment_param = NULL; > > + } > > spin_unlock(&resource_alignment_lock); > > + > > + kfree(old); > > and again here? > > > return count; [...] Yes, good catch! I am going to send v7 shortly. Apologies! Krzysztof
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index 5ed316ea5831..b46445a49543 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -6439,34 +6439,40 @@ 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; + + if (count >= (PAGE_SIZE - 1)) + return -EINVAL; + 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; }
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> --- drivers/pci/pci.c | 34 ++++++++++++++++++++-------------- 1 file changed, 20 insertions(+), 14 deletions(-)