| Message ID | 20240823-cleanup-h-guard-pm-domain-v1-9-8320722eaf39@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | pmdomain: Simplify with cleanup.h | expand |
Hi Krzysztof, On Fri, Aug 23, 2024 at 2:51 PM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > Obtain the device node reference with scoped/cleanup.h to reduce error > handling and make the code a bit simpler. > > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Thanks for your patch! > --- a/drivers/pmdomain/renesas/rcar-gen4-sysc.c > +++ b/drivers/pmdomain/renesas/rcar-gen4-sysc.c > @@ -303,12 +304,12 @@ static int __init rcar_gen4_sysc_pd_init(void) > const struct rcar_gen4_sysc_info *info; > const struct of_device_id *match; > struct rcar_gen4_pm_domains *domains; > - struct device_node *np; > void __iomem *base; > unsigned int i; > int error; > > - np = of_find_matching_node_and_match(NULL, rcar_gen4_sysc_matches, &match); > + struct device_node *np __free(device_node) = > + of_find_matching_node_and_match(NULL, rcar_gen4_sysc_matches, &match); This breaks the declarations/blank-line/code structure, so please move this up. If you insist on keeping assignment to and validation of np together, the line should be split in declaration and assignment. > if (!np) > return -ENODEV; > > @@ -369,14 +365,12 @@ static int __init rcar_gen4_sysc_pd_init(void) > if (error) { > pr_warn("Failed to add PM subdomain %s to parent %u\n", > area->name, area->parent); > - goto out_put; > + return error; > } > } > > error = of_genpd_add_provider_onecell(np, &domains->onecell_data); > > -out_put: > - of_node_put(np); > return error; return of_genpd_add_provider_onecell(...); > } > early_initcall(rcar_gen4_sysc_pd_init); Gr{oetje,eeting}s, Geert
On 27/08/2024 09:48, Geert Uytterhoeven wrote: > Hi Krzysztof, > > On Fri, Aug 23, 2024 at 2:51 PM Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: >> Obtain the device node reference with scoped/cleanup.h to reduce error >> handling and make the code a bit simpler. >> >> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > Thanks for your patch! > >> --- a/drivers/pmdomain/renesas/rcar-gen4-sysc.c >> +++ b/drivers/pmdomain/renesas/rcar-gen4-sysc.c >> @@ -303,12 +304,12 @@ static int __init rcar_gen4_sysc_pd_init(void) >> const struct rcar_gen4_sysc_info *info; >> const struct of_device_id *match; >> struct rcar_gen4_pm_domains *domains; >> - struct device_node *np; >> void __iomem *base; >> unsigned int i; >> int error; >> >> - np = of_find_matching_node_and_match(NULL, rcar_gen4_sysc_matches, &match); >> + struct device_node *np __free(device_node) = >> + of_find_matching_node_and_match(NULL, rcar_gen4_sysc_matches, &match); > > This breaks the declarations/blank-line/code structure, so please move > this up. What do you mean "declaration structure"? That's the way how variables with constructors are expected to be declared - within the code. > > If you insist on keeping assignment to and validation of np together, > the line should be split in declaration and assignment. No, that would be inconsistent with cleanup/constructor coding style. Maybe this is something new, so let me bring previous discussions: https://lore.kernel.org/all/CAHk-=wicfvWPuRVDG5R1mZSxD8Xg=-0nLOiHay2T_UJ0yDX42g@mail.gmail.com/ https://lore.kernel.org/all/CAHk-=wgRHiV5VSxtfXA4S6aLUmcQYEuB67u3BJPJPtuESs1JyA@mail.gmail.com/ https://lore.kernel.org/all/CAHk-=whvOGL3aNhtps0YksGtzvaob_bvZpbaTcVEqGwNMxB6xg@mail.gmail.com/ and finally it will reach documentation (although it focuses on unwinding process to be specific - "When the unwind order ..."): https://lore.kernel.org/all/171175585714.2192972.12661675876300167762.stgit@dwillia2-xfh.jf.intel.com/ > >> if (!np) >> return -ENODEV; >> > >> @@ -369,14 +365,12 @@ static int __init rcar_gen4_sysc_pd_init(void) >> if (error) { >> pr_warn("Failed to add PM subdomain %s to parent %u\n", >> area->name, area->parent); >> - goto out_put; >> + return error; >> } >> } >> >> error = of_genpd_add_provider_onecell(np, &domains->onecell_data); >> >> -out_put: >> - of_node_put(np); >> return error; > > return of_genpd_add_provider_onecell(...); Ack. Best regards, Krzysztof
On 27/08/2024 11:33, Krzysztof Kozlowski wrote: > On 27/08/2024 09:48, Geert Uytterhoeven wrote: >> Hi Krzysztof, >> >> On Fri, Aug 23, 2024 at 2:51 PM Krzysztof Kozlowski >> <krzysztof.kozlowski@linaro.org> wrote: >>> Obtain the device node reference with scoped/cleanup.h to reduce error >>> handling and make the code a bit simpler. >>> >>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >> >> Thanks for your patch! >> >>> --- a/drivers/pmdomain/renesas/rcar-gen4-sysc.c >>> +++ b/drivers/pmdomain/renesas/rcar-gen4-sysc.c >>> @@ -303,12 +304,12 @@ static int __init rcar_gen4_sysc_pd_init(void) >>> const struct rcar_gen4_sysc_info *info; >>> const struct of_device_id *match; >>> struct rcar_gen4_pm_domains *domains; >>> - struct device_node *np; >>> void __iomem *base; >>> unsigned int i; >>> int error; >>> >>> - np = of_find_matching_node_and_match(NULL, rcar_gen4_sysc_matches, &match); >>> + struct device_node *np __free(device_node) = >>> + of_find_matching_node_and_match(NULL, rcar_gen4_sysc_matches, &match); >> >> This breaks the declarations/blank-line/code structure, so please move >> this up. > > What do you mean "declaration structure"? That's the way how variables > with constructors are expected to be declared - within the code. Continuing thoughts, so you prefer: struct rcar_gen4_pm_domains *domains; void __iomem *base; struct device_node *np __free(device_node) = of_find_matching_node_and_match(NULL, rcar_gen4_sysc_matches, &match); (assuming I will put it at the end of declarations). Are you sure this is more readable? It's really long line so it obfuscates a bit the declarations. The point of the scoped assignment is that you declare it at point of need/first use. > >> >> If you insist on keeping assignment to and validation of np together, >> the line should be split in declaration and assignment. > > No, that would be inconsistent with cleanup/constructor coding style. > Maybe this is something new, so let me bring previous discussions: > > https://lore.kernel.org/all/CAHk-=wicfvWPuRVDG5R1mZSxD8Xg=-0nLOiHay2T_UJ0yDX42g@mail.gmail.com/ > > https://lore.kernel.org/all/CAHk-=wgRHiV5VSxtfXA4S6aLUmcQYEuB67u3BJPJPtuESs1JyA@mail.gmail.com/ > > https://lore.kernel.org/all/CAHk-=whvOGL3aNhtps0YksGtzvaob_bvZpbaTcVEqGwNMxB6xg@mail.gmail.com/ > > and finally it will reach documentation (although it focuses on > unwinding process to be specific - "When the unwind order ..."): > https://lore.kernel.org/all/171175585714.2192972.12661675876300167762.stgit@dwillia2-xfh.jf.intel.com/ > >> >>> if (!np) >>> return -ENODEV; >>> >> >>> @@ -369,14 +365,12 @@ static int __init rcar_gen4_sysc_pd_init(void) >>> if (error) { >>> pr_warn("Failed to add PM subdomain %s to parent %u\n", >>> area->name, area->parent); >>> - goto out_put; >>> + return error; >>> } >>> } >>> >>> error = of_genpd_add_provider_onecell(np, &domains->onecell_data); >>> >>> -out_put: >>> - of_node_put(np); >>> return error; >> >> return of_genpd_add_provider_onecell(...); > > Ack. > > Best regards, > Krzysztof > Best regards, Krzysztof
Hi Krzysztof, On Tue, Aug 27, 2024 at 11:39 AM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > On 27/08/2024 11:33, Krzysztof Kozlowski wrote: > > On 27/08/2024 09:48, Geert Uytterhoeven wrote: > >> On Fri, Aug 23, 2024 at 2:51 PM Krzysztof Kozlowski > >> <krzysztof.kozlowski@linaro.org> wrote: > >>> Obtain the device node reference with scoped/cleanup.h to reduce error > >>> handling and make the code a bit simpler. > >>> > >>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > >> > >> Thanks for your patch! > >> > >>> --- a/drivers/pmdomain/renesas/rcar-gen4-sysc.c > >>> +++ b/drivers/pmdomain/renesas/rcar-gen4-sysc.c > >>> @@ -303,12 +304,12 @@ static int __init rcar_gen4_sysc_pd_init(void) > >>> const struct rcar_gen4_sysc_info *info; > >>> const struct of_device_id *match; > >>> struct rcar_gen4_pm_domains *domains; > >>> - struct device_node *np; > >>> void __iomem *base; > >>> unsigned int i; > >>> int error; > >>> > >>> - np = of_find_matching_node_and_match(NULL, rcar_gen4_sysc_matches, &match); > >>> + struct device_node *np __free(device_node) = > >>> + of_find_matching_node_and_match(NULL, rcar_gen4_sysc_matches, &match); > >> > >> This breaks the declarations/blank-line/code structure, so please move > >> this up. > > > > What do you mean "declaration structure"? That's the way how variables First a block with declarations, then a blank line, followed by the actual code (yeah, the pre-C99 style ;-) > > with constructors are expected to be declared - within the code. When it matters. > Continuing thoughts, so you prefer: > > struct rcar_gen4_pm_domains *domains; > void __iomem *base; > struct device_node *np __free(device_node) = > of_find_matching_node_and_match(NULL, rcar_gen4_sysc_matches, &match); > > (assuming I will put it at the end of declarations). > > Are you sure this is more readable? It's really long line so it > obfuscates a bit the declarations. The point of the scoped assignment is that > you declare it at point of need/first use. You're missing reverse Christmas tree order... > >> If you insist on keeping assignment to and validation of np together, > >> the line should be split in declaration and assignment. > > > > No, that would be inconsistent with cleanup/constructor coding style. > > Maybe this is something new, so let me bring previous discussions: [...] > > and finally it will reach documentation (although it focuses on Oh, "finally" as in not yet upstream ;-) > > unwinding process to be specific - "When the unwind order ..."): > > https://lore.kernel.org/all/171175585714.2192972.12661675876300167762.stgit@dwillia2-xfh.jf.intel.com/ "When the unwind order matters..." So it's perfectly fine to have: static int __init rcar_gen4_sysc_pd_init(void) { struct device_node *np __free(device_node) = NULL; struct rcar_gen4_pm_domains *domains; const struct rcar_gen4_sysc_info *info; const struct of_device_id *match; void __iomem *base; unsigned int i; int error; np = of_find_matching_node_and_match(NULL, rcar_gen4_sysc_matches, &match); if (!np) return -ENODEV; ... } But my first suggestion: static int __init rcar_gen4_sysc_pd_init(void) { struct device_node *np __free(device_node) = of_find_matching_node_and_match(NULL, rcar_gen4_sysc_matches, &match); struct rcar_gen4_pm_domains *domains; const struct rcar_gen4_sysc_info *info; const struct of_device_id *match; void __iomem *base; unsigned int i; int error; if (!np) return -ENODEV; ... } is safer w.r.t. to future modification. Gr{oetje,eeting}s, Geert
On 27/08/2024 12:55, Geert Uytterhoeven wrote: > > So it's perfectly fine to have: > > static int __init rcar_gen4_sysc_pd_init(void) > { > struct device_node *np __free(device_node) = NULL; > struct rcar_gen4_pm_domains *domains; > const struct rcar_gen4_sysc_info *info; > const struct of_device_id *match; > void __iomem *base; > unsigned int i; > int error; > > np = of_find_matching_node_and_match(NULL, > rcar_gen4_sysc_matches, &match); > if (!np) > return -ENODEV; > > ... > } It is not perfectly fine because it does not match the preference of having declaration with the constructor. See responses from Linus. > > But my first suggestion: > > static int __init rcar_gen4_sysc_pd_init(void) > { > struct device_node *np __free(device_node) = > of_find_matching_node_and_match(NULL, > rcar_gen4_sysc_matches, &match); > struct rcar_gen4_pm_domains *domains; > const struct rcar_gen4_sysc_info *info; > const struct of_device_id *match; > void __iomem *base; > unsigned int i; > int error; > > if (!np) > return -ENODEV; > > ... > } > > is safer w.r.t. to future modification. Indeed, sure, I will re-write it above. Best regards, Krzysztof
Hi Krzysztof, On Fri, Aug 23, 2024 at 2:51 PM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > Obtain the device node reference with scoped/cleanup.h to reduce error > handling and make the code a bit simpler. > > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Revisiting an old patch (the same applies to 10/10)... > --- a/drivers/pmdomain/renesas/rcar-gen4-sysc.c > +++ b/drivers/pmdomain/renesas/rcar-gen4-sysc.c > @@ -6,6 +6,7 @@ > */ > > #include <linux/bits.h> > +#include <linux/cleanup.h> > #include <linux/clk/renesas.h> > #include <linux/delay.h> > #include <linux/err.h> > @@ -303,12 +304,12 @@ static int __init rcar_gen4_sysc_pd_init(void) > const struct rcar_gen4_sysc_info *info; > const struct of_device_id *match; > struct rcar_gen4_pm_domains *domains; > - struct device_node *np; > void __iomem *base; > unsigned int i; > int error; > > - np = of_find_matching_node_and_match(NULL, rcar_gen4_sysc_matches, &match); > + struct device_node *np __free(device_node) = > + of_find_matching_node_and_match(NULL, rcar_gen4_sysc_matches, &match); > if (!np) > return -ENODEV; [...] > @@ -369,14 +365,12 @@ static int __init rcar_gen4_sysc_pd_init(void) > if (error) { > pr_warn("Failed to add PM subdomain %s to parent %u\n", > area->name, area->parent); > - goto out_put; > + return error; > } > } > > error = of_genpd_add_provider_onecell(np, &domains->onecell_data); np is passed to of_genpd_add_provider_onecell(), which stores a copy for later use, so I think it must not be released in case of success? I.e. both the old and the new code are wrong? > > -out_put: > - of_node_put(np); > return error; > } > early_initcall(rcar_gen4_sysc_pd_init); Gr{oetje,eeting}s, Geert
diff --git a/drivers/pmdomain/renesas/rcar-gen4-sysc.c b/drivers/pmdomain/renesas/rcar-gen4-sysc.c index 66409cff2083..4ca85dbdedc2 100644 --- a/drivers/pmdomain/renesas/rcar-gen4-sysc.c +++ b/drivers/pmdomain/renesas/rcar-gen4-sysc.c @@ -6,6 +6,7 @@ */ #include <linux/bits.h> +#include <linux/cleanup.h> #include <linux/clk/renesas.h> #include <linux/delay.h> #include <linux/err.h> @@ -303,12 +304,12 @@ static int __init rcar_gen4_sysc_pd_init(void) const struct rcar_gen4_sysc_info *info; const struct of_device_id *match; struct rcar_gen4_pm_domains *domains; - struct device_node *np; void __iomem *base; unsigned int i; int error; - np = of_find_matching_node_and_match(NULL, rcar_gen4_sysc_matches, &match); + struct device_node *np __free(device_node) = + of_find_matching_node_and_match(NULL, rcar_gen4_sysc_matches, &match); if (!np) return -ENODEV; @@ -317,17 +318,14 @@ static int __init rcar_gen4_sysc_pd_init(void) base = of_iomap(np, 0); if (!base) { pr_warn("%pOF: Cannot map regs\n", np); - error = -ENOMEM; - goto out_put; + return -ENOMEM; } rcar_gen4_sysc_base = base; domains = kzalloc(sizeof(*domains), GFP_KERNEL); - if (!domains) { - error = -ENOMEM; - goto out_put; - } + if (!domains) + return -ENOMEM; domains->onecell_data.domains = domains->domains; domains->onecell_data.num_domains = ARRAY_SIZE(domains->domains); @@ -345,10 +343,8 @@ static int __init rcar_gen4_sysc_pd_init(void) n = strlen(area->name) + 1; pd = kzalloc(sizeof(*pd) + n, GFP_KERNEL); - if (!pd) { - error = -ENOMEM; - goto out_put; - } + if (!pd) + return -ENOMEM; memcpy(pd->name, area->name, n); pd->genpd.name = pd->name; @@ -357,7 +353,7 @@ static int __init rcar_gen4_sysc_pd_init(void) error = rcar_gen4_sysc_pd_setup(pd); if (error) - goto out_put; + return error; domains->domains[area->pdr] = &pd->genpd; @@ -369,14 +365,12 @@ static int __init rcar_gen4_sysc_pd_init(void) if (error) { pr_warn("Failed to add PM subdomain %s to parent %u\n", area->name, area->parent); - goto out_put; + return error; } } error = of_genpd_add_provider_onecell(np, &domains->onecell_data); -out_put: - of_node_put(np); return error; } early_initcall(rcar_gen4_sysc_pd_init);
Obtain the device node reference with scoped/cleanup.h to reduce error handling and make the code a bit simpler. Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> --- drivers/pmdomain/renesas/rcar-gen4-sysc.c | 26 ++++++++++---------------- 1 file changed, 10 insertions(+), 16 deletions(-)