Re: [PATCH v3 03/14] PCI: Use sysfs_emit() and sysfs_emit_at() in "show" functions

[Bjorn Helgaas <helgaas@kernel.org>]

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@linux.com>
> Reviewed-by: Logan Gunthorpe <logang@deltatee.com>
> ---
> 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
>