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 <[email protected]>
> ---
> 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