Re: [PATCH resent] clk: at91: sama7g5: Add two jump labels in sama7g5_pmc_setup()

Re: [PATCH resent] clk: at91: sama7g5: Add two jump labels in sama7g5_pmc_setup()

[Nicolas Ferre]

On 25/03/2023 at 15:05, Markus Elfring wrote:
> Date: Fri, 17 Mar 2023 20:02:34 +0100
> 
> The label “err_free” was used to jump to another pointer check despite of
> the detail in the implementation of the function “sama7g5_pmc_setup”
> that it was determined already that the corresponding variable contained
> a null pointer (because of a failed memory allocation).
> 
> * Thus use additional labels.
> 
> * Delete an extra pointer check at the end which became unnecessary
>    with this refactoring.
> 
> This issue was detected by using the Coccinelle software.

Fine, but I'm sorry that it complexity the function for no real value. 
Other clk drivers have the same pattern so I want them to all stay the same.

This is a NACK, sorry about that.

Regards,
   Nicolas

> Fixes: cb783bbbcf54c36256006895c215e86c5e7266d8 ("clk: at91: sama7g5: add clock support for sama7g5")
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>   drivers/clk/at91/sama7g5.c | 23 +++++++++++------------
>   1 file changed, 11 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/clk/at91/sama7g5.c b/drivers/clk/at91/sama7g5.c
> index f135b662f1ff..224b1f2ebef2 100644
> --- a/drivers/clk/at91/sama7g5.c
> +++ b/drivers/clk/at91/sama7g5.c
> @@ -927,25 +927,25 @@ static void __init sama7g5_pmc_setup(struct device_node *np)
>                              (ARRAY_SIZE(sama7g5_mckx) + ARRAY_SIZE(sama7g5_gck)),
>                              GFP_KERNEL);
>          if (!alloc_mem)
> -               goto err_free;
> +               goto free_pmc;
> 
>          hw = at91_clk_register_main_rc_osc(regmap, "main_rc_osc", 12000000,
>                                             50000000);
>          if (IS_ERR(hw))
> -               goto err_free;
> +               goto free_alloc_mem;
> 
>          bypass = of_property_read_bool(np, "atmel,osc-bypass");
> 
>          hw = at91_clk_register_main_osc(regmap, "main_osc", mainxtal_name,
>                                          bypass);
>          if (IS_ERR(hw))
> -               goto err_free;
> +               goto free_alloc_mem;
> 
>          parent_names[0] = "main_rc_osc";
>          parent_names[1] = "main_osc";
>          hw = at91_clk_register_sam9x5_main(regmap, "mainck", parent_names, 2);
>          if (IS_ERR(hw))
> -               goto err_free;
> +               goto free_alloc_mem;
> 
>          sama7g5_pmc->chws[PMC_MAIN] = hw;
> 
> @@ -987,7 +987,7 @@ static void __init sama7g5_pmc_setup(struct device_node *np)
>                          }
> 
>                          if (IS_ERR(hw))
> -                               goto err_free;
> +                               goto free_alloc_mem;
> 
>                          if (sama7g5_plls[i][j].eid)
>                                  sama7g5_pmc->chws[sama7g5_plls[i][j].eid] = hw;
> @@ -999,7 +999,7 @@ static void __init sama7g5_pmc_setup(struct device_node *np)
>                                            &mck0_layout, &mck0_characteristics,
>                                            &pmc_mck0_lock, CLK_GET_RATE_NOCACHE, 5);
>          if (IS_ERR(hw))
> -               goto err_free;
> +               goto free_alloc_mem;
> 
>          sama7g5_pmc->chws[PMC_MCK] = hw;
> 
> @@ -1128,12 +1128,11 @@ static void __init sama7g5_pmc_setup(struct device_node *np)
>          return;
> 
>   err_free:
> -       if (alloc_mem) {
> -               for (i = 0; i < alloc_mem_size; i++)
> -                       kfree(alloc_mem[i]);
> -               kfree(alloc_mem);
> -       }
> -
> +       for (i = 0; i < alloc_mem_size; i++)
> +               kfree(alloc_mem[i]);
> +free_alloc_mem:
> +       kfree(alloc_mem);
> +free_pmc:
>          kfree(sama7g5_pmc);
>   }
> 
> --
> 2.40.0
> 

-- 
Nicolas Ferre

Re: [PATCH resent] clk: at91: sama7g5: Add two jump labels in sama7g5_pmc_setup()

[Alexandre Belloni]

On 28/03/2023 21:24:00+0200, Markus Elfring wrote:
> >> The label “err_free” was used to jump to another pointer check despite of
> >> the detail in the implementation of the function “sama7g5_pmc_setup”
> >> that it was determined already that the corresponding variable contained
> >> a null pointer (because of a failed memory allocation).
> >>
> >> * Thus use additional labels.
> >>
> >> * Delete an extra pointer check at the end which became unnecessary
> >>    with this refactoring.
> >>
> >> This issue was detected by using the Coccinelle software.
> >
> > Fine, but I'm sorry that it complexity the function for no real value.
> 
> Under which circumstances can advice be taken better into account
> also from another information source?
> https://wiki.sei.cmu.edu/confluence/display/c/MEM12-C.+Consider+using+a+goto+chain+when+leaving+a+function+on+error+when+using+and+releasing+resources#MEM12C.Considerusingagotochainwhenleavingafunctiononerrorwhenusingandreleasingresources-CompliantSolution%28POSIX,GotoChain%29
> 
> 
> 
> > Other clk drivers have the same pattern so I want them to all stay the same.
> > This is a NACK, sorry about that.
> 
> I am curious if other contributors (or code reviewers) would like to influence
> the software situation a bit more.
> 

I agree with Nicolas here, this is useless churn.


-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com