On Tue, May 18, 2021 at 03:40:58AM +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.
>
> Modify the function dsm_label_utf16s_to_utf8s() to directly return the
> number of bytes written into the buffer so that the strlen() used later
> to calculate the length of the buffer can be removed as it would no
> longer be needed.
>
> 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@xxxxxxxxx>
> Reviewed-by: Logan Gunthorpe <logang@xxxxxxxxxxxx>
> ---
> Changes in v2:
> None.
> Changes in v3:
> Added Logan Gunthorpe's "Reviewed-by".
> Change style to the preferred one in the drivers/pci/slot.c file.
>
> drivers/pci/pci-label.c | 18 ++++++++++--------
> drivers/pci/slot.c | 18 +++++++++---------
> 2 files changed, 19 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/pci/pci-label.c b/drivers/pci/pci-label.c
> index c32f3b7540e8..000e169c7197 100644
> --- a/drivers/pci/pci-label.c
> +++ b/drivers/pci/pci-label.c
> @@ -139,14 +139,17 @@ enum acpi_attr_enum {
> ACPI_ATTR_INDEX_SHOW,
> };
>
> -static void dsm_label_utf16s_to_utf8s(union acpi_object *obj, char *buf)
> +static int dsm_label_utf16s_to_utf8s(union acpi_object *obj, char *buf)
> {
> int len;
> +
> len = utf16s_to_utf8s((const wchar_t *)obj->buffer.pointer,
> obj->buffer.length,
> UTF16_LITTLE_ENDIAN,
> buf, PAGE_SIZE);
> buf[len] = '\n';
> +
> + return len;
> }
>
> static int dsm_get_label(struct device *dev, char *buf,
> @@ -154,7 +157,7 @@ static int dsm_get_label(struct device *dev, char *buf,
> {
> acpi_handle handle = ACPI_HANDLE(dev);
> union acpi_object *obj, *tmp;
> - int len = -1;
> + int len = 0;
>
> if (!handle)
> return -1;
> @@ -175,20 +178,19 @@ static int dsm_get_label(struct device *dev, char *buf,
> * this entry must return a null string.
> */
> if (attr == ACPI_ATTR_INDEX_SHOW) {
> - scnprintf(buf, PAGE_SIZE, "%llu\n", tmp->integer.value);
> + len = sysfs_emit(buf, "%llu\n", tmp->integer.value);
> } else if (attr == ACPI_ATTR_LABEL_SHOW) {
> if (tmp[1].type == ACPI_TYPE_STRING)
> - scnprintf(buf, PAGE_SIZE, "%s\n",
> - tmp[1].string.pointer);
> + len = sysfs_emit(buf, "%s\n",
> + tmp[1].string.pointer);
> else if (tmp[1].type == ACPI_TYPE_BUFFER)
> - dsm_label_utf16s_to_utf8s(tmp + 1, buf);
> + len = dsm_label_utf16s_to_utf8s(tmp + 1, buf);
> }
> - len = strlen(buf) > 0 ? strlen(buf) : -1;
> }
>
> ACPI_FREE(obj);
>
> - return len;
> + return len > 0 ? len : -1;
I really like this, but I would like it even better if the
sysfs_emit() change were easier to review.
It seems pointless that the current code uses strlen() when
scnprintf() and dsm_label_utf16s_to_utf8s() both have that
information and we just throw it away.
I think it should be possible to split the len and
dsm_label_utf16s_to_utf8s() changes to a separate patch, which would
remove the need for the strlen, and then the conversion to
sysfs_emit() would be completely trivial like all the rest of them.
My goal is to make all the sysfs_emit() changes look almost
mechanical, with the non-trivial parts separated out.
> }
>
> static ssize_t label_show(struct device *dev, struct device_attribute *attr,
> diff --git a/drivers/pci/slot.c b/drivers/pci/slot.c
> index d627dd9179b4..751a26668e3a 100644
> --- a/drivers/pci/slot.c
> +++ b/drivers/pci/slot.c
> @@ -39,19 +39,19 @@ static const struct sysfs_ops pci_slot_sysfs_ops = {
> static ssize_t address_read_file(struct pci_slot *slot, char *buf)
> {
> if (slot->number == 0xff)
> - return sprintf(buf, "%04x:%02x\n",
> - pci_domain_nr(slot->bus),
> - slot->bus->number);
> - else
> - return sprintf(buf, "%04x:%02x:%02x\n",
> - pci_domain_nr(slot->bus),
> - slot->bus->number,
> - slot->number);
> + return sysfs_emit(buf, "%04x:%02x\n",
> + pci_domain_nr(slot->bus),
> + slot->bus->number);
> +
> + return sysfs_emit(buf, "%04x:%02x:%02x\n",
> + pci_domain_nr(slot->bus),
> + slot->bus->number,
> + slot->number);
> }
>
> static ssize_t bus_speed_read(enum pci_bus_speed speed, char *buf)
> {
> - return sprintf(buf, "%s\n", pci_speed_string(speed));
> + return sysfs_emit(buf, "%s\n", pci_speed_string(speed));
> }
>
> static ssize_t max_speed_read_file(struct pci_slot *slot, char *buf)
> --
> 2.31.1
>
