| Message ID | 20241011172003.1242841-1-fabrizio.castro.jz@renesas.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | d038109ac1c6bf619473dda03a16a6de58170f7f |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | [v4] irqchip/renesas-rzg2l: Fix missing put_device | expand |
> 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: >> 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.
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 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 > >
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
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
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
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:
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(-)