by Julia Lawall

[permalink] [raw]

Subject: Re: [RFC PATCH 0/5] of: automate of_node_put() - new approach to loops.

On Sun, 4 Feb 2024, Jonathan Cameron wrote:

> On Wed, 31 Jan 2024 22:38:21 +0100 (CET)
> Julia Lawall <[email protected]> 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", &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