[PATCH v3 0/3] clk: socfpga: register the SP timer clocks early

[PATCH v3 0/3] clk: socfpga: register the SP timer clocks early

[Adrian Ng Ho Yin]

The DW APB timers on Stratix10, Agilex (including eASIC N5X) and Agilex5
are probed through TIMER_OF_DECLARE() from time_init(), but their clock,
l4_sp_clk, comes from a clock manager that is a platform driver
registered at core_initcall(). The timer's clk_get() returns
-EPROBE_DEFER, which timer_probe() does not report, and the timer is
never registered because TIMER_OF_DECLARE() callbacks are not retried.

Rather than moving the whole clock manager to CLK_OF_DECLARE() as v1 did,
this version keeps the platform driver and, following
clk-mt8173-infracfg.c, uses CLK_OF_DECLARE_DRIVER() to register only
l4_sp_clk and the clocks its rate depends on from of_clk_init():

  Agilex/N5X: boot_clk, main_pll, periph_pll, main_pll_c1, peri_pll_c1,
              noc_free_clk, l4_sp_clk
  Agilex5:    boot_clk, main_pll, periph_pll, main_pll_c3, peri_pll_c1,
              noc_free_clk, l4_sp_clk
  Stratix10:  boot_clk, main_pll, periph_pll, main_noc_base_clk,
              peri_noc_base_clk, noc_free_clk, l4_sp_clk

All other clock IDs return -EPROBE_DEFER until the platform driver probes,
registers the remaining clocks into the same provider and turns any
unused IDs into -ENOENT. If the early registration fails, probe falls
back to registering everything as before.

Boot-tested on Agilex7 (FM61), Stratix10 L-tile and Agilex5 (065B Premium
devkit), comparing each board with and without this series:

  - with the series, timer1 is registered as a clocksource within 0.5 ms
    of boot and timer0 as the broadcast clockevent device; without it,
    neither is registered and bc_hrtimer is used for broadcast instead
  - the clock manager still binds as a platform device, no devices are
    left deferred, and clocks registered at probe (EMAC, SD/MMC, USB,
    DMA, watchdog) have their expected rates and consumers

eASIC N5X is compile-tested only.

Changes in v3:
- Drop redundant #include <linux/io.h>; of_address.h already provides
  iounmap() (Dinh)
- Always publish clk_data before adding the provider; use of_iomap() for
  both early and probe-fallback setup so a failed provider add cannot
  double-register early clocks or leave clocks pointing at a
  devm-unmapped base

Changes in v2:
- Keep the clock managers as platform drivers instead of converting
  them to CLK_OF_DECLARE() (Brian)
- Register only l4_sp_clk and its parents early, using
  CLK_OF_DECLARE_DRIVER() as in clk-mt8173-infracfg.c, instead of the
  whole controller (Jerome)
- Return the result of of_clk_add_hw_provider() from probe
- Update subjects and commit messages to match the new approach

v2: https://lore.kernel.org/all/cover.1790935401.git.adrian.ho.yin.ng@altera.com/
v1: https://lore.kernel.org/all/cover.1789096458.git.adrian.ho.yin.ng@altera.com/

Adrian Ng Ho Yin (3):
  clk: socfpga: agilex: register the SP timer clocks early
  clk: socfpga: agilex5: register the SP timer clocks early
  clk: socfpga: stratix10: register the SP timer clocks early

 drivers/clk/socfpga/clk-agilex.c  | 242 +++++++++++++++++++++++-------
 drivers/clk/socfpga/clk-agilex5.c | 155 +++++++++++++++----
 drivers/clk/socfpga/clk-s10.c     | 146 +++++++++++++++---
 3 files changed, 434 insertions(+), 109 deletions(-)


base-commit: 1e7f56205813a2c48cdb3e9a4b0a24f49fd9a548
-- 
2.49.GIT

[PATCH v3 1/3] clk: socfpga: agilex: register the SP timer clocks early

[Adrian Ng Ho Yin]

The DW APB timers are probed through TIMER_OF_DECLARE() from time_init()
and cannot defer, but the clock manager is a platform driver registered
at core_initcall(). The timer's clk_get() therefore fails and the timer
is never brought up.

Register only l4_sp_clk and its parents (boot_clk, main_pll, periph_pll,
main_pll_c1, peri_pll_c1 and noc_free_clk) from of_clk_init() using
CLK_OF_DECLARE_DRIVER(), with every other clock left as -EPROBE_DEFER.
The remaining clocks are still registered when the platform driver
probes, which reuses the early provider.

Signed-off-by: Adrian Ng Ho Yin <adrian.ho.yin.ng@altera.com>
---
 drivers/clk/socfpga/clk-agilex.c | 242 ++++++++++++++++++++++++-------
 1 file changed, 187 insertions(+), 55 deletions(-)

diff --git a/drivers/clk/socfpga/clk-agilex.c b/drivers/clk/socfpga/clk-agilex.c
index 2bdea1997b5e..445d814e2f3a 100644
--- a/drivers/clk/socfpga/clk-agilex.c
+++ b/drivers/clk/socfpga/clk-agilex.c
@@ -5,6 +5,7 @@
 #include <linux/slab.h>
 #include <linux/clk-provider.h>
 #include <linux/of.h>
+#include <linux/of_address.h>
 #include <linux/platform_device.h>
 
 #include <dt-bindings/clock/agilex-clock.h>
@@ -232,24 +233,49 @@ static const struct stratix10_pll_clock agilex_pll_clks[] = {
 	  0, 0x9c},
 };
 
