Received-SPF: pass (google.com: domain of linux-kernel+bounces-180382-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
References: <20240515202915.16214-3-shresthprasad7@gmail.com>
In-Reply-To: <20240515202915.16214-3-shresthprasad7@gmail.com>
From: Saravana Kannan <saravanak@google.com>
Date: Wed, 15 May 2024 23:22:42 +0200
Message-ID: <CAGETcx9Bq2=4OiqZndWaW+ZVcDv5sXOEpd2YHzZ_SYBxKf54dw@mail.gmail.com>
Subject: Re: [PATCH][next] of: property: Remove calls to of_node_put
To: Shresth Prasad <shresthprasad7@gmail.com>
Cc: robh@kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, 
	skhan@linuxfoundation.org, javier.carrasco.cruz@gmail.com, 
	julia.lawall@inria.fr
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, May 15, 2024 at 10:35=E2=80=AFPM Shresth Prasad
<shresthprasad7@gmail.com> wrote:
>
> Add __free cleanup handler to some variable initialisations, which
> ensures that the resource is freed as soon as the variable goes out of
> scope. Thus removing the need to manually free up the resource using
> of_node_put.
>
> Suggested-by: Julia Lawall <julia.lawall@inria.fr>
> Signed-off-by: Shresth Prasad <shresthprasad7@gmail.com>
> ---
> I had submitted a similar patch a couple weeks ago addressing the same
> issue, but as it turns out I wasn't thorough enough and had left a couple
> instances.
>
> I hope this isn't too big an issue.

I didn't see the previous patch from a couple weeks ago, but this
patch looks good.

Reviewed-by: Saravana Kannan <saravanak@google.com>

-Saravana

> ---
>  drivers/of/property.c | 27 +++++++++++----------------
>  1 file changed, 11 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index 17b294e16c56..96a74f6a8d64 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -773,15 +773,14 @@ EXPORT_SYMBOL(of_graph_get_port_parent);
>  struct device_node *of_graph_get_remote_port_parent(
>                                const struct device_node *node)
>  {
> -       struct device_node *np, *pp;
> +       struct device_node *pp;
>
>         /* Get remote endpoint node. */
> -       np =3D of_graph_get_remote_endpoint(node);
> +       struct device_node *np __free(device_node) =3D
> +                           of_graph_get_remote_endpoint(node);
>
>         pp =3D of_graph_get_port_parent(np);
>
> -       of_node_put(np);
> -
>         return pp;
>  }
>  EXPORT_SYMBOL(of_graph_get_remote_port_parent);
> @@ -835,17 +834,18 @@ EXPORT_SYMBOL(of_graph_get_endpoint_count);
>  struct device_node *of_graph_get_remote_node(const struct device_node *n=
ode,
>                                              u32 port, u32 endpoint)
>  {
> -       struct device_node *endpoint_node, *remote;
> +       struct device_node *endpoint_node __free(device_node) =3D
> +                           of_graph_get_endpoint_by_regs(node, port, end=
point);
> +
> +       struct device_node *remote __free(device_node) =3D
> +                           of_graph_get_remote_port_parent(endpoint_node=
);
>
> -       endpoint_node =3D of_graph_get_endpoint_by_regs(node, port, endpo=
int);
>         if (!endpoint_node) {
>                 pr_debug("no valid endpoint (%d, %d) for node %pOF\n",
>                          port, endpoint, node);
>                 return NULL;
>         }
>
> -       remote =3D of_graph_get_remote_port_parent(endpoint_node);
> -       of_node_put(endpoint_node);
>         if (!remote) {
>                 pr_debug("no valid remote node\n");
>                 return NULL;
> @@ -853,7 +853,6 @@ struct device_node *of_graph_get_remote_node(const st=
ruct device_node *node,
>
>         if (!of_device_is_available(remote)) {
>                 pr_debug("not available for remote node\n");
> -               of_node_put(remote);
>                 return NULL;
>         }
>
> @@ -1064,19 +1063,15 @@ static void of_link_to_phandle(struct device_node=
 *con_np,
>                               struct device_node *sup_np,
>                               u8 flags)
>  {
> -       struct device_node *tmp_np =3D of_node_get(sup_np);
> +       struct device_node *tmp_np __free(device_node) =3D of_node_get(su=
p_np);
>
>         /* Check that sup_np and its ancestors are available. */
>         while (tmp_np) {
> -               if (of_fwnode_handle(tmp_np)->dev) {
> -                       of_node_put(tmp_np);
> +               if (of_fwnode_handle(tmp_np)->dev)
>                         break;
> -               }
>
> -               if (!of_device_is_available(tmp_np)) {
> -                       of_node_put(tmp_np);
> +               if (!of_device_is_available(tmp_np))
>                         return;
> -               }
>
>                 tmp_np =3D of_get_next_parent(tmp_np);
>         }
> --
> 2.45.1
>