[PATCH 0/3] PCI/sysfs: move to sysfs funcs to pci-sysfs.c & do small tweaks

[PATCH 0/3] PCI/sysfs: move to sysfs funcs to pci-sysfs.c & do small tweaks

[Ilpo Järvinen]

Hi all,

This series moves reset related sysfs functions into the correct file
and does small style related improvements.

This series is based on top of commit 2985b1844f3f ("PCI: Fix
reset_method_store() memory leak") in pci/misc branch to avoid
conflict with the code move.


Ilpo Järvinen (3):
  PCI/sysfs: Move reset related sysfs code to correct file
  PCI/sysfs: Use __free() in reset_method_store()
  PCI/sysfs: Remove unnecessary zero in initializer

 drivers/pci/pci-sysfs.c | 108 ++++++++++++++++++++++++++++++++++
 drivers/pci/pci.c       | 125 +---------------------------------------
 drivers/pci/pci.h       |   3 +-
 3 files changed, 110 insertions(+), 126 deletions(-)

-- 
2.39.5

[PATCH 1/3] PCI/sysfs: Move reset related sysfs code to correct file

[Ilpo Järvinen]

Most PCI sysfs code and structs are in a dedicated file but a few reset
related things remain in pci.c. Move also them to pci-sysfs.c and drop
pci_dev_reset_method_attr_is_visible() as it is 100% duplicate of
pci_dev_reset_attr_is_visible().

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/pci/pci-sysfs.c | 112 +++++++++++++++++++++++++++++++++++
 drivers/pci/pci.c       | 125 +---------------------------------------
 drivers/pci/pci.h       |   3 +-
 3 files changed, 114 insertions(+), 126 deletions(-)

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 5d0f4db1cab7..507478082454 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -1391,6 +1391,118 @@ static const struct attribute_group pci_dev_reset_attr_group = {
 	.is_visible = pci_dev_reset_attr_is_visible,
 };
 
+static ssize_t reset_method_show(struct device *dev,
+				 struct device_attribute *attr, char *buf)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+	ssize_t len = 0;
+	int i, m;
+
+	for (i = 0; i < PCI_NUM_RESET_METHODS; i++) {
+		m = pdev->reset_methods[i];
+		if (!m)
+			break;
+
+		len += sysfs_emit_at(buf, len, "%s%s", len ? " " : "",
+				     pci_reset_fn_methods[m].name);
+	}
+
+	if (len)
+		len += sysfs_emit_at(buf, len, "\n");
+
+	return len;
+}
+
+static int reset_method_lookup(const char *name)
+{
+	int m;
+
+	for (m = 1; m < PCI_NUM_RESET_METHODS; m++) {
+		if (sysfs_streq(name, pci_reset_fn_methods[m].name))
+			return m;
+	}
+
+	return 0;	/* not found */
+}
+
+static ssize_t reset_method_store(struct device *dev,
+				  struct device_attribute *attr,
+				  const char *buf, size_t count)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+	char *options, *tmp_options, *name;
+	int m, n;
+	u8 reset_methods[PCI_NUM_RESET_METHODS] = { 0 };
+
+	if (sysfs_streq(buf, "")) {
+		pdev->reset_methods[0] = 0;
+		pci_warn(pdev, "All device reset methods disabled by user");
+		return count;
+	}
+
+	if (sysfs_streq(buf, "default")) {
+		pci_init_reset_methods(pdev);
+		return count;
+	}
+
+	options = kstrndup(buf, count, GFP_KERNEL);
+	if (!options)
+		return -ENOMEM;
+
+	n = 0;
+	tmp_options = options;
+	while ((name = strsep(&tmp_options, " ")) != NULL) {
+		if (sysfs_streq(name, ""))
+			continue;
+
+		name = strim(name);
+
+		m = reset_method_lookup(name);
+		if (!m) {
+			pci_err(pdev, "Invalid reset method '%s'", name);
+			goto error;
+		}
+
+		if (pci_reset_fn_methods[m].reset_fn(pdev, PCI_RESET_PROBE)) {
+			pci_err(pdev, "Unsupported reset method '%s'", name);
+			goto error;
+		}
+
+		if (n == PCI_NUM_RESET_METHODS - 1) {
+			pci_err(pdev, "Too many reset methods\n");
+			goto error;
+		}
+
+		reset_methods[n++] = m;
+	}
+
+	reset_methods[n] = 0;
+
+	/* Warn if dev-specific supported but not highest priority */
+	if (pci_reset_fn_methods[1].reset_fn(pdev, PCI_RESET_PROBE) == 0 &&
+	    reset_methods[0] != 1)
+		pci_warn(pdev, "Device-specific reset disabled/de-prioritized by user");
+	memcpy(pdev->reset_methods, reset_methods, sizeof(pdev->reset_methods));
+	kfree(options);
+	return count;
+
+error:
+	/* Leave previous methods unchanged */
+	kfree(options);
+	return -EINVAL;
+}
+static DEVICE_ATTR_RW(reset_method);
+
+static struct attribute *pci_dev_reset_method_attrs[] = {
+	&dev_attr_reset_method.attr,
+	NULL,
+};
+
+static const struct attribute_group pci_dev_reset_method_attr_group = {
+	.attrs = pci_dev_reset_method_attrs,
+	.is_visible = pci_dev_reset_attr_is_visible,
+};
+
 static ssize_t __resource_resize_show(struct device *dev, int n, char *buf)
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 0e6562ff3dcf..17831b7d9531 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -5194,7 +5194,7 @@ static void pci_dev_restore(struct pci_dev *dev)
 }
 
 /* dev->reset_methods[] is a 0-terminated list of indices into this array */
