| Message ID | 20210515052434.1413236-1-kw@linux.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | [v2,01/14] PCI: Use sysfs_emit() and sysfs_emit_at() in "show" functions | expand |
Hello,
[...]
> Reviewed-by: Logan Gunthorpe <logang@deltatee.com>
Please disregard this "Reviewed-by" from Logan for this version, as I've
forgotten to remove it before sending v2 after pulling patches using b4.
Apologies.
Krzysztof
On Sat, 2021-05-15 at 05:24 +0000, Krzysztof Wilczyński wrote: > The sysfs_emit() and sysfs_emit_at() functions were introduced to make > it less ambiguous which function is preferred when writing to the output > buffer in a device attribute's "show" callback [1]. > > Convert the PCI sysfs object "show" functions from sprintf(), snprintf() > and scnprintf() to sysfs_emit() and sysfs_emit_at() accordingly, as the > latter is aware of the PAGE_SIZE buffer and correctly returns the number > of bytes written into the buffer. [] > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c [] > @@ -6439,7 +6439,7 @@ static ssize_t resource_alignment_show(struct bus_type *bus, char *buf) > > > spin_lock(&resource_alignment_lock); > if (resource_alignment_param) > - count = scnprintf(buf, PAGE_SIZE, "%s", resource_alignment_param); > + count = sysfs_emit(buf, "%s", resource_alignment_param); > spin_unlock(&resource_alignment_lock); Ideally, the additional newline check below this would use sysfs_emit_at drivers/pci/pci.c- /* drivers/pci/pci.c: * When set by the command line, resource_alignment_param will not drivers/pci/pci.c- * have a trailing line feed, which is ugly. So conditionally add drivers/pci/pci.c- * it here. drivers/pci/pci.c- */ drivers/pci/pci.c- if (count >= 2 && buf[count - 2] != '\n' && count < PAGE_SIZE - 1) { drivers/pci/pci.c- buf[count - 1] = '\n'; drivers/pci/pci.c- buf[count++] = 0; drivers/pci/pci.c- } drivers/pci/pci.c- drivers/pci/pci.c- return count;
Hi Joe, [...] > Ideally, the additional newline check below this would use sysfs_emit_at > > drivers/pci/pci.c- /* > drivers/pci/pci.c: * When set by the command line, resource_alignment_param will not > drivers/pci/pci.c- * have a trailing line feed, which is ugly. So conditionally add > drivers/pci/pci.c- * it here. > drivers/pci/pci.c- */ > drivers/pci/pci.c- if (count >= 2 && buf[count - 2] != '\n' && count < PAGE_SIZE - 1) { > drivers/pci/pci.c- buf[count - 1] = '\n'; > drivers/pci/pci.c- buf[count++] = 0; > drivers/pci/pci.c- } > drivers/pci/pci.c- > drivers/pci/pci.c- return count; I found some inconsistencies with adding newline this way, and decided to change the code slightly, see: https://lore.kernel.org/linux-pci/20210515052434.1413236-12-kw@linux.com/ Krzysztof
On 2021-05-14 11:24 p.m., Krzysztof Wilczyński wrote: > The sysfs_emit() and sysfs_emit_at() functions were introduced to make > it less ambiguous which function is preferred when writing to the output > buffer in a device attribute's "show" callback [1]. > > Convert the PCI sysfs object "show" functions from sprintf(), snprintf() > and scnprintf() to sysfs_emit() and sysfs_emit_at() accordingly, as the > latter is aware of the PAGE_SIZE buffer and correctly returns the number > of bytes written into the buffer. > > No functional change intended. > > [1] Documentation/filesystems/sysfs.rst > > Related to: > commit ad025f8e46f3 ("PCI/sysfs: Use sysfs_emit() and sysfs_emit_at() in "show" functions") > > Signed-off-by: Krzysztof Wilczyński <kw@linux.com> > Reviewed-by: Logan Gunthorpe <logang@deltatee.com> I re-reviewed the whole series. It still looks good to me. Very nice solution in patch 12 to the new line issue. Reviewed-by: Logan Gunthorpe <logang@deltatee.com> Thanks, Logan
Hi Logan, > > The sysfs_emit() and sysfs_emit_at() functions were introduced to make > > it less ambiguous which function is preferred when writing to the output > > buffer in a device attribute's "show" callback [1]. > > > > Convert the PCI sysfs object "show" functions from sprintf(), snprintf() > > and scnprintf() to sysfs_emit() and sysfs_emit_at() accordingly, as the > > latter is aware of the PAGE_SIZE buffer and correctly returns the number > > of bytes written into the buffer. > > > > No functional change intended. > > > > [1] Documentation/filesystems/sysfs.rst > > > > Related to: > > commit ad025f8e46f3 ("PCI/sysfs: Use sysfs_emit() and sysfs_emit_at() in "show" functions") > > I re-reviewed the whole series. It still looks good to me. > > Very nice solution in patch 12 to the new line issue. > > Reviewed-by: Logan Gunthorpe <logang@deltatee.com> > > Thanks, Thank you! I will send v3 incorporating the style change as per Joe's suggestion and carry-over your "Reviewed-by", if you don't mind, as it will be a trivial change. Krzysztof
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index b717680377a9..5ed316ea5831 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -6439,7 +6439,7 @@ static ssize_t resource_alignment_show(struct bus_type *bus, char *buf) spin_lock(&resource_alignment_lock); if (resource_alignment_param) - count = scnprintf(buf, PAGE_SIZE, "%s", resource_alignment_param); + count = sysfs_emit(buf, "%s", resource_alignment_param); spin_unlock(&resource_alignment_lock); /*
The sysfs_emit() and sysfs_emit_at() functions were introduced to make it less ambiguous which function is preferred when writing to the output buffer in a device attribute's "show" callback [1]. Convert the PCI sysfs object "show" functions from sprintf(), snprintf() and scnprintf() to sysfs_emit() and sysfs_emit_at() accordingly, as the latter is aware of the PAGE_SIZE buffer and correctly returns the number of bytes written into the buffer. No functional change intended. [1] Documentation/filesystems/sysfs.rst Related to: commit ad025f8e46f3 ("PCI/sysfs: Use sysfs_emit() and sysfs_emit_at() in "show" functions") Signed-off-by: Krzysztof Wilczyński <kw@linux.com> Reviewed-by: Logan Gunthorpe <logang@deltatee.com> --- drivers/pci/pci.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)