| Message ID | 9e3705dc-7a70-c584-916e-ae582c7667b6@web.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [resent] clk: at91: sama7g5: Add two jump labels in sama7g5_pmc_setup() | expand |
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 >
>> 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. Regards, Markus
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.
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); }
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. 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(-) -- 2.40.0