linux-kernel - Re: [PATCH resent] clk: at91: sama7g5: Add two jump labels in sama7g5_pmc

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Message-ID: <7890284f-5809-2f46-9d5f-52e20a3ec327@microchip.com>
Date:   Tue, 28 Mar 2023 10:30:34 +0200
From:   Nicolas Ferre <nicolas.ferre@...rochip.com>
To:     Markus Elfring <Markus.Elfring@....de>,
        <kernel-janitors@...r.kernel.org>, <linux-clk@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Claudiu Beznea <claudiu.beznea@...rochip.com>,
        Michael Turquette <mturquette@...libre.com>,
        Stephen Boyd <sboyd@...nel.org>
CC:     <cocci@...ia.fr>, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH resent] clk: at91: sama7g5: Add two jump labels in
 sama7g5_pmc_setup()

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@...rs.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