-static const struct pci_reset_fn_method pci_reset_fn_methods[] = {
+const struct pci_reset_fn_method pci_reset_fn_methods[] = {
 	{ },
 	{ pci_dev_specific_reset, .name = "device_specific" },
 	{ pci_dev_acpi_reset, .name = "acpi" },
@@ -5205,129 +5205,6 @@ static const struct pci_reset_fn_method pci_reset_fn_methods[] = {
 	{ cxl_reset_bus_function, .name = "cxl_bus" },
 };
 
-static ssize_t reset_method_show(struct device *dev,
-				 struct device_attribute *attr, char *buf)
-{
-	struct pci_dev *pdev = to_pci_dev(dev);
-	ssize_t len = 0;
-	int i, m;
-
-	for (i = 0; i < PCI_NUM_RESET_METHODS; i++) {
-		m = pdev->reset_methods[i];
-		if (!m)
-			break;
-
-		len += sysfs_emit_at(buf, len, "%s%s", len ? " " : "",
-				     pci_reset_fn_methods[m].name);
-	}
-
-	if (len)
-		len += sysfs_emit_at(buf, len, "\n");
-
-	return len;
-}
-
-static int reset_method_lookup(const char *name)
-{
-	int m;
-
-	for (m = 1; m < PCI_NUM_RESET_METHODS; m++) {
-		if (sysfs_streq(name, pci_reset_fn_methods[m].name))
-			return m;
-	}
-
-	return 0;	/* not found */
-}
-
-static ssize_t reset_method_store(struct device *dev,
-				  struct device_attribute *attr,
-				  const char *buf, size_t count)
-{
-	struct pci_dev *pdev = to_pci_dev(dev);
-	char *options, *tmp_options, *name;
-	int m, n;
-	u8 reset_methods[PCI_NUM_RESET_METHODS] = { 0 };
-
-	if (sysfs_streq(buf, "")) {
-		pdev->reset_methods[0] = 0;
-		pci_warn(pdev, "All device reset methods disabled by user");
-		return count;
-	}
-
-	if (sysfs_streq(buf, "default")) {
-		pci_init_reset_methods(pdev);
-		return count;
-	}
-
-	options = kstrndup(buf, count, GFP_KERNEL);
-	if (!options)
-		return -ENOMEM;
-
-	n = 0;
-	tmp_options = options;
-	while ((name = strsep(&tmp_options, " ")) != NULL) {
-		if (sysfs_streq(name, ""))
-			continue;
-
-		name = strim(name);
-
-		m = reset_method_lookup(name);
-		if (!m) {
-			pci_err(pdev, "Invalid reset method '%s'", name);
-			goto error;
-		}
-
-		if (pci_reset_fn_methods[m].reset_fn(pdev, PCI_RESET_PROBE)) {
-			pci_err(pdev, "Unsupported reset method '%s'", name);
-			goto error;
-		}
-
-		if (n == PCI_NUM_RESET_METHODS - 1) {
-			pci_err(pdev, "Too many reset methods\n");
-			goto error;
-		}
-
-		reset_methods[n++] = m;
-	}
-
-	reset_methods[n] = 0;
-
-	/* Warn if dev-specific supported but not highest priority */
-	if (pci_reset_fn_methods[1].reset_fn(pdev, PCI_RESET_PROBE) == 0 &&
-	    reset_methods[0] != 1)
-		pci_warn(pdev, "Device-specific reset disabled/de-prioritized by user");
-	memcpy(pdev->reset_methods, reset_methods, sizeof(pdev->reset_methods));
-	kfree(options);
-	return count;
-
-error:
-	/* Leave previous methods unchanged */
-	kfree(options);
-	return -EINVAL;
-}
-static DEVICE_ATTR_RW(reset_method);
-
-static struct attribute *pci_dev_reset_method_attrs[] = {
-	&dev_attr_reset_method.attr,
-	NULL,
-};
-
-static umode_t pci_dev_reset_method_attr_is_visible(struct kobject *kobj,
-						    struct attribute *a, int n)
-{
-	struct pci_dev *pdev = to_pci_dev(kobj_to_dev(kobj));
-
-	if (!pci_reset_supported(pdev))
-		return 0;
-
-	return a->mode;
-}
-
-const struct attribute_group pci_dev_reset_method_attr_group = {
-	.attrs = pci_dev_reset_method_attrs,
-	.is_visible = pci_dev_reset_method_attr_is_visible,
-};
-
 /**
  * __pci_reset_function_locked - reset a PCI device function while holding
  * the @dev mutex lock.
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 14d00ce45bfa..e65dcc8e4832 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -702,6 +702,7 @@ struct pci_reset_fn_method {
 	int (*reset_fn)(struct pci_dev *pdev, bool probe);
 	char *name;
 };
+extern const struct pci_reset_fn_method pci_reset_fn_methods[];
 
 #ifdef CONFIG_PCI_QUIRKS
 int pci_dev_specific_reset(struct pci_dev *dev, bool probe);
@@ -891,8 +892,6 @@ static inline pci_power_t acpi_pci_choose_state(struct pci_dev *pdev)
 extern const struct attribute_group aspm_ctrl_attr_group;
 #endif
 
-extern const struct attribute_group pci_dev_reset_method_attr_group;
-
 #ifdef CONFIG_X86_INTEL_MID
 bool pci_use_mid_pm(void);
 int mid_pci_set_power_state(struct pci_dev *pdev, pci_power_t state);
-- 
2.39.5

[PATCH 2/3] PCI/sysfs: Use __free() in reset_method_store()

[Ilpo Järvinen]

Use __free() from  cleanup.h to handle freeing options in
reset_method_store() as it simplifies the code flow.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/pci/pci-sysfs.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 507478082454..74e4e0917898 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -13,6 +13,7 @@
  */
 
 #include <linux/bitfield.h>
+#include <linux/cleanup.h>
 #include <linux/kernel.h>
 #include <linux/sched.h>
 #include <linux/pci.h>
@@ -1430,7 +1431,7 @@ static ssize_t reset_method_store(struct device *dev,
 				  const char *buf, size_t count)
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
-	char *options, *tmp_options, *name;
+	char *tmp_options, *name;
 	int m, n;
 	u8 reset_methods[PCI_NUM_RESET_METHODS] = { 0 };
 
@@ -1445,7 +1446,7 @@ static ssize_t reset_method_store(struct device *dev,
 		return count;
 	}
 
-	options = kstrndup(buf, count, GFP_KERNEL);
+	char *options __free(kfree) = kstrndup(buf, count, GFP_KERNEL);
 	if (!options)
 		return -ENOMEM;
 
@@ -1457,20 +1458,21 @@ static ssize_t reset_method_store(struct device *dev,
 
 		name = strim(name);
 
+		/* Leave previous methods unchanged if input is invalid */
 		m = reset_method_lookup(name);
 		if (!m) {
 			pci_err(pdev, "Invalid reset method '%s'", name);
-			goto error;
+			return -EINVAL;
 		}
 
 		if (pci_reset_fn_methods[m].reset_fn(pdev, PCI_RESET_PROBE)) {
 			pci_err(pdev, "Unsupported reset method '%s'", name);
-			goto error;
+			return -EINVAL;
 		}
 
 		if (n == PCI_NUM_RESET_METHODS - 1) {
 			pci_err(pdev, "Too many reset methods\n");
-			goto error;
+			return -EINVAL;
 		}
 
 		reset_methods[n++] = m;
@@ -1483,13 +1485,7 @@ static ssize_t reset_method_store(struct device *dev,
 	    reset_methods[0] != 1)
 		pci_warn(pdev, "Device-specific reset disabled/de-prioritized by user");
 	memcpy(pdev->reset_methods, reset_methods, sizeof(pdev->reset_methods));
-	kfree(options);
 	return count;
-
-error:
-	/* Leave previous methods unchanged */
-	kfree(options);
-	return -EINVAL;
 }
 static DEVICE_ATTR_RW(reset_method);
 
-- 
2.39.5

[PATCH 3/3] PCI/sysfs: Remove unnecessary zero in initializer

[Ilpo Järvinen]

Providing empty initializer for an array is enough to set its elements
to zero. Thus, remove the redundant 0 from the initializer.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.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 74e4e0917898..19da2f8e98e4 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -1433,7 +1433,7 @@ static ssize_t reset_method_store(struct device *dev,
 	struct pci_dev *pdev = to_pci_dev(dev);
 	char *tmp_options, *name;
 	int m, n;
-	u8 reset_methods[PCI_NUM_RESET_METHODS] = { 0 };
+	u8 reset_methods[PCI_NUM_RESET_METHODS] = {};
 
 	if (sysfs_streq(buf, "")) {
 		pdev->reset_methods[0] = 0;
-- 
2.39.5

Re: [PATCH 2/3] PCI/sysfs: Use __free() in reset_method_store()

[Keith Busch]

On Mon, Oct 28, 2024 at 07:40:45PM +0200, Ilpo Järvinen wrote:
> @@ -1430,7 +1431,7 @@ static ssize_t reset_method_store(struct device *dev,
>  				  const char *buf, size_t count)
>  {
>  	struct pci_dev *pdev = to_pci_dev(dev);
> -	char *options, *tmp_options, *name;
> +	char *tmp_options, *name;
>  	int m, n;
>  	u8 reset_methods[PCI_NUM_RESET_METHODS] = { 0 };
>  
> @@ -1445,7 +1446,7 @@ static ssize_t reset_method_store(struct device *dev,
>  		return count;
>  	}
>  
> -	options = kstrndup(buf, count, GFP_KERNEL);
> +	char *options __free(kfree) = kstrndup(buf, count, GFP_KERNEL);

We should avoid mixing declarations with code. Please declare it with
the cleanup attribute at the top like before, and just initialize it to
NULL.

Re: [PATCH 2/3] PCI/sysfs: Use __free() in reset_method_store()

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1214 bytes --]

On Mon, 28 Oct 2024, Keith Busch wrote:

> On Mon, Oct 28, 2024 at 07:40:45PM +0200, Ilpo Järvinen wrote:
> > @@ -1430,7 +1431,7 @@ static ssize_t reset_method_store(struct device *dev,
> >  				  const char *buf, size_t count)
> >  {
> >  	struct pci_dev *pdev = to_pci_dev(dev);
> > -	char *options, *tmp_options, *name;
> > +	char *tmp_options, *name;
> >  	int m, n;
> >  	u8 reset_methods[PCI_NUM_RESET_METHODS] = { 0 };
> >  
> > @@ -1445,7 +1446,7 @@ static ssize_t reset_method_store(struct device *dev,
> >  		return count;
> >  	}
> >  
> > -	options = kstrndup(buf, count, GFP_KERNEL);
> > +	char *options __free(kfree) = kstrndup(buf, count, GFP_KERNEL);
> 
> We should avoid mixing declarations with code. Please declare it with
> the cleanup attribute at the top like before, and just initialize it to
> NULL.

Hi,

I don't exactly disagree with you myself and would prefer to keep 
declarations at top, but I think as done now is exactly what Bjorn wants 
for the specific case where __free() is used. This was discussed earlier 
on the list.

If I misunderstood the conclusion of the earlier cleanup related 
discussion, can you please correct me Bjorn?

-- 
 i.

Re: [PATCH 2/3] PCI/sysfs: Use __free() in reset_method_store()

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 2804 bytes --]

On Mon, 28 Oct 2024, Ilpo Järvinen wrote:

> On Mon, 28 Oct 2024, Keith Busch wrote:
> 
> > On Mon, Oct 28, 2024 at 07:40:45PM +0200, Ilpo Järvinen wrote:
> > > @@ -1430,7 +1431,7 @@ static ssize_t reset_method_store(struct device *dev,
> > >  				  const char *buf, size_t count)
> > >  {
> > >  	struct pci_dev *pdev = to_pci_dev(dev);
> > > -	char *options, *tmp_options, *name;
> > > +	char *tmp_options, *name;
> > >  	int m, n;
> > >  	u8 reset_methods[PCI_NUM_RESET_METHODS] = { 0 };
> > >  
> > > @@ -1445,7 +1446,7 @@ static ssize_t reset_method_store(struct device *dev,
> > >  		return count;
> > >  	}
> > >  
> > > -	options = kstrndup(buf, count, GFP_KERNEL);
> > > +	char *options __free(kfree) = kstrndup(buf, count, GFP_KERNEL);
> > 
> > We should avoid mixing declarations with code. Please declare it with
> > the cleanup attribute at the top like before, and just initialize it to
> > NULL.
> 
> Hi,
> 
> I don't exactly disagree with you myself and would prefer to keep 
> declarations at top, but I think as done now is exactly what Bjorn wants 
> for the specific case where __free() is used. This was discussed earlier 
> on the list.

Hi again,

I went to archives and found out it had already made itself into 
include/linux/cleanup.h which now says this:

"
 * Now, when a function uses both __free() and guard(), or multiple
 * instances of __free(), the LIFO order of variable definition order
 * matters. GCC documentation says:
 *
 * "When multiple variables in the same scope have cleanup attributes,
 * at exit from the scope their associated cleanup functions are run in
 * reverse order of definition (last defined, first cleanup)."
 *
 * When the unwind order matters it requires that variables be defined
 * mid-function scope rather than at the top of the file.

[...snip examples...]

 * Given that the "__free(...) = NULL" pattern for variables defined at
 * the top of the function poses this potential interdependency problem
 * the recommendation is to always define and assign variables in one
 * statement and not group variable definitions at the top of the
 * function when __free() is used.
"

After reading the documentation for real now myself :-), I realized it's
not just about maintainer preferences but about order of releasing things, 
so it's a BAD PATTERN to put those declarations into the usual place when
using __free().


For completeness, the discussion thread (there might have been another 
thread earlier than these):

https://lore.kernel.org/linux-pci/171140738438.1574931.15717256954707430472.stgit@dwillia2-xfh.jf.intel.com/T/#u

> If I misunderstood the conclusion of the earlier cleanup related 
> discussion, can you please correct me Bjorn?
> 
> 

-- 
 i.

Re: [PATCH 2/3] PCI/sysfs: Use __free() in reset_method_store()

[Keith Busch]

On Mon, Oct 28, 2024 at 08:18:39PM +0200, Ilpo Järvinen wrote:
> I went to archives and found out it had already made itself into 
> include/linux/cleanup.h which now says this:
> 
> "
>  * Now, when a function uses both __free() and guard(), or multiple
>  * instances of __free(), the LIFO order of variable definition order
>  * matters. GCC documentation says:
>  *
>  * "When multiple variables in the same scope have cleanup attributes,
>  * at exit from the scope their associated cleanup functions are run in
>  * reverse order of definition (last defined, first cleanup)."
>  *
>  * When the unwind order matters it requires that variables be defined
>  * mid-function scope rather than at the top of the file.
> 
> [...snip examples...]
> 
>  * Given that the "__free(...) = NULL" pattern for variables defined at
>  * the top of the function poses this potential interdependency problem
>  * the recommendation is to always define and assign variables in one
>  * statement and not group variable definitions at the top of the
>  * function when __free() is used.
> "
> 
> After reading the documentation for real now myself :-), I realized it's
> not just about maintainer preferences but about order of releasing things, 
> so it's a BAD PATTERN to put those declarations into the usual place when
> using __free().

Okay, I can't argue against that... It still looks unpleasant. :)

Re: [PATCH 0/3] PCI/sysfs: move to sysfs funcs to pci-sysfs.c & do small tweaks

[Krzysztof Wilczyński]

Hello,

> This series moves reset related sysfs functions into the correct file
> and does small style related improvements.
> 
> This series is based on top of commit 2985b1844f3f ("PCI: Fix
> reset_method_store() memory leak") in pci/misc branch to avoid
> conflict with the code move.

Applied to pci-sysfs for v6.14, thank you!

	Krzysztof