Message ID | 20221018160352.1591428-1-dario.binacchi@amarulasolutions.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:4ac7:0:0:0:0:0 with SMTP id y7csp2040552wrs; Tue, 18 Oct 2022 09:12:24 -0700 (PDT) X-Google-Smtp-Source: AMsMyM71c5g7vmn5mCBhElP/h4Nn21igSwE7m5PaFmzODv/1vbZX1/RR6taeHhc6FlHNl3JU2r6k X-Received: by 2002:a17:902:b708:b0:184:3921:df30 with SMTP id d8-20020a170902b70800b001843921df30mr3797361pls.43.1666109543709; Tue, 18 Oct 2022 09:12:23 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1666109543; cv=none; d=google.com; s=arc-20160816; b=FGOh3C+3vU+aPdUN3Tuh+qgVUWn5gfkej5aH75kR5oZKkZEmzeqV1FexWVmX4trVId SHX4gQ9W4OeR4OEfvW+8WC+VzmXALU361F4T6zLzL13ObTLrCmMARPn7ePhJFmSb6maE lNVA2viDY4OTzvDUBa6pQ5Fd45OyUvvML+Ym7T/zEWjPXUeTr/56Rn8vT6mCBaqNqcFU l72a9iHGh/iVjEZVYa73rsY46I7C2jtfJvpZFFgON1/zVeMzMuhiK+lXIPcqgpk4we1S DmAEa0G7imDuZiKg5NsAUZtSo8ndltEA9DVS/cx7BIX2PxgsXnR9HScl3FD7CumSs2PZ KXwA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from:dkim-signature; bh=8JlbMErcGNycCCtodyD6GpAsyH/iGxyyMdZWiKDQlaU=; b=ybfL2P66hsLWglNzkxslkpm/FjRrTlyS0DRDY6NwTQTa96pH6daLZzdF2NlgTa7p8e SITU5QojleXFI5EWHPvnSMQWi6mt1CBtyFQ2TpJHa3V8Qv/uMry/dM59BUw10YevJnHv qGuB0FTe1VCnqP2yuIfMnGH7Fjbbw+GzTn4diLsK9jEa8dmwRGcwazXPkGafF4Yz4oIT l5r98BZJoXPJAVrmPpE7R0fYIEgRxIot+C15g5fH181us8d36958cGlOuyBy1bDU6eh7 Hlo3ikh2FyjEmEbZJQLTcvnD+E5W3+v80YWs2wFoX8yx1U4a0mX3rSjuFSNu2lrD0dJi 2iIQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@amarulasolutions.com header.s=google header.b=mxSI9U6A; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=amarulasolutions.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id c21-20020a63d515000000b00448f268c5c6si15705078pgg.191.2022.10.18.09.12.01; Tue, 18 Oct 2022 09:12:23 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@amarulasolutions.com header.s=google header.b=mxSI9U6A; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=amarulasolutions.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230059AbiJRQEF (ORCPT <rfc822;carlos.wei.hk@gmail.com> + 99 others); Tue, 18 Oct 2022 12:04:05 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:35024 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231258AbiJRQEB (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Tue, 18 Oct 2022 12:04:01 -0400 Received: from mail-ed1-x530.google.com (mail-ed1-x530.google.com [IPv6:2a00:1450:4864:20::530]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 1212E9A2B9 for <linux-kernel@vger.kernel.org>; Tue, 18 Oct 2022 09:03:57 -0700 (PDT) Received: by mail-ed1-x530.google.com with SMTP id t16so1943242edd.2 for <linux-kernel@vger.kernel.org>; Tue, 18 Oct 2022 09:03:57 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=amarulasolutions.com; s=google; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:from:to:cc:subject:date:message-id:reply-to; bh=8JlbMErcGNycCCtodyD6GpAsyH/iGxyyMdZWiKDQlaU=; b=mxSI9U6ANwBzyr2DzsUzEZ5jHidPM/I7Z91CgyqWEgjleLppoOBKuSA3cHaE6L7Vki UrFxXOXbRGPO6Fj91mhIQOqLC7PRzKhDFMUX6/7XYewOA17AIyLTwyxRRLEe5rB55XXk 9WHTSp6t0S0SQRb4u99G6OnjP7E70rJxF+Kns= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=8JlbMErcGNycCCtodyD6GpAsyH/iGxyyMdZWiKDQlaU=; b=E5QL3x/+ex72k12Edglzn23zggpUoth8+0ixakwPXG/SUXlQPCwuWnOzV6Umg0NlQG 1uS8XDqwdvwVd/UcLpLypa/zQl+/MFn3H5Td5Ms+n3nmudgDgkNHHnLTSSo0p2FvmhKu y5cQyc48IGV46CQ702hXYNd0LoFUH/L0yQITAtIZxUCkTklGAV9O1TMq6I6sQdIlgH2S yS5tEEhIo3sTpXHxddOvnrm4lIFr1AbrZ4VDzUy2f4G6jY922JvRfkNEk29LgKo8n1Zg qWgmsni6fyc0VPlvc7HhggbYhPwqldBB0e6CCGGQS0X5ajcF6goS/zPPl64NgP3XhsrF xcYA== X-Gm-Message-State: ACrzQf2r/zm/+zL0b8Qr4wcCitGLARvIw/pEFPrsgpLDQruwmORz4tbY SeGCdw+rwzVi7bYalVe2/DqJydpfEHPRJA== X-Received: by 2002:a05:6402:34cd:b0:45d:a345:764 with SMTP id w13-20020a05640234cd00b0045da3450764mr3286646edc.415.1666109035532; Tue, 18 Oct 2022 09:03:55 -0700 (PDT) Received: from dario-ThinkPad-T14s-Gen-2i.homenet.telecomitalia.it (host-95-244-101-110.retail.telecomitalia.it. [95.244.101.110]) by smtp.gmail.com with ESMTPSA id j10-20020a17090623ea00b007919ba4295esm1166014ejg.216.2022.10.18.09.03.54 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 18 Oct 2022 09:03:54 -0700 (PDT) From: Dario Binacchi <dario.binacchi@amarulasolutions.com> To: linux-kernel@vger.kernel.org Cc: Amarula patchwork <linux-amarula@amarulasolutions.com>, michael@amarulasolutions.com, Dario Binacchi <dario.binacchi@amarulasolutions.com>, kernel test robot <lkp@intel.com>, Allison Randal <allison@lohutok.net>, Miaoqian Lin <linmq006@gmail.com>, Michael Turquette <mturquette@baylibre.com>, Stephen Boyd <sboyd@kernel.org>, Tero Kristo <kristo@kernel.org>, Thomas Gleixner <tglx@linutronix.de>, Tony Lindgren <tony@atomide.com>, linux-clk@vger.kernel.org, linux-omap@vger.kernel.org Subject: [PATCH v2] clk: ti: dra7-atl: don't allocate `parent_names' variable Date: Tue, 18 Oct 2022 18:03:52 +0200 Message-Id: <20221018160352.1591428-1-dario.binacchi@amarulasolutions.com> X-Mailer: git-send-email 2.32.0 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE, SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1747042480843498253?= X-GMAIL-MSGID: =?utf-8?q?1747042480843498253?= |
Series |
[v2] clk: ti: dra7-atl: don't allocate `parent_names' variable
|
|
Commit Message
Dario Binacchi
Oct. 18, 2022, 4:03 p.m. UTC
The `parent_names' variable was freed also in case of kzalloc() error.
Instead of modifying the code to perform a proper memory release, I
decided to fix the bug by not allocating memory.
Since only one parent name is referenced, it is not necessary to
allocate this variable at runtime and therefore you can avoid calling
the kzalloc() function. This simplifies the code (even calls to kfree
can be removed) and improves the performance of the routine.
Note: Although no operation is performed by kfree() on a NULL pointer,
it was however suboptimal and semantically wrong doing it.
Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
Reported-by: kernel test robot <lkp@intel.com>
---
Changes in v2:
- Fix compiling error
- Add kernel test robot's Reported-by tag.
drivers/clk/ti/clk-dra7-atl.c | 11 ++---------
1 file changed, 2 insertions(+), 9 deletions(-)
Comments
Quoting Dario Binacchi (2022-10-18 09:03:52) > diff --git a/drivers/clk/ti/clk-dra7-atl.c b/drivers/clk/ti/clk-dra7-atl.c > index ff4d6a951681..78482d1a4a33 100644 > --- a/drivers/clk/ti/clk-dra7-atl.c > +++ b/drivers/clk/ti/clk-dra7-atl.c > @@ -188,24 +188,17 @@ static void __init of_dra7_atl_clock_setup(struct device_node *node) > goto cleanup; > } > > - parent_names = kzalloc(sizeof(char *), GFP_KERNEL); > - > - if (!parent_names) > - goto cleanup; > - > parent_names[0] = of_clk_get_parent_name(node, 0); Can you use struct clk_parent_data instead and assign index to 0? Then we don't even need to use of_clk_get_parent_name() here.
Hi Stephen, On Fri, Oct 28, 2022 at 2:27 AM Stephen Boyd <sboyd@kernel.org> wrote: > > Quoting Dario Binacchi (2022-10-18 09:03:52) > > diff --git a/drivers/clk/ti/clk-dra7-atl.c b/drivers/clk/ti/clk-dra7-atl.c > > index ff4d6a951681..78482d1a4a33 100644 > > --- a/drivers/clk/ti/clk-dra7-atl.c > > +++ b/drivers/clk/ti/clk-dra7-atl.c > > @@ -188,24 +188,17 @@ static void __init of_dra7_atl_clock_setup(struct device_node *node) > > goto cleanup; > > } > > > > - parent_names = kzalloc(sizeof(char *), GFP_KERNEL); > > - > > - if (!parent_names) > > - goto cleanup; > > - > > parent_names[0] = of_clk_get_parent_name(node, 0); > > Can you use struct clk_parent_data instead and assign index to 0? Then > we don't even need to use of_clk_get_parent_name() here. I tried to test your suggestions on another platform (I don't have the hw to test the driver change) but if I don't add pdata.name = of_clk_get_parent_name () the board boot up fails. As far I can see from the clk_core_populate_parent_map() .... /* Copy everything over because it might be __initdata */ for (i = 0, parent = parents; i < num_parents; i++, parent++) { parent->index = -1; if (parent_names) { /* throw a WARN if any entries are NULL */ WARN(!parent_names[i], "%s: invalid NULL in %s's .parent_names\n", __func__, core->name); ret = clk_cpy_name(&parent->name, parent_names[i], true); } else if (parent_data) { parent->hw = parent_data[i].hw; parent->index = parent_data[i].index; ret = clk_cpy_name(&parent->fw_name, parent_data[i].fw_name, false); if (!ret) ret = clk_cpy_name(&parent->name, parent_data[i].name, false); ... The function clk_cpy_name() is called with the parameter "mus_exist" to true in the path "parent_names" and false in the path "parent_data". Therefore, in the path "parent_data" it is allowed that parent-> name is not set. In doing so, therefore, the change would not even be backward compatible. So, IMHO, there are 2 possible options: 1 okay to use parent_data, but we keep using of_clk_get_parent_name () to set parent_data::name. 2 okay to use the version v2 of the patch. What do you think? Thanks and regards, Dario
Quoting Dario Binacchi (2022-10-30 06:00:46) > > I tried to test your suggestions on another platform (I don't have the > hw to test the driver change) but if I > don't add pdata.name = of_clk_get_parent_name () the board boot up fails. > > As far I can see from the clk_core_populate_parent_map() > > .... > /* Copy everything over because it might be __initdata */ > for (i = 0, parent = parents; i < num_parents; i++, parent++) { > parent->index = -1; > if (parent_names) { > /* throw a WARN if any entries are NULL */ > WARN(!parent_names[i], > "%s: invalid NULL in %s's .parent_names\n", > __func__, core->name); > ret = clk_cpy_name(&parent->name, parent_names[i], > true); > } else if (parent_data) { > parent->hw = parent_data[i].hw; > parent->index = parent_data[i].index; > ret = clk_cpy_name(&parent->fw_name, > parent_data[i].fw_name, false); > if (!ret) > ret = clk_cpy_name(&parent->name, > parent_data[i].name, > false); > ... > > > The function clk_cpy_name() is called with the parameter "mus_exist" > to true in the path "parent_names" and false > in the path "parent_data". Therefore, in the path "parent_data" it is > allowed that parent-> name is not set. > In doing so, therefore, the change would not even be backward compatible. > > So, IMHO, there are 2 possible options: > 1 okay to use parent_data, but we keep using of_clk_get_parent_name > () to set parent_data::name. > 2 okay to use the version v2 of the patch. > > What do you think? I am confused. The struct clk_parent_data::name being used is whatever string is returned by of_clk_get_parent_name(node, 0). That is the same as setting struct clk_parent_data::index to 0, and not assigning the 'name' or 'fw_name' field of the parent data structure. This is a compatible change because of_clk_get_parent_name() is getting the name of the clk in 'clocks' for 'node' at index 0. Using the index 0 in clk_parent_data tells clk framework that the parent of the clk being registered is the clk in 'clocks' for the 'dev->node' that is passed in during clk_register(). If you don't have a device pointer, use of_clk_hw_register() to pass 'node' directly. It looks like you will have to do that in this case to get the node pointer registered with this clk.
diff --git a/drivers/clk/ti/clk-dra7-atl.c b/drivers/clk/ti/clk-dra7-atl.c index ff4d6a951681..78482d1a4a33 100644 --- a/drivers/clk/ti/clk-dra7-atl.c +++ b/drivers/clk/ti/clk-dra7-atl.c @@ -164,7 +164,7 @@ static void __init of_dra7_atl_clock_setup(struct device_node *node) { struct dra7_atl_desc *clk_hw = NULL; struct clk_init_data init = { NULL }; - const char **parent_names = NULL; + const char *parent_names[1]; const char *name; struct clk *clk; @@ -188,24 +188,17 @@ static void __init of_dra7_atl_clock_setup(struct device_node *node) goto cleanup; } - parent_names = kzalloc(sizeof(char *), GFP_KERNEL); - - if (!parent_names) - goto cleanup; - parent_names[0] = of_clk_get_parent_name(node, 0); init.parent_names = parent_names; clk = ti_clk_register(NULL, &clk_hw->hw, name); - if (!IS_ERR(clk)) { of_clk_add_provider(node, of_clk_src_simple_get, clk); - kfree(parent_names); return; } + cleanup: - kfree(parent_names); kfree(clk_hw); } CLK_OF_DECLARE(dra7_atl_clock, "ti,dra7-atl-clock", of_dra7_atl_clock_setup);