| Message ID | 20241028174046.1736-3-ilpo.jarvinen@linux.intel.com (mailing list archive) |
|---|---|
| State | Under Review |
| Delegated to: | Krzysztof Wilczyński |
| Headers | show |
| Series | PCI/sysfs: move to sysfs funcs to pci-sysfs.c & do small tweaks | expand |
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.
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?
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? > >
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. :)
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);
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(-)