| Message ID | 20240128160542.178315-1-jic23@kernel.org |
|---|---|
| Headers | show |
| Series | of: automate of_node_put() - new approach to loops. | expand |
On Sun, Jan 28, 2024 at 04:05:37PM +0000, Jonathan Cameron wrote: > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > +CC includes peopleinterested in property.h equivalents to minimize > duplication of discussion. Outcome of this discussion will affect: > https://lore.kernel.org/all/20240114172009.179893-1-jic23@kernel.org/ > [PATCH 00/13] device property / IIO: Use cleanup.h magic for fwnode_handle_put() handling. > > In discussion of previous approach with Rob Herring we talked about various > ways to avoid a disconnect between the declaration of the __free(device_node) > and the first non NULL assignment. Making this connection clear is useful for 2 > reasons: > 1) Avoids out of order cleanup with respect to other cleanup.h usage. > 2) Avoids disconnect between how cleanup is to be done and how the reference > was acquired in the first place. > > https://lore.kernel.org/all/20240117194743.GA2888190-robh@kernel.org/ > > The options we discussed are: > > 1) Ignore this issue and merge original set. > > 2) Always put the declaration just before the for loop and don't set it NULL. > > { > int ret; > > ret = ... and other fun code. > > struct device_node *child __free(device_node); > for_each_child_of_node(np, child) { > } > } > > This works but careful review is needed to ensure that this unusual pattern is > followed. We don't set it to NULL as the loop will do that anyway if there are > no child nodes, or the loop finishes without an early break or return. > > 3) Introduced the pointer to auto put device_node only within the > for loop scope. > > +#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; \ Just child; \ > + child = of_get_next_available_child(parent, child)) > + > > This series is presenting option 3. I only implemented this loop out of > all the similar ones and it is only compile tested. > > Disadvantage Rob raised is that it isn't obvious this macro will instantiate > a struct device_node *child. I can't see a way around that other than option 2 > above, but all suggestions welcome. Note that if a conversion leaves an > 'external' struct device_node *child variable, in many cases the compiler > will catch that as an unused variable. We don't currently run shaddow > variable detection in normal kernel builds, but that could also be used > to catch such bugs.
> > 3) Introduced the pointer to auto put device_node only within the > > for loop scope. > > > > +#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; \ > > Just > > child; Agreed that's the same, but was thinking to follow local style. I don't feel strongly though so fine with dropping the != NULL > > > + child = of_get_next_available_child(parent, child)) > > + > > > > This series is presenting option 3. I only implemented this loop out of > > all the similar ones and it is only compile tested. > > > > Disadvantage Rob raised is that it isn't obvious this macro will instantiate > > a struct device_node *child. I can't see a way around that other than option 2 > > above, but all suggestions welcome. Note that if a conversion leaves an > > 'external' struct device_node *child variable, in many cases the compiler > > will catch that as an unused variable. We don't currently run shaddow > > variable detection in normal kernel builds, but that could also be used > > to catch such bugs. >
On Wed, 31 Jan 2024 22:38:21 +0100 (CET) Julia Lawall <julia.lawall@inria.fr> wrote: > Here are some loop cases. The semantic patch is as follows: > > #spatch --allow-inconsistent-paths > > @@ > expression node; > identifier child; > symbol drop_me; > iterator name for_each_child_of_node; > @@ > > for_each_child_of_node(node,child) { > ... > + of_node_put(drop_me, child); > } > > @@ > expression node; > identifier child; > symbol drop_me; > iterator name for_each_child_of_node, for_each_child_of_node_scoped; > identifier L; > @@ > > - struct device_node *child; > ... when != child > -for_each_child_of_node > +for_each_child_of_node_scoped > (node,child) { > ... when strict > ( > - { > - of_node_put(child); > return ...; > - } > | > - { > - of_node_put(child); > goto L; > - } > | > - { > - of_node_put(child); > break; > - } > | > continue; > | > - of_node_put(child); > return ...; > | > - of_node_put(child); > break; > | > - of_node_put(drop_me, child); > ) > } > ... when != child > > @@ > expression child; > @@ > > - of_node_put(drop_me, child); > > ------------------------------- > > This is quite conservative, in that it requires the only use of the child > variable to be in a single for_each_child_of_node loop at top level. > > The drop_me thing is a hack to be able to refer to the bottom of the loop > in the same way as of_node_puts in front of returns etc are referenced. > > This works fine when multiple device_node variables are declared at once. > > The result is below. > Very nice! One issue is that Rob is keen that we also take this opportunity to evaluate if the _available_ form is the more appropriate one. Given that access either no defined "status" in the child node or it being set to "okay" it is what should be used in the vast majority of cases. For reference, the property.h version only uses the available form. So I think we'll need some hand checking of each case but for vast majority it will be very straight forward. One question is whether it is worth the scoped loops in cases where there isn't a patch where we break out of or return from the loop before it finishes. Do we put them in as a defensive measure? Sometimes we are going to want to combine this refactor with some of the ones your previous script caught in a single patch given it's roughly the same sort of change. > julia > > diff -u -p a/drivers/of/unittest.c b/drivers/of/unittest.c > --- a/drivers/of/unittest.c > +++ b/drivers/of/unittest.c > @@ -2789,7 +2789,7 @@ static int unittest_i2c_mux_probe(struct > int i, nchans; > struct device *dev = &client->dev; > struct i2c_adapter *adap = client->adapter; > - struct device_node *np = client->dev.of_node, *child; > + struct device_node *np = client->dev.of_node; > struct i2c_mux_core *muxc; > u32 reg, max_reg; > > @@ -2801,7 +2801,7 @@ static int unittest_i2c_mux_probe(struct > } > > max_reg = (u32)-1; > - for_each_child_of_node(np, child) { > + for_each_child_of_node_scoped(np, child) { This was a case I left alone in the original series because the auto cleanup doesn't end up doing anything in any paths. > if (of_property_read_u32(child, "reg", ®)) > continue; > if (max_reg == (u32)-1 || reg > max_reg) > > diff -u -p a/drivers/regulator/scmi-regulator.c b/drivers/regulator/scmi-regulator.c > --- a/drivers/regulator/scmi-regulator.c > +++ b/drivers/regulator/scmi-regulator.c > @@ -297,7 +297,7 @@ static int process_scmi_regulator_of_nod > static int scmi_regulator_probe(struct scmi_device *sdev) > { > int d, ret, num_doms; > - struct device_node *np, *child; > + struct device_node *np; > const struct scmi_handle *handle = sdev->handle; > struct scmi_regulator_info *rinfo; > struct scmi_protocol_handle *ph; > @@ -341,13 +341,11 @@ static int scmi_regulator_probe(struct s > */ > of_node_get(handle->dev->of_node); > np = of_find_node_by_name(handle->dev->of_node, "regulators"); > - for_each_child_of_node(np, child) { > + for_each_child_of_node_scoped(np, child) { > ret = process_scmi_regulator_of_node(sdev, ph, child, rinfo); > /* abort on any mem issue */ > - if (ret == -ENOMEM) { > - of_node_put(child); > + if (ret == -ENOMEM) > return ret; > - } Current code leaks np in this path :( > } > of_node_put(np); > /* > diff -u -p a/drivers/crypto/nx/nx-common-powernv.c b/drivers/crypto/nx/nx-common-powernv.c > --- a/drivers/crypto/nx/nx-common-powernv.c > +++ b/drivers/crypto/nx/nx-common-powernv.c > @@ -907,7 +907,6 @@ static int __init nx_powernv_probe_vas(s > { > int chip_id, vasid, ret = 0; > int ct_842 = 0, ct_gzip = 0; > - struct device_node *dn; > > chip_id = of_get_ibm_chip_id(pn); > if (chip_id < 0) { > @@ -921,7 +920,7 @@ static int __init nx_powernv_probe_vas(s > return -EINVAL; > } > > - for_each_child_of_node(pn, dn) { > + for_each_child_of_node_scoped(pn, dn) { > ret = find_nx_device_tree(dn, chip_id, vasid, NX_CT_842, > "ibm,p9-nx-842", &ct_842); > > @@ -929,10 +928,8 @@ static int __init nx_powernv_probe_vas(s > ret = find_nx_device_tree(dn, chip_id, vasid, > NX_CT_GZIP, "ibm,p9-nx-gzip", &ct_gzip); The handling in here is odd (buggy?). There is an of_node_put() in the failure path inside find_nx_device_tree() as well as out here. > > - if (ret) { > - of_node_put(dn); > + if (ret) > return ret; > - } > } > > if (!ct_842 || !ct_gzip) { I've glanced at a few of the others and some of them are hard. This refactor is fine, but the other device_node handling often is complex and I think fragile. So definitely room for improvement! Jonathan
On Sun, 4 Feb 2024, Jonathan Cameron wrote: > On Wed, 31 Jan 2024 22:38:21 +0100 (CET) > Julia Lawall <julia.lawall@inria.fr> wrote: > > > Here are some loop cases. The semantic patch is as follows: > > > > #spatch --allow-inconsistent-paths > > > > @@ > > expression node; > > identifier child; > > symbol drop_me; > > iterator name for_each_child_of_node; > > @@ > > > > for_each_child_of_node(node,child) { > > ... > > + of_node_put(drop_me, child); > > } > > > > @@ > > expression node; > > identifier child; > > symbol drop_me; > > iterator name for_each_child_of_node, for_each_child_of_node_scoped; > > identifier L; > > @@ > > > > - struct device_node *child; > > ... when != child > > -for_each_child_of_node > > +for_each_child_of_node_scoped > > (node,child) { > > ... when strict > > ( > > - { > > - of_node_put(child); > > return ...; > > - } > > | > > - { > > - of_node_put(child); > > goto L; > > - } > > | > > - { > > - of_node_put(child); > > break; > > - } > > | > > continue; > > | > > - of_node_put(child); > > return ...; > > | > > - of_node_put(child); > > break; > > | > > - of_node_put(drop_me, child); > > ) > > } > > ... when != child > > > > @@ > > expression child; > > @@ > > > > - of_node_put(drop_me, child); > > > > ------------------------------- > > > > This is quite conservative, in that it requires the only use of the child > > variable to be in a single for_each_child_of_node loop at top level. > > > > The drop_me thing is a hack to be able to refer to the bottom of the loop > > in the same way as of_node_puts in front of returns etc are referenced. > > > > This works fine when multiple device_node variables are declared at once. > > > > The result is below. > > > Very nice! > > One issue is that Rob is keen that we also take this opportunity to > evaluate if the _available_ form is the more appropriate one. > > Given that access either no defined "status" in the child node or > it being set to "okay" it is what should be used in the vast majority of > cases. > > For reference, the property.h version only uses the available form. > > So I think we'll need some hand checking of each case but for vast majority > it will be very straight forward. I'm not sure to follow this. If the check is straightforward, perhaps it can be integrated into the rule? But I'm not sure what to check for. > One question is whether it is worth the scoped loops in cases > where there isn't a patch where we break out of or return from the loop > before it finishes. Do we put them in as a defensive measure? I wondered about this also. My thought was that it is better to be uniform. And maybe a break would be added later. > Sometimes we are going to want to combine this refactor with > some of the ones your previous script caught in a single patch given > it's roughly the same sort of change. Agreed. Some blocks of code should indeed become much simpler. > > > > julia > > > > diff -u -p a/drivers/of/unittest.c b/drivers/of/unittest.c > > --- a/drivers/of/unittest.c > > +++ b/drivers/of/unittest.c > > @@ -2789,7 +2789,7 @@ static int unittest_i2c_mux_probe(struct > > int i, nchans; > > struct device *dev = &client->dev; > > struct i2c_adapter *adap = client->adapter; > > - struct device_node *np = client->dev.of_node, *child; > > + struct device_node *np = client->dev.of_node; > > struct i2c_mux_core *muxc; > > u32 reg, max_reg; > > > > @@ -2801,7 +2801,7 @@ static int unittest_i2c_mux_probe(struct > > } > > > > max_reg = (u32)-1; > > - for_each_child_of_node(np, child) { > > + for_each_child_of_node_scoped(np, child) { > > This was a case I left alone in the original series because the auto > cleanup doesn't end up doing anything in any paths. > > > if (of_property_read_u32(child, "reg", ®)) > > continue; > > if (max_reg == (u32)-1 || reg > max_reg) > > > > > > > diff -u -p a/drivers/regulator/scmi-regulator.c b/drivers/regulator/scmi-regulator.c > > --- a/drivers/regulator/scmi-regulator.c > > +++ b/drivers/regulator/scmi-regulator.c > > @@ -297,7 +297,7 @@ static int process_scmi_regulator_of_nod > > static int scmi_regulator_probe(struct scmi_device *sdev) > > { > > int d, ret, num_doms; > > - struct device_node *np, *child; > > + struct device_node *np; > > const struct scmi_handle *handle = sdev->handle; > > struct scmi_regulator_info *rinfo; > > struct scmi_protocol_handle *ph; > > @@ -341,13 +341,11 @@ static int scmi_regulator_probe(struct s > > */ > > of_node_get(handle->dev->of_node); > > np = of_find_node_by_name(handle->dev->of_node, "regulators"); > > - for_each_child_of_node(np, child) { > > + for_each_child_of_node_scoped(np, child) { > > ret = process_scmi_regulator_of_node(sdev, ph, child, rinfo); > > /* abort on any mem issue */ > > - if (ret == -ENOMEM) { > > - of_node_put(child); > > + if (ret == -ENOMEM) > > return ret; > > - } > Current code leaks np in this path :( > > > } > > of_node_put(np); > > /* > > > > diff -u -p a/drivers/crypto/nx/nx-common-powernv.c b/drivers/crypto/nx/nx-common-powernv.c > > --- a/drivers/crypto/nx/nx-common-powernv.c > > +++ b/drivers/crypto/nx/nx-common-powernv.c > > @@ -907,7 +907,6 @@ static int __init nx_powernv_probe_vas(s > > { > > int chip_id, vasid, ret = 0; > > int ct_842 = 0, ct_gzip = 0; > > - struct device_node *dn; > > > > chip_id = of_get_ibm_chip_id(pn); > > if (chip_id < 0) { > > @@ -921,7 +920,7 @@ static int __init nx_powernv_probe_vas(s > > return -EINVAL; > > } > > > > - for_each_child_of_node(pn, dn) { > > + for_each_child_of_node_scoped(pn, dn) { > > ret = find_nx_device_tree(dn, chip_id, vasid, NX_CT_842, > > "ibm,p9-nx-842", &ct_842); > > > > @@ -929,10 +928,8 @@ static int __init nx_powernv_probe_vas(s > > ret = find_nx_device_tree(dn, chip_id, vasid, > > NX_CT_GZIP, "ibm,p9-nx-gzip", &ct_gzip); > The handling in here is odd (buggy?). There is an of_node_put() > in the failure path inside find_nx_device_tree() as well as out here. > > > > - if (ret) { > > - of_node_put(dn); > > + if (ret) > > return ret; > > - } > > } > > > > if (!ct_842 || !ct_gzip) { > > I've glanced at a few of the others and some of them are hard. > This refactor is fine, but the other device_node handling often > is complex and I think fragile. So definitely room for improvement! I agree with all the above comments. julia
On Sun, 4 Feb 2024 22:34:25 +0100 (CET) Julia Lawall <julia.lawall@inria.fr> wrote: > On Sun, 4 Feb 2024, Jonathan Cameron wrote: > > > On Wed, 31 Jan 2024 22:38:21 +0100 (CET) > > Julia Lawall <julia.lawall@inria.fr> wrote: > > > > > Here are some loop cases. The semantic patch is as follows: > > > > > > #spatch --allow-inconsistent-paths > > > > > > @@ > > > expression node; > > > identifier child; > > > symbol drop_me; > > > iterator name for_each_child_of_node; > > > @@ > > > > > > for_each_child_of_node(node,child) { > > > ... > > > + of_node_put(drop_me, child); > > > } > > > > > > @@ > > > expression node; > > > identifier child; > > > symbol drop_me; > > > iterator name for_each_child_of_node, for_each_child_of_node_scoped; > > > identifier L; > > > @@ > > > > > > - struct device_node *child; > > > ... when != child > > > -for_each_child_of_node > > > +for_each_child_of_node_scoped > > > (node,child) { > > > ... when strict > > > ( > > > - { > > > - of_node_put(child); > > > return ...; > > > - } > > > | > > > - { > > > - of_node_put(child); > > > goto L; > > > - } > > > | > > > - { > > > - of_node_put(child); > > > break; > > > - } > > > | > > > continue; > > > | > > > - of_node_put(child); > > > return ...; > > > | > > > - of_node_put(child); > > > break; > > > | > > > - of_node_put(drop_me, child); > > > ) > > > } > > > ... when != child > > > > > > @@ > > > expression child; > > > @@ > > > > > > - of_node_put(drop_me, child); > > > > > > ------------------------------- > > > > > > This is quite conservative, in that it requires the only use of the child > > > variable to be in a single for_each_child_of_node loop at top level. > > > > > > The drop_me thing is a hack to be able to refer to the bottom of the loop > > > in the same way as of_node_puts in front of returns etc are referenced. > > > > > > This works fine when multiple device_node variables are declared at once. > > > > > > The result is below. > > > > > Very nice! > > > > One issue is that Rob is keen that we also take this opportunity to > > evaluate if the _available_ form is the more appropriate one. > > > > Given that access either no defined "status" in the child node or > > it being set to "okay" it is what should be used in the vast majority of > > cases. > > > > For reference, the property.h version only uses the available form. > > > > So I think we'll need some hand checking of each case but for vast majority > > it will be very straight forward. > > I'm not sure to follow this. If the check is straightforward, perhaps it > can be integrated into the rule? But I'm not sure what to check for. I don't think it will be easy. Perhaps Rob can help on when the _available_ case is the wrong one? Is this ever a characteristic of the binding. I guess in some cases it might be a characteristic of the binding? > > > One question is whether it is worth the scoped loops in cases > > where there isn't a patch where we break out of or return from the loop > > before it finishes. Do we put them in as a defensive measure? > > I wondered about this also. My thought was that it is better to be > uniform. And maybe a break would be added later. Rob and other DT folk, what do you think on this? Shall we convert cases where there isn't currently a path in which the autocleanup receives anything other than a NULL. i.e. for_each_available_of_child_node_scoped(x, y) { no breaks or returns form in here. } on basis it's not 'wrong' and is defensive against future changes that would require manual cleanup or the scoped for loop. > > > Sometimes we are going to want to combine this refactor with > > some of the ones your previous script caught in a single patch given > > it's roughly the same sort of change. > > Agreed. Some blocks of code should indeed become much simpler. > > > > > > > > julia > > > > > > diff -u -p a/drivers/of/unittest.c b/drivers/of/unittest.c > > > --- a/drivers/of/unittest.c > > > +++ b/drivers/of/unittest.c > > > @@ -2789,7 +2789,7 @@ static int unittest_i2c_mux_probe(struct > > > int i, nchans; > > > struct device *dev = &client->dev; > > > struct i2c_adapter *adap = client->adapter; > > > - struct device_node *np = client->dev.of_node, *child; > > > + struct device_node *np = client->dev.of_node; > > > struct i2c_mux_core *muxc; > > > u32 reg, max_reg; > > > > > > @@ -2801,7 +2801,7 @@ static int unittest_i2c_mux_probe(struct > > > } > > > > > > max_reg = (u32)-1; > > > - for_each_child_of_node(np, child) { > > > + for_each_child_of_node_scoped(np, child) { > > > > This was a case I left alone in the original series because the auto > > cleanup doesn't end up doing anything in any paths. > > > > > if (of_property_read_u32(child, "reg", ®)) > > > continue; > > > if (max_reg == (u32)-1 || reg > max_reg) > > > > > > > > > > > > diff -u -p a/drivers/regulator/scmi-regulator.c b/drivers/regulator/scmi-regulator.c > > > --- a/drivers/regulator/scmi-regulator.c > > > +++ b/drivers/regulator/scmi-regulator.c > > > @@ -297,7 +297,7 @@ static int process_scmi_regulator_of_nod > > > static int scmi_regulator_probe(struct scmi_device *sdev) > > > { > > > int d, ret, num_doms; > > > - struct device_node *np, *child; > > > + struct device_node *np; > > > const struct scmi_handle *handle = sdev->handle; > > > struct scmi_regulator_info *rinfo; > > > struct scmi_protocol_handle *ph; > > > @@ -341,13 +341,11 @@ static int scmi_regulator_probe(struct s > > > */ > > > of_node_get(handle->dev->of_node); > > > np = of_find_node_by_name(handle->dev->of_node, "regulators"); > > > - for_each_child_of_node(np, child) { > > > + for_each_child_of_node_scoped(np, child) { > > > ret = process_scmi_regulator_of_node(sdev, ph, child, rinfo); > > > /* abort on any mem issue */ > > > - if (ret == -ENOMEM) { > > > - of_node_put(child); > > > + if (ret == -ENOMEM) > > > return ret; > > > - } > > Current code leaks np in this path :( > > > > > } > > > of_node_put(np); > > > /* > > > > > > > diff -u -p a/drivers/crypto/nx/nx-common-powernv.c b/drivers/crypto/nx/nx-common-powernv.c > > > --- a/drivers/crypto/nx/nx-common-powernv.c > > > +++ b/drivers/crypto/nx/nx-common-powernv.c > > > @@ -907,7 +907,6 @@ static int __init nx_powernv_probe_vas(s > > > { > > > int chip_id, vasid, ret = 0; > > > int ct_842 = 0, ct_gzip = 0; > > > - struct device_node *dn; > > > > > > chip_id = of_get_ibm_chip_id(pn); > > > if (chip_id < 0) { > > > @@ -921,7 +920,7 @@ static int __init nx_powernv_probe_vas(s > > > return -EINVAL; > > > } > > > > > > - for_each_child_of_node(pn, dn) { > > > + for_each_child_of_node_scoped(pn, dn) { > > > ret = find_nx_device_tree(dn, chip_id, vasid, NX_CT_842, > > > "ibm,p9-nx-842", &ct_842); > > > > > > @@ -929,10 +928,8 @@ static int __init nx_powernv_probe_vas(s > > > ret = find_nx_device_tree(dn, chip_id, vasid, > > > NX_CT_GZIP, "ibm,p9-nx-gzip", &ct_gzip); > > The handling in here is odd (buggy?). There is an of_node_put() > > in the failure path inside find_nx_device_tree() as well as out here. > > > > > > - if (ret) { > > > - of_node_put(dn); > > > + if (ret) > > > return ret; > > > - } > > > } > > > > > > if (!ct_842 || !ct_gzip) { > > > > I've glanced at a few of the others and some of them are hard. > > This refactor is fine, but the other device_node handling often > > is complex and I think fragile. So definitely room for improvement! > > I agree with all the above comments. > > julia
From: Jonathan Cameron <Jonathan.Cameron@huawei.com> +CC includes peopleinterested in property.h equivalents to minimize duplication of discussion. Outcome of this discussion will affect: https://lore.kernel.org/all/20240114172009.179893-1-jic23@kernel.org/ [PATCH 00/13] device property / IIO: Use cleanup.h magic for fwnode_handle_put() handling. In discussion of previous approach with Rob Herring we talked about various ways to avoid a disconnect between the declaration of the __free(device_node) and the first non NULL assignment. Making this connection clear is useful for 2 reasons: 1) Avoids out of order cleanup with respect to other cleanup.h usage. 2) Avoids disconnect between how cleanup is to be done and how the reference was acquired in the first place. https://lore.kernel.org/all/20240117194743.GA2888190-robh@kernel.org/ The options we discussed are: 1) Ignore this issue and merge original set. 2) Always put the declaration just before the for loop and don't set it NULL. { int ret; ret = ... and other fun code. struct device_node *child __free(device_node); for_each_child_of_node(np, child) { } } This works but careful review is needed to ensure that this unusual pattern is followed. We don't set it to NULL as the loop will do that anyway if there are no child nodes, or the loop finishes without an early break or return. 3) Introduced the pointer to auto put device_node only within the for loop scope. +#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_available_child(parent, child)) + This series is presenting option 3. I only implemented this loop out of all the similar ones and it is only compile tested. Disadvantage Rob raised is that it isn't obvious this macro will instantiate a struct device_node *child. I can't see a way around that other than option 2 above, but all suggestions welcome. Note that if a conversion leaves an 'external' struct device_node *child variable, in many cases the compiler will catch that as an unused variable. We don't currently run shaddow variable detection in normal kernel builds, but that could also be used to catch such bugs. All comments welcome. Jonathan Cameron (5): of: Add cleanup.h based auto release via __free(device_node) markings. of: Introduce for_each_child_of_node_scoped() to automate of_node_put() handling of: unittest: Use __free(device_node) iio: adc: fsl-imx25-gcq: Use for_each_child_node_scoped() iio: adc: rcar-gyroadc: use for_each_child_node_scoped() drivers/iio/adc/fsl-imx25-gcq.c | 13 +++---------- drivers/iio/adc/rcar-gyroadc.c | 21 ++++++--------------- drivers/of/unittest.c | 11 +++-------- include/linux/of.h | 8 ++++++++ 4 files changed, 20 insertions(+), 33 deletions(-)