linux-kernel - Re: [PATCH] clk: iproc: Do not rely on node name for correct PLL setup

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>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <d2ad5cda0e9786e43d5cd235e5dcec59@milecki.pl>
Date:   Mon, 05 Sep 2022 09:15:28 +0200
From:   Rafał Miłecki <rafal@...ecki.pl>
To:     Florian Fainelli <f.fainelli@...il.com>
Cc:     linux-clk@...r.kernel.org,
        Michael Turquette <mturquette@...libre.com>,
        Stephen Boyd <sboyd@...nel.org>, Ray Jui <rjui@...adcom.com>,
        Scott Branden <sbranden@...adcom.com>,
        Broadcom internal kernel review list 
        <bcm-kernel-feedback-list@...adcom.com>,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] clk: iproc: Do not rely on node name for correct PLL
 setup

On 2022-09-04 17:00, Rafał Miłecki wrote:
> On 2022-08-03 04:58, Florian Fainelli wrote:
>> After commit 31fd9b79dc58 ("ARM: dts: BCM5301X: update CRU block
>> description") a warning from clk-iproc-pll.c was generated due to a
>> duplicate PLL name as well as the console stopped working. Upon closer
>> inspection it became clear that iproc_pll_clk_setup() used the Device
>> Tree node unit name as an unique identifier as well as a parent name 
>> to
>> parent all clocks under the PLL.
>> 
>> BCM5301X was the first platform on which that got noticed because of 
>> the
>> DT node unit name renaming but the same assumptions hold true for any
>> user of the iproc_pll_clk_setup() function.
>> 
>> The first 'clock-output-names' property is always guaranteed to be
>> unique as well as providing the actual desired PLL clock name, so we
>> utilize that to register the PLL and as a parent name of all children
>> clock.
>> 
>> Fixes: 5fe225c105fd ("clk: iproc: add initial common clock support")
>> Signed-off-by: Florian Fainelli <f.fainelli@...il.com>
> 
> Acked-by: Rafał Miłecki <rafal@...ecki.pl>
> 
> 
> Thanks for looking into this!
> 
> In the past I debugged this too and even developed a simple fix:
> clk & clock-controller@ DT nodes: __clk_core_init: clk
> clock-controller already initialized
> https://www.spinics.net/lists/linux-clk/msg63855.html
> 
> For some reason my old fix didn't work with usbclk clock.

I compared your changes with my old attempt and found the missing bit. I
forgot about updating parent_name.

FWIW something like below would work too.

Thanks again for taking care of this old regression.


diff --git a/drivers/clk/bcm/clk-iproc-pll.c 
b/drivers/clk/bcm/clk-iproc-pll.c
index 33da30f99..af9ca32b8 100644
--- a/drivers/clk/bcm/clk-iproc-pll.c
+++ b/drivers/clk/bcm/clk-iproc-pll.c
@@ -783,7 +783,7 @@ void iproc_pll_clk_setup(struct device_node *node,
  	iclk = &iclk_array[0];
  	iclk->pll = pll;

-	init.name = node->name;
+	init.name = node->full_name;
  	init.ops = &iproc_pll_ops;
  	init.flags = 0;
  	parent_name = of_clk_get_parent_name(node, 0);
@@ -809,7 +809,7 @@ void iproc_pll_clk_setup(struct device_node *node,
  		const char *clk_name;

  		memset(&init, 0, sizeof(init));
-		parent_name = node->name;
+		parent_name = node->full_name;

  		ret = of_property_read_string_index(node, "clock-output-names",
  						    i, &clk_name);