| Message ID | 20210518034109.158450-3-kw@linux.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | [v3,01/14] PCI: Use sysfs_emit() and sysfs_emit_at() in "show" functions | expand |
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 >
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@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. 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
Hi Bjorn, [...] > > 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 Got it! I will send v4 updated as per the above suggestion. Also, if Logan does not mind, I will carry his "Reviewed-by" over as there will be no changes to the actual code, just how the patch will be arranged. Krzysztof
On 2021-05-25 4:32 a.m., Krzysztof Wilczyński wrote: > Hi Bjorn, > > [...] >>> 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 > > Got it! I will send v4 updated as per the above suggestion. Also, if > Logan does not mind, I will carry his "Reviewed-by" over as there will > be no changes to the actual code, just how the patch will be arranged. Yup, I'm good with that. Still looks fine by me. Logan
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; } 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)
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(-)