+/*
+ * The SP timers are probed from time_init() and cannot defer, so their
+ * l4_sp_clk and its parents are registered from of_clk_init(). Everything
+ * else is registered when the platform driver probes.
+ */
+static const struct n5x_perip_c_clock n5x_early_perip_c_clks[] = {
+	{ AGILEX_MAIN_PLL_C1_CLK, "main_pll_c1", "main_pll", NULL, 1, 0, 0x54, 8},
+	{ AGILEX_PERIPH_PLL_C1_CLK, "peri_pll_c1", "periph_pll", NULL, 1, 0, 0xA8, 8},
+};
+
+static const struct stratix10_perip_c_clock agilex_early_perip_c_clks[] = {
+	{ AGILEX_MAIN_PLL_C1_CLK, "main_pll_c1", "main_pll", NULL, 1, 0, 0x5C},
+	{ AGILEX_PERIPH_PLL_C1_CLK, "peri_pll_c1", "periph_pll", NULL, 1, 0, 0xB0},
+};
+
+static const struct stratix10_perip_cnt_clock agilex_early_perip_cnt_clks[] = {
+	{ AGILEX_NOC_FREE_CLK, "noc_free_clk", NULL, noc_free_mux, ARRAY_SIZE(noc_free_mux),
+	  0, 0x40, 0, 0, 0},
+};
+
+static const struct stratix10_gate_clock agilex_early_gate_clks[] = {
+	/*
+	 * The l4_sp_clk feeds a 100 MHz clock to various peripherals, one of them
+	 * being the SP timers, thus cannot get gated.
+	 */
+	{ AGILEX_L4_SP_CLK, "l4_sp_clk", NULL, noc_mux, ARRAY_SIZE(noc_mux), CLK_IS_CRITICAL, 0x24,
+	  3, 0x44, 16, 2, 0x30, 1, 0},
+};
+
 static const struct n5x_perip_c_clock n5x_main_perip_c_clks[] = {
 	{ AGILEX_MAIN_PLL_C0_CLK, "main_pll_c0", "main_pll", NULL, 1, 0, 0x54, 0},
-	{ AGILEX_MAIN_PLL_C1_CLK, "main_pll_c1", "main_pll", NULL, 1, 0, 0x54, 8},
 	{ AGILEX_MAIN_PLL_C2_CLK, "main_pll_c2", "main_pll", NULL, 1, 0, 0x54, 16},
 	{ AGILEX_MAIN_PLL_C3_CLK, "main_pll_c3", "main_pll", NULL, 1, 0, 0x54, 24},
 	{ AGILEX_PERIPH_PLL_C0_CLK, "peri_pll_c0", "periph_pll", NULL, 1, 0, 0xA8, 0},
-	{ AGILEX_PERIPH_PLL_C1_CLK, "peri_pll_c1", "periph_pll", NULL, 1, 0, 0xA8, 8},
 	{ AGILEX_PERIPH_PLL_C2_CLK, "peri_pll_c2", "periph_pll", NULL, 1, 0, 0xA8, 16},
 	{ AGILEX_PERIPH_PLL_C3_CLK, "peri_pll_c3", "periph_pll", NULL, 1, 0, 0xA8, 24},
 };
 
 static const struct stratix10_perip_c_clock agilex_main_perip_c_clks[] = {
 	{ AGILEX_MAIN_PLL_C0_CLK, "main_pll_c0", "main_pll", NULL, 1, 0, 0x58},
-	{ AGILEX_MAIN_PLL_C1_CLK, "main_pll_c1", "main_pll", NULL, 1, 0, 0x5C},
 	{ AGILEX_MAIN_PLL_C2_CLK, "main_pll_c2", "main_pll", NULL, 1, 0, 0x64},
 	{ AGILEX_MAIN_PLL_C3_CLK, "main_pll_c3", "main_pll", NULL, 1, 0, 0x68},
 	{ AGILEX_PERIPH_PLL_C0_CLK, "peri_pll_c0", "periph_pll", NULL, 1, 0, 0xAC},
-	{ AGILEX_PERIPH_PLL_C1_CLK, "peri_pll_c1", "periph_pll", NULL, 1, 0, 0xB0},
 	{ AGILEX_PERIPH_PLL_C2_CLK, "peri_pll_c2", "periph_pll", NULL, 1, 0, 0xB8},
 	{ AGILEX_PERIPH_PLL_C3_CLK, "peri_pll_c3", "periph_pll", NULL, 1, 0, 0xBC},
 };
