From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756366AbcIOUky (ORCPT ); Thu, 15 Sep 2016 16:40:54 -0400 Received: from mout.web.de ([217.72.192.78]:56184 "EHLO mout.web.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751241AbcIOUkq (ORCPT ); Thu, 15 Sep 2016 16:40:46 -0400 Subject: Re: [PATCH 3/4] clk/Renesas-MSTP: Less function calls in cpg_mstp_clocks_init() after error detection To: Geert Uytterhoeven References: <566ABCD9.1060404@users.sourceforge.net> <1575ddf1-bd1a-fe98-046a-a586eb30fe47@users.sourceforge.net> Cc: linux-clk , Geert Uytterhoeven , Laurent Pinchart , Michael Turquette , Simon Horman , Stephen Boyd , Ulf Hansson , LKML , "kernel-janitors@vger.kernel.org" , Julia Lawall From: SF Markus Elfring Message-ID: <50646b53-663f-f77f-a79e-2422b5687688@users.sourceforge.net> Date: Thu, 15 Sep 2016 22:40:22 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.3.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-Provags-ID: V03:K0:oCyxO8/dqEW4e1UFr3g21RiQl1SU62+PvkUyWIjl2MOdQGeld6K fm/QxCtJU/GCjUJ2/r3mIQlq07uNaqc7M2I7KcxX+xRd4/k45QBfT1LG5yOY94jm3RTvvvF hYDwxx+aq0t6R2NZqR+M5Bd5GmR26pDd28fWf9PvNaMSkQzAQKMHzDufSQIDuOPkG7ELdyc mWs2Q8lHbGwakDMnygGdg== X-UI-Out-Filterresults: notjunk:1;V01:K0:w/ipSykjCD0=:sfyi+siFFrj2vVBAbZOpyI YOp4LXJG/MA9iwAEF+FaFMf8fZ9Zu3DxFY7KG6C/9/jE1O9Xh7ZPWtVDxX6F1OHCHouGlo9JV ibE4RTkJDKL2a3DXs39QW/FHrnLVDMoq6vB1Su7w8saZ8udFPCcF4bOl3iNkfBFIcN1CAqGz6 wSqWBFFsIyIFOyIkFXj6b5ASF6/T/UWdhFK8kRkhLESsScJeFofBQtWRaQaLiZWMsMd/fBMdu GcD4hjp/lNmDN6+75wYDkPU8FcE9Feq3vB4NWTq7qTpUewKpy0rgGhRrBqLnkhjIg9qmdkWzb ZOQxW89j0JvsA85Bh3H9CTGejwN/JHZlaS8luP8rSsvMu8jg79bH5iiVF+eabWrinkhSAB8qX 8p3owTdzEXW9y2dOM3fQ2o6Feqkl0E45QzTtyy3jVQhylW4yXFUMKKmQhOBregJiJrVSAwfne c5yYxsyfi9zWoPX+nf5TeUTEPil9TSEiOdAn3AlMkU7F5RpwACGlPrK7q+7fzkWTYMfqrKPeE zeIY95iijB3+g8jnGjmASlzCJAjQvKidaIQsfdq1M1qs9TGtRXwZjUN/BhlR8WmZ9av9Q3t04 xDWJrIssmvGHlH8iMeyi5Ko+E0eMa45JFTJO9AsdSBIHhPAUU5+Z5MvxD4sy0/lPDQmloEZ06 TvX1DeNToGDWh6y5X6oN/lJTrRqthgJT+wp+wpuYc5RMqnWsB/T6azpDyrV3N2KTV56l+LY5k wwOSjJX3j9mmCy2TMMuwwxCWvI7lLY35Ikay/dNXnyeE0gmgv2LkXILPQMFCBDJ2ySpEHG1V8 0dJImw6 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org > 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