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

[PATCH v4] 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.
This also gets reported by make coccicheck.

Make use of the cleanup interfaces from cleanup.h to call into
__free_put_device (which in turn calls into put_device) when
leaving function rzg2l_irqc_common_init and variable "dev" goes
out of scope.

Mind that we don't want to "put" "dev" when rzg2l_irqc_common_init
completes successfully, therefore assign NULL to "dev" to prevent
__free_put_device from calling into put_device within the successful
path.

"make coccicheck" will still complain about missing put_device calls,
but those are false positives now.

Fixes: 3fed09559cd8 ("irqchip: Add RZ/G2L IA55 Interrupt Controller driver")
Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
---

v3->v4:
 * switched to using the cleanup interfaces as an alternative to using
   goto chains

 drivers/irqchip/irq-renesas-rzg2l.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/irqchip/irq-renesas-rzg2l.c b/drivers/irqchip/irq-renesas-rzg2l.c
index 693ff285ca2c..99e27e01b0b1 100644
--- a/drivers/irqchip/irq-renesas-rzg2l.c
+++ b/drivers/irqchip/irq-renesas-rzg2l.c
@@ -8,6 +8,7 @@
  */
 
 #include <linux/bitfield.h>
+#include <linux/cleanup.h>
 #include <linux/clk.h>
 #include <linux/err.h>
 #include <linux/io.h>
