Re: [PATCH 9/9] clk: Overwrite clk_hw::init with NULL during clk_register()

[Claudiu Beznea <claudiu.beznea@tuxon.dev>]

Hi, Stephen,

On 31.07.2019 22:35, Stephen Boyd wrote:
> We don't want clk provider drivers to use the init structure after clk
> registration time, but we leave a dangling reference to it by means of
> clk_hw::init. Let's overwrite the member with NULL during clk_register()
> so that this can't be used anymore after registration time.
> 
> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> Cc: Doug Anderson <dianders@chromium.org>
> Signed-off-by: Stephen Boyd <sboyd@kernel.org>
> ---
> 
> Please ack so I can take this through clk tree
> 
>  drivers/clk/clk.c            | 24 ++++++++++++++++--------
>  include/linux/clk-provider.h |  3 ++-
>  2 files changed, 18 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index c0990703ce54..efac620264a2 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -3484,9 +3484,9 @@ static int clk_cpy_name(const char **dst_p, const char *src, bool must_exist)
>  	return 0;
>  }
>  
> -static int clk_core_populate_parent_map(struct clk_core *core)
> +static int clk_core_populate_parent_map(struct clk_core *core,
> +					const struct clk_init_data *init)
>  {
> -	const struct clk_init_data *init = core->hw->init;
>  	u8 num_parents = init->num_parents;
>  	const char * const *parent_names = init->parent_names;
>  	const struct clk_hw **parent_hws = init->parent_hws;
> @@ -3566,6 +3566,14 @@ __clk_register(struct device *dev, struct device_node *np, struct clk_hw *hw)
>  {
>  	int ret;
>  	struct clk_core *core;
> +	const struct clk_init_data *init = hw->init;
> +
> +	/*
> +	 * The init data is not supposed to be used outside of registration path.
> +	 * Set it to NULL so that provider drivers can't use it either and so that
> +	 * we catch use of hw->init early on in the core.
> +	 */
> +	hw->init = NULL;

I found that this line impact the drivers/clk/clk-versaclock3.c driver when
doing unbind/bind. The driver provides statically initialized hw.init
object for registration, the hw.init pointers are set to NULL after clock
registrations then the next registration (when re-binding) fails.

In the driver probe the clock are registered like:

	for (i = 0; i < ARRAY_SIZE(clk_pfd_mux); i++) {
		clk_pfd_mux[i].regmap = regmap;
		ret = devm_clk_hw_register(dev, &clk_pfd_mux[i].hw);
		if (ret)
			return dev_err_probe(dev, ret, "%s failed\n",
					     clk_pfd_mux[i].hw.init->name);
	}

and entries in clk_pfd_mux[] are like:

static struct vc3_hw_data clk_pfd_mux[] = {
	[VC3_PFD2_MUX] = {
		.data = &(struct vc3_clk_data) {
			.offs = VC3_PLL_OP_CTRL,
			.bitmsk = BIT(VC3_PLL_OP_CTRL_PLL2_REFIN_SEL)
		},
		.hw.init = &(struct clk_init_data) {
			.name = "pfd2_mux",
			.ops = &vc3_pfd_mux_ops,
			.parent_data = pfd_mux_parent_data,
			.num_parents = 2,
			.flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_NO_REPARENT
		}
	},

// ...

};

I tried with the following diff, too:

diff --git a/drivers/clk/clk-versaclock3.c b/drivers/clk/clk-versaclock3.c
index 9fe27dace111..c1776c313e9b 100644
--- a/drivers/clk/clk-versaclock3.c
+++ b/drivers/clk/clk-versaclock3.c
@@ -1022,10 +1022,11 @@ static int vc3_probe(struct i2c_client *client)
        /* Register pfd muxes */
        for (i = 0; i < ARRAY_SIZE(clk_pfd_mux); i++) {
                clk_pfd_mux[i].regmap = regmap;
+               pr_err("%s(): hw init=%08x\n", __func__,
clk_pfd_mux[i].hw.init);
                ret = devm_clk_hw_register(dev, &clk_pfd_mux[i].hw);
                if (ret)
                        return dev_err_probe(dev, ret, "%s failed\n",
-                                            clk_pfd_mux[i].hw.init->name);
+                                            "test");
//clk_pfd_mux[i].hw.init->name);
        }

        /* Register pfd's */
@@ -1034,7 +1035,7 @@ static int vc3_probe(struct i2c_client *client)
                ret = devm_clk_hw_register(dev, &clk_pfd[i].hw);
                if (ret)
                        return dev_err_probe(dev, ret, "%s failed\n",
-                                            clk_pfd[i].hw.init->name);
+                                            "test");
//clk_pfd[i].hw.init->name);
        }

        data = i2c_get_match_data(client);
@@ -1050,7 +1051,7 @@ static int vc3_probe(struct i2c_client *client)
                ret = devm_clk_hw_register(dev, &clk_pll[i].hw);
                if (ret)
                        return dev_err_probe(dev, ret, "%s failed\n",
-                                            clk_pll[i].hw.init->name);
+                                            "test");
//clk_pll[i].hw.init->name);
        }

        /* Register divider muxes */
@@ -1059,7 +1060,7 @@ static int vc3_probe(struct i2c_client *client)
                ret = devm_clk_hw_register(dev, &clk_div_mux[i].hw);
                if (ret)
                        return dev_err_probe(dev, ret, "%s failed\n",
-                                            clk_div_mux[i].hw.init->name);
+
"test");//clk_div_mux[i].hw.init->name);
        }

        /* Register dividers */
@@ -1068,7 +1069,7 @@ static int vc3_probe(struct i2c_client *client)
                ret = devm_clk_hw_register(dev, &clk_div[i].hw);
                if (ret)
                        return dev_err_probe(dev, ret, "%s failed\n",
-                                            clk_div[i].hw.init->name);
+                                            "test");
//clk_div[i].hw.init->name);
        }

        /* Register clk muxes */
@@ -1082,7 +1083,7 @@ static int vc3_probe(struct i2c_client *client)
                ret = devm_clk_hw_register(dev, &clk_mux[i].hw);
                if (ret)
                        return dev_err_probe(dev, ret, "%s failed\n",
-                                            clk_mux[i].hw.init->name);
+
"test");//clk_mux[i].hw.init->name);
        }

        /* Register clk outputs */

Can you please let me know if this is a wrong pattern? I noticed there are
other drivers following similar approach, e.g.:
- drivers/clk/clk-axm5516.c
- drivers/clk/qcom/camcc-sm6350.c
- drivers/clk/meson/c3-pll.c

Full console log with my trials can be found here:
https://p.fr33tux.org/9d976f

Thank you,
Claudiu