@@ -257,8 +283,6 @@ static const struct stratix10_perip_c_clock agilex_main_perip_c_clks[] = {
 static const struct stratix10_perip_cnt_clock agilex_main_perip_cnt_clks[] = {
 	{ AGILEX_MPU_FREE_CLK, "mpu_free_clk", NULL, mpu_free_mux, ARRAY_SIZE(mpu_free_mux),
 	   0, 0x3C, 0, 0, 0},
-	{ AGILEX_NOC_FREE_CLK, "noc_free_clk", NULL, noc_free_mux, ARRAY_SIZE(noc_free_mux),
-	  0, 0x40, 0, 0, 0},
 	{ AGILEX_L3_MAIN_FREE_CLK, "l3_main_free_clk", "noc_free_clk", NULL,
 	  1, 0, 0, 1, 0, 0},
 	{ AGILEX_L4_SYS_FREE_CLK, "l4_sys_free_clk", NULL, noc_mux, ARRAY_SIZE(noc_mux), 0,
@@ -292,12 +316,6 @@ static const struct stratix10_gate_clock agilex_gate_clks[] = {
 	  1, 0x44, 0, 2, 0x30, 1, 0},
 	{ AGILEX_L4_MP_CLK, "l4_mp_clk", NULL, noc_mux, ARRAY_SIZE(noc_mux), 0, 0x24,
 	  2, 0x44, 8, 2, 0x30, 1, 0},
-	/*
-	 * The l4_sp_clk feeds a 100 MHz clock to various peripherals, one of them
-	 * being the SP timers, thus cannot get gated.
-	 */
-	{ AGILEX_L4_SP_CLK, "l4_sp_clk", NULL, noc_mux, ARRAY_SIZE(noc_mux), CLK_IS_CRITICAL, 0x24,
-	  3, 0x44, 16, 2, 0x30, 1, 0},
 	{ AGILEX_CS_AT_CLK, "cs_at_clk", NULL, noc_mux, ARRAY_SIZE(noc_mux), 0, 0x24,
 	  4, 0x44, 24, 2, 0x30, 1, 0},
 	{ AGILEX_CS_TRACE_CLK, "cs_trace_clk", NULL, noc_mux, ARRAY_SIZE(noc_mux), 0, 0x24,
@@ -454,33 +472,166 @@ static int n5x_clk_register_pll(const struct stratix10_pll_clock *clks,
 	return 0;
 }
 
-static int agilex_clkmgr_init(struct platform_device *pdev)
+static struct stratix10_clock_data *agilex_clk_data;
+
+static struct stratix10_clock_data *agilex_clk_data_alloc(void __iomem *base)
 {
-	struct device_node *np = pdev->dev.of_node;
-	struct device *dev = &pdev->dev;
 	struct stratix10_clock_data *clk_data;
-	void __iomem *base;
-	int i, num_clks;
-
-	base = devm_platform_ioremap_resource(pdev, 0);
-	if (IS_ERR(base))
-		return PTR_ERR(base);
-
-	num_clks = AGILEX_NUM_CLKS;
+	int i;
 
-	clk_data = devm_kzalloc(dev, struct_size(clk_data, clk_data.hws,
-				num_clks), GFP_KERNEL);
+	clk_data = kzalloc(struct_size(clk_data, clk_data.hws, AGILEX_NUM_CLKS),
+			   GFP_KERNEL);
 	if (!clk_data)
-		return -ENOMEM;
+		return NULL;
 
-	clk_data->clk_data.num = num_clks;
 	clk_data->base = base;
+	clk_data->clk_data.num = AGILEX_NUM_CLKS;
 
-	for (i = 0; i < num_clks; i++)
-		clk_data->clk_data.hws[i] = ERR_PTR(-ENOENT);
+	for (i = 0; i < AGILEX_NUM_CLKS; i++)
+		clk_data->clk_data.hws[i] = ERR_PTR(-EPROBE_DEFER);
+
+	return clk_data;
+}
 
+static void agilex_clk_register_early(struct stratix10_clock_data *clk_data)
+{
 	agilex_clk_register_pll(agilex_pll_clks, ARRAY_SIZE(agilex_pll_clks), clk_data);
 
+	agilex_clk_register_c_perip(agilex_early_perip_c_clks,
+				    ARRAY_SIZE(agilex_early_perip_c_clks), clk_data);
+
+	agilex_clk_register_cnt_perip(agilex_early_perip_cnt_clks,
+				      ARRAY_SIZE(agilex_early_perip_cnt_clks),
+				      clk_data);
+
+	agilex_clk_register_gate(agilex_early_gate_clks,
+				 ARRAY_SIZE(agilex_early_gate_clks), clk_data);
+}
+
+static void n5x_clk_register_early(struct stratix10_clock_data *clk_data)
+{
+	n5x_clk_register_pll(agilex_pll_clks, ARRAY_SIZE(agilex_pll_clks), clk_data);
+
+	n5x_clk_register_c_perip(n5x_early_perip_c_clks,
+				 ARRAY_SIZE(n5x_early_perip_c_clks), clk_data);
+
+	agilex_clk_register_cnt_perip(agilex_early_perip_cnt_clks,
+				      ARRAY_SIZE(agilex_early_perip_cnt_clks),
+				      clk_data);
+
+	agilex_clk_register_gate(agilex_early_gate_clks,
+				 ARRAY_SIZE(agilex_early_gate_clks), clk_data);
+}
+
+/*
+ * The registered clocks keep using the mapping and clk_data, so neither is
+ * released once the early clocks have been registered.
+ */
+static int
+agilex_clkmgr_setup(struct device_node *np,
+		    void (*register_early)(struct stratix10_clock_data *))
+{
+	struct stratix10_clock_data *clk_data;
+	void __iomem *base;
+
+	base = of_iomap(np, 0);
+	if (!base) {
+		pr_err("%s: failed to map clock registers\n", __func__);
+		return -ENOMEM;
+	}
+
+	clk_data = agilex_clk_data_alloc(base);
+	if (!clk_data) {
+		iounmap(base);
+		return -ENOMEM;
+	}
+
+	register_early(clk_data);
+	agilex_clk_data = clk_data;
+
+	return 0;
+}
+
+static int agilex_clkmgr_add_provider(struct device_node *np)
+{
+	static bool added;
+	int ret;
+
+	if (added)
+		return 0;
+
+	ret = of_clk_add_hw_provider(np, of_clk_hw_onecell_get,
+				     &agilex_clk_data->clk_data);
+	if (ret)
+		return ret;
+
+	added = true;
+
+	return 0;
+}
+
+static void __init
+agilex_clkmgr_of_init(struct device_node *np,
+		      void (*register_early)(struct stratix10_clock_data *))
+{
+	if (agilex_clkmgr_setup(np, register_early))
+		return;
+
+	if (agilex_clkmgr_add_provider(np))
+		pr_err("%s: failed to add clock provider\n", __func__);
+}
+
+static void __init agilex_clkmgr_early_init(struct device_node *np)
+{
+	agilex_clkmgr_of_init(np, agilex_clk_register_early);
+}
+
+CLK_OF_DECLARE_DRIVER(agilex_clkmgr, "intel,agilex-clkmgr",
+		      agilex_clkmgr_early_init);
+
+static void __init n5x_clkmgr_early_init(struct device_node *np)
+{
+	agilex_clkmgr_of_init(np, n5x_clk_register_early);
+}
+
+CLK_OF_DECLARE_DRIVER(n5x_clkmgr, "intel,easic-n5x-clkmgr",
+		      n5x_clkmgr_early_init);
+
+static struct stratix10_clock_data *
+agilex_clkmgr_get_clk_data(struct platform_device *pdev,
+			   void (*register_early)(struct stratix10_clock_data *))
+{
+	int ret;
+
+	if (!agilex_clk_data) {
+		ret = agilex_clkmgr_setup(pdev->dev.of_node, register_early);
+		if (ret)
+			return ERR_PTR(ret);
+	}
+
+	return agilex_clk_data;
+}
+
+static int agilex_clkmgr_finish(struct platform_device *pdev,
+				struct stratix10_clock_data *clk_data)
+{
+	int i;
+
+	for (i = 0; i < AGILEX_NUM_CLKS; i++)
+		if (clk_data->clk_data.hws[i] == ERR_PTR(-EPROBE_DEFER))
+			clk_data->clk_data.hws[i] = ERR_PTR(-ENOENT);
+
+	return agilex_clkmgr_add_provider(pdev->dev.of_node);
+}
+
+static int agilex_clkmgr_init(struct platform_device *pdev)
+{
+	struct stratix10_clock_data *clk_data;
+
+	clk_data = agilex_clkmgr_get_clk_data(pdev, agilex_clk_register_early);
+	if (IS_ERR(clk_data))
+		return PTR_ERR(clk_data);
+
 	agilex_clk_register_c_perip(agilex_main_perip_c_clks,
 				 ARRAY_SIZE(agilex_main_perip_c_clks), clk_data);
 
@@ -490,36 +641,17 @@ static int agilex_clkmgr_init(struct platform_device *pdev)
 
 	agilex_clk_register_gate(agilex_gate_clks, ARRAY_SIZE(agilex_gate_clks),
 			      clk_data);
-	of_clk_add_hw_provider(np, of_clk_hw_onecell_get, &clk_data->clk_data);
-	return 0;
+
+	return agilex_clkmgr_finish(pdev, clk_data);
 }
 
 static int n5x_clkmgr_init(struct platform_device *pdev)
 {
-	struct device_node *np = pdev->dev.of_node;
-	struct device *dev = &pdev->dev;
 	struct stratix10_clock_data *clk_data;
-	void __iomem *base;
-	int i, num_clks;
 
-	base = devm_platform_ioremap_resource(pdev, 0);
-	if (IS_ERR(base))
-		return PTR_ERR(base);
-
-	num_clks = AGILEX_NUM_CLKS;
-
-	clk_data = devm_kzalloc(dev, struct_size(clk_data, clk_data.hws,
-				num_clks), GFP_KERNEL);
-	if (!clk_data)
-		return -ENOMEM;
-
-	clk_data->base = base;
-	clk_data->clk_data.num = num_clks;
-
-	for (i = 0; i < num_clks; i++)
-		clk_data->clk_data.hws[i] = ERR_PTR(-ENOENT);
-
-	n5x_clk_register_pll(agilex_pll_clks, ARRAY_SIZE(agilex_pll_clks), clk_data);
+	clk_data = agilex_clkmgr_get_clk_data(pdev, n5x_clk_register_early);
+	if (IS_ERR(clk_data))
+		return PTR_ERR(clk_data);
 
 	n5x_clk_register_c_perip(n5x_main_perip_c_clks,
 				 ARRAY_SIZE(n5x_main_perip_c_clks), clk_data);
@@ -530,8 +662,8 @@ static int n5x_clkmgr_init(struct platform_device *pdev)
 
 	agilex_clk_register_gate(agilex_gate_clks, ARRAY_SIZE(agilex_gate_clks),
 			      clk_data);
-	of_clk_add_hw_provider(np, of_clk_hw_onecell_get, &clk_data->clk_data);
-	return 0;
+
+	return agilex_clkmgr_finish(pdev, clk_data);
 }
 
 static int agilex_clkmgr_probe(struct platform_device *pdev)
-- 
2.49.GIT

Re: [PATCH v3 1/3] clk: socfpga: agilex: register the SP timer clocks early

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] clk: socfpga: lockless publication of clk_hw pointers without memory barriers
- [Medium] clk: socfpga: silent failure on clock registration OOM due to missing NULL check
--

commit fc4b5a56305f3513e2f688af55d3564b1822bc8c
Author: Adrian Ng Ho Yin <adrian.ho.yin.ng@altera.com>

clk: socfpga: agilex: register the SP timer clocks early

The DW APB timers are probed from time_init() and cannot defer. This commit
registers l4_sp_clk and its parents early using CLK_OF_DECLARE_DRIVER(),
leaving the rest as -EPROBE_DEFER, which are subsequently registered when
the platform driver probes.

> diff --git a/drivers/clk/socfpga/clk-agilex.c b/drivers/clk/socfpga/clk-agilex.c
> index 2bdea1997b5ef..445d814e2f3ac 100644
> --- a/drivers/clk/socfpga/clk-agilex.c
> +++ b/drivers/clk/socfpga/clk-agilex.c
[ ... ]
> @@ -454,33 +472,166 @@ static int n5x_clk_register_pll(const struct stratix10_pll_clock *clks,
[ ... ]
> +static int agilex_clkmgr_add_provider(struct device_node *np)
> +{
> +	static bool added;
> +	int ret;
> +
> +	if (added)
> +		return 0;
> +
> +	ret = of_clk_add_hw_provider(np, of_clk_hw_onecell_get,
> +				     &agilex_clk_data->clk_data);

[Severity: High]
Does exposing the clock data array here during early boot allow
concurrent readers to see uninitialized memory?

The global array is exposed via of_clk_add_hw_provider() before all
clocks are populated. Later, during the platform driver probe in
agilex_clkmgr_init(), the remaining clocks are initialized and their
pointers are published locklessly:

drivers/clk/socfpga/clk-agilex.c:agilex_clk_register_c_perip() {
	...
	hw_clk = s10_register_periph(&clks[i], base);
	...
	data->clk_data.hws[clks[i].id] = hw_clk;
}

Since the array is mutated without memory barriers like smp_store_release()
or shared locks, could weakly ordered architectures reorder the publication
of the pointer before the initialization of the memory?

If a concurrent consumer requests a clock during this window, they might read
the new hw_clk pointer but observe uninitialized memory inside the clk_hw
structure, potentially leading to data corruption or a kernel panic.

[ ... ]
> +static int agilex_clkmgr_finish(struct platform_device *pdev,
> +				struct stratix10_clock_data *clk_data)
> +{
> +	int i;
> +
> +	for (i = 0; i < AGILEX_NUM_CLKS; i++)
> +		if (clk_data->clk_data.hws[i] == ERR_PTR(-EPROBE_DEFER))
> +			clk_data->clk_data.hws[i] = ERR_PTR(-ENOENT);

[Severity: High]
Is it safe to mutate the globally exposed hws array locklessly here?

This loop is running during the delayed platform probe, modifying the array
while consumers might be concurrently accessing it.

> +
> +	return agilex_clkmgr_add_provider(pdev->dev.of_node);
> +}
> +
> +static int agilex_clkmgr_init(struct platform_device *pdev)
> +{
> +	struct stratix10_clock_data *clk_data;
> +
> +	clk_data = agilex_clkmgr_get_clk_data(pdev, agilex_clk_register_early);
> +	if (IS_ERR(clk_data))
> +		return PTR_ERR(clk_data);
> +
>  	agilex_clk_register_c_perip(agilex_main_perip_c_clks,
>  				 ARRAY_SIZE(agilex_main_perip_c_clks), clk_data);

[Severity: Medium]
Does the missing NULL check in agilex_clk_register_c_perip() cause a silent
failure on memory allocation errors?

In agilex_clk_register_c_perip(), the return value of s10_register_periph()
is checked with IS_ERR():

drivers/clk/socfpga/clk-agilex.c:agilex_clk_register_c_perip() {
	...
	hw_clk = s10_register_periph(&clks[i], base);
	if (IS_ERR(hw_clk)) {
		pr_err("%s: failed to register clock %s\n",
		       __func__, clks[i].name);
		continue;
	}
	data->clk_data.hws[clks[i].id] = hw_clk;
}

However, s10_register_periph() returns NULL on memory allocation failure.
This means the IS_ERR() check evaluates to false, failing to catch the error.

Could this cause NULL to silently overwrite the original -EPROBE_DEFER in the
hws array? If consumers request this clock, they would receive a NULL clock
pointer, which the common clock framework treats as a valid optional dummy
clock. This masks the initialization failure and might allow dependent hardware
to proceed without being clocked.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1791871090.git.adrian.ho.yin.ng@altera.com?part=1

[PATCH v3 2/3] clk: socfpga: agilex5: register the SP timer clocks early

[Adrian Ng Ho Yin]

The DW APB timers are probed through TIMER_OF_DECLARE() from time_init()
and cannot defer, but the clock manager is a platform driver registered
at core_initcall(). The timer's clk_get() therefore fails and the timer
is never brought up.

Register only l4_sp_clk and its parents (boot_clk, main_pll, periph_pll,
main_pll_c3, peri_pll_c1 and noc_free_clk) from of_clk_init() using
CLK_OF_DECLARE_DRIVER(), with every other clock left as -EPROBE_DEFER.
The remaining clocks are still registered when the platform driver
probes, which reuses the early provider.

Signed-off-by: Adrian Ng Ho Yin <adrian.ho.yin.ng@altera.com>
---
 drivers/clk/socfpga/clk-agilex5.c | 155 ++++++++++++++++++++++++------
 1 file changed, 126 insertions(+), 29 deletions(-)

diff --git a/drivers/clk/socfpga/clk-agilex5.c b/drivers/clk/socfpga/clk-agilex5.c
index f7f0ad884f64..50dba11ab45e 100644
--- a/drivers/clk/socfpga/clk-agilex5.c
+++ b/drivers/clk/socfpga/clk-agilex5.c
@@ -6,6 +6,7 @@
 #include <linux/slab.h>
 #include <linux/clk-provider.h>
 #include <linux/of.h>
+#include <linux/of_address.h>
 #include <linux/platform_device.h>
 #include  <dt-bindings/clock/intel,agilex5-clkmgr.h>
 #include "stratix10-clk.h"
@@ -233,7 +234,29 @@ static const struct agilex5_pll_clock agilex5_pll_clks[] = {
 	},
 };
 
-/* Main PLL C0, C1, C2, C3 and Peri PLL C0, C1, C2, C3. With ping-pong counter. */
+/*
+ * The SP timers are probed from time_init() and cannot defer, so their
+ * l4_sp_clk and its parents are registered from of_clk_init(). Everything
+ * else is registered when the platform driver probes.
+ */
+static const struct stratix10_perip_c_clock agilex5_early_perip_c_clks[] = {
+	{ AGILEX5_MAIN_PLL_C3_CLK, "main_pll_c3", "main_pll", NULL, 1, 0,
+	  0x68 },
+	{ AGILEX5_PERIPH_PLL_C1_CLK, "peri_pll_c1", "periph_pll", NULL, 1, 0,
+	  0xB4 },
+};
+
+static const struct agilex5_perip_cnt_clock agilex5_early_perip_cnt_clks[] = {
+	{ AGILEX5_NOC_FREE_CLK, "noc_free_clk", noc_free_mux,
+	ARRAY_SIZE(noc_free_mux), 0, 0x40, 0, 0, 0 },
+};
+
+static const struct agilex5_gate_clock agilex5_early_gate_clks[] = {
+	{ AGILEX5_L4_SP_CLK, "l4_sp_clk", noc_mux, ARRAY_SIZE(noc_mux),
+	  CLK_IS_CRITICAL, 0x24, 3, 0x44, 6, 2, 0x30, 1, 0 },
+};
+
+/* Main PLL C0, C1, C2 and Peri PLL C0, C2, C3. With ping-pong counter. */
 static const struct stratix10_perip_c_clock agilex5_main_perip_c_clks[] = {
 	{ AGILEX5_MAIN_PLL_C0_CLK, "main_pll_c0", "main_pll", NULL, 1, 0,
 	  0x5C },
@@ -241,12 +264,8 @@ static const struct stratix10_perip_c_clock agilex5_main_perip_c_clks[] = {
 	  0x60 },
 	{ AGILEX5_MAIN_PLL_C2_CLK, "main_pll_c2", "main_pll", NULL, 1, 0,
 	  0x64 },
-	{ AGILEX5_MAIN_PLL_C3_CLK, "main_pll_c3", "main_pll", NULL, 1, 0,
-	  0x68 },
 	{ AGILEX5_PERIPH_PLL_C0_CLK, "peri_pll_c0", "periph_pll", NULL, 1, 0,
 	  0xB0 },
-	{ AGILEX5_PERIPH_PLL_C1_CLK, "peri_pll_c1", "periph_pll", NULL, 1, 0,
-	  0xB4 },
 	{ AGILEX5_PERIPH_PLL_C2_CLK, "peri_pll_c2", "periph_pll", NULL, 1, 0,
 	  0xB8 },
 	{ AGILEX5_PERIPH_PLL_C3_CLK, "peri_pll_c3", "periph_pll", NULL, 1, 0,
@@ -265,8 +284,6 @@ static const struct agilex5_perip_cnt_clock agilex5_main_perip_cnt_clks[] = {
 	ARRAY_SIZE(core3_free_mux), 0, 0x0110, 0, 0, 0},
 	{ AGILEX5_DSU_FREE_CLK, "dsu_free_clk", dsu_free_mux,
 	ARRAY_SIZE(dsu_free_mux), 0, 0xfc, 0, 0, 0},
-	{ AGILEX5_NOC_FREE_CLK, "noc_free_clk", noc_free_mux,
-	ARRAY_SIZE(noc_free_mux), 0, 0x40, 0, 0, 0 },
 	{ AGILEX5_EMAC_A_FREE_CLK, "emaca_free_clk", emaca_free_mux,
 	ARRAY_SIZE(emaca_free_mux), 0, 0xD4, 0, 0x88, 0 },
 	{ AGILEX5_EMAC_B_FREE_CLK, "emacb_free_clk", emacb_free_mux,
@@ -313,8 +330,6 @@ static const struct agilex5_gate_clock agilex5_gate_clks[] = {
 	  0x24, 2, 0x44, 4, 2, 0x30, 1, 0 },
 	{ AGILEX5_L4_SYS_FREE_CLK, "l4_sys_free_clk", noc_mux,
 	  ARRAY_SIZE(noc_mux), 0, 0, 0, 0x44, 2, 2, 0x30, 1, 0 },
-	{ AGILEX5_L4_SP_CLK, "l4_sp_clk", noc_mux, ARRAY_SIZE(noc_mux),
-	  CLK_IS_CRITICAL, 0x24, 3, 0x44, 6, 2, 0x30, 1, 0 },
 
 	/* Core sight clocks*/
 	{ AGILEX5_CS_AT_CLK, "cs_at_clk", noc_mux, ARRAY_SIZE(noc_mux), 0,
@@ -486,35 +501,114 @@ static int agilex5_clk_register_pll(const struct agilex5_pll_clock *clks,
 	return 0;
 }
 
-static int agilex5_clkmgr_init(struct platform_device *pdev)
+static struct stratix10_clock_data *agilex5_clk_data;
+
+static struct stratix10_clock_data *agilex5_clk_data_alloc(void __iomem *base)
 {
-	struct device_node *np = pdev->dev.of_node;
-	struct device *dev = &pdev->dev;
 	struct stratix10_clock_data *clk_data;
-	void __iomem *base;
-	int i, num_clks;
-
-	base = devm_platform_ioremap_resource(pdev, 0);
-	if (IS_ERR(base))
-		return PTR_ERR(base);
-
-	num_clks = AGILEX5_NUM_CLKS;
+	int i;
 
-	clk_data = devm_kzalloc(dev, struct_size(clk_data, clk_data.hws,
-				num_clks), GFP_KERNEL);
+	clk_data = kzalloc(struct_size(clk_data, clk_data.hws, AGILEX5_NUM_CLKS),
+			   GFP_KERNEL);
 	if (!clk_data)
-		return -ENOMEM;
+		return NULL;
 
 	clk_data->base = base;
-	clk_data->clk_data.num = num_clks;
+	clk_data->clk_data.num = AGILEX5_NUM_CLKS;
+
+	for (i = 0; i < AGILEX5_NUM_CLKS; i++)
+		clk_data->clk_data.hws[i] = ERR_PTR(-EPROBE_DEFER);
 
-	for (i = 0; i < num_clks; i++)
-		clk_data->clk_data.hws[i] = ERR_PTR(-ENOENT);
+	return clk_data;
+}
 
+static void agilex5_clk_register_early(struct stratix10_clock_data *clk_data)
+{
 	agilex5_clk_register_pll(agilex5_pll_clks, ARRAY_SIZE(agilex5_pll_clks),
 				 clk_data);
 
-	/* mainPLL C0, C1, C2, C3 and periph PLL C0, C1, C2, C3*/
+	agilex5_clk_register_c_perip(agilex5_early_perip_c_clks,
+				     ARRAY_SIZE(agilex5_early_perip_c_clks),
+				     clk_data);
+
+	agilex5_clk_register_cnt_perip(agilex5_early_perip_cnt_clks,
+				       ARRAY_SIZE(agilex5_early_perip_cnt_clks),
+				       clk_data);
+
+	agilex5_clk_register_gate(agilex5_early_gate_clks,
+				  ARRAY_SIZE(agilex5_early_gate_clks), clk_data);
+}
+
+/*
+ * The registered clocks keep using the mapping and clk_data, so neither is
+ * released once the early clocks have been registered.
+ */
+static int agilex5_clkmgr_setup(struct device_node *np)
+{
+	struct stratix10_clock_data *clk_data;
+	void __iomem *base;
+
+	base = of_iomap(np, 0);
+	if (!base) {
+		pr_err("%s: failed to map clock registers\n", __func__);
+		return -ENOMEM;
+	}
+
+	clk_data = agilex5_clk_data_alloc(base);
+	if (!clk_data) {
+		iounmap(base);
+		return -ENOMEM;
+	}
+
+	agilex5_clk_register_early(clk_data);
+	agilex5_clk_data = clk_data;
+
+	return 0;
+}
+
+static int agilex5_clkmgr_add_provider(struct device_node *np)
+{
+	static bool added;
+	int ret;
+
+	if (added)
+		return 0;
+
+	ret = of_clk_add_hw_provider(np, of_clk_hw_onecell_get,
+				     &agilex5_clk_data->clk_data);
+	if (ret)
+		return ret;
+
+	added = true;
+
+	return 0;
+}
+
+static void __init agilex5_clkmgr_early_init(struct device_node *np)
+{
+	if (agilex5_clkmgr_setup(np))
+		return;
+
+	if (agilex5_clkmgr_add_provider(np))
+		pr_err("%s: failed to add clock provider\n", __func__);
+}
+
+CLK_OF_DECLARE_DRIVER(agilex5_clkmgr, "intel,agilex5-clkmgr",
+		      agilex5_clkmgr_early_init);
+
+static int agilex5_clkmgr_init(struct platform_device *pdev)
+{
+	struct stratix10_clock_data *clk_data;
+	int i, ret;
+
+	if (!agilex5_clk_data) {
+		ret = agilex5_clkmgr_setup(pdev->dev.of_node);
+		if (ret)
+			return ret;
+	}
+
+	clk_data = agilex5_clk_data;
+
 	agilex5_clk_register_c_perip(agilex5_main_perip_c_clks,
 				     ARRAY_SIZE(agilex5_main_perip_c_clks),
 				     clk_data);
@@ -526,8 +620,11 @@ static int agilex5_clkmgr_init(struct platform_device *pdev)
 	agilex5_clk_register_gate(agilex5_gate_clks,
 				  ARRAY_SIZE(agilex5_gate_clks), clk_data);
 
-	of_clk_add_hw_provider(np, of_clk_hw_onecell_get, &clk_data->clk_data);
-	return 0;
+	for (i = 0; i < AGILEX5_NUM_CLKS; i++)
+		if (clk_data->clk_data.hws[i] == ERR_PTR(-EPROBE_DEFER))
+			clk_data->clk_data.hws[i] = ERR_PTR(-ENOENT);
+
+	return agilex5_clkmgr_add_provider(pdev->dev.of_node);
 }
 
 static int agilex5_clkmgr_probe(struct platform_device *pdev)
-- 
2.49.GIT

Re: [PATCH v3 2/3] clk: socfpga: agilex5: register the SP timer clocks early

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] clk: data race and missing barriers when mutating exposed hws array
--

commit b3256fb3551a9baf9b48df9a42da38b26efc5524
Author: Adrian Ng Ho Yin <adrian.ho.yin.ng@altera.com>

clk: socfpga: agilex5: register the SP timer clocks early

The DW APB timers are probed through TIMER_OF_DECLARE() from time_init()
and cannot defer, but the clock manager is a platform driver registered
at core_initcall(). The timer's clk_get() therefore fails and the timer
is never brought up.

Register only l4_sp_clk and its parents (boot_clk, main_pll, periph_pll,
main_pll_c3, peri_pll_c1 and noc_free_clk) from of_clk_init() using
CLK_OF_DECLARE_DRIVER(), with every other clock left as -EPROBE_DEFER.
The remaining clocks are still registered when the platform driver
probes, which reuses the early provider.

> diff --git a/drivers/clk/socfpga/clk-agilex5.c b/drivers/clk/socfpga/clk-agilex5.c
> index f7f0ad884f645..50dba11ab45ec 100644
> --- a/drivers/clk/socfpga/clk-agilex5.c
> +++ b/drivers/clk/socfpga/clk-agilex5.c

[ ... ]

> @@ -599,22 +620,25 @@ static int agilex5_clkmgr_init(struct platform_device *pdev)
>  {
>  	struct stratix10_clock_data *clk_data;
>  	int i, ret;
>  
>  	if (!agilex5_clk_data) {
>  		ret = agilex5_clkmgr_setup(pdev->dev.of_node);
>  		if (ret)
>  			return ret;
>  	}
>  
>  	clk_data = agilex5_clk_data;
>  
>  	agilex5_clk_register_c_perip(agilex5_main_perip_c_clks,
>  				     ARRAY_SIZE(agilex5_main_perip_c_clks),
>  				     clk_data);
>  
>  	agilex5_clk_register_cnt_perip(agilex5_main_perip_cnt_clks,
>  				       ARRAY_SIZE(agilex5_main_perip_cnt_clks),
>  				       clk_data);
>  
>  	agilex5_clk_register_gate(agilex5_gate_clks,
>  				  ARRAY_SIZE(agilex5_gate_clks), clk_data);

[Severity: High]
Since agilex5_clkmgr_early_init() publishes the hws array early via
of_clk_add_hw_provider(), can concurrent clock consumers access this array
while agilex5_clkmgr_init() is mutating it?

When agilex5_clk_register_c_perip() and agilex5_clk_register_gate() are
called here, they write newly allocated clocks directly into the shared
array:

agilex5_clk_register_c_perip() {
    ...
    data->clk_data.hws[clks[i].id] = hw_clk;
    ...
}

Because these assignments lack smp_store_release() and are not protected by
of_clk_mutex, could a consumer's of_clk_hw_onecell_get() observe the hw_clk
pointer before its initialization stores are visible in memory, leading to a
NULL pointer dereference or memory corruption?

> -	of_clk_add_hw_provider(np, of_clk_hw_onecell_get, &clk_data->clk_data);
> -	return 0;
> +	for (i = 0; i < AGILEX5_NUM_CLKS; i++)
> +		if (clk_data->clk_data.hws[i] == ERR_PTR(-EPROBE_DEFER))
> +			clk_data->clk_data.hws[i] = ERR_PTR(-ENOENT);

[Severity: High]
Can this loop mutate the exposed array concurrently with potential readers,
since it doesn't hold of_clk_mutex?

> +
> +	return agilex5_clkmgr_add_provider(pdev->dev.of_node);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1791871090.git.adrian.ho.yin.ng@altera.com?part=2

[PATCH v3 3/3] clk: socfpga: stratix10: register the SP timer clocks early

[Adrian Ng Ho Yin]

The DW APB timers are probed through TIMER_OF_DECLARE() from time_init()
and cannot defer, but the clock manager is a platform driver registered
at core_initcall(). The timer's clk_get() therefore fails and the timer
is never brought up.

Register only l4_sp_clk and its parents (boot_clk, main_pll, periph_pll,
main_noc_base_clk, peri_noc_base_clk and noc_free_clk) from
of_clk_init() using CLK_OF_DECLARE_DRIVER(), with every other clock left
as -EPROBE_DEFER. The remaining clocks are still registered when the
platform driver probes, which reuses the early provider.

Signed-off-by: Adrian Ng Ho Yin <adrian.ho.yin.ng@altera.com>
---
 drivers/clk/socfpga/clk-s10.c | 146 ++++++++++++++++++++++++++++------
 1 file changed, 121 insertions(+), 25 deletions(-)

diff --git a/drivers/clk/socfpga/clk-s10.c b/drivers/clk/socfpga/clk-s10.c
index b4bf4e2d38e1..1976789b5166 100644
--- a/drivers/clk/socfpga/clk-s10.c
+++ b/drivers/clk/socfpga/clk-s10.c
@@ -5,6 +5,7 @@
 #include <linux/slab.h>
 #include <linux/clk-provider.h>
 #include <linux/of.h>
+#include <linux/of_address.h>
 #include <linux/platform_device.h>
 
 #include <dt-bindings/clock/stratix10-clock.h>
@@ -188,20 +189,36 @@ static const struct stratix10_pll_clock s10_pll_clks[] = {
 	  0, 0xe4},
 };
 
+/*
+ * The SP timers are probed from time_init() and cannot defer, so their
+ * l4_sp_clk and its parents are registered from of_clk_init(). Everything
+ * else is registered when the platform driver probes.
+ */
+static const struct stratix10_perip_c_clock s10_early_perip_c_clks[] = {
+	{ STRATIX10_MAIN_NOC_BASE_CLK, "main_noc_base_clk", "main_pll", NULL, 1, 0, 0x88},
+	{ STRATIX10_PERI_NOC_BASE_CLK, "peri_noc_base_clk", "periph_pll", NULL, 1, 0,
+	  0xF8},
+};
+
+static const struct stratix10_perip_cnt_clock s10_early_perip_cnt_clks[] = {
+	{ STRATIX10_NOC_FREE_CLK, "noc_free_clk", NULL, noc_free_mux, ARRAY_SIZE(noc_free_mux),
+	  0, 0x4C, 0, 0x3C, 1},
+};
+
+static const struct stratix10_gate_clock s10_early_gate_clks[] = {
+	{ STRATIX10_L4_SP_CLK, "l4_sp_clk", NULL, noc_mux, ARRAY_SIZE(noc_mux),
+	  CLK_IS_CRITICAL, 0x30, 3, 0x70, 16, 2, 0x3C, 1, 0},
+};
+
 static const struct stratix10_perip_c_clock s10_main_perip_c_clks[] = {
 	{ STRATIX10_MAIN_MPU_BASE_CLK, "main_mpu_base_clk", "main_pll", NULL, 1, 0, 0x84},
-	{ STRATIX10_MAIN_NOC_BASE_CLK, "main_noc_base_clk", "main_pll", NULL, 1, 0, 0x88},
 	{ STRATIX10_PERI_MPU_BASE_CLK, "peri_mpu_base_clk", "periph_pll", NULL, 1, 0,
 	  0xF4},
-	{ STRATIX10_PERI_NOC_BASE_CLK, "peri_noc_base_clk", "periph_pll", NULL, 1, 0,
-	  0xF8},
 };
 
 static const struct stratix10_perip_cnt_clock s10_main_perip_cnt_clks[] = {
 	{ STRATIX10_MPU_FREE_CLK, "mpu_free_clk", NULL, mpu_free_mux, ARRAY_SIZE(mpu_free_mux),
 	   0, 0x48, 0, 0, 0},
-	{ STRATIX10_NOC_FREE_CLK, "noc_free_clk", NULL, noc_free_mux, ARRAY_SIZE(noc_free_mux),
-	  0, 0x4C, 0, 0x3C, 1},
 	{ STRATIX10_MAIN_EMACA_CLK, "main_emaca_clk", "main_noc_base_clk", NULL, 1, 0,
 	  0x50, 0, 0, 0},
 	{ STRATIX10_MAIN_EMACB_CLK, "main_emacb_clk", "main_noc_base_clk", NULL, 1, 0,
@@ -263,8 +280,6 @@ static const struct stratix10_gate_clock s10_gate_clks[] = {
 	  1, 0x70, 0, 2, 0x3C, 1, 0},
 	{ STRATIX10_L4_MP_CLK, "l4_mp_clk", NULL, noc_mux, ARRAY_SIZE(noc_mux), 0, 0x30,
 	  2, 0x70, 8, 2, 0x3C, 1, 0},
-	{ STRATIX10_L4_SP_CLK, "l4_sp_clk", NULL, noc_mux, ARRAY_SIZE(noc_mux), CLK_IS_CRITICAL, 0x30,
-	  3, 0x70, 16, 2, 0x3C, 1, 0},
 	{ STRATIX10_CS_AT_CLK, "cs_at_clk", NULL, noc_mux, ARRAY_SIZE(noc_mux), 0, 0x30,
 	  4, 0x70, 24, 2, 0x3C, 1, 0},
 	{ STRATIX10_CS_TRACE_CLK, "cs_trace_clk", NULL, noc_mux, ARRAY_SIZE(noc_mux), 0, 0x30,
@@ -382,33 +397,111 @@ static int s10_clk_register_pll(const struct stratix10_pll_clock *clks,
 	return 0;
 }
 
-static int s10_clkmgr_init(struct platform_device *pdev)
+static struct stratix10_clock_data *s10_clk_data;
+
+static struct stratix10_clock_data *s10_clk_data_alloc(void __iomem *base)
+{
+	struct stratix10_clock_data *clk_data;
+	int i;
+
+	clk_data = kzalloc(struct_size(clk_data, clk_data.hws, STRATIX10_NUM_CLKS),
+			   GFP_KERNEL);
+	if (!clk_data)
+		return NULL;
+
+	clk_data->base = base;
+	clk_data->clk_data.num = STRATIX10_NUM_CLKS;
+
+	for (i = 0; i < STRATIX10_NUM_CLKS; i++)
+		clk_data->clk_data.hws[i] = ERR_PTR(-EPROBE_DEFER);
+
+	return clk_data;
+}
+
+static void s10_clk_register_early(struct stratix10_clock_data *clk_data)
+{
+	s10_clk_register_pll(s10_pll_clks, ARRAY_SIZE(s10_pll_clks), clk_data);
+
+	s10_clk_register_c_perip(s10_early_perip_c_clks,
+				 ARRAY_SIZE(s10_early_perip_c_clks), clk_data);
+
+	s10_clk_register_cnt_perip(s10_early_perip_cnt_clks,
+				   ARRAY_SIZE(s10_early_perip_cnt_clks),
+				   clk_data);
+
+	s10_clk_register_gate(s10_early_gate_clks,
+			      ARRAY_SIZE(s10_early_gate_clks), clk_data);
+}
+
+/*
+ * The registered clocks keep using the mapping and clk_data, so neither is
+ * released once the early clocks have been registered.
+ */
+static int s10_clkmgr_setup(struct device_node *np)
 {
-	struct device_node *np = pdev->dev.of_node;
-	struct device *dev = &pdev->dev;
 	struct stratix10_clock_data *clk_data;
 	void __iomem *base;
-	int i, num_clks;
 
-	base = devm_platform_ioremap_resource(pdev, 0);
-	if (IS_ERR(base)) {
+	base = of_iomap(np, 0);
+	if (!base) {
 		pr_err("%s: failed to map clock registers\n", __func__);
-		return PTR_ERR(base);
+		return -ENOMEM;
 	}
 
-	num_clks = STRATIX10_NUM_CLKS;
-	clk_data = devm_kzalloc(dev, struct_size(clk_data, clk_data.hws,
-						 num_clks), GFP_KERNEL);
-	if (!clk_data)
+	clk_data = s10_clk_data_alloc(base);
+	if (!clk_data) {
+		iounmap(base);
 		return -ENOMEM;
+	}
 
-	clk_data->base = base;
-	clk_data->clk_data.num = num_clks;
+	s10_clk_register_early(clk_data);
+	s10_clk_data = clk_data;
 
-	for (i = 0; i < num_clks; i++)
-		clk_data->clk_data.hws[i] = ERR_PTR(-ENOENT);
+	return 0;
+}
 
-	s10_clk_register_pll(s10_pll_clks, ARRAY_SIZE(s10_pll_clks), clk_data);
+static int s10_clkmgr_add_provider(struct device_node *np)
+{
+	static bool added;
+	int ret;
+
+	if (added)
+		return 0;
+
+	ret = of_clk_add_hw_provider(np, of_clk_hw_onecell_get,
+				     &s10_clk_data->clk_data);
+	if (ret)
+		return ret;
+
+	added = true;
+
+	return 0;
+}
+
+static void __init s10_clkmgr_early_init(struct device_node *np)
+{
+	if (s10_clkmgr_setup(np))
+		return;
+
+	if (s10_clkmgr_add_provider(np))
+		pr_err("%s: failed to add clock provider\n", __func__);
+}
+
+CLK_OF_DECLARE_DRIVER(stratix10_clkmgr, "intel,stratix10-clkmgr",
+		      s10_clkmgr_early_init);
+
+static int s10_clkmgr_init(struct platform_device *pdev)
+{
+	struct stratix10_clock_data *clk_data;
+	int i, ret;
+
+	if (!s10_clk_data) {
+		ret = s10_clkmgr_setup(pdev->dev.of_node);
+		if (ret)
+			return ret;
+	}
+
+	clk_data = s10_clk_data;
 
 	s10_clk_register_c_perip(s10_main_perip_c_clks,
 				 ARRAY_SIZE(s10_main_perip_c_clks), clk_data);
@@ -420,8 +513,11 @@ static int s10_clkmgr_init(struct platform_device *pdev)
 	s10_clk_register_gate(s10_gate_clks, ARRAY_SIZE(s10_gate_clks),
 			      clk_data);
 
-	of_clk_add_hw_provider(np, of_clk_hw_onecell_get, &clk_data->clk_data);
-	return 0;
+	for (i = 0; i < STRATIX10_NUM_CLKS; i++)
+		if (clk_data->clk_data.hws[i] == ERR_PTR(-EPROBE_DEFER))
+			clk_data->clk_data.hws[i] = ERR_PTR(-ENOENT);
+
+	return s10_clkmgr_add_provider(pdev->dev.of_node);
 }
 
 static int s10_clkmgr_probe(struct platform_device *pdev)
-- 
2.49.GIT

Re: [PATCH v3 3/3] clk: socfpga: stratix10: register the SP timer clocks early

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] clk: socfpga: data race in early provider hws array modification
--

commit dfd4eb57c5dabe82235a8f50e5c912f34f5c229f
Author: Adrian Ng Ho Yin <adrian.ho.yin.ng@altera.com>

clk: socfpga: stratix10: register the SP timer clocks early

This commit changes the Stratix10 clock manager to register the l4_sp_clk
and its parents early via CLK_OF_DECLARE_DRIVER(). The remaining clocks
are deferred and registered later when the platform driver probes. The clock
manager structure, including the hardware clock array, is reused between
the early and late init phases.

> diff --git a/drivers/clk/socfpga/clk-s10.c b/drivers/clk/socfpga/clk-s10.c
> --- a/drivers/clk/socfpga/clk-s10.c
> +++ b/drivers/clk/socfpga/clk-s10.c

[ ... ]

> +static int s10_clkmgr_init(struct platform_device *pdev)
> +{
> +	struct stratix10_clock_data *clk_data;
> +	int i, ret;
> +
> +	if (!s10_clk_data) {
> +		ret = s10_clkmgr_setup(pdev->dev.of_node);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	clk_data = s10_clk_data;
>  
> +	s10_clk_register_c_perip(s10_main_perip_c_clks,
> +				 ARRAY_SIZE(s10_main_perip_c_clks), clk_data);

[Severity: Medium]
Does this introduce a data race with concurrent clock consumers?

Because s10_clkmgr_early_init() previously exposed the clock provider array
by calling of_clk_add_hw_provider(), the array can now be read locklessly
by other threads via of_clk_hw_onecell_get() while consumer drivers probe.

When s10_clk_register_c_perip() runs here, it writes newly registered clock
pointers directly into the array:

s10_clk_register_c_perip()
    data->clk_data.hws[clks[i].id] = hw_clk;

Modifying this array using standard C assignments without annotations like
WRITE_ONCE() or a lock while it is exposed to readers constitutes an
unannotated data race under the Linux Kernel Memory Model. This could 
potentially lead to compiler tearing or KCSAN splats.

[ ... ]

> +	for (i = 0; i < STRATIX10_NUM_CLKS; i++)
> +		if (clk_data->clk_data.hws[i] == ERR_PTR(-EPROBE_DEFER))
> +			clk_data->clk_data.hws[i] = ERR_PTR(-ENOENT);

[Severity: Medium]
Could modifying these deferral pointers concurrently cause issues for readers?

The same exposed clk_data->clk_data.hws array is modified directly without
WRITE_ONCE(). A concurrent reader accessing the array via
of_clk_hw_onecell_get() could observe partial updates or inconsistent states.

> +
> +	return s10_clkmgr_add_provider(pdev->dev.of_node);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1791871090.git.adrian.ho.yin.ng@altera.com?part=3