Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 14 Jul 2023 09:06:23 +0100
Message-ID: <86mszyvrwg.wl-maz@kernel.org>
From:   Marc Zyngier <maz@kernel.org>
To:     liubin001@208suo.com
Cc:     tglx@linutronix.de, linux-kernel@vger.kernel.org,
        Geert Uytterhoeven <geert@linux-m68k.org>
Subject: Re: Fwd: [PATCH] irqchip:Use the "put_device" function to release the memory after calling the of_find_device_by_node function.
In-Reply-To: <66d48e6ce0db4f89737172a2963195a9@208suo.com>
References: <tencent_C69953B5D14B406504315830D8DE7112EC0A@qq.com>
        <66d48e6ce0db4f89737172a2963195a9@208suo.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue)
 FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/28.2
 (aarch64-unknown-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

Please don't forward emails like this. If you've missed a recipient,
repost it.

+Geert for the Renesas stuff.

On Fri, 14 Jul 2023 04:43:28 +0100,
liubin001@208suo.com wrote:
> 
> Write the put_device function before return

Why? What is wrong with the current code? What does it affect?

> 
> Signed-off-by: Liu Bin <liubin001@208suo.com>
> ---
>  drivers/irqchip/irq-renesas-rzg2l.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/irqchip/irq-renesas-rzg2l.c
> b/drivers/irqchip/irq-renesas-rzg2l.c
> index 4bbfa2b0a4df..a9d75135ba24 100644
> --- a/drivers/irqchip/irq-renesas-rzg2l.c
> +++ b/drivers/irqchip/irq-renesas-rzg2l.c
> @@ -325,35 +325,42 @@ static int rzg2l_irqc_init(struct device_node
> *node, struct device_node *parent)
> 
>      pdev = of_find_device_by_node(node);
>      if (!pdev)
> +        put_device(pdev);
>          return -ENODEV;
> 
>      parent_domain = irq_find_host(parent);
>      if (!parent_domain) {
>          dev_err(&pdev->dev, "cannot find parent domain\n");
> +        put_device(pdev);
>          return -ENODEV;
>      }
> 
>      priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
>      if (!priv)
> +        put_device(pdev);
>          return -ENOMEM;
> 
>      priv->base = devm_of_iomap(&pdev->dev, pdev->dev.of_node, 0, NULL);
>      if (IS_ERR(priv->base))
> +        put_device(pdev);
>          return PTR_ERR(priv->base);
> 
>      ret = rzg2l_irqc_parse_interrupts(priv, node);
>      if (ret) {
>          dev_err(&pdev->dev, "cannot parse interrupts: %d\n", ret);
> +        put_device(pdev);
>          return ret;
>      }
> 
>      resetn = devm_reset_control_get_exclusive(&pdev->dev, NULL);
>      if (IS_ERR(resetn))
> +        put_device(pdev);
>          return PTR_ERR(resetn);
> 
>      ret = reset_control_deassert(resetn);
>      if (ret) {
>          dev_err(&pdev->dev, "failed to deassert resetn pin, %d\n",
> ret);
> +        put_device(pdev);
>          return ret;
>      }
> 
> @@ -374,7 +381,7 @@ static int rzg2l_irqc_init(struct device_node
> *node, struct device_node *parent)
>          ret = -ENOMEM;
>          goto pm_put;
>      }
> -
> +    put_device(pdev);
>      return 0;
> 
>  pm_put:
> @@ -382,6 +389,7 @@ static int rzg2l_irqc_init(struct device_node
> *node, struct device_node *parent)
>  pm_disable:
>      pm_runtime_disable(&pdev->dev);
>      reset_control_assert(resetn);
> +    put_device(pdev);
>      return ret;
>  }

Can't you spot an easy way to make the whole thing more readable and
less error prone? Such as having a *single* exit point to the
function, doing the put_device() and returning the error code?

	M.

-- 
Without deviation from the norm, progress is not possible.