drivers/irqchip/irq-renesas-rzg2l.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-)
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.1On 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
>
>
> 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
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
>>> 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
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
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.
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:© 2016 - 2024 Red Hat, Inc.