From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
To: Julia Lawall <julia.lawall@inria.fr>,
"Rafael J . Wysocki" <rafael@kernel.org>
Cc: "Jonathan Cameron" <jic23@kernel.org>,
linux-iio@vger.kernel.org, "Rob Herring" <robh@kernel.org>,
"Frank Rowand" <frowand.list@gmail.com>,
linux-kernel@vger.kernel.org,
"Nicolas Palix" <nicolas.palix@imag.fr>,
"Sumera Priyadarsini" <sylphrenadin@gmail.com>,
"Len Brown" <lenb@kernel.org>,
linux-acpi@vger.kernel.org,
"Andy Shevchenko" <andriy.shevchenko@linux.intel.com>,
"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
"Nuno Sá" <nuno.sa@analog.com>
Subject: Re: [RFC PATCH 0/5] of: automate of_node_put() - new approach to loops.
Date: Mon, 5 Feb 2024 09:27:26 +0000 [thread overview]
Message-ID: <20240205092726.00002eb0@Huawei.com> (raw)
In-Reply-To: <alpine.DEB.2.22.394.2402042224280.3137@hadrien>
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
next prev parent reply other threads:[~2024-02-05 9:27 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-01-28 16:05 [RFC PATCH 0/5] of: automate of_node_put() - new approach to loops Jonathan Cameron
2024-01-28 16:05 ` [RFC PATCH 1/5] of: Add cleanup.h based auto release via __free(device_node) markings Jonathan Cameron
2024-01-28 16:05 ` [RFC PATCH 2/5] of: Introduce for_each_child_of_node_scoped() to automate of_node_put() handling Jonathan Cameron
2024-01-28 21:11 ` David Lechner
2024-01-29 6:54 ` Julia Lawall
2024-01-29 11:44 ` Jonathan Cameron
2024-01-31 23:51 ` Rob Herring
2024-02-01 15:17 ` Jonathan Cameron
2024-02-04 19:56 ` Jonathan Cameron
2024-02-04 20:52 ` Jonathan Cameron
2024-01-28 16:05 ` [RFC PATCH 3/5] of: unittest: Use for_each_child_of_node_scoped() Jonathan Cameron
2024-01-28 16:05 ` [RFC PATCH 4/5] iio: adc: fsl-imx25-gcq: Use for_each_child_node_scoped() Jonathan Cameron
2024-01-28 16:05 ` [RFC PATCH 5/5] iio: adc: rcar-gyroadc: use for_each_child_node_scoped() Jonathan Cameron
2024-01-28 18:06 ` [RFC PATCH 0/5] of: automate of_node_put() - new approach to loops Julia Lawall
2024-01-29 11:42 ` Jonathan Cameron
2024-01-29 14:02 ` Julia Lawall
2024-01-29 19:52 ` Jonathan Cameron
2024-01-29 20:29 ` Julia Lawall
2024-01-30 9:38 ` Jonathan Cameron
2024-01-30 10:26 ` Julia Lawall
2024-01-31 21:38 ` Julia Lawall
2024-02-04 21:08 ` Jonathan Cameron
2024-02-04 21:34 ` Julia Lawall
2024-02-05 9:27 ` Jonathan Cameron [this message]
2024-02-01 11:20 ` Andy Shevchenko
2024-02-01 15:21 ` Jonathan Cameron
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240205092726.00002eb0@Huawei.com \
--to=jonathan.cameron@huawei.com \
--cc=andriy.shevchenko@linux.intel.com \
--cc=frowand.list@gmail.com \
--cc=gregkh@linuxfoundation.org \
--cc=jic23@kernel.org \
--cc=julia.lawall@inria.fr \
--cc=lenb@kernel.org \
--cc=linux-acpi@vger.kernel.org \
--cc=linux-iio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=nicolas.palix@imag.fr \
--cc=nuno.sa@analog.com \
--cc=rafael@kernel.org \
--cc=robh@kernel.org \
--cc=sylphrenadin@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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 NNTP newsgroup(s).