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

[Jonathan Cameron]

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", &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