On Sun, 4 Feb 2024 22:34:25 +0100 (CET)
Julia Lawall <julia.lawall@xxxxxxxx> wrote:
> On Sun, 4 Feb 2024, Jonathan Cameron wrote:
>
> > On Wed, 31 Jan 2024 22:38:21 +0100 (CET)
> > Julia Lawall <julia.lawall@xxxxxxxx> 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
