| 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 > >
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(-)