Message-ID: <63ed6e27.050a0220.ed51a.aae8@mx.google.com>
Date:   Thu, 16 Feb 2023 00:34:51 +0100
From:   Christian Marangi <ansuelsmth@gmail.com>
To:     Michael Turquette <mturquette@baylibre.com>,
        Stephen Boyd <sboyd@kernel.org>, linux-clk@vger.kernel.org,
        linux-kernel@vger.kernel.org
Cc:     Miquel Raynal <miquel.raynal@bootlin.com>,
        Jerome Brunet <jbrunet@baylibre.com>,
        Russell King <linux@armlinux.org.uk>,
        Jeffrey Hugo <jhugo@codeaurora.org>,
        Chen-Yu Tsai <wens@csie.org>
Subject: Re: [PATCH] clk: Fix wrong clock returned in parent_data with .name
 and no .index
References: <20230215232712.17072-1-ansuelsmth@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230215232712.17072-1-ansuelsmth@gmail.com>
Precedence: bulk

On Thu, Feb 16, 2023 at 12:27:12AM +0100, Christian Marangi wrote:
> Commit 601b6e93304a ("clk: Allow parents to be specified via clkspec index")
> introduced a regression due to a "fragile" implementation present in some very
> corner case.
> 
> Such commit introduced the support for parents to be specified using
> clkspec index. The index is an int and should be -1 if the feature
> should not be used. This is the case with parent_hws or legacy
> parent_names used and the index value is set to -1 by default.
> With parent_data the situation is different, since it's a struct that
> can have multiple value (.index, .name, .fw_name), it's init to all 0 by
> default. This cause the index value to be set to 0 everytime even if not
> intended to be defined and used.
> 
> This simple "fragile" implementation cause side-effect and unintended
> behaviour.
> 
> Assuming the following scenario (to repro the corner case and doesn't
> reflect real code):
> 
> In dt we have a node like this:
> 		acc1: clock-controller@2098000 {
> 			compatible = "qcom,kpss-acc-v1";
> 			reg = <0x02098000 0x1000>, <0x02008000 0x1000>;
> 			clock-output-names = "acpu1_aux";
> 			clocks = <&pxo_board>;
> 			clock-names = "pxo";
> 			#clock-cells = <0>;
> 		};
> 
> And on the relevant driver we have the parent data defined as such:
> 		static const struct clk_parent_data aux_parents[] = {
> 			{ .name = "pll8_vote" },
> 			{ .fw_name = "pxo", .name = "pxo_board" },
> 		};
> 
> Someone would expect the first parent to be globally searched and set to
> point to the clock named "pll8_vote".
> But this is not the case and instead under the hood, the parent point to
> the pxo clock. This happen without any warning and was discovered on
> another platform while the gcc driver was converted to parent_data and
> only .name was defined.
> 
> The reason is hard to discover but very simple.
> 
> Due to the introduction of index support, clk_core_get() won't return
> -ENOENT but insted will correctly return a clock.
> This is because of_parse_clkspec() will use the index (that is set to 0
> due to how things are allocated) and correctly find in the DT node a
> clock at index 0. That in the provided example is exactly the phandle to
> pxo_board.
> 
> Clock is found so the parent is now wrongly linked to the pxo_board
> clock.
> 
> This only happens in this specific scenario but it's still worth to be
> handled and currently there are some driver that hardcode the
> parent_data and may suffer from this.
> 
> To fix this and handle it correctly we can use the following logic:
> 1. With a .fw_name not defined (index searching is skipped if a named
>    clock is provided)
> 2. Check if .name is provided
> 3. Compare the provided .name with what clockspec found
> 4. Return -ENOENT if the name doesn't match, return the clock if it does.
> 
> Returning -ENOENT permit clk core code flow to fallback to global
> searching and correctly search the right clock.
> 
> Fixes: 601b6e93304a ("clk: Allow parents to be specified via clkspec index")
> Cc: Miquel Raynal <miquel.raynal@bootlin.com>
> Cc: Jerome Brunet <jbrunet@baylibre.com>
> Cc: Russell King <linux@armlinux.org.uk>
> Cc: Michael Turquette <mturquette@baylibre.com>
> Cc: Jeffrey Hugo <jhugo@codeaurora.org>
> Cc: Chen-Yu Tsai <wens@csie.org>
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>

Think this should also be backported to stable kernel just like it was
done with 4f8c6aba37da199155a121c6cdc38505a9eb0259 ?

> ---
>  drivers/clk/clk.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 998676d78029..42e297fcfe45 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -395,6 +395,7 @@ of_clk_get_hw_from_clkspec(struct of_phandle_args *clkspec)
>   */
>  static struct clk_core *clk_core_get(struct clk_core *core, u8 p_index)
>  {
> +	const char *global_name = core->parents[p_index].name;
>  	const char *name = core->parents[p_index].fw_name;
>  	int index = core->parents[p_index].index;
>  	struct clk_hw *hw = ERR_PTR(-ENOENT);
> @@ -407,6 +408,23 @@ static struct clk_core *clk_core_get(struct clk_core *core, u8 p_index)
>  	    !of_parse_clkspec(np, index, name, &clkspec)) {
>  		hw = of_clk_get_hw_from_clkspec(&clkspec);
>  		of_node_put(clkspec.np);
> +
> +		/*
> +		 * The returned hw may be incorrect and extra check are required in
> +		 * some corner case.
> +		 *
> +		 * In case a .fw_name is not set of_parse_clkspec will use the index
> +		 * to search the related clock.
> +		 * But index may be never set and actually never intended to be used
> +		 * in the defined parent_data since a 0 value is also accepted and that
> +		 * is what by default each struct is initialized.
> +		 *
> +		 * In the following case check if we have .name and check if the returned
> +		 * clock name match the globally name defined for the parent in the
> +		 * parent_data .name value.
> +		 */
> +		if (!name && global_name && strcmp(global_name, hw->core->name))
> +			return ERR_PTR(-ENOENT);
>  	} else if (name) {
>  		/*
>  		 * If the DT search above couldn't find the provider fallback to
> -- 
> 2.38.1
> 

-- 
	Ansuel