@@ -530,12 +531,12 @@ static int rzg2l_irqc_parse_interrupts(struct rzg2l_irqc_priv *priv,
 static int rzg2l_irqc_common_init(struct device_node *node, struct device_node *parent,
 				  const struct irq_chip *irq_chip)
 {
+	struct platform_device *pdev = of_find_device_by_node(node);
+	struct device *dev __free(put_device) = pdev ? &pdev->dev : NULL;
 	struct irq_domain *irq_domain, *parent_domain;
-	struct platform_device *pdev;
 	struct reset_control *resetn;
 	int ret;
 
-	pdev = of_find_device_by_node(node);
 	if (!pdev)
 		return -ENODEV;
 
@@ -591,6 +592,17 @@ static int rzg2l_irqc_common_init(struct device_node *node, struct device_node *
 
 	register_syscore_ops(&rzg2l_irqc_syscore_ops);
 
+	/*
+	 * Prevent the cleanup function from invoking put_device by assigning
+	 * NULL to dev.
+	 *
+	 * make coccicheck will complain about missing put_device calls, but
+	 * those are false positives, as dev will be automatically "put" via
+	 * __free_put_device on the failing path.
+	 * On the successful path we don't actually want to "put" dev.
+	 */
+	dev = NULL;
+
 	return 0;
 
 pm_put:
-- 
2.34.1

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

[Markus Elfring]

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

How do you think about to append parentheses to function names
(so that they can be distinguished a bit easier from other identifiers)?


> Make use of the cleanup interfaces from cleanup.h to call into
> __free_put_device (which in turn calls into put_device) when

Can it help to influence the understanding of this programming
interface by mentioning the usage of a special attribute?


> leaving function rzg2l_irqc_common_init and variable "dev" goes
> out of scope.
>
> Mind that we don't want to "put" "dev" when rzg2l_irqc_common_init
> completes successfully, therefore assign NULL to "dev" to prevent
> __free_put_device from calling into put_device within the successful
> path.

Will further software design options become applicable here?

Can any pointer type be used for the return value
(instead of the data type “int”)?


> "make coccicheck" will still complain about missing put_device calls,
> but those are false positives now.

Would you like to discuss any adjustment possibilities for this
development tool?


…
> +++ b/drivers/irqchip/irq-renesas-rzg2l.c
> @@ -8,6 +8,7 @@
>   */
>
>  #include <linux/bitfield.h>
> +#include <linux/cleanup.h>
…

This header file would usually be included by an other inclusion statement already,
wouldn't it?
https://elixir.bootlin.com/linux/v6.12-rc2/source/include/linux/device.h#L33


…
> @@ -530,12 +531,12 @@ static int rzg2l_irqc_parse_interrupts(struct rzg2l_irqc_priv *priv,
>  static int rzg2l_irqc_common_init(struct device_node *node, struct device_node *parent,
>  				  const struct irq_chip *irq_chip)
>  {
> +	struct platform_device *pdev = of_find_device_by_node(node);
> +	struct device *dev __free(put_device) = pdev ? &pdev->dev : NULL;
>  	struct irq_domain *irq_domain, *parent_domain;
> -	struct platform_device *pdev;
>  	struct reset_control *resetn;
>  	int ret;
>
> -	pdev = of_find_device_by_node(node);
>  	if (!pdev)
>  		return -ENODEV;
…

Would you dare to reduce the scopes for any local variables here?
https://refactoring.com/catalog/reduceScopeOfVariable.html

Regards,
Markus

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

[Thomas Gleixner]

On Fri, Oct 11 2024 at 20:48, Markus Elfring wrote:
>> rzg2l_irqc_common_init calls of_find_device_by_node, but the
>> corresponding put_device call is missing.
>
> How do you think about to append parentheses to function names
> (so that they can be distinguished a bit easier from other identifiers)?
>
>
>> Make use of the cleanup interfaces from cleanup.h to call into
>> __free_put_device (which in turn calls into put_device) when
>
> Can it help to influence the understanding of this programming
> interface by mentioning the usage of a special attribute?

Can you please stop pestering people with incomprehensible word salad?

>> leaving function rzg2l_irqc_common_init and variable "dev" goes
>> out of scope.
>>
>> Mind that we don't want to "put" "dev" when rzg2l_irqc_common_init
>> completes successfully, therefore assign NULL to "dev" to prevent
>> __free_put_device from calling into put_device within the successful
>> path.
>
> Will further software design options become applicable here?
>
> Can any pointer type be used for the return value
> (instead of the data type “int”)?

How is this relevant here?

>
>> "make coccicheck" will still complain about missing put_device calls,
>> but those are false positives now.
>
> Would you like to discuss any adjustment possibilities for this
> development tool?

Would you like to get useful work done insteead of telling everyone what
to do? There is nothing to discuss.

>> +++ b/drivers/irqchip/irq-renesas-rzg2l.c
>> @@ -8,6 +8,7 @@
>>   */
>>
>>  #include <linux/bitfield.h>
>> +#include <linux/cleanup.h>
> …
>
> This header file would usually be included by an other inclusion statement already,
> wouldn't it?

Relying on indirect includes is not necessarily a good idea/

>> @@ -530,12 +531,12 @@ static int rzg2l_irqc_parse_interrupts(struct rzg2l_irqc_priv *priv,
>>  static int rzg2l_irqc_common_init(struct device_node *node, struct device_node *parent,
>>  				  const struct irq_chip *irq_chip)
>>  {
>> +	struct platform_device *pdev = of_find_device_by_node(node);
>> +	struct device *dev __free(put_device) = pdev ? &pdev->dev : NULL;
>>  	struct irq_domain *irq_domain, *parent_domain;
>> -	struct platform_device *pdev;
>>  	struct reset_control *resetn;
>>  	int ret;
>>
>> -	pdev = of_find_device_by_node(node);
>>  	if (!pdev)
>>  		return -ENODEV;
> …
>
> Would you dare to reduce the scopes for any local variables here?
> https://refactoring.com/catalog/reduceScopeOfVariable.html

Can you keep your refactoring links for yourself please? We are aware of
this.

But this change fixes a bug and that's it. We are not doing cleanups in
a bug fix. Please read and understand Documentation/process before
giving people ill defined advise.

Thanks,

        tglx

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

[Markus Elfring]

>>> rzg2l_irqc_common_init calls of_find_device_by_node, but the
>>> corresponding put_device call is missing.
…
>>> Make use of the cleanup interfaces from cleanup.h to call into
>>> __free_put_device (which in turn calls into put_device) when
>>
>> Can it help to influence the understanding of this programming
>> interface by mentioning the usage of a special attribute?
>
> Can you please stop pestering people with incomprehensible word salad?

Which patch review comments would you find more appropriate here?


>>> leaving function rzg2l_irqc_common_init and variable "dev" goes
>>> out of scope.
>>>
>>> Mind that we don't want to "put" "dev" when rzg2l_irqc_common_init
>>> completes successfully, therefore assign NULL to "dev" to prevent
>>> __free_put_device from calling into put_device within the successful
>>> path.
>>
>> Will further software design options become applicable here?
>>
>> Can any pointer type be used for the return value
>> (instead of the data type “int”)?
>
> How is this relevant here?

I imagine that the usage of error pointers can occasionally be helpful
for such programming interfaces.


>>> "make coccicheck" will still complain about missing put_device calls,
>>> but those are false positives now.
>>
>> Would you like to discuss any adjustment possibilities for this
>> development tool?
>
> Would you like to get useful work done insteead of telling everyone what
> to do? There is nothing to discuss.

I got other impressions for corresponding development opportunities.


> But this change fixes a bug and that's it.

Maybe.


> We are not doing cleanups in a bug fix.

Additional adjustments can be offered in subsequent update steps
(within a patch series?).

Regards,
Markus

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

[Thomas Gleixner]

On Wed, Oct 16 2024 at 11:38, Markus Elfring wrote:
>> We are not doing cleanups in a bug fix.
>
> Additional adjustments can be offered in subsequent update steps
> (within a patch series?).

Feel free to send patches.

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

[Thomas Gleixner]

On Wed, Oct 16 2024 at 11:38, Markus Elfring wrote:
>> But this change fixes a bug and that's it.
>
> Maybe.

Maybe?

There is no maybe. You clearly failed to read and understand the
documentation I asked you to read and understand.

Either you are impersonating a badly implemented LLM or you are actually
living in the delusion that you can teach people who are actually doing
work based on your particular flavor of hubris.

Your answer to this mail will clearly tell me into which category you
fall into, but neither of them are in any way useful to the Linux kernel
community.

Whatever the answer is, I don't care because your input is completely
irrelevant. You have proven that over the years.

Thanks,

        tglx

[tip: irq/urgent] irqchip/renesas-rzg2l: Fix missing put_device

[tip-bot2 for Fabrizio Castro]

The following commit has been merged into the irq/urgent branch of tip:

Commit-ID:     d038109ac1c6bf619473dda03a16a6de58170f7f
Gitweb:        https://git.kernel.org/tip/d038109ac1c6bf619473dda03a16a6de58170f7f
Author:        Fabrizio Castro <fabrizio.castro.jz@renesas.com>
AuthorDate:    Fri, 11 Oct 2024 18:20:03 +01:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Tue, 15 Oct 2024 23:54:35 +02:00

irqchip/renesas-rzg2l: Fix missing put_device

rzg2l_irqc_common_init() calls of_find_device_by_node(), but the
corresponding put_device() call is missing.  This also gets reported by
make coccicheck.

Make use of the cleanup interfaces from cleanup.h to call into
__free_put_device(), which in turn calls into put_device when leaving
function rzg2l_irqc_common_init() and variable "dev" goes out of scope.

To prevent that the device is put on successful completion, assign NULL to
"dev" to prevent __free_put_device() from calling into put_device() within
the successful path.

"make coccicheck" will still complain about missing put_device() calls,
but those are false positives now.

Fixes: 3fed09559cd8 ("irqchip: Add RZ/G2L IA55 Interrupt Controller driver")
Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Link: https://lore.kernel.org/all/20241011172003.1242841-1-fabrizio.castro.jz@renesas.com
---
 drivers/irqchip/irq-renesas-rzg2l.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/irqchip/irq-renesas-rzg2l.c b/drivers/irqchip/irq-renesas-rzg2l.c
index 693ff28..99e27e0 100644
--- a/drivers/irqchip/irq-renesas-rzg2l.c
+++ b/drivers/irqchip/irq-renesas-rzg2l.c
@@ -8,6 +8,7 @@
  */
 
 #include <linux/bitfield.h>
+#include <linux/cleanup.h>
 #include <linux/clk.h>
 #include <linux/err.h>
 #include <linux/io.h>
@@ -530,12 +531,12 @@ static int rzg2l_irqc_parse_interrupts(struct rzg2l_irqc_priv *priv,
 static int rzg2l_irqc_common_init(struct device_node *node, struct device_node *parent,
 				  const struct irq_chip *irq_chip)
 {
+	struct platform_device *pdev = of_find_device_by_node(node);
+	struct device *dev __free(put_device) = pdev ? &pdev->dev : NULL;
 	struct irq_domain *irq_domain, *parent_domain;
-	struct platform_device *pdev;
 	struct reset_control *resetn;
 	int ret;
 
-	pdev = of_find_device_by_node(node);
 	if (!pdev)
 		return -ENODEV;
 
@@ -591,6 +592,17 @@ static int rzg2l_irqc_common_init(struct device_node *node, struct device_node *
 
 	register_syscore_ops(&rzg2l_irqc_syscore_ops);
 
+	/*
+	 * Prevent the cleanup function from invoking put_device by assigning
+	 * NULL to dev.
+	 *
+	 * make coccicheck will complain about missing put_device calls, but
+	 * those are false positives, as dev will be automatically "put" via
+	 * __free_put_device on the failing path.
+	 * On the successful path we don't actually want to "put" dev.
+	 */
+	dev = NULL;
+
 	return 0;
 
 pm_put:

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

[Lad, Prabhakar]

On Fri, Oct 11, 2024 at 6:20 PM 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.
> This also gets reported by make coccicheck.
>
> Make use of the cleanup interfaces from cleanup.h to call into
> __free_put_device (which in turn calls into put_device) when
> leaving function rzg2l_irqc_common_init and variable "dev" goes
> out of scope.
>
> Mind that we don't want to "put" "dev" when rzg2l_irqc_common_init
> completes successfully, therefore assign NULL to "dev" to prevent
> __free_put_device from calling into put_device within the successful
> path.
>
> "make coccicheck" will still complain about missing put_device calls,
> but those are false positives now.
>
> Fixes: 3fed09559cd8 ("irqchip: Add RZ/G2L IA55 Interrupt Controller driver")
> Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
> ---
>
> v3->v4:
>  * switched to using the cleanup interfaces as an alternative to using
>    goto chains
>
>  drivers/irqchip/irq-renesas-rzg2l.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Cheers,
Prabhakar

> diff --git a/drivers/irqchip/irq-renesas-rzg2l.c b/drivers/irqchip/irq-renesas-rzg2l.c
> index 693ff285ca2c..99e27e01b0b1 100644
> --- a/drivers/irqchip/irq-renesas-rzg2l.c
> +++ b/drivers/irqchip/irq-renesas-rzg2l.c
> @@ -8,6 +8,7 @@
>   */
>
>  #include <linux/bitfield.h>
> +#include <linux/cleanup.h>
>  #include <linux/clk.h>
>  #include <linux/err.h>
>  #include <linux/io.h>
> @@ -530,12 +531,12 @@ static int rzg2l_irqc_parse_interrupts(struct rzg2l_irqc_priv *priv,
>  static int rzg2l_irqc_common_init(struct device_node *node, struct device_node *parent,
>                                   const struct irq_chip *irq_chip)
>  {
> +       struct platform_device *pdev = of_find_device_by_node(node);
> +       struct device *dev __free(put_device) = pdev ? &pdev->dev : NULL;
>         struct irq_domain *irq_domain, *parent_domain;
> -       struct platform_device *pdev;
>         struct reset_control *resetn;
>         int ret;
>
> -       pdev = of_find_device_by_node(node);
>         if (!pdev)
>                 return -ENODEV;
>
> @@ -591,6 +592,17 @@ static int rzg2l_irqc_common_init(struct device_node *node, struct device_node *
>
>         register_syscore_ops(&rzg2l_irqc_syscore_ops);
>
> +       /*
> +        * Prevent the cleanup function from invoking put_device by assigning
> +        * NULL to dev.
> +        *
> +        * make coccicheck will complain about missing put_device calls, but
> +        * those are false positives, as dev will be automatically "put" via
> +        * __free_put_device on the failing path.
> +        * On the successful path we don't actually want to "put" dev.
> +        */
> +       dev = NULL;
> +
>         return 0;
>
>  pm_put:
> --
> 2.34.1
>
>

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

[Geert Uytterhoeven]

Hi Fabrizio,

On Fri, 11 Oct 2024 at 19:20, 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.
> This also gets reported by make coccicheck.
>
> Make use of the cleanup interfaces from cleanup.h to call into
> __free_put_device (which in turn calls into put_device) when
> leaving function rzg2l_irqc_common_init and variable "dev" goes
> out of scope.
>
> Mind that we don't want to "put" "dev" when rzg2l_irqc_common_init
> completes successfully, therefore assign NULL to "dev" to prevent
> __free_put_device from calling into put_device within the successful
> path.
>
> "make coccicheck" will still complain about missing put_device calls,
> but those are false positives now.
>
> Fixes: 3fed09559cd8 ("irqchip: Add RZ/G2L IA55 Interrupt Controller driver")
> Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>

Revisiting commit d038109ac1c6bf61 ("irqchip/renesas-rzg2l: Fix missing
put_device")...

> --- a/drivers/irqchip/irq-renesas-rzg2l.c
> +++ b/drivers/irqchip/irq-renesas-rzg2l.c
> @@ -8,6 +8,7 @@
>   */
>
>  #include <linux/bitfield.h>
> +#include <linux/cleanup.h>
>  #include <linux/clk.h>
>  #include <linux/err.h>
>  #include <linux/io.h>
> @@ -530,12 +531,12 @@ static int rzg2l_irqc_parse_interrupts(struct rzg2l_irqc_priv *priv,
>  static int rzg2l_irqc_common_init(struct device_node *node, struct device_node *parent,
>                                   const struct irq_chip *irq_chip)
>  {
> +       struct platform_device *pdev = of_find_device_by_node(node);
> +       struct device *dev __free(put_device) = pdev ? &pdev->dev : NULL;

Now there is a variable holding "&pdev->dev", all below references
to the latter can be replaced by "dev"...

>         struct irq_domain *irq_domain, *parent_domain;
> -       struct platform_device *pdev;
>         struct reset_control *resetn;
>         int ret;
>
> -       pdev = of_find_device_by_node(node);
>         if (!pdev)
>                 return -ENODEV;
>
> @@ -591,6 +592,17 @@ static int rzg2l_irqc_common_init(struct device_node *node, struct device_node *
>
>         register_syscore_ops(&rzg2l_irqc_syscore_ops);
>
> +       /*
> +        * Prevent the cleanup function from invoking put_device by assigning
> +        * NULL to dev.
> +        *
> +        * make coccicheck will complain about missing put_device calls, but
> +        * those are false positives, as dev will be automatically "put" via
> +        * __free_put_device on the failing path.
> +        * On the successful path we don't actually want to "put" dev.
> +        */
> +       dev = NULL;
> +
>         return 0;

Can't the above be replaced by

    no_free_ptr(dev);

? Or would Coccinelle still complain?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Fabrizio Castro]

Hi Geert,

Thanks for your feedback!

> From: Geert Uytterhoeven <geert@linux-m68k.org>
> Sent: 11 February 2025 15:12
> Subject: Re: [PATCH v4] irqchip/renesas-rzg2l: Fix missing put_device
> 
> Hi Fabrizio,
> 
> On Fri, 11 Oct 2024 at 19:20, 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.
> > This also gets reported by make coccicheck.
> >
> > Make use of the cleanup interfaces from cleanup.h to call into
> > __free_put_device (which in turn calls into put_device) when
> > leaving function rzg2l_irqc_common_init and variable "dev" goes
> > out of scope.
> >
> > Mind that we don't want to "put" "dev" when rzg2l_irqc_common_init
> > completes successfully, therefore assign NULL to "dev" to prevent
> > __free_put_device from calling into put_device within the successful
> > path.
> >
> > "make coccicheck" will still complain about missing put_device calls,
> > but those are false positives now.
> >
> > Fixes: 3fed09559cd8 ("irqchip: Add RZ/G2L IA55 Interrupt Controller driver")
> > Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
> 
> Revisiting commit d038109ac1c6bf61 ("irqchip/renesas-rzg2l: Fix missing
> put_device")...
> 
> > --- a/drivers/irqchip/irq-renesas-rzg2l.c
> > +++ b/drivers/irqchip/irq-renesas-rzg2l.c
> > @@ -8,6 +8,7 @@
> >   */
> >
> >  #include <linux/bitfield.h>
> > +#include <linux/cleanup.h>
> >  #include <linux/clk.h>
> >  #include <linux/err.h>
> >  #include <linux/io.h>
> > @@ -530,12 +531,12 @@ static int rzg2l_irqc_parse_interrupts(struct rzg2l_irqc_priv *priv,
> >  static int rzg2l_irqc_common_init(struct device_node *node, struct device_node *parent,
> >                                   const struct irq_chip *irq_chip)
> >  {
> > +       struct platform_device *pdev = of_find_device_by_node(node);
> > +       struct device *dev __free(put_device) = pdev ? &pdev->dev : NULL;
> 
> Now there is a variable holding "&pdev->dev", all below references
> to the latter can be replaced by "dev"...

Right! I will shortly send a patch for this.

> 
> >         struct irq_domain *irq_domain, *parent_domain;
> > -       struct platform_device *pdev;
> >         struct reset_control *resetn;
> >         int ret;
> >
> > -       pdev = of_find_device_by_node(node);
> >         if (!pdev)
> >                 return -ENODEV;
> >
> > @@ -591,6 +592,17 @@ static int rzg2l_irqc_common_init(struct device_node *node, struct device_node
> *
> >
> >         register_syscore_ops(&rzg2l_irqc_syscore_ops);
> >
> > +       /*
> > +        * Prevent the cleanup function from invoking put_device by assigning
> > +        * NULL to dev.
> > +        *
> > +        * make coccicheck will complain about missing put_device calls, but
> > +        * those are false positives, as dev will be automatically "put" via
> > +        * __free_put_device on the failing path.
> > +        * On the successful path we don't actually want to "put" dev.
> > +        */
> > +       dev = NULL;
> > +
> >         return 0;
> 
> Can't the above be replaced by
> 
>     no_free_ptr(dev);
> 
> ? Or would Coccinelle still complain?

If I use no_free_ptr the compiler complains that its return value is not used:

In file included from ../drivers/irqchip/irq-renesas-rzg2l.c:11:
../drivers/irqchip/irq-renesas-rzg2l.c: In function ‘rzg2l_irqc_common_init’:
../include/linux/cleanup.h:215:15: warning: ignoring return value of ‘__must_check_fn’ declared with attribute ‘warn_unused_result’ [-Wunused-result]
  215 |  ((typeof(p)) __must_check_fn(__get_and_null(p, NULL)))
      |               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../drivers/irqchip/irq-renesas-rzg2l.c:595:2: note: in expansion of macro ‘no_free_ptr’
  595 |  no_free_ptr(dev);
      |  ^~~~~~~~~~~


Cheers,
Fab

> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

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

[Geert Uytterhoeven]

Hi Fabrizio,

On Tue, 11 Feb 2025 at 16:49, Fabrizio Castro
<fabrizio.castro.jz@renesas.com> wrote:
> > From: Geert Uytterhoeven <geert@linux-m68k.org>
> > Sent: 11 February 2025 15:12
> > Subject: Re: [PATCH v4] irqchip/renesas-rzg2l: Fix missing put_device
> >
> > On Fri, 11 Oct 2024 at 19:20, 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.
> > > This also gets reported by make coccicheck.
> > >
> > > Make use of the cleanup interfaces from cleanup.h to call into
> > > __free_put_device (which in turn calls into put_device) when
> > > leaving function rzg2l_irqc_common_init and variable "dev" goes
> > > out of scope.
> > >
> > > Mind that we don't want to "put" "dev" when rzg2l_irqc_common_init
> > > completes successfully, therefore assign NULL to "dev" to prevent
> > > __free_put_device from calling into put_device within the successful
> > > path.
> > >
> > > "make coccicheck" will still complain about missing put_device calls,
> > > but those are false positives now.
> > >
> > > Fixes: 3fed09559cd8 ("irqchip: Add RZ/G2L IA55 Interrupt Controller driver")
> > > Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
> >
> > Revisiting commit d038109ac1c6bf61 ("irqchip/renesas-rzg2l: Fix missing
> > put_device")...
> >
> > > --- a/drivers/irqchip/irq-renesas-rzg2l.c
> > > +++ b/drivers/irqchip/irq-renesas-rzg2l.c
> > > @@ -8,6 +8,7 @@
> > >   */
> > >
> > >  #include <linux/bitfield.h>
> > > +#include <linux/cleanup.h>
> > >  #include <linux/clk.h>
> > >  #include <linux/err.h>
> > >  #include <linux/io.h>
> > > @@ -530,12 +531,12 @@ static int rzg2l_irqc_parse_interrupts(struct rzg2l_irqc_priv *priv,
> > >  static int rzg2l_irqc_common_init(struct device_node *node, struct device_node *parent,
> > >                                   const struct irq_chip *irq_chip)
> > >  {
> > > +       struct platform_device *pdev = of_find_device_by_node(node);
> > > +       struct device *dev __free(put_device) = pdev ? &pdev->dev : NULL;
> >
> > Now there is a variable holding "&pdev->dev", all below references
> > to the latter can be replaced by "dev"...
>
> Right! I will shortly send a patch for this.

Thanks in advance!

> > >         struct irq_domain *irq_domain, *parent_domain;
> > > -       struct platform_device *pdev;
> > >         struct reset_control *resetn;
> > >         int ret;
> > >
> > > -       pdev = of_find_device_by_node(node);
> > >         if (!pdev)
> > >                 return -ENODEV;
> > >
> > > @@ -591,6 +592,17 @@ static int rzg2l_irqc_common_init(struct device_node *node, struct device_node
> > *
> > >
> > >         register_syscore_ops(&rzg2l_irqc_syscore_ops);
> > >
> > > +       /*
> > > +        * Prevent the cleanup function from invoking put_device by assigning
> > > +        * NULL to dev.
> > > +        *
> > > +        * make coccicheck will complain about missing put_device calls, but
> > > +        * those are false positives, as dev will be automatically "put" via
> > > +        * __free_put_device on the failing path.
> > > +        * On the successful path we don't actually want to "put" dev.
> > > +        */
> > > +       dev = NULL;
> > > +
> > >         return 0;
> >
> > Can't the above be replaced by
> >
> >     no_free_ptr(dev);
> >
> > ? Or would Coccinelle still complain?
>
> If I use no_free_ptr the compiler complains that its return value is not used:
>
> In file included from ../drivers/irqchip/irq-renesas-rzg2l.c:11:
> ../drivers/irqchip/irq-renesas-rzg2l.c: In function ‘rzg2l_irqc_common_init’:
> ../include/linux/cleanup.h:215:15: warning: ignoring return value of ‘__must_check_fn’ declared with attribute ‘warn_unused_result’ [-Wunused-result]
>   215 |  ((typeof(p)) __must_check_fn(__get_and_null(p, NULL)))
>       |               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ../drivers/irqchip/irq-renesas-rzg2l.c:595:2: note: in expansion of macro ‘no_free_ptr’
>   595 |  no_free_ptr(dev);
>       |  ^~~~~~~~~~~

I hadn't tried it myself, and thus hadn't noticed that warning.
Interestingly, the addition of __must_check_fn()[1] predates the
documentation[2] that shows the above construct...

[1] commit 85be6d842447067c ("cleanup: Make no_free_ptr() __must_check"),
[2] commit d5934e76316e84ec ("cleanup: Add usage and style documentation").

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds