* [PATCH 0/4] of: Automate handling of of_node_put()
@ 2024-01-14 16:53 Jonathan Cameron
2024-01-14 16:53 ` [PATCH 1/4] of: Add cleanup.h based auto release via __free(device_node) markings Jonathan Cameron
` (3 more replies)
0 siblings, 4 replies; 10+ messages in thread
From: Jonathan Cameron @ 2024-01-14 16:53 UTC (permalink / raw
To: linux-iio, devicetree, Rob Herring, Frank Rowand
Cc: Julia Lawall, Nicolas Palix, Sumera Priyadarsini,
Jonathan Cameron
From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Changes since RFC: Thanks to Rob Herring for reviewing.
- Patch description typo fixes
- Add some info on the coccinelle script to patch 1. Primarily stating
that we don't seem to cause false positives with this change and that
any scripting to find cases to update like this can wait for now.
- Note the if (_T) is left in place as general consensus from similar
discussions on other cleanup.h use cases is that it can be helpful
to let the compiler optimize out the call, even when the call would
be safe with a NULL value.
Recent addition of scope based cleanup (linux/cleanup.h) allows us
to avoid a large number of places where error handlers and early
returns have to carefully deal with left over resources.
The need to call of_node_put() on breaking out of loops over child
nodes is one of these cases and this series is to address that.
A similar series has been posted for property.h equivalent case.
https://lore.kernel.org/linux-iio/20240101172611.694830-1-jic23@kernel.org/
(will be updates shortly).
If everyone is happy with this series, I'd propose an immutable branch
(either in iio.git or somewhere else) so that we can pull the first 2
patches into other trees without having to wait a whole cycle to start
making more use of this.
Jonathan Cameron (4):
of: Add cleanup.h based auto release via __free(device_node) markings.
of: unittest: Use __free(device_node)
iio: adc: fsl-imx25-gcq: Use __free(device_node)
iio: adc: rcar-gyroadc: use __free(device_node)
drivers/iio/adc/fsl-imx25-gcq.c | 12 +++---------
drivers/iio/adc/rcar-gyroadc.c | 20 ++++++--------------
drivers/of/unittest.c | 10 +++-------
include/linux/of.h | 2 ++
4 files changed, 14 insertions(+), 30 deletions(-)
--
2.43.0
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/4] of: Add cleanup.h based auto release via __free(device_node) markings.
2024-01-14 16:53 [PATCH 0/4] of: Automate handling of of_node_put() Jonathan Cameron
@ 2024-01-14 16:53 ` Jonathan Cameron
2024-01-14 16:53 ` [PATCH 2/4] of: unittest: Use __free(device_node) Jonathan Cameron
` (2 subsequent siblings)
3 siblings, 0 replies; 10+ messages in thread
From: Jonathan Cameron @ 2024-01-14 16:53 UTC (permalink / raw
To: linux-iio, devicetree, Rob Herring, Frank Rowand
Cc: Julia Lawall, Nicolas Palix, Sumera Priyadarsini,
Jonathan Cameron
From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
The recent addition of scope based cleanup support to the kernel
provides a convenient tool to reduce the chances of leaking reference
counts where of_node_put() should have been called in an error path.
This enables
struct device_node *child __free(device_node) = NULL;
for_each_child_of_node(np, child) {
if (test)
return test;
}
with no need for a manual call of of_node_put()
In this simple example the gains are small but there are some very
complex error handling cases buried in these loops that will be
greatly simplified by enabling early returns with out the need
for this manual of_node_put() call.
Note that there are coccinelle checks in
scripts/coccinelle/iterators/for_each_child.cocci to detect a failure
to call of_node_put(). This new approach does not cause false positives.
Longer term we may want to add scripting to check this new approach is
done correctly with no double of_node_put() calls being introduced due
to the auto cleanup. It may also be useful to script finding places
this new approach is useful.
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
include/linux/of.h | 2 ++
1 file changed, 2 insertions(+)
diff --git a/include/linux/of.h b/include/linux/of.h
index 6a9ddf20e79a..50e882ee91da 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -13,6 +13,7 @@
*/
#include <linux/types.h>
#include <linux/bitops.h>
+#include <linux/cleanup.h>
#include <linux/errno.h>
#include <linux/kobject.h>
#include <linux/mod_devicetable.h>
@@ -134,6 +135,7 @@ static inline struct device_node *of_node_get(struct device_node *node)
}
static inline void of_node_put(struct device_node *node) { }
#endif /* !CONFIG_OF_DYNAMIC */
+DEFINE_FREE(device_node, struct device_node *, if (_T) of_node_put(_T))
/* Pointer for first entry in chain of all nodes. */
extern struct device_node *of_root;
--
2.43.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/4] of: unittest: Use __free(device_node)
2024-01-14 16:53 [PATCH 0/4] of: Automate handling of of_node_put() Jonathan Cameron
2024-01-14 16:53 ` [PATCH 1/4] of: Add cleanup.h based auto release via __free(device_node) markings Jonathan Cameron
@ 2024-01-14 16:53 ` Jonathan Cameron
2024-01-16 18:26 ` Rob Herring
2024-01-14 16:53 ` [PATCH 3/4] iio: adc: fsl-imx25-gcq: " Jonathan Cameron
2024-01-14 16:53 ` [PATCH 4/4] iio: adc: rcar-gyroadc: use __free(device_node) Jonathan Cameron
3 siblings, 1 reply; 10+ messages in thread
From: Jonathan Cameron @ 2024-01-14 16:53 UTC (permalink / raw
To: linux-iio, devicetree, Rob Herring, Frank Rowand
Cc: Julia Lawall, Nicolas Palix, Sumera Priyadarsini,
Jonathan Cameron
From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
A simple example of the utility of this autocleanup approach to
handling of_node_put()
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
drivers/of/unittest.c | 10 +++-------
1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index e9e90e96600e..b6d9edb831f0 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -233,27 +233,23 @@ static void __init of_unittest_dynamic(void)
static int __init of_unittest_check_node_linkage(struct device_node *np)
{
- struct device_node *child;
+ struct device_node *child __free(device_node) = NULL;
int count = 0, rc;
for_each_child_of_node(np, child) {
if (child->parent != np) {
pr_err("Child node %pOFn links to wrong parent %pOFn\n",
child, np);
- rc = -EINVAL;
- goto put_child;
+ return -EINVAL;
}
rc = of_unittest_check_node_linkage(child);
if (rc < 0)
- goto put_child;
+ return rc;
count += rc;
}
return count + 1;
-put_child:
- of_node_put(child);
- return rc;
}
static void __init of_unittest_check_tree_linkage(void)
--
2.43.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 3/4] iio: adc: fsl-imx25-gcq: Use __free(device_node)
2024-01-14 16:53 [PATCH 0/4] of: Automate handling of of_node_put() Jonathan Cameron
2024-01-14 16:53 ` [PATCH 1/4] of: Add cleanup.h based auto release via __free(device_node) markings Jonathan Cameron
2024-01-14 16:53 ` [PATCH 2/4] of: unittest: Use __free(device_node) Jonathan Cameron
@ 2024-01-14 16:53 ` Jonathan Cameron
2024-01-14 16:53 ` [PATCH 4/4] iio: adc: rcar-gyroadc: use __free(device_node) Jonathan Cameron
3 siblings, 0 replies; 10+ messages in thread
From: Jonathan Cameron @ 2024-01-14 16:53 UTC (permalink / raw
To: linux-iio, devicetree, Rob Herring, Frank Rowand
Cc: Julia Lawall, Nicolas Palix, Sumera Priyadarsini,
Jonathan Cameron
From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Using automated cleanup reduces chance of an reference count leak
and simplfies the code.
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
drivers/iio/adc/fsl-imx25-gcq.c | 12 +++---------
1 file changed, 3 insertions(+), 9 deletions(-)
diff --git a/drivers/iio/adc/fsl-imx25-gcq.c b/drivers/iio/adc/fsl-imx25-gcq.c
index 68c813de0605..e04f92d7a953 100644
--- a/drivers/iio/adc/fsl-imx25-gcq.c
+++ b/drivers/iio/adc/fsl-imx25-gcq.c
@@ -199,7 +199,7 @@ static int mx25_gcq_setup_cfgs(struct platform_device *pdev,
struct mx25_gcq_priv *priv)
{
struct device_node *np = pdev->dev.of_node;
- struct device_node *child;
+ struct device_node *child __free(device_node) = NULL;
struct device *dev = &pdev->dev;
int ret, i;
@@ -224,14 +224,12 @@ static int mx25_gcq_setup_cfgs(struct platform_device *pdev,
ret = of_property_read_u32(child, "reg", ®);
if (ret) {
dev_err(dev, "Failed to get reg property\n");
- of_node_put(child);
return ret;
}
if (reg >= MX25_NUM_CFGS) {
dev_err(dev,
"reg value is greater than the number of available configuration registers\n");
- of_node_put(child);
return -EINVAL;
}
@@ -243,10 +241,9 @@ static int mx25_gcq_setup_cfgs(struct platform_device *pdev,
case MX25_ADC_REFP_XP:
case MX25_ADC_REFP_YP:
ret = mx25_gcq_ext_regulator_setup(&pdev->dev, priv, refp);
- if (ret) {
- of_node_put(child);
+ if (ret)
return ret;
- }
+
priv->channel_vref_mv[reg] =
regulator_get_voltage(priv->vref[refp]);
/* Conversion from uV to mV */
@@ -257,7 +254,6 @@ static int mx25_gcq_setup_cfgs(struct platform_device *pdev,
break;
default:
dev_err(dev, "Invalid positive reference %d\n", refp);
- of_node_put(child);
return -EINVAL;
}
@@ -270,12 +266,10 @@ static int mx25_gcq_setup_cfgs(struct platform_device *pdev,
if ((refp & MX25_ADCQ_CFG_REFP_MASK) != refp) {
dev_err(dev, "Invalid fsl,adc-refp property value\n");
- of_node_put(child);
return -EINVAL;
}
if ((refn & MX25_ADCQ_CFG_REFN_MASK) != refn) {
dev_err(dev, "Invalid fsl,adc-refn property value\n");
- of_node_put(child);
return -EINVAL;
}
--
2.43.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 4/4] iio: adc: rcar-gyroadc: use __free(device_node)
2024-01-14 16:53 [PATCH 0/4] of: Automate handling of of_node_put() Jonathan Cameron
` (2 preceding siblings ...)
2024-01-14 16:53 ` [PATCH 3/4] iio: adc: fsl-imx25-gcq: " Jonathan Cameron
@ 2024-01-14 16:53 ` Jonathan Cameron
3 siblings, 0 replies; 10+ messages in thread
From: Jonathan Cameron @ 2024-01-14 16:53 UTC (permalink / raw
To: linux-iio, devicetree, Rob Herring, Frank Rowand
Cc: Julia Lawall, Nicolas Palix, Sumera Priyadarsini,
Jonathan Cameron
From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Using automated cleanup to replace of_node_put() handling allows for
a simplfied flow by enabling direct returns on errors.
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
drivers/iio/adc/rcar-gyroadc.c | 20 ++++++--------------
1 file changed, 6 insertions(+), 14 deletions(-)
diff --git a/drivers/iio/adc/rcar-gyroadc.c b/drivers/iio/adc/rcar-gyroadc.c
index d524f2e8e927..9d6227b31724 100644
--- a/drivers/iio/adc/rcar-gyroadc.c
+++ b/drivers/iio/adc/rcar-gyroadc.c
@@ -318,7 +318,7 @@ static int rcar_gyroadc_parse_subdevs(struct iio_dev *indio_dev)
struct rcar_gyroadc *priv = iio_priv(indio_dev);
struct device *dev = priv->dev;
struct device_node *np = dev->of_node;
- struct device_node *child;
+ struct device_node *child __free(device_node) = NULL;
struct regulator *vref;
unsigned int reg;
unsigned int adcmode = -1, childmode;
@@ -352,7 +352,7 @@ static int rcar_gyroadc_parse_subdevs(struct iio_dev *indio_dev)
num_channels = ARRAY_SIZE(rcar_gyroadc_iio_channels_3);
break;
default:
- goto err_e_inval;
+ return -EINVAL;
}
/*
@@ -369,7 +369,7 @@ static int rcar_gyroadc_parse_subdevs(struct iio_dev *indio_dev)
dev_err(dev,
"Failed to get child reg property of ADC \"%pOFn\".\n",
child);
- goto err_of_node_put;
+ return ret;
}
/* Channel number is too high. */
@@ -377,7 +377,7 @@ static int rcar_gyroadc_parse_subdevs(struct iio_dev *indio_dev)
dev_err(dev,
"Only %i channels supported with %pOFn, but reg = <%i>.\n",
num_channels, child, reg);
- goto err_e_inval;
+ return -EINVAL;
}
}
@@ -386,7 +386,7 @@ static int rcar_gyroadc_parse_subdevs(struct iio_dev *indio_dev)
dev_err(dev,
"Channel %i uses different ADC mode than the rest.\n",
reg);
- goto err_e_inval;
+ return -EINVAL;
}
/* Channel is valid, grab the regulator. */
@@ -396,8 +396,7 @@ static int rcar_gyroadc_parse_subdevs(struct iio_dev *indio_dev)
if (IS_ERR(vref)) {
dev_dbg(dev, "Channel %i 'vref' supply not connected.\n",
reg);
- ret = PTR_ERR(vref);
- goto err_of_node_put;
+ return PTR_ERR(vref);
}
priv->vref[reg] = vref;
@@ -422,7 +421,6 @@ static int rcar_gyroadc_parse_subdevs(struct iio_dev *indio_dev)
* we can stop parsing here.
*/
if (childmode == RCAR_GYROADC_MODE_SELECT_1_MB88101A) {
- of_node_put(child);
break;
}
}
@@ -433,12 +431,6 @@ static int rcar_gyroadc_parse_subdevs(struct iio_dev *indio_dev)
}
return 0;
-
-err_e_inval:
- ret = -EINVAL;
-err_of_node_put:
- of_node_put(child);
- return ret;
}
static void rcar_gyroadc_deinit_supplies(struct iio_dev *indio_dev)
--
2.43.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/4] of: unittest: Use __free(device_node)
2024-01-14 16:53 ` [PATCH 2/4] of: unittest: Use __free(device_node) Jonathan Cameron
@ 2024-01-16 18:26 ` Rob Herring
2024-01-17 17:01 ` Jonathan Cameron
0 siblings, 1 reply; 10+ messages in thread
From: Rob Herring @ 2024-01-16 18:26 UTC (permalink / raw
To: Jonathan Cameron
Cc: linux-iio, devicetree, Frank Rowand, Julia Lawall, Nicolas Palix,
Sumera Priyadarsini, Jonathan Cameron
On Sun, Jan 14, 2024 at 10:54 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> A simple example of the utility of this autocleanup approach to
> handling of_node_put()
>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
> drivers/of/unittest.c | 10 +++-------
> 1 file changed, 3 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
> index e9e90e96600e..b6d9edb831f0 100644
> --- a/drivers/of/unittest.c
> +++ b/drivers/of/unittest.c
> @@ -233,27 +233,23 @@ static void __init of_unittest_dynamic(void)
>
> static int __init of_unittest_check_node_linkage(struct device_node *np)
> {
> - struct device_node *child;
> + struct device_node *child __free(device_node) = NULL;
In another thread[1], it seems that initializing to NULL is bad form
according to the chief penguin. But as this is a refcounted pointer
rather than an allocation, IDK?
Rob
[1] https://lore.kernel.org/all/289c4af00bcc46e83555dacbc76f56477126d645.camel@pengutronix.de
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/4] of: unittest: Use __free(device_node)
2024-01-16 18:26 ` Rob Herring
@ 2024-01-17 17:01 ` Jonathan Cameron
2024-01-17 17:09 ` Jonathan Cameron
2024-01-17 19:47 ` Rob Herring
0 siblings, 2 replies; 10+ messages in thread
From: Jonathan Cameron @ 2024-01-17 17:01 UTC (permalink / raw
To: Rob Herring
Cc: Jonathan Cameron, linux-iio, devicetree, Frank Rowand,
Julia Lawall, Nicolas Palix, Sumera Priyadarsini
On Tue, 16 Jan 2024 12:26:49 -0600
Rob Herring <robh@kernel.org> wrote:
> On Sun, Jan 14, 2024 at 10:54 AM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> >
> > A simple example of the utility of this autocleanup approach to
> > handling of_node_put()
> >
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > ---
> > drivers/of/unittest.c | 10 +++-------
> > 1 file changed, 3 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
> > index e9e90e96600e..b6d9edb831f0 100644
> > --- a/drivers/of/unittest.c
> > +++ b/drivers/of/unittest.c
> > @@ -233,27 +233,23 @@ static void __init of_unittest_dynamic(void)
> >
> > static int __init of_unittest_check_node_linkage(struct device_node *np)
> > {
> > - struct device_node *child;
> > + struct device_node *child __free(device_node) = NULL;
>
> In another thread[1], it seems that initializing to NULL is bad form
> according to the chief penguin. But as this is a refcounted pointer
> rather than an allocation, IDK?
I'm not sure the argument applies here. My understanding is it's not
really about the = NULL, but more about where the __free(device_node) is.
The ordering of that cleanup wrt to other similar clean up is to do it
in reverse order of declaration and in some cases that might cause trouble.
Here, the only way we could ensure the allocation was done at the right
point and we didn't have that __free before it was set, would be to add
variants of for_each_child_of_node() etc that did something like
#define for_each_child_of_node_scoped(parent, child) \
for (struct device_node *child __free(device_node) = \
of_get_next_child(parent, NULL); \
child != NULL; \
child = of_get_next_child(parent, child))
So that the child variable doesn't exist at all outside of the scope
of the loop.
I thought about proposing that style of solution but it felt more invasive
than a simple __free() annotation. I don't mind going that way though
if you prefer it.
Alternative is just to make sure the struct device_node * is always
declared just before the for loop and not bother setting it to NULL
(which is pointless anyway - it just felt fragile to not do so!)
Jonathan
>
> Rob
>
> [1] https://lore.kernel.org/all/289c4af00bcc46e83555dacbc76f56477126d645.camel@pengutronix.de
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/4] of: unittest: Use __free(device_node)
2024-01-17 17:01 ` Jonathan Cameron
@ 2024-01-17 17:09 ` Jonathan Cameron
2024-01-17 19:47 ` Rob Herring
1 sibling, 0 replies; 10+ messages in thread
From: Jonathan Cameron @ 2024-01-17 17:09 UTC (permalink / raw
To: Rob Herring
Cc: Jonathan Cameron, linux-iio, devicetree, Frank Rowand,
Julia Lawall, Nicolas Palix, Sumera Priyadarsini
On Wed, 17 Jan 2024 17:01:44 +0000
Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
> On Tue, 16 Jan 2024 12:26:49 -0600
> Rob Herring <robh@kernel.org> wrote:
>
> > On Sun, Jan 14, 2024 at 10:54 AM Jonathan Cameron <jic23@kernel.org> wrote:
> > >
> > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > >
> > > A simple example of the utility of this autocleanup approach to
> > > handling of_node_put()
> > >
> > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > ---
> > > drivers/of/unittest.c | 10 +++-------
> > > 1 file changed, 3 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
> > > index e9e90e96600e..b6d9edb831f0 100644
> > > --- a/drivers/of/unittest.c
> > > +++ b/drivers/of/unittest.c
> > > @@ -233,27 +233,23 @@ static void __init of_unittest_dynamic(void)
> > >
> > > static int __init of_unittest_check_node_linkage(struct device_node *np)
> > > {
> > > - struct device_node *child;
> > > + struct device_node *child __free(device_node) = NULL;
> >
> > In another thread[1], it seems that initializing to NULL is bad form
> > according to the chief penguin. But as this is a refcounted pointer
> > rather than an allocation, IDK?
>
> I'm not sure the argument applies here. My understanding is it's not
> really about the = NULL, but more about where the __free(device_node) is.
> The ordering of that cleanup wrt to other similar clean up is to do it
> in reverse order of declaration and in some cases that might cause trouble.
>
> Here, the only way we could ensure the allocation was done at the right
> point and we didn't have that __free before it was set, would be to add
> variants of for_each_child_of_node() etc that did something like
>
> #define for_each_child_of_node_scoped(parent, child) \
> for (struct device_node *child __free(device_node) = \
> of_get_next_child(parent, NULL); \
> child != NULL; \
> child = of_get_next_child(parent, child))
>
> So that the child variable doesn't exist at all outside of the scope
> of the loop.
>
> I thought about proposing that style of solution but it felt more invasive
> than a simple __free() annotation. I don't mind going that way though
> if you prefer it.
Note that if we did this I'd expect us to convert all current use case
and then we can probably do a global name change back to
for_each_child_of_node() as I can't see why we'd retain the other variant.
The rare (I think) case of breaking out of the loop whilst holding the
handle can be covered by pointer stealing anyway.
Jonathan
>
> Alternative is just to make sure the struct device_node * is always
> declared just before the for loop and not bother setting it to NULL
> (which is pointless anyway - it just felt fragile to not do so!)
>
> Jonathan
>
> >
> > Rob
> >
> > [1] https://lore.kernel.org/all/289c4af00bcc46e83555dacbc76f56477126d645.camel@pengutronix.de
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/4] of: unittest: Use __free(device_node)
2024-01-17 17:01 ` Jonathan Cameron
2024-01-17 17:09 ` Jonathan Cameron
@ 2024-01-17 19:47 ` Rob Herring
2024-01-17 20:13 ` Jonathan Cameron
1 sibling, 1 reply; 10+ messages in thread
From: Rob Herring @ 2024-01-17 19:47 UTC (permalink / raw
To: Jonathan Cameron
Cc: Jonathan Cameron, linux-iio, devicetree, Frank Rowand,
Julia Lawall, Nicolas Palix, Sumera Priyadarsini
On Wed, Jan 17, 2024 at 05:01:44PM +0000, Jonathan Cameron wrote:
> On Tue, 16 Jan 2024 12:26:49 -0600
> Rob Herring <robh@kernel.org> wrote:
>
> > On Sun, Jan 14, 2024 at 10:54 AM Jonathan Cameron <jic23@kernel.org> wrote:
> > >
> > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > >
> > > A simple example of the utility of this autocleanup approach to
> > > handling of_node_put()
> > >
> > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > ---
> > > drivers/of/unittest.c | 10 +++-------
> > > 1 file changed, 3 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
> > > index e9e90e96600e..b6d9edb831f0 100644
> > > --- a/drivers/of/unittest.c
> > > +++ b/drivers/of/unittest.c
> > > @@ -233,27 +233,23 @@ static void __init of_unittest_dynamic(void)
> > >
> > > static int __init of_unittest_check_node_linkage(struct device_node *np)
> > > {
> > > - struct device_node *child;
> > > + struct device_node *child __free(device_node) = NULL;
> >
> > In another thread[1], it seems that initializing to NULL is bad form
> > according to the chief penguin. But as this is a refcounted pointer
> > rather than an allocation, IDK?
>
> I'm not sure the argument applies here. My understanding is it's not
> really about the = NULL, but more about where the __free(device_node) is.
> The ordering of that cleanup wrt to other similar clean up is to do it
> in reverse order of declaration and in some cases that might cause trouble.
Rereading Linus' reply again, I think it is more that you see how
something is freed right where it is allocated, and the way to do that
is the allocation and declaration are together.
> Here, the only way we could ensure the allocation was done at the right
> point and we didn't have that __free before it was set, would be to add
> variants of for_each_child_of_node() etc that did something like
>
> #define for_each_child_of_node_scoped(parent, child) \
Note that you don't need child here except to define the name of child.
Otherwise, the variable name you need for the loop is implicit. OTOH,
defining a name which has no type defined anywhere in the user function
isn't great for readability either.
WRT the whole renaming, it might be better to keep 'scoped' in the name
so that it is obvious how child needs to be handled. Or is the compiler
smart enough to catch any case of doing it wrong?
> for (struct device_node *child __free(device_node) = \
> of_get_next_child(parent, NULL); \
> child != NULL; \
> child = of_get_next_child(parent, child))
>
> So that the child variable doesn't exist at all outside of the scope
> of the loop.
>
> I thought about proposing that style of solution but it felt more invasive
> than a simple __free() annotation. I don't mind going that way though
> if you prefer it.
My only preference currently is to not get yelled at. :)
> Alternative is just to make sure the struct device_node * is always
> declared just before the for loop and not bother setting it to NULL
> (which is pointless anyway - it just felt fragile to not do so!)
Would the compiler know to avoid invoking __free if exiting before the
variable is set?
Rob
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/4] of: unittest: Use __free(device_node)
2024-01-17 19:47 ` Rob Herring
@ 2024-01-17 20:13 ` Jonathan Cameron
0 siblings, 0 replies; 10+ messages in thread
From: Jonathan Cameron @ 2024-01-17 20:13 UTC (permalink / raw
To: Rob Herring
Cc: Jonathan Cameron, linux-iio, devicetree, Frank Rowand,
Julia Lawall, Nicolas Palix, Sumera Priyadarsini, Peter Zijlstra
On Wed, 17 Jan 2024 13:47:43 -0600
Rob Herring <robh@kernel.org> wrote:
> On Wed, Jan 17, 2024 at 05:01:44PM +0000, Jonathan Cameron wrote:
> > On Tue, 16 Jan 2024 12:26:49 -0600
> > Rob Herring <robh@kernel.org> wrote:
> >
> > > On Sun, Jan 14, 2024 at 10:54 AM Jonathan Cameron <jic23@kernel.org> wrote:
> > > >
> > > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > >
> > > > A simple example of the utility of this autocleanup approach to
> > > > handling of_node_put()
> > > >
> > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > ---
> > > > drivers/of/unittest.c | 10 +++-------
> > > > 1 file changed, 3 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
> > > > index e9e90e96600e..b6d9edb831f0 100644
> > > > --- a/drivers/of/unittest.c
> > > > +++ b/drivers/of/unittest.c
> > > > @@ -233,27 +233,23 @@ static void __init of_unittest_dynamic(void)
> > > >
> > > > static int __init of_unittest_check_node_linkage(struct device_node *np)
> > > > {
> > > > - struct device_node *child;
> > > > + struct device_node *child __free(device_node) = NULL;
> > >
> > > In another thread[1], it seems that initializing to NULL is bad form
> > > according to the chief penguin. But as this is a refcounted pointer
> > > rather than an allocation, IDK?
> >
> > I'm not sure the argument applies here. My understanding is it's not
> > really about the = NULL, but more about where the __free(device_node) is.
> > The ordering of that cleanup wrt to other similar clean up is to do it
> > in reverse order of declaration and in some cases that might cause trouble.
>
> Rereading Linus' reply again, I think it is more that you see how
> something is freed right where it is allocated, and the way to do that
> is the allocation and declaration are together.
Ok. Maybe both issues surfaced in different threads. Both are valid points
I've seen made about this stuff.
>
> > Here, the only way we could ensure the allocation was done at the right
> > point and we didn't have that __free before it was set, would be to add
> > variants of for_each_child_of_node() etc that did something like
> >
> > #define for_each_child_of_node_scoped(parent, child) \
>
> Note that you don't need child here except to define the name of child.
> Otherwise, the variable name you need for the loop is implicit.
Agreed.
> OTOH,
> defining a name which has no type defined anywhere in the user function
> isn't great for readability either.
Agreed it's not great to have the type missing, but I couldn't
think of a better option. So I think if we want these toys and to not
have it as a 2 part statement that's what we get.
>
> WRT the whole renaming, it might be better to keep 'scoped' in the name
> so that it is obvious how child needs to be handled. Or is the compiler
> smart enough to catch any case of doing it wrong?
I'm not sure we have variable name shadowing detection turned on.
If that was on we should see a warning on someone not realizing there
is a local scoped version. I'm fine with keeping the name.
>
> > for (struct device_node *child __free(device_node) = \
> > of_get_next_child(parent, NULL); \
> > child != NULL; \
> > child = of_get_next_child(parent, child))
> >
> > So that the child variable doesn't exist at all outside of the scope
> > of the loop.
> >
> > I thought about proposing that style of solution but it felt more invasive
> > than a simple __free() annotation. I don't mind going that way though
> > if you prefer it.
>
> My only preference currently is to not get yelled at. :)
Always nice ;)
>
> > Alternative is just to make sure the struct device_node * is always
> > declared just before the for loop and not bother setting it to NULL
> > (which is pointless anyway - it just felt fragile to not do so!)
>
> Would the compiler know to avoid invoking __free if exiting before the
> variable is set?
If it's declared just before the loop, there wouldn't be such a path
as the init condition of the loop sets it to a valid handle or NULL.
I looked at about the first half (alphabetical order) of the 127 files
with for_each_child_of_node().
Vast majority are easily converted. I think I counted 4 possible bugs
that need a closer look and a bunch were care would be needed (typically
steal the pointer for a return).
Happy to do a small set of them and see what it looks like if you think
that is a promising direction to try?
+CC Peter Zijlstra as person most likely to have seen similar discussions
or provide input based on his own cleanup.h conversions.
Jonathan
>
> Rob
>
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2024-01-17 20:14 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-01-14 16:53 [PATCH 0/4] of: Automate handling of of_node_put() Jonathan Cameron
2024-01-14 16:53 ` [PATCH 1/4] of: Add cleanup.h based auto release via __free(device_node) markings Jonathan Cameron
2024-01-14 16:53 ` [PATCH 2/4] of: unittest: Use __free(device_node) Jonathan Cameron
2024-01-16 18:26 ` Rob Herring
2024-01-17 17:01 ` Jonathan Cameron
2024-01-17 17:09 ` Jonathan Cameron
2024-01-17 19:47 ` Rob Herring
2024-01-17 20:13 ` Jonathan Cameron
2024-01-14 16:53 ` [PATCH 3/4] iio: adc: fsl-imx25-gcq: " Jonathan Cameron
2024-01-14 16:53 ` [PATCH 4/4] iio: adc: rcar-gyroadc: use __free(device_node) Jonathan Cameron
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).