* [PATCH] irqchip/renesas-rzg2l: Fix missing put_device
@ 2024-09-30 14:55 Fabrizio Castro
2024-09-30 15:50 ` Marc Zyngier
0 siblings, 1 reply; 6+ messages in thread
From: Fabrizio Castro @ 2024-09-30 14:55 UTC (permalink / raw)
To: Thomas Gleixner, Geert Uytterhoeven
Cc: Fabrizio Castro, Lad Prabhakar, Marc Zyngier, linux-kernel,
Chris Paterson, Biju Das, linux-renesas-soc
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] irqchip/renesas-rzg2l: Fix missing put_device
2024-09-30 14:55 [PATCH] irqchip/renesas-rzg2l: Fix missing put_device Fabrizio Castro
@ 2024-09-30 15:50 ` Marc Zyngier
2024-09-30 16:36 ` Fabrizio Castro
0 siblings, 1 reply; 6+ messages in thread
From: Marc Zyngier @ 2024-09-30 15:50 UTC (permalink / raw)
To: Fabrizio Castro
Cc: Thomas Gleixner, Geert Uytterhoeven, Lad Prabhakar, linux-kernel,
Chris Paterson, Biju Das, linux-renesas-soc
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] irqchip/renesas-rzg2l: Fix missing put_device
2024-09-30 15:50 ` Marc Zyngier
@ 2024-09-30 16:36 ` Fabrizio Castro
2024-09-30 19:14 ` Marc Zyngier
0 siblings, 1 reply; 6+ messages in thread
From: Fabrizio Castro @ 2024-09-30 16:36 UTC (permalink / raw)
To: Marc Zyngier
Cc: Thomas Gleixner, Geert Uytterhoeven, Prabhakar Mahadev Lad,
linux-kernel@vger.kernel.org, Chris Paterson, Biju Das,
linux-renesas-soc@vger.kernel.org
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] irqchip/renesas-rzg2l: Fix missing put_device
2024-09-30 16:36 ` Fabrizio Castro
@ 2024-09-30 19:14 ` Marc Zyngier
2024-10-01 11:54 ` Fabrizio Castro
0 siblings, 1 reply; 6+ messages in thread
From: Marc Zyngier @ 2024-09-30 19:14 UTC (permalink / raw)
To: Fabrizio Castro
Cc: Thomas Gleixner, Geert Uytterhoeven, Prabhakar Mahadev Lad,
linux-kernel@vger.kernel.org, Chris Paterson, Biju Das,
linux-renesas-soc@vger.kernel.org
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] irqchip/renesas-rzg2l: Fix missing put_device
2024-09-30 19:14 ` Marc Zyngier
@ 2024-10-01 11:54 ` Fabrizio Castro
2024-10-06 15:53 ` Marc Zyngier
0 siblings, 1 reply; 6+ messages in thread
From: Fabrizio Castro @ 2024-10-01 11:54 UTC (permalink / raw)
To: Marc Zyngier
Cc: Thomas Gleixner, Geert Uytterhoeven, Prabhakar Mahadev Lad,
linux-kernel@vger.kernel.org, Chris Paterson, Biju Das,
linux-renesas-soc@vger.kernel.org
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] irqchip/renesas-rzg2l: Fix missing put_device
2024-10-01 11:54 ` Fabrizio Castro
@ 2024-10-06 15:53 ` Marc Zyngier
0 siblings, 0 replies; 6+ messages in thread
From: Marc Zyngier @ 2024-10-06 15:53 UTC (permalink / raw)
To: Fabrizio Castro
Cc: Thomas Gleixner, Geert Uytterhoeven, Prabhakar Mahadev Lad,
linux-kernel@vger.kernel.org, Chris Paterson, Biju Das,
linux-renesas-soc@vger.kernel.org
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-10-06 15:53 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-09-30 14:55 [PATCH] irqchip/renesas-rzg2l: Fix missing put_device Fabrizio Castro
2024-09-30 15:50 ` Marc Zyngier
2024-09-30 16:36 ` Fabrizio Castro
2024-09-30 19:14 ` Marc Zyngier
2024-10-01 11:54 ` Fabrizio Castro
2024-10-06 15:53 ` Marc Zyngier
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox