Messages in this thread | | | Subject | Re: [PATCH 3/4] clk/Renesas-MSTP: Less function calls in cpg_mstp_clocks_init() after error detection | From | SF Markus Elfring <> | Date | Thu, 15 Sep 2016 22:40:22 +0200 |
| |
> It's perfectly legal to call kfree() on a NULL pointer.
I know this function property well.
>> * Split a condition check for memory allocation failures so that >> each pointer from these function calls will be checked immediately. >> >> See also background information: >> Topic "CWE-754: Improper check for unusual or exceptional conditions" >> Link: https://cwe.mitre.org/data/definitions/754.html >> >> * Return directly after a call of the function "kzalloc" failed >> at the beginning. > > Both calls are already close together.
Can it be that an other software development concern is eventually overlooked because of this "neighbourship" (or is categorised with a lower priority)?
I suggest to reconsider this design detail if it is really acceptable for the safe implementation of such a software module.
* How much will it matter in general that one function call was performed in this use case without checking its return values immediately?
* Should it usually be determined quicker if a required resource like memory could be acquired before trying the next allocation?
> In addition, your patch increases the LoC, IMHO without improving the code.
I find this consequence still debatable.
>> diff --git a/drivers/clk/renesas/clk-mstp.c b/drivers/clk/renesas/clk-mstp.c >> index 1fdc44b..6c82e0e 100644 >> --- a/drivers/clk/renesas/clk-mstp.c >> +++ b/drivers/clk/renesas/clk-mstp.c >> @@ -167,10 +167,12 @@ static void __init cpg_mstp_clocks_init(struct device_node *np) >> unsigned int i; >> >> group = kzalloc(sizeof(*group), GFP_KERNEL); >> + if (!group) >> + return; >> + >> clks = kmalloc_array(MSTP_MAX_CLOCKS, sizeof(*clks), GFP_KERNEL); >> - if (group == NULL || clks == NULL) { >> + if (!clks) { >> kfree(group); >> - kfree(clks); >> return; >> }
Is this update suggestion worth for another look?
Regards, Markus
| |