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

[Jonathan Cameron <jic23@kernel.org>]

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