[PATCH v6 0/6] PCI/sysfs: Use sysfs_emit() and sysfs_emit_at() in "show" functions

[PATCH v6 0/6] PCI/sysfs: Use sysfs_emit() and sysfs_emit_at() in "show" functions

[Krzysztof Wilczyński]

Hello,

This series aims to bring support for the sysfs_emit() and
sysfs_emit_at() functions to the existing PCI-related sysfs objects.
These new 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].

Thus, the existing PCI sysfs objects "show" functions will be converted
from the using the sprintf(), snprintf() and scnprintf() functions to
sysfs_emit() and sysfs_emit_at() accordingly, as the latter is aware of
the PAGE_SIZE buffer limit that the sysfs object has and correctly
returns the number of bytes written into the buffer.

This series will also address inconsistency related to the presence (or
lack of thereof) of a trailing newline in the show() functions adding it
where it's currently missing.  This will allow for utilities such as the
"cat" command to display the result of the read from a particular sysfs
object correctly in a shell.

While working on this series a problem with newline handling related to
how the value of the "resource_alignment" sysfs object was parsed and
then persisted has been found and corrected.  Also, while at it,
a change enabling support for the value of "resource_alignment"
previously set using either a command-line argument or through the sysfs
object to be cleared at run-time was also included, and thus aligning
this particular sysfs object with the behaviour of other such objects
that allow for the value to be dynamically updated and cleared as
required.

Additionally, a fix to a potential buffer overrun that has been found in
the dsm_label_utf16s_to_utf8s() function that is responsible for the
character conversion from UTF16 to UTF8 of the buffer that holds the
device label obtained through the ACPI _DSM mechanism is included as
part of this series.

Finally, a minor fix is also included in this series that has been added
to ensure that the value of the "driver_override" variable is only
exposed through the corresponding sysfs object when a value is set or
otherwise if the value has not been set, the object would return
a string representation of the NULL value.  This will also align this
particular sysfs object's behaviour with others, where when there is no
value then nothing is returned.

[1] Documentation/filesystems/sysfs.rst

This series is related to:
  commit ad025f8 ("PCI/sysfs: Use sysfs_emit() and sysfs_emit_at() in "show" functions")

Signed-off-by: Krzysztof Wilczyński <kw@linux.com>

---
Changes in v2:
  None.

Changes in v3:
  Added Logan Gunthorpe's "Reviewed-by".

Changes in v4:
  Separated and squashed all the trivial sysfs_emit()/sysfs_emit_at()
  changes into a single patch as per Bjorn Helgaas' request.
  Carried Logan Gunthorpe's "Reviewed-by" over.

Changes in v5:
  Added check to the resource_alignment_show() function to ensure that
  there is an extra space left in the buffer for the newline character,
  assuming that it might be provided.

Changes in v6:
  Added a cover letter as per Bjorn Helgaas' request.
  New patch addressing a potential buffer overrun in the
  dsm_label_utf16s_to_utf8s() function has been added.

Krzysztof Wilczyński (6):
  PCI/sysfs: Use sysfs_emit() and sysfs_emit_at() in "show" functions
  PCI/sysfs: Use return value from dsm_label_utf16s_to_utf8s() directly
  PCI/sysfs: Fix trailing newline handling of resource_alignment_param
  PCI/sysfs: Add missing trailing newline to devspec_show()
  PCI/sysfs: Only show value when driver_override is not NULL
  PCI/sysfs: Fix a buffer overrun problem with
    dsm_label_utf16s_to_utf8s()

 drivers/pci/hotplug/pci_hotplug_core.c |  8 +++---
 drivers/pci/hotplug/rpadlpar_sysfs.c   |  4 +--
 drivers/pci/hotplug/shpchp_sysfs.c     | 38 ++++++++++++++------------
 drivers/pci/iov.c                      | 12 ++++----
 drivers/pci/msi.c                      |  8 +++---
 drivers/pci/p2pdma.c                   |  7 ++---
 drivers/pci/pci-label.c                | 22 ++++++++-------
 drivers/pci/pci-sysfs.c                |  7 +++--
 drivers/pci/pci.c                      | 34 +++++++++++++----------
 drivers/pci/pcie/aer.c                 | 20 ++++++++------
 drivers/pci/pcie/aspm.c                |  4 +--
 drivers/pci/slot.c                     | 18 ++++++------
 drivers/pci/switch/switchtec.c         | 18 ++++++------
 13 files changed, 107 insertions(+), 93 deletions(-)

-- 
2.31.1

[PATCH v6 1/6] PCI/sysfs: Use sysfs_emit() and sysfs_emit_at() in "show" functions

[Krzysztof Wilczyński]

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/hotplug/pci_hotplug_core.c |  8 +++---
 drivers/pci/hotplug/rpadlpar_sysfs.c   |  4 +--
 drivers/pci/hotplug/shpchp_sysfs.c     | 38 ++++++++++++++------------
 drivers/pci/iov.c                      | 12 ++++----
 drivers/pci/msi.c                      |  8 +++---
 drivers/pci/p2pdma.c                   |  7 ++---
 drivers/pci/pci-label.c                |  6 ++--
 drivers/pci/pci.c                      |  2 +-
 drivers/pci/pcie/aer.c                 | 20 ++++++++------
 drivers/pci/pcie/aspm.c                |  4 +--
 drivers/pci/slot.c                     | 18 ++++++------
 drivers/pci/switch/switchtec.c         | 18 ++++++------
 12 files changed, 75 insertions(+), 70 deletions(-)

diff --git a/drivers/pci/hotplug/pci_hotplug_core.c b/drivers/pci/hotplug/pci_hotplug_core.c
index 5ac31f683b85..058d5937d8a9 100644
--- a/drivers/pci/hotplug/pci_hotplug_core.c
+++ b/drivers/pci/hotplug/pci_hotplug_core.c
@@ -73,7 +73,7 @@ static ssize_t power_read_file(struct pci_slot *pci_slot, char *buf)
 	if (retval)
 		return retval;
 
-	return sprintf(buf, "%d\n", value);
+	return sysfs_emit(buf, "%d\n", value);
 }
 
 static ssize_t power_write_file(struct pci_slot *pci_slot, const char *buf,
@@ -130,7 +130,7 @@ static ssize_t attention_read_file(struct pci_slot *pci_slot, char *buf)
 	if (retval)
 		return retval;
 
-	return sprintf(buf, "%d\n", value);
+	return sysfs_emit(buf, "%d\n", value);
 }
 
 static ssize_t attention_write_file(struct pci_slot *pci_slot, const char *buf,
@@ -175,7 +175,7 @@ static ssize_t latch_read_file(struct pci_slot *pci_slot, char *buf)
 	if (retval)
 		return retval;
 
-	return sprintf(buf, "%d\n", value);
+	return sysfs_emit(buf, "%d\n", value);
 }
 
 static struct pci_slot_attribute hotplug_slot_attr_latch = {
@@ -192,7 +192,7 @@ static ssize_t presence_read_file(struct pci_slot *pci_slot, char *buf)
 	if (retval)
 		return retval;
 
-	return sprintf(buf, "%d\n", value);
+	return sysfs_emit(buf, "%d\n", value);
 }
 
 static struct pci_slot_attribute hotplug_slot_attr_presence = {
diff --git a/drivers/pci/hotplug/rpadlpar_sysfs.c b/drivers/pci/hotplug/rpadlpar_sysfs.c
index dbfa0b55d31a..068b7810a574 100644
--- a/drivers/pci/hotplug/rpadlpar_sysfs.c
+++ b/drivers/pci/hotplug/rpadlpar_sysfs.c
@@ -50,7 +50,7 @@ static ssize_t add_slot_store(struct kobject *kobj, struct kobj_attribute *attr,
 static ssize_t add_slot_show(struct kobject *kobj,
 			     struct kobj_attribute *attr, char *buf)
 {
-	return sprintf(buf, "0\n");
+	return sysfs_emit(buf, "0\n");
 }
 
 static ssize_t remove_slot_store(struct kobject *kobj,
@@ -80,7 +80,7 @@ static ssize_t remove_slot_store(struct kobject *kobj,
 static ssize_t remove_slot_show(struct kobject *kobj,
 				struct kobj_attribute *attr, char *buf)
 {
-	return sprintf(buf, "0\n");
+	return sysfs_emit(buf, "0\n");
 }
 
 static struct kobj_attribute add_slot_attr =
diff --git a/drivers/pci/hotplug/shpchp_sysfs.c b/drivers/pci/hotplug/shpchp_sysfs.c
index 45658bb5c554..64beed7a26be 100644
--- a/drivers/pci/hotplug/shpchp_sysfs.c
+++ b/drivers/pci/hotplug/shpchp_sysfs.c
@@ -24,50 +24,54 @@
 static ssize_t show_ctrl(struct device *dev, struct device_attribute *attr, char *buf)
 {
 	struct pci_dev *pdev;
-	char *out = buf;
 	int index, busnr;
 	struct resource *res;
 	struct pci_bus *bus;
+	size_t len = 0;
 
 	pdev = to_pci_dev(dev);
 	bus = pdev->subordinate;
 
-	out += sprintf(buf, "Free resources: memory\n");
+	len += sysfs_emit_at(buf, len, "Free resources: memory\n");
 	pci_bus_for_each_resource(bus, res, index) {
 		if (res && (res->flags & IORESOURCE_MEM) &&
 				!(res->flags & IORESOURCE_PREFETCH)) {
-			out += sprintf(out, "start = %8.8llx, length = %8.8llx\n",
-				       (unsigned long long)res->start,
-				       (unsigned long long)resource_size(res));
+			len += sysfs_emit_at(buf, len,
+					     "start = %8.8llx, length = %8.8llx\n",
+					     (unsigned long long)res->start,
+					     (unsigned long long)resource_size(res));
 		}
 	}
-	out += sprintf(out, "Free resources: prefetchable memory\n");
+	len += sysfs_emit_at(buf, len, "Free resources: prefetchable memory\n");
 	pci_bus_for_each_resource(bus, res, index) {
 		if (res && (res->flags & IORESOURCE_MEM) &&
 			       (res->flags & IORESOURCE_PREFETCH)) {
-			out += sprintf(out, "start = %8.8llx, length = %8.8llx\n",
-				       (unsigned long long)res->start,
-				       (unsigned long long)resource_size(res));
+			len += sysfs_emit_at(buf, len,
+					     "start = %8.8llx, length = %8.8llx\n",
+					     (unsigned long long)res->start,
+					     (unsigned long long)resource_size(res));
 		}
 	}
-	out += sprintf(out, "Free resources: IO\n");
+	len += sysfs_emit_at(buf, len, "Free resources: IO\n");
 	pci_bus_for_each_resource(bus, res, index) {
 		if (res && (res->flags & IORESOURCE_IO)) {
-			out += sprintf(out, "start = %8.8llx, length = %8.8llx\n",
-				       (unsigned long long)res->start,
-				       (unsigned long long)resource_size(res));
+			len += sysfs_emit_at(buf, len,
+					     "start = %8.8llx, length = %8.8llx\n",
+					     (unsigned long long)res->start,
+					     (unsigned long long)resource_size(res));
 		}
 	}
-	out += sprintf(out, "Free resources: bus numbers\n");
+	len += sysfs_emit_at(buf, len, "Free resources: bus numbers\n");
 	for (busnr = bus->busn_res.start; busnr <= bus->busn_res.end; busnr++) {
 		if (!pci_find_bus(pci_domain_nr(bus), busnr))
 			break;
 	}
 	if (busnr < bus->busn_res.end)
-		out += sprintf(out, "start = %8.8x, length = %8.8x\n",
-				busnr, (int)(bus->busn_res.end - busnr));
+		len += sysfs_emit_at(buf, len,
+				     "start = %8.8x, length = %8.8x\n",
+				     busnr, (int)(bus->busn_res.end - busnr));
 
-	return out - buf;
+	return len;
 }
 static DEVICE_ATTR(ctrl, S_IRUGO, show_ctrl, NULL);
 
diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
index afc06e6ce115..a71258347323 100644
--- a/drivers/pci/iov.c
+++ b/drivers/pci/iov.c
@@ -346,7 +346,7 @@ static ssize_t sriov_totalvfs_show(struct device *dev,
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
 
-	return sprintf(buf, "%u\n", pci_sriov_get_totalvfs(pdev));
+	return sysfs_emit(buf, "%u\n", pci_sriov_get_totalvfs(pdev));
 }
 
 static ssize_t sriov_numvfs_show(struct device *dev,
@@ -361,7 +361,7 @@ static ssize_t sriov_numvfs_show(struct device *dev,
 	num_vfs = pdev->sriov->num_VFs;
 	device_unlock(&pdev->dev);
 
-	return sprintf(buf, "%u\n", num_vfs);
+	return sysfs_emit(buf, "%u\n", num_vfs);
 }
 
 /*
@@ -435,7 +435,7 @@ static ssize_t sriov_offset_show(struct device *dev,
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
 
-	return sprintf(buf, "%u\n", pdev->sriov->offset);
+	return sysfs_emit(buf, "%u\n", pdev->sriov->offset);
 }
 
 static ssize_t sriov_stride_show(struct device *dev,
@@ -444,7 +444,7 @@ static ssize_t sriov_stride_show(struct device *dev,
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
 
-	return sprintf(buf, "%u\n", pdev->sriov->stride);
+	return sysfs_emit(buf, "%u\n", pdev->sriov->stride);
 }
 
 static ssize_t sriov_vf_device_show(struct device *dev,
@@ -453,7 +453,7 @@ static ssize_t sriov_vf_device_show(struct device *dev,
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
 
-	return sprintf(buf, "%x\n", pdev->sriov->vf_device);
+	return sysfs_emit(buf, "%x\n", pdev->sriov->vf_device);
 }
 
 static ssize_t sriov_drivers_autoprobe_show(struct device *dev,
@@ -462,7 +462,7 @@ static ssize_t sriov_drivers_autoprobe_show(struct device *dev,
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
 
-	return sprintf(buf, "%u\n", pdev->sriov->drivers_autoprobe);
+	return sysfs_emit(buf, "%u\n", pdev->sriov->drivers_autoprobe);
 }
 
 static ssize_t sriov_drivers_autoprobe_store(struct device *dev,
diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index 217dc9f0231f..9232255c8515 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -464,11 +464,11 @@ static ssize_t msi_mode_show(struct device *dev, struct device_attribute *attr,
 		return retval;
 
 	entry = irq_get_msi_desc(irq);
-	if (entry)
-		return sprintf(buf, "%s\n",
-				entry->msi_attrib.is_msix ? "msix" : "msi");
+	if (!entry)
+		return -ENODEV;
 
-	return -ENODEV;
+	return sysfs_emit(buf, "%s\n",
+			  entry->msi_attrib.is_msix ? "msix" : "msi");
 }
 
 static int populate_msi_sysfs(struct pci_dev *pdev)
diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
index 196382630363..a1351b3e2c4c 100644
--- a/drivers/pci/p2pdma.c
+++ b/drivers/pci/p2pdma.c
@@ -53,7 +53,7 @@ static ssize_t size_show(struct device *dev, struct device_attribute *attr,
 	if (pdev->p2pdma->pool)
 		size = gen_pool_size(pdev->p2pdma->pool);
 
-	return scnprintf(buf, PAGE_SIZE, "%zd\n", size);
+	return sysfs_emit(buf, "%zd\n", size);
 }
 static DEVICE_ATTR_RO(size);
 
@@ -66,7 +66,7 @@ static ssize_t available_show(struct device *dev, struct device_attribute *attr,
 	if (pdev->p2pdma->pool)
 		avail = gen_pool_avail(pdev->p2pdma->pool);
 
-	return scnprintf(buf, PAGE_SIZE, "%zd\n", avail);
+	return sysfs_emit(buf, "%zd\n", avail);
 }
 static DEVICE_ATTR_RO(available);
 
@@ -75,8 +75,7 @@ static ssize_t published_show(struct device *dev, struct device_attribute *attr,
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
 
-	return scnprintf(buf, PAGE_SIZE, "%d\n",
-			 pdev->p2pdma->p2pmem_published);
+	return sysfs_emit(buf, "%d\n", pdev->p2pdma->p2pmem_published);
 }
 static DEVICE_ATTR_RO(published);
 
diff --git a/drivers/pci/pci-label.c b/drivers/pci/pci-label.c
index c32f3b7540e8..311dd48e2881 100644
--- a/drivers/pci/pci-label.c
+++ b/drivers/pci/pci-label.c
@@ -175,11 +175,11 @@ 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);
+			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);
+				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);
 		}
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);
 
 	/*
diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
index ec943cee5ecc..40ef7bed7a77 100644
--- a/drivers/pci/pcie/aer.c
+++ b/drivers/pci/pcie/aer.c
@@ -529,21 +529,23 @@ static const char *aer_agent_string[] = {
 		     char *buf)						\
 {									\
 	unsigned int i;							\
-	char *str = buf;						\
 	struct pci_dev *pdev = to_pci_dev(dev);				\
 	u64 *stats = pdev->aer_stats->stats_array;			\
+	size_t len = 0;							\
 									\
 	for (i = 0; i < ARRAY_SIZE(strings_array); i++) {		\
 		if (strings_array[i])					\
-			str += sprintf(str, "%s %llu\n",		\
-				       strings_array[i], stats[i]);	\
+			len += sysfs_emit_at(buf, len, "%s %llu\n",	\
+					     strings_array[i],		\
+					     stats[i]);			\
 		else if (stats[i])					\
-			str += sprintf(str, #stats_array "_bit[%d] %llu\n",\
-				       i, stats[i]);			\
+			len += sysfs_emit_at(buf, len,			\
+					     #stats_array "_bit[%d] %llu\n",\
+					     i, stats[i]);		\
 	}								\
-	str += sprintf(str, "TOTAL_%s %llu\n", total_string,		\
-		       pdev->aer_stats->total_field);			\
-	return str-buf;							\
+	len += sysfs_emit_at(buf, len, "TOTAL_%s %llu\n", total_string,	\
+			     pdev->aer_stats->total_field);		\
+	return len;							\
 }									\
 static DEVICE_ATTR_RO(name)
 
@@ -563,7 +565,7 @@ aer_stats_dev_attr(aer_dev_nonfatal, dev_nonfatal_errs,
 		     char *buf)						\
 {									\
 	struct pci_dev *pdev = to_pci_dev(dev);				\
-	return sprintf(buf, "%llu\n", pdev->aer_stats->field);		\
+	return sysfs_emit(buf, "%llu\n", pdev->aer_stats->field);	\
 }									\
 static DEVICE_ATTR_RO(name)
 
diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index ac0557a305af..013a47f587ce 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -1208,7 +1208,7 @@ static ssize_t aspm_attr_show_common(struct device *dev,
 	struct pci_dev *pdev = to_pci_dev(dev);
 	struct pcie_link_state *link = pcie_aspm_get_link(pdev);
 
-	return sprintf(buf, "%d\n", (link->aspm_enabled & state) ? 1 : 0);
+	return sysfs_emit(buf, "%d\n", (link->aspm_enabled & state) ? 1 : 0);
 }
 
 static ssize_t aspm_attr_store_common(struct device *dev,
@@ -1265,7 +1265,7 @@ static ssize_t clkpm_show(struct device *dev,
 	struct pci_dev *pdev = to_pci_dev(dev);
 	struct pcie_link_state *link = pcie_aspm_get_link(pdev);
 
-	return sprintf(buf, "%d\n", link->clkpm_enabled);
+	return sysfs_emit(buf, "%d\n", link->clkpm_enabled);
 }
 
 static ssize_t clkpm_store(struct device *dev,
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)
diff --git a/drivers/pci/switch/switchtec.c b/drivers/pci/switch/switchtec.c
index ba52459928f7..0b301f8be9ed 100644
--- a/drivers/pci/switch/switchtec.c
+++ b/drivers/pci/switch/switchtec.c
@@ -280,7 +280,7 @@ static ssize_t device_version_show(struct device *dev,
 
 	ver = ioread32(&stdev->mmio_sys_info->device_version);
 
-	return sprintf(buf, "%x\n", ver);
+	return sysfs_emit(buf, "%x\n", ver);
 }
 static DEVICE_ATTR_RO(device_version);
 
@@ -292,7 +292,7 @@ static ssize_t fw_version_show(struct device *dev,
 
 	ver = ioread32(&stdev->mmio_sys_info->firmware_version);
 
-	return sprintf(buf, "%08x\n", ver);
+	return sysfs_emit(buf, "%08x\n", ver);
 }
 static DEVICE_ATTR_RO(fw_version);
 
@@ -344,7 +344,7 @@ static ssize_t component_vendor_show(struct device *dev,
 
 	/* component_vendor field not supported after gen3 */
 	if (stdev->gen != SWITCHTEC_GEN3)
-		return sprintf(buf, "none\n");
+		return sysfs_emit(buf, "none\n");
 
 	return io_string_show(buf, &si->gen3.component_vendor,
 			      sizeof(si->gen3.component_vendor));
@@ -359,9 +359,9 @@ static ssize_t component_id_show(struct device *dev,
 
 	/* component_id field not supported after gen3 */
 	if (stdev->gen != SWITCHTEC_GEN3)
-		return sprintf(buf, "none\n");
+		return sysfs_emit(buf, "none\n");
 
-	return sprintf(buf, "PM%04X\n", id);
+	return sysfs_emit(buf, "PM%04X\n", id);
 }
 static DEVICE_ATTR_RO(component_id);
 
@@ -373,9 +373,9 @@ static ssize_t component_revision_show(struct device *dev,
 
 	/* component_revision field not supported after gen3 */
 	if (stdev->gen != SWITCHTEC_GEN3)
-		return sprintf(buf, "255\n");
+		return sysfs_emit(buf, "255\n");
 
-	return sprintf(buf, "%d\n", rev);
+	return sysfs_emit(buf, "%d\n", rev);
 }
 static DEVICE_ATTR_RO(component_revision);
 
@@ -384,7 +384,7 @@ static ssize_t partition_show(struct device *dev,
 {
 	struct switchtec_dev *stdev = to_stdev(dev);
 
-	return sprintf(buf, "%d\n", stdev->partition);
+	return sysfs_emit(buf, "%d\n", stdev->partition);
 }
 static DEVICE_ATTR_RO(partition);
 
@@ -393,7 +393,7 @@ static ssize_t partition_count_show(struct device *dev,
 {
 	struct switchtec_dev *stdev = to_stdev(dev);
 
-	return sprintf(buf, "%d\n", stdev->partition_count);
+	return sysfs_emit(buf, "%d\n", stdev->partition_count);
 }
 static DEVICE_ATTR_RO(partition_count);
 
-- 
2.31.1

[PATCH v6 2/6] PCI/sysfs: Use return value from dsm_label_utf16s_to_utf8s() directly

[Krzysztof Wilczyński]

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.

Signed-off-by: Krzysztof Wilczyński <kw@linux.com>
Reviewed-by: Logan Gunthorpe <logang@deltatee.com>
---
 drivers/pci/pci-label.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/pci/pci-label.c b/drivers/pci/pci-label.c
index 311dd48e2881..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) {
-			sysfs_emit(buf, "%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)
-				sysfs_emit(buf, "%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,
-- 
2.31.1

[PATCH v6 3/6] PCI/sysfs: Fix trailing newline handling of resource_alignment_param

[Krzysztof Wilczyński]

The value of the "resource_alignment" can be specified using a kernel
command-line argument (using the "pci=resource_alignment=") or through
the corresponding sysfs object under the /sys/bus/pci path.

Currently, when the value is set via the kernel command-line argument,
and then subsequently accessed through sysfs object, the value read back
will not be correct, as per:

  # grep -oE 'pci=resource_alignment.+' /proc/cmdline
  pci=resource_alignment=20@00:1f.2
  # cat /sys/bus/pci/resource_alignment
  20@00:1f.

This is also true when the value is set through the sysfs object, but
the trailing newline has not been included, as per:

  # echo -n 20@00:1f.2 > /sys/bus/pci/resource_alignment
  # cat /sys/bus/pci/resource_alignment
  20@00:1f.

When the value set through the sysfs object includes the trailing
newline, then reading it back will work as intended, as per:

  # echo 20@00:1f.2 > /sys/bus/pci/resource_alignment
  # cat /sys/bus/pci/resource_alignment
  20@00:1f.2

To fix this inconsistency, append a trailing newline in the show()
function and strip the trailing line in the store() function if one is
present.

Also, allow for the value previously set using either a command-line
argument or through the sysfs object to be cleared at run-time.

Fixes: e499081da1a2 ("PCI: Force trailing new line to resource_alignment_param in sysfs")
Signed-off-by: Krzysztof Wilczyński <kw@linux.com>
Reviewed-by: Logan Gunthorpe <logang@deltatee.com>
---
 drivers/pci/pci.c | 34 ++++++++++++++++++++--------------
 1 file changed, 20 insertions(+), 14 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 5ed316ea5831..b46445a49543 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -6439,34 +6439,40 @@ static ssize_t resource_alignment_show(struct bus_type *bus, char *buf)
 
 	spin_lock(&resource_alignment_lock);
 	if (resource_alignment_param)
-		count = sysfs_emit(buf, "%s", resource_alignment_param);
+		count = sysfs_emit(buf, "%s\n", resource_alignment_param);
 	spin_unlock(&resource_alignment_lock);
 
-	/*
-	 * When set by the command line, resource_alignment_param will not
-	 * have a trailing line feed, which is ugly. So conditionally add
-	 * it here.
-	 */
-	if (count >= 2 && buf[count - 2] != '\n' && count < PAGE_SIZE - 1) {
-		buf[count - 1] = '\n';
-		buf[count++] = 0;
-	}
-
 	return count;
 }
 
 static ssize_t resource_alignment_store(struct bus_type *bus,
 					const char *buf, size_t count)
 {
-	char *param = kstrndup(buf, count, GFP_KERNEL);
+	char *param, *old, *end;
+
+	if (count >= (PAGE_SIZE - 1))
+		return -EINVAL;
 
+	param = kstrndup(buf, count, GFP_KERNEL);
 	if (!param)
 		return -ENOMEM;
 
+	end = strchr(param, '\n');
+	if (end)
+		*end = '\0';
+
 	spin_lock(&resource_alignment_lock);
-	kfree(resource_alignment_param);
-	resource_alignment_param = param;
+	old = resource_alignment_param;
+	if (strlen(param)) {
+		resource_alignment_param = param;
+	} else {
+		kfree(resource_alignment_param);
+		resource_alignment_param = NULL;
+	}
 	spin_unlock(&resource_alignment_lock);
+
+	kfree(old);
+
 	return count;
 }
 
-- 
2.31.1

[PATCH v6 4/6] PCI/sysfs: Add missing trailing newline to devspec_show()

[Krzysztof Wilczyński]

At the moment, when the value of the "devspec" sysfs object is read from
the user space there will be no newline present, and the utilities such
as the "cat" command won't display the result of the read correctly in
a shell, as the trailing newline is currently missing.

To fix this, append a newline character in the show() function.

Signed-off-by: Krzysztof Wilczyński <kw@linux.com>
Reviewed-by: Logan Gunthorpe <logang@deltatee.com>
---
 drivers/pci/pci-sysfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index beb8d1f4fafe..5d63df7c1820 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -537,7 +537,7 @@ static ssize_t devspec_show(struct device *dev,
 
 	if (np == NULL)
 		return 0;
-	return sysfs_emit(buf, "%pOF", np);
+	return sysfs_emit(buf, "%pOF\n", np);
 }
 static DEVICE_ATTR_RO(devspec);
 #endif
-- 
2.31.1

[PATCH v6 5/6] PCI/sysfs: Only show value when driver_override is not NULL

[Krzysztof Wilczyński]

Only expose the value of the "driver_override" variable through the
corresponding sysfs object when a value is actually set.

Signed-off-by: Krzysztof Wilczyński <kw@linux.com>
Reviewed-by: Logan Gunthorpe <logang@deltatee.com>
---
 drivers/pci/pci-sysfs.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 5d63df7c1820..4e9f582ca10f 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -580,10 +580,11 @@ static ssize_t driver_override_show(struct device *dev,
 				    struct device_attribute *attr, char *buf)
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
-	ssize_t len;
+	ssize_t len = 0;
 
 	device_lock(dev);
-	len = sysfs_emit(buf, "%s\n", pdev->driver_override);
+	if (pdev->driver_override)
+		len = sysfs_emit(buf, "%s\n", pdev->driver_override);
 	device_unlock(dev);
 	return len;
 }
-- 
2.31.1

[PATCH v6 6/6] PCI/sysfs: Fix a buffer overrun problem with dsm_label_utf16s_to_utf8s()

[Krzysztof Wilczyński]

When using ACPI to retrieve a device label from _DSM using the
dsm_get_label() function the result can be retrieved either as a string
value or a pointer to a buffer.

Should it be the latter, then an additional function called
dsm_label_utf16s_to_utf8s() will be invoked to convert any UTF16
characters to UTF8 so that the value could be safely stored and later
displayed through a relevant sysfs object.

Internally, dsm_label_utf16s_to_utf8s() calls the utf16s_to_utf8s()
function to handle the conversion setting the limit of the data that can
be saved into a provided buffer as a result of the character conversion
to be a total of PAGE_SIZE.  The utf16s_to_utf8s() function returns an
integer value denoting the number of bytes that have been written into
the provided buffer.

Following the execution of the utf16s_to_utf8s() a newline character
will be added at the end of the resulting buffer so that when the value
is read in the userspace through the sysfs object then it would include
newline making it more accessible when working with the sysfs file
system in the shell, etc.  Normally, this wouldn't be a problem, but if
the function utf16s_to_utf8s() happens to return the number of bytes
written to be precisely PAGE_SIZE, then we would overrun the buffer and
write the newline character outside the allotted space which can have
undefined consequences or result in a failure.

To fix this buffer overrun, ensure that there always is enough space
left for the newline character to be safely appended.

Fixes: 6058989bad05 ("PCI: Export ACPI _DSM provided firmware instance number and string name to sysfs")
Reported-by: Joe Perches <joe@perches.com>
Signed-off-by: Krzysztof Wilczyński <kw@linux.com>
---
 drivers/pci/pci-label.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/pci-label.c b/drivers/pci/pci-label.c
index 000e169c7197..0c6446519640 100644
--- a/drivers/pci/pci-label.c
+++ b/drivers/pci/pci-label.c
@@ -146,8 +146,8 @@ static int dsm_label_utf16s_to_utf8s(union acpi_object *obj, char *buf)
 	len = utf16s_to_utf8s((const wchar_t *)obj->buffer.pointer,
 			      obj->buffer.length,
 			      UTF16_LITTLE_ENDIAN,
-			      buf, PAGE_SIZE);
-	buf[len] = '\n';
+			      buf, PAGE_SIZE - 1);
+	buf[len++] = '\n';
 
 	return len;
 }
-- 
2.31.1

Re: [PATCH v6 5/6] PCI/sysfs: Only show value when driver_override is not NULL

[Bjorn Helgaas]

On Thu, Jun 03, 2021 at 12:01:11AM +0000, Krzysztof Wilczyński wrote:
> Only expose the value of the "driver_override" variable through the
> corresponding sysfs object when a value is actually set.

What's the reason for this change?  The above tells me what it *does*,
but not *why* or whether it affects users.

Is this to avoid trying to print a NULL pointer as %s?  Do we print
"(null)" or something in that case now?  I assume sprintf() doesn't
actually oops.  If we change what appears in sysfs, we should mention
that here.  And maybe consider whether there's any chance of breaking
user code that might know what to do with "(null)" but not with an
empty string.

There are six other driver_override_show() methods.  Five don't check
the ->driver_override pointer at all; one (spi.c) checks like this:

  len = snprintf(buf, PAGE_SIZE, "%s\n", spi->driver_override ? : "");

Do the others need similar fixes?  Most of them still use sprintf()
also.

> Signed-off-by: Krzysztof Wilczyński <kw@linux.com>
> Reviewed-by: Logan Gunthorpe <logang@deltatee.com>
> ---
>  drivers/pci/pci-sysfs.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index 5d63df7c1820..4e9f582ca10f 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -580,10 +580,11 @@ static ssize_t driver_override_show(struct device *dev,
>  				    struct device_attribute *attr, char *buf)
>  {
>  	struct pci_dev *pdev = to_pci_dev(dev);
> -	ssize_t len;
> +	ssize_t len = 0;
>  
>  	device_lock(dev);
> -	len = sysfs_emit(buf, "%s\n", pdev->driver_override);
> +	if (pdev->driver_override)
> +		len = sysfs_emit(buf, "%s\n", pdev->driver_override);
>  	device_unlock(dev);
>  	return len;
>  }
> -- 
> 2.31.1
> 

Re: [PATCH v6 5/6] PCI/sysfs: Only show value when driver_override is not NULL

[Bjorn Helgaas]

On Thu, Jun 03, 2021 at 04:17:41PM -0500, Bjorn Helgaas wrote:
> On Thu, Jun 03, 2021 at 12:01:11AM +0000, Krzysztof Wilczyński wrote:
> > Only expose the value of the "driver_override" variable through the
> > corresponding sysfs object when a value is actually set.
> 
> What's the reason for this change?  The above tells me what it *does*,
> but not *why* or whether it affects users.
> 
> Is this to avoid trying to print a NULL pointer as %s?  Do we print
> "(null)" or something in that case now?  I assume sprintf() doesn't
> actually oops.  If we change what appears in sysfs, we should mention
> that here.  And maybe consider whether there's any chance of breaking
> user code that might know what to do with "(null)" but not with an
> empty string.
> 
> There are six other driver_override_show() methods.  Five don't check
> the ->driver_override pointer at all; one (spi.c) checks like this:
> 
>   len = snprintf(buf, PAGE_SIZE, "%s\n", spi->driver_override ? : "");
> 
> Do the others need similar fixes?  Most of them still use sprintf()
> also.

I can't remember if there's a reason for holding device_lock() around
this.  Of the seven: amba, platform, vmbus, pci, s390, and spi hold
it, while fsl-mc does not.

Since we're only reading a single scalar, I don't see the reason for
device_lock().  If we do need it, it would be nice to have a brief
comment explaining why.  Obviously not an issue with this patch :)

> > Signed-off-by: Krzysztof Wilczyński <kw@linux.com>
> > Reviewed-by: Logan Gunthorpe <logang@deltatee.com>
> > ---
> >  drivers/pci/pci-sysfs.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> > index 5d63df7c1820..4e9f582ca10f 100644
> > --- a/drivers/pci/pci-sysfs.c
> > +++ b/drivers/pci/pci-sysfs.c
> > @@ -580,10 +580,11 @@ static ssize_t driver_override_show(struct device *dev,
> >  				    struct device_attribute *attr, char *buf)
> >  {
> >  	struct pci_dev *pdev = to_pci_dev(dev);
> > -	ssize_t len;
> > +	ssize_t len = 0;
> >  
> >  	device_lock(dev);
> > -	len = sysfs_emit(buf, "%s\n", pdev->driver_override);
> > +	if (pdev->driver_override)
> > +		len = sysfs_emit(buf, "%s\n", pdev->driver_override);
> >  	device_unlock(dev);
> >  	return len;
> >  }
> > -- 
> > 2.31.1
> > 

Re: [PATCH v6 5/6] PCI/sysfs: Only show value when driver_override is not NULL

[Krzysztof Wilczyński]

Hi Bjorn,

Thank you for the review!

[...]
> > Only expose the value of the "driver_override" variable through the
> > corresponding sysfs object when a value is actually set.
> 
> What's the reason for this change?  The above tells me what it *does*,
> but not *why* or whether it affects users.

Good point!  I am going to update the commit message with more details
about why this change in the next version.  Sorry about this!

> Is this to avoid trying to print a NULL pointer as %s?  Do we print
> "(null)" or something in that case now?  I assume sprintf() doesn't
> actually oops.  If we change what appears in sysfs, we should mention
> that here.  And maybe consider whether there's any chance of breaking
> user code that might know what to do with "(null)" but not with an
> empty string.

Yes, sprintf() deals with a NULL pointer and prints "(null)", and
I personally think that this is not very useful, especially in the case
of this particular sysfs object - I would also prefer not to expose the
notion of a NULL-value to the userspace this way, something we ought to
handle properly, see for example how the "resource_alignment" sysfs
object behaves when there is no value set, as per:

  # wc /sys/bus/pci/resource_alignment
     0    0    0 /sys/bus/pci/resource_alignment

> There are six other driver_override_show() methods.  Five don't check
> the ->driver_override pointer at all; one (spi.c) checks like this:
> 
>   len = snprintf(buf, PAGE_SIZE, "%s\n", spi->driver_override ? : "");

For an empty sysfs object, as per the sysfs recommendations, I believe
that having 0 bytes might be more appropriate to indicate lack of any
sensible value.  Using "" (empty string) like in the example above would
still warrant adding a trailing newline character, which would make it
empty (technically), but certainly not 0 bytes.  Having said that, some
drivers opt not to add a particular sysfs object at all should there be
no value for it.

> Do the others need similar fixes?  Most of them still use sprintf()
> also.
[...]

I would like to think that we should handle NULL-value in sysfs objects
sensibly everywhere, so the other cases you were able to find could be
updated.

	Krzysztof

Re: [PATCH v6 4/6] PCI/sysfs: Add missing trailing newline to devspec_show()

[Bjorn Helgaas]

[+cc Sebastian]

On Thu, Jun 03, 2021 at 12:01:10AM +0000, Krzysztof Wilczyński wrote:
> At the moment, when the value of the "devspec" sysfs object is read from
> the user space there will be no newline present, and the utilities such
> as the "cat" command won't display the result of the read correctly in
> a shell, as the trailing newline is currently missing.
> 
> To fix this, append a newline character in the show() function.

There are a few other devspec_show() functions:

  arch/powerpc/platforms/pseries/ibmebus.c
  arch/powerpc/platforms/pseries/vio.c
  arch/sparc/kernel/vio.c
  drivers/usb/core/sysfs.c

and they all include the newline.  So I assume it's not likely that
this minor user-visible change will break something.

I did cc: Sebastian, since his dfc73e7acd99 ("PCI: Move Open Firmware
devspec attribute to PCI common code") moved this code to pci-sysfs.c
in the first place (it came from microblaze and powerpc code that
*also* did not include the newline).

I notice that pci-sysfs.c is the only one that returns 0 if of_node is
NULL.  Probably makes more sense than what the others do.  I'm
guessing the others would print "(null)" which doesn't seem quite
right for a sysfs attribute.

But pci-sysfs.c also goes to a little more work than necessary to look
up of_node:

  struct pci_dev *pdev = to_pci_dev(dev);
  struct device_node *np = pci_device_to_OF_node(pdev);

when I think this would be equivalent:

  struct device_node *np = dev->of_node;

Some of the others do a similar dance with struct platform_device.

Why'd I write all the above?  I dunno; this looks good to me, no
action required for you :)

> Signed-off-by: Krzysztof Wilczyński <kw@linux.com>
> Reviewed-by: Logan Gunthorpe <logang@deltatee.com>
> ---
>  drivers/pci/pci-sysfs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index beb8d1f4fafe..5d63df7c1820 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -537,7 +537,7 @@ static ssize_t devspec_show(struct device *dev,
>  
>  	if (np == NULL)
>  		return 0;
> -	return sysfs_emit(buf, "%pOF", np);
> +	return sysfs_emit(buf, "%pOF\n", np);
>  }
>  static DEVICE_ATTR_RO(devspec);
>  #endif
> -- 
> 2.31.1
> 

Re: [PATCH v6 5/6] PCI/sysfs: Only show value when driver_override is not NULL

[Bjorn Helgaas]

[+cc Alex, FYI]

On Thu, Jun 03, 2021 at 12:01:11AM +0000, Krzysztof Wilczyński wrote:
> Only expose the value of the "driver_override" variable through the
> corresponding sysfs object when a value is actually set.

This changes the attribute contents from "(null)" to an empty
(zero-length) file when no driver override has been set.

There are a few other driver_override_show() functions.  Most don't
check the pointer so they'll show "(null)".  One (spi.c) checks and
shows an empty string ("", file containing a single NULL character)
instead of an empty (zero-length) file.

> Signed-off-by: Krzysztof Wilczyński <kw@linux.com>
> Reviewed-by: Logan Gunthorpe <logang@deltatee.com>
> ---
>  drivers/pci/pci-sysfs.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index 5d63df7c1820..4e9f582ca10f 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -580,10 +580,11 @@ static ssize_t driver_override_show(struct device *dev,
>  				    struct device_attribute *attr, char *buf)
>  {
>  	struct pci_dev *pdev = to_pci_dev(dev);
> -	ssize_t len;
> +	ssize_t len = 0;
>  
>  	device_lock(dev);
> -	len = sysfs_emit(buf, "%s\n", pdev->driver_override);
> +	if (pdev->driver_override)
> +		len = sysfs_emit(buf, "%s\n", pdev->driver_override);
>  	device_unlock(dev);
>  	return len;
>  }
> -- 
> 2.31.1
> 

Re: [PATCH v6 3/6] PCI/sysfs: Fix trailing newline handling of resource_alignment_param

[Bjorn Helgaas]

On Thu, Jun 03, 2021 at 12:01:09AM +0000, Krzysztof Wilczyński wrote:
> The value of the "resource_alignment" can be specified using a kernel
> command-line argument (using the "pci=resource_alignment=") or through
> the corresponding sysfs object under the /sys/bus/pci path.
> 
> Currently, when the value is set via the kernel command-line argument,
> and then subsequently accessed through sysfs object, the value read back
> will not be correct, as per:
> 
>   # grep -oE 'pci=resource_alignment.+' /proc/cmdline
>   pci=resource_alignment=20@00:1f.2
>   # cat /sys/bus/pci/resource_alignment
>   20@00:1f.
> 
> This is also true when the value is set through the sysfs object, but
> the trailing newline has not been included, as per:
> 
>   # echo -n 20@00:1f.2 > /sys/bus/pci/resource_alignment
>   # cat /sys/bus/pci/resource_alignment
>   20@00:1f.
> 
> When the value set through the sysfs object includes the trailing
> newline, then reading it back will work as intended, as per:
> 
>   # echo 20@00:1f.2 > /sys/bus/pci/resource_alignment
>   # cat /sys/bus/pci/resource_alignment
>   20@00:1f.2
> 
> To fix this inconsistency, append a trailing newline in the show()
> function and strip the trailing line in the store() function if one is
> present.
> 
> Also, allow for the value previously set using either a command-line
> argument or through the sysfs object to be cleared at run-time.
> 
> Fixes: e499081da1a2 ("PCI: Force trailing new line to resource_alignment_param in sysfs")
> Signed-off-by: Krzysztof Wilczyński <kw@linux.com>
> Reviewed-by: Logan Gunthorpe <logang@deltatee.com>
> ---
>  drivers/pci/pci.c | 34 ++++++++++++++++++++--------------
>  1 file changed, 20 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 5ed316ea5831..b46445a49543 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -6439,34 +6439,40 @@ static ssize_t resource_alignment_show(struct bus_type *bus, char *buf)
>  
>  	spin_lock(&resource_alignment_lock);
>  	if (resource_alignment_param)
> -		count = sysfs_emit(buf, "%s", resource_alignment_param);
> +		count = sysfs_emit(buf, "%s\n", resource_alignment_param);
>  	spin_unlock(&resource_alignment_lock);
>  
> -	/*
> -	 * When set by the command line, resource_alignment_param will not
> -	 * have a trailing line feed, which is ugly. So conditionally add
> -	 * it here.
> -	 */
> -	if (count >= 2 && buf[count - 2] != '\n' && count < PAGE_SIZE - 1) {
> -		buf[count - 1] = '\n';
> -		buf[count++] = 0;
> -	}
> -
>  	return count;
>  }
>  
>  static ssize_t resource_alignment_store(struct bus_type *bus,
>  					const char *buf, size_t count)
>  {
> -	char *param = kstrndup(buf, count, GFP_KERNEL);
> +	char *param, *old, *end;
> +
> +	if (count >= (PAGE_SIZE - 1))
> +		return -EINVAL;
>  
> +	param = kstrndup(buf, count, GFP_KERNEL);
>  	if (!param)
>  		return -ENOMEM;
>  
> +	end = strchr(param, '\n');
> +	if (end)
> +		*end = '\0';
> +
>  	spin_lock(&resource_alignment_lock);
> -	kfree(resource_alignment_param);
> -	resource_alignment_param = param;
> +	old = resource_alignment_param;
> +	if (strlen(param)) {
> +		resource_alignment_param = param;
> +	} else {
> +		kfree(resource_alignment_param);

When "strlen(param) == 0", don't we kfree resource_alignment_param
twice?  Once here,

> +		resource_alignment_param = NULL;
> +	}
>  	spin_unlock(&resource_alignment_lock);
> +
> +	kfree(old);

and again here?

>  	return count;
>  }
>  
> -- 
> 2.31.1
> 

Re: [PATCH v6 5/6] PCI/sysfs: Only show value when driver_override is not NULL

[Alex Williamson]

On Thu, 3 Jun 2021 18:23:34 -0500
Bjorn Helgaas <helgaas@kernel.org> wrote:

> [+cc Alex, FYI]
> 
> On Thu, Jun 03, 2021 at 12:01:11AM +0000, Krzysztof Wilczyński wrote:
> > Only expose the value of the "driver_override" variable through the
> > corresponding sysfs object when a value is actually set.  
> 
> This changes the attribute contents from "(null)" to an empty
> (zero-length) file when no driver override has been set.
> 
> There are a few other driver_override_show() functions.  Most don't
> check the pointer so they'll show "(null)".  One (spi.c) checks and
> shows an empty string ("", file containing a single NULL character)
> instead of an empty (zero-length) file.

Yeah, "(null)" was the expected output in this case.  It looks like
this might break driverctl.  Thanks,

Alex
 
> > Signed-off-by: Krzysztof Wilczyński <kw@linux.com>
> > Reviewed-by: Logan Gunthorpe <logang@deltatee.com>
> > ---
> >  drivers/pci/pci-sysfs.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> > index 5d63df7c1820..4e9f582ca10f 100644
> > --- a/drivers/pci/pci-sysfs.c
> > +++ b/drivers/pci/pci-sysfs.c
> > @@ -580,10 +580,11 @@ static ssize_t driver_override_show(struct device *dev,
> >  				    struct device_attribute *attr, char *buf)
> >  {
> >  	struct pci_dev *pdev = to_pci_dev(dev);
> > -	ssize_t len;
> > +	ssize_t len = 0;
> >  
> >  	device_lock(dev);
> > -	len = sysfs_emit(buf, "%s\n", pdev->driver_override);
> > +	if (pdev->driver_override)
> > +		len = sysfs_emit(buf, "%s\n", pdev->driver_override);
> >  	device_unlock(dev);
> >  	return len;
> >  }
> > -- 
> > 2.31.1
> >   
> 

Re: [PATCH v6 5/6] PCI/sysfs: Only show value when driver_override is not NULL

[Krzysztof Wilczyński]

Hi Alex and Bjorn,

[...]
> > This changes the attribute contents from "(null)" to an empty
> > (zero-length) file when no driver override has been set.
> > 
> > There are a few other driver_override_show() functions.  Most don't
> > check the pointer so they'll show "(null)".  One (spi.c) checks and
> > shows an empty string ("", file containing a single NULL character)
> > instead of an empty (zero-length) file.
> 
> Yeah, "(null)" was the expected output in this case.  It looks like
> this might break driverctl.  Thanks,
[...]

I had a look and it does, indeed, rely on explicitly checking whether
the value is "(null)", as per:

  https://gitlab.com/driverctl/driverctl/-/blob/master/driverctl#L96-131

I think, to avoid breaking this and other userspace tools that might
rely on this, we should drop this patch.

Having said that, I would love to be able to correct the behaviour we
have at the moment, but it seems that this ship has sail, so to speak.

	Krzysztof

Re: [PATCH v6 0/6] PCI/sysfs: Use sysfs_emit() and sysfs_emit_at() in "show" functions

[Bjorn Helgaas]

On Thu, Jun 03, 2021 at 12:01:06AM +0000, Krzysztof Wilczyński wrote:
> Hello,
> 
> This series aims to bring support for the sysfs_emit() and
> sysfs_emit_at() functions to the existing PCI-related sysfs objects.
> These new 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].
> 
> Thus, the existing PCI sysfs objects "show" functions will be converted
> from the using the sprintf(), snprintf() and scnprintf() functions to
> sysfs_emit() and sysfs_emit_at() accordingly, as the latter is aware of
> the PAGE_SIZE buffer limit that the sysfs object has and correctly
> returns the number of bytes written into the buffer.
> 
> This series will also address inconsistency related to the presence (or
> lack of thereof) of a trailing newline in the show() functions adding it
> where it's currently missing.  This will allow for utilities such as the
> "cat" command to display the result of the read from a particular sysfs
> object correctly in a shell.
> 
> While working on this series a problem with newline handling related to
> how the value of the "resource_alignment" sysfs object was parsed and
> then persisted has been found and corrected.  Also, while at it,
> a change enabling support for the value of "resource_alignment"
> previously set using either a command-line argument or through the sysfs
> object to be cleared at run-time was also included, and thus aligning
> this particular sysfs object with the behaviour of other such objects
> that allow for the value to be dynamically updated and cleared as
> required.
> 
> Additionally, a fix to a potential buffer overrun that has been found in
> the dsm_label_utf16s_to_utf8s() function that is responsible for the
> character conversion from UTF16 to UTF8 of the buffer that holds the
> device label obtained through the ACPI _DSM mechanism is included as
> part of this series.
> 
> Finally, a minor fix is also included in this series that has been added
> to ensure that the value of the "driver_override" variable is only
> exposed through the corresponding sysfs object when a value is set or
> otherwise if the value has not been set, the object would return
> a string representation of the NULL value.  This will also align this
> particular sysfs object's behaviour with others, where when there is no
> value then nothing is returned.
> 
> [1] Documentation/filesystems/sysfs.rst
> 
> This series is related to:
>   commit ad025f8 ("PCI/sysfs: Use sysfs_emit() and sysfs_emit_at() in "show" functions")
> 
> Signed-off-by: Krzysztof Wilczyński <kw@linux.com>
> 
> ---
> Changes in v2:
>   None.
> 
> Changes in v3:
>   Added Logan Gunthorpe's "Reviewed-by".
> 
> Changes in v4:
>   Separated and squashed all the trivial sysfs_emit()/sysfs_emit_at()
>   changes into a single patch as per Bjorn Helgaas' request.
>   Carried Logan Gunthorpe's "Reviewed-by" over.
> 
> Changes in v5:
>   Added check to the resource_alignment_show() function to ensure that
>   there is an extra space left in the buffer for the newline character,
>   assuming that it might be provided.
> 
> Changes in v6:
>   Added a cover letter as per Bjorn Helgaas' request.
>   New patch addressing a potential buffer overrun in the
>   dsm_label_utf16s_to_utf8s() function has been added.
> 
> Krzysztof Wilczyński (6):
>   PCI/sysfs: Use sysfs_emit() and sysfs_emit_at() in "show" functions
>   PCI/sysfs: Use return value from dsm_label_utf16s_to_utf8s() directly
>   PCI/sysfs: Fix trailing newline handling of resource_alignment_param
>   PCI/sysfs: Add missing trailing newline to devspec_show()
>   PCI/sysfs: Only show value when driver_override is not NULL
>   PCI/sysfs: Fix a buffer overrun problem with
>     dsm_label_utf16s_to_utf8s()
> 
>  drivers/pci/hotplug/pci_hotplug_core.c |  8 +++---
>  drivers/pci/hotplug/rpadlpar_sysfs.c   |  4 +--
>  drivers/pci/hotplug/shpchp_sysfs.c     | 38 ++++++++++++++------------
>  drivers/pci/iov.c                      | 12 ++++----
>  drivers/pci/msi.c                      |  8 +++---
>  drivers/pci/p2pdma.c                   |  7 ++---
>  drivers/pci/pci-label.c                | 22 ++++++++-------
>  drivers/pci/pci-sysfs.c                |  7 +++--
>  drivers/pci/pci.c                      | 34 +++++++++++++----------
>  drivers/pci/pcie/aer.c                 | 20 ++++++++------
>  drivers/pci/pcie/aspm.c                |  4 +--
>  drivers/pci/slot.c                     | 18 ++++++------
>  drivers/pci/switch/switchtec.c         | 18 ++++++------
>  13 files changed, 107 insertions(+), 93 deletions(-)

I applied these to pci/resource for v5.14, thanks!

I dropped the driver_override change to avoid breaking driverctl.

I reordered the dsm_label_utf16s_to_utf8s()-related patches to make
the buffer overflow fix slightly smaller.

If we need to tweak the resource_alignment_param patch, I can just
squash in incremental changes.

Re: [PATCH v6 3/6] PCI/sysfs: Fix trailing newline handling of resource_alignment_param

[Krzysztof Wilczyński]

Hi Bjorn,

[...]
> > +	old = resource_alignment_param;
> > +	if (strlen(param)) {
> > +		resource_alignment_param = param;
> > +	} else {
> > +		kfree(resource_alignment_param);
> 
> When "strlen(param) == 0", don't we kfree resource_alignment_param
> twice?  Once here,

Yes, we should be freeing the allocation made using kstrndup() here,
this is my bad, I completely missed that the wrong variable name was
used.

> > +		resource_alignment_param = NULL;
> > +	}
> >  	spin_unlock(&resource_alignment_lock);
> > +
> > +	kfree(old);
> 
> and again here?
> 
> >  	return count;
[...]

Yes, good catch!  I am going to send v7 shortly.  Apologies!

	Krzysztof

Re: [PATCH v6 0/6] PCI/sysfs: Use sysfs_emit() and sysfs_emit_at() in "show" functions

[Krzysztof Wilczyński]

Hello Bjorn,

[...]
> I applied these to pci/resource for v5.14, thanks!

Thank you!

[...]
> If we need to tweak the resource_alignment_param patch, I can just
> squash in incremental changes.

If you can pick the patch from the following:
  https://lore.kernel.org/linux-pci/20210604133230.983956-4-kw@linux.com/T/#u

Sorry for troubles!

	Krzysztof