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@xxxxxxxxx>
> Reviewed-by: Logan Gunthorpe <logang@xxxxxxxxxxxx>
> ---
> 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
>
