On Mon, May 24, 2021 at 04:14:32PM -0500, Bjorn Helgaas wrote:
> 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.
And BTW, when all the sysfs_emit() changes are trivial like that, I
would probably squash them all into one patch that converts all of
drivers/pci/ at once.
That would still leave a few separate patches:
- This dsm_label_utf16s_to_utf8s() change
- The resource_alignment newline change
- The devspec_show newline change
- The driver_override change
