[PATCH] irqchip/renesas-rzg2l: Fix missing put_device

[PATCH] irqchip/renesas-rzg2l: Fix missing put_device

[Fabrizio Castro]

rzg2l_irqc_common_init calls of_find_device_by_node, but the
corresponding put_device call is missing.

Make sure we call put_device both when failing and when
succeeding.

Fixes: 3fed09559cd8 ("irqchip: Add RZ/G2L IA55 Interrupt Controller driver")
Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
---
 drivers/irqchip/irq-renesas-rzg2l.c | 29 ++++++++++++++++++++---------
 1 file changed, 20 insertions(+), 9 deletions(-)

diff --git a/drivers/irqchip/irq-renesas-rzg2l.c b/drivers/irqchip/irq-renesas-rzg2l.c
index 693ff285ca2c..2bc9d3befa61 100644
--- a/drivers/irqchip/irq-renesas-rzg2l.c
+++ b/drivers/irqchip/irq-renesas-rzg2l.c
@@ -542,33 +542,40 @@ static int rzg2l_irqc_common_init(struct device_node *node, struct device_node *
 	parent_domain = irq_find_host(parent);
 	if (!parent_domain) {
 		dev_err(&pdev->dev, "cannot find parent domain\n");
-		return -ENODEV;
+		ret = -ENODEV;
+		goto put_dev;
 	}
 
 	rzg2l_irqc_data = devm_kzalloc(&pdev->dev, sizeof(*rzg2l_irqc_data), GFP_KERNEL);
-	if (!rzg2l_irqc_data)
-		return -ENOMEM;
+	if (!rzg2l_irqc_data) {
+		ret = -ENOMEM;
+		goto put_dev;
+	}
 
 	rzg2l_irqc_data->irqchip = irq_chip;
 
 	rzg2l_irqc_data->base = devm_of_iomap(&pdev->dev, pdev->dev.of_node, 0, NULL);
-	if (IS_ERR(rzg2l_irqc_data->base))
-		return PTR_ERR(rzg2l_irqc_data->base);
+	if (IS_ERR(rzg2l_irqc_data->base)) {
+		ret = PTR_ERR(rzg2l_irqc_data->base);
+		goto put_dev;
+	}
 
 	ret = rzg2l_irqc_parse_interrupts(rzg2l_irqc_data, node);
 	if (ret) {
 		dev_err(&pdev->dev, "cannot parse interrupts: %d\n", ret);
-		return ret;
+		goto put_dev;
 	}
 
 	resetn = devm_reset_control_get_exclusive(&pdev->dev, NULL);
-	if (IS_ERR(resetn))
-		return PTR_ERR(resetn);
+	if (IS_ERR(resetn)) {
+		ret = PTR_ERR(resetn);
+		goto put_dev;
+	}
 
 	ret = reset_control_deassert(resetn);
 	if (ret) {
 		dev_err(&pdev->dev, "failed to deassert resetn pin, %d\n", ret);
-		return ret;
+		goto put_dev;
 	}
 
 	pm_runtime_enable(&pdev->dev);
@@ -591,6 +598,7 @@ static int rzg2l_irqc_common_init(struct device_node *node, struct device_node *
 
 	register_syscore_ops(&rzg2l_irqc_syscore_ops);
 
+	put_device(&pdev->dev);
 	return 0;
 
 pm_put:
@@ -598,6 +606,9 @@ static int rzg2l_irqc_common_init(struct device_node *node, struct device_node *
 pm_disable:
 	pm_runtime_disable(&pdev->dev);
 	reset_control_assert(resetn);
+put_dev:
+	put_device(&pdev->dev);
+
 	return ret;
 }
 
-- 
2.34.1

Re: [PATCH] irqchip/renesas-rzg2l: Fix missing put_device

[Marc Zyngier]

On Mon, 30 Sep 2024 15:55:39 +0100,
Fabrizio Castro <fabrizio.castro.jz@renesas.com> wrote:
> 
> rzg2l_irqc_common_init calls of_find_device_by_node, but the
> corresponding put_device call is missing.
> 
> Make sure we call put_device both when failing and when
> succeeding.

What sort of lifetime are you trying to enforce?

It looks to me that you'd be better off doing *one* device_put() right
after you have found the parent domain, but that completely depends on
the above.

Thanks,

	M.

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

RE: [PATCH] irqchip/renesas-rzg2l: Fix missing put_device

[Fabrizio Castro]

Hi Marc,

Thanks for your feedback.

> From: Marc Zyngier <maz@kernel.org>
> Sent: Monday, September 30, 2024 4:50 PM
> To: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
> Subject: Re: [PATCH] irqchip/renesas-rzg2l: Fix missing put_device
> 
> On Mon, 30 Sep 2024 15:55:39 +0100,
> Fabrizio Castro <fabrizio.castro.jz@renesas.com> wrote:
> >
> > rzg2l_irqc_common_init calls of_find_device_by_node, but the
> > corresponding put_device call is missing.
> >
> > Make sure we call put_device both when failing and when succeeding.
> 
> What sort of lifetime are you trying to enforce?

Function rzg2l_irqc_common_init uses pdev->dev until its very end.
My understanding is that we should decrement the reference counter
once we are fully done with it. Is my understanding correct?

Thanks,
Fab

> 
> It looks to me that you'd be better off doing *one* device_put() right after you have found the parent
> domain, but that completely depends on the above.
> 
> Thanks,
> 
> 	M.
> 
> --
> Without deviation from the norm, progress is not possible.

Re: [PATCH] irqchip/renesas-rzg2l: Fix missing put_device

[Marc Zyngier]

On Mon, 30 Sep 2024 17:36:20 +0100,
Fabrizio Castro <fabrizio.castro.jz@renesas.com> wrote:
> 
> Hi Marc,
> 
> Thanks for your feedback.
> 
> > From: Marc Zyngier <maz@kernel.org>
> > Sent: Monday, September 30, 2024 4:50 PM
> > To: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
> > Subject: Re: [PATCH] irqchip/renesas-rzg2l: Fix missing put_device
> > 
> > On Mon, 30 Sep 2024 15:55:39 +0100,
> > Fabrizio Castro <fabrizio.castro.jz@renesas.com> wrote:
> > >
> > > rzg2l_irqc_common_init calls of_find_device_by_node, but the
> > > corresponding put_device call is missing.
> > >
> > > Make sure we call put_device both when failing and when succeeding.
> > 
> > What sort of lifetime are you trying to enforce?
> 
> Function rzg2l_irqc_common_init uses pdev->dev until its very end.
> My understanding is that we should decrement the reference counter
> once we are fully done with it. Is my understanding correct?

"done with it" is what scares me. Specially when I see code like this:

	rzg2l_irqc_data = devm_kzalloc(&pdev->dev, sizeof(*rzg2l_irqc_data), GFP_KERNEL);
	if (!rzg2l_irqc_data)
		return -ENOMEM;

	rzg2l_irqc_data->irqchip = irq_chip;

	rzg2l_irqc_data->base = devm_of_iomap(&pdev->dev, pdev->dev.of_node, 0, NULL);
	if (IS_ERR(rzg2l_irqc_data->base))
		return PTR_ERR(rzg2l_irqc_data->base);

If you drop the reference on the device, you are allowing it to be
removed, and everything the driver cares about to disappear behind its
back.

I can't really see how this is safe, because in general, removing an
interrupt controller driver from the system is a pretty bad idea, and
I'm worried that's you are implicitly enabling.

	M.

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

RE: [PATCH] irqchip/renesas-rzg2l: Fix missing put_device

[Fabrizio Castro]

Hi Marc,

thank you for your reply.

> From: Marc Zyngier <maz@kernel.org>
> Sent: Monday, September 30, 2024 8:15 PM
> To: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
> Subject: Re: [PATCH] irqchip/renesas-rzg2l: Fix missing put_device
> 
> On Mon, 30 Sep 2024 17:36:20 +0100,
> Fabrizio Castro <fabrizio.castro.jz@renesas.com> wrote:
> >
> > Hi Marc,
> >
> > Thanks for your feedback.
> >
> > > From: Marc Zyngier <maz@kernel.org>
> > > Sent: Monday, September 30, 2024 4:50 PM
> > > To: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
> > > Subject: Re: [PATCH] irqchip/renesas-rzg2l: Fix missing put_device
> > >
> > > On Mon, 30 Sep 2024 15:55:39 +0100,
> > > Fabrizio Castro <fabrizio.castro.jz@renesas.com> wrote:
> > > >
> > > > rzg2l_irqc_common_init calls of_find_device_by_node, but the
> > > > corresponding put_device call is missing.
> > > >
> > > > Make sure we call put_device both when failing and when succeeding.
> > >
> > > What sort of lifetime are you trying to enforce?
> >
> > Function rzg2l_irqc_common_init uses pdev->dev until its very end.
> > My understanding is that we should decrement the reference counter
> > once we are fully done with it. Is my understanding correct?
> 
> "done with it" is what scares me. Specially when I see code like this:
> 
> 	rzg2l_irqc_data = devm_kzalloc(&pdev->dev, sizeof(*rzg2l_irqc_data), GFP_KERNEL);
> 	if (!rzg2l_irqc_data)
> 		return -ENOMEM;
> 
> 	rzg2l_irqc_data->irqchip = irq_chip;
> 
> 	rzg2l_irqc_data->base = devm_of_iomap(&pdev->dev, pdev->dev.of_node, 0, NULL);
> 	if (IS_ERR(rzg2l_irqc_data->base))
> 		return PTR_ERR(rzg2l_irqc_data->base);
> 
> If you drop the reference on the device, you are allowing it to be removed, and everything the driver
> cares about to disappear behind its back.

Thanks for the explanation. I think this means that we don't need to put the device on the successful path,
but we still need to put the device on the error path.

If I take out the put_device for the successful path, and I run make coccicheck, I get the below:
drivers/irqchip/irq-renesas-rzg2l.c:601:1-7: ERROR: missing put_device; call of_find_device_by_node on line 538, but without a corresponding object release within this function.

Can I just ignore it?

Thanks!

Kind regards,
Fab

> 
> I can't really see how this is safe, because in general, removing an interrupt controller driver from
> the system is a pretty bad idea, and I'm worried that's you are implicitly enabling.
> 
> 	M.
> 
> --
> Without deviation from the norm, progress is not possible.

Re: [PATCH] irqchip/renesas-rzg2l: Fix missing put_device

[Marc Zyngier]

On Tue, 01 Oct 2024 12:54:30 +0100,
Fabrizio Castro <fabrizio.castro.jz@renesas.com> wrote:
> 
> Hi Marc,
> 
> thank you for your reply.
> 
> > From: Marc Zyngier <maz@kernel.org>
> > Sent: Monday, September 30, 2024 8:15 PM
> > To: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
> > Subject: Re: [PATCH] irqchip/renesas-rzg2l: Fix missing put_device
> > 
> > On Mon, 30 Sep 2024 17:36:20 +0100,
> > Fabrizio Castro <fabrizio.castro.jz@renesas.com> wrote:
> > >
> > > Hi Marc,
> > >
> > > Thanks for your feedback.
> > >
> > > > From: Marc Zyngier <maz@kernel.org>
> > > > Sent: Monday, September 30, 2024 4:50 PM
> > > > To: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
> > > > Subject: Re: [PATCH] irqchip/renesas-rzg2l: Fix missing put_device
> > > >
> > > > On Mon, 30 Sep 2024 15:55:39 +0100,
> > > > Fabrizio Castro <fabrizio.castro.jz@renesas.com> wrote:
> > > > >
> > > > > rzg2l_irqc_common_init calls of_find_device_by_node, but the
> > > > > corresponding put_device call is missing.
> > > > >
> > > > > Make sure we call put_device both when failing and when succeeding.
> > > >
> > > > What sort of lifetime are you trying to enforce?
> > >
> > > Function rzg2l_irqc_common_init uses pdev->dev until its very end.
> > > My understanding is that we should decrement the reference counter
> > > once we are fully done with it. Is my understanding correct?
> > 
> > "done with it" is what scares me. Specially when I see code like this:
> > 
> > 	rzg2l_irqc_data = devm_kzalloc(&pdev->dev, sizeof(*rzg2l_irqc_data), GFP_KERNEL);
> > 	if (!rzg2l_irqc_data)
> > 		return -ENOMEM;
> > 
> > 	rzg2l_irqc_data->irqchip = irq_chip;
> > 
> > 	rzg2l_irqc_data->base = devm_of_iomap(&pdev->dev, pdev->dev.of_node, 0, NULL);
> > 	if (IS_ERR(rzg2l_irqc_data->base))
> > 		return PTR_ERR(rzg2l_irqc_data->base);
> > 
> > If you drop the reference on the device, you are allowing it to be
> > removed, and everything the driver cares about to disappear behind
> > its back.
> 
> Thanks for the explanation. I think this means that we don't need to
> put the device on the successful path, but we still need to put the
> device on the error path.

That I would agree.

> If I take out the put_device for the successful path, and I run make
> coccicheck, I get the below:
> drivers/irqchip/irq-renesas-rzg2l.c:601:1-7: ERROR: missing
> put_device; call of_find_device_by_node on line 538, but without a
> corresponding object release within this function.
> 
> Can I just ignore it?

My general approach is that these scripts are not a substitute for
reasoning, and in this instance, the advise seems pretty misplaced.

I would suggest you add a comment to keep the next script kiddie at
bay.

	M.

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