[PATCH v2] clk: scmi: Fix children encountered before parents case

[PATCH v2] clk: scmi: Fix children encountered before parents case

[Sascha Hauer]

When it comes to clocks with parents the SCMI clk driver assumes that
parents are always initialized before their children which might not
always be the case.

During initialization of the parent_data array we have:

	sclk->parent_data[i].hw = hws[sclk->info->parents[i]];

hws[sclk->info->parents[i]] will not yet be initialized when children
are encountered before their possible parents. Solve this by allocating
all struct scmi_clk as an array first and populating all hws[] upfront.

Fixes: 65a8a3dd3b95f ("clk: scmi: Add support for clock {set,get}_parent")
Reviewed-by: peng.fan@nxp.com
Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>
Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
Changes in v2:
- Collect reviewed-by from Cristian Marussi and Peng Fan
- Do not use a local variable that is only used once (Christian)
- Link to v1: https://lore.kernel.org/r/20250604-clk-scmi-children-parent-fix-v1-1-be206954d866@pengutronix.de
---
 drivers/clk/clk-scmi.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c
index 15510c2ff21c0335f5cb30677343bd4ef59c0738..1b1561c84127b9e41bfb6096ceb3626f32c4fee0 100644
--- a/drivers/clk/clk-scmi.c
+++ b/drivers/clk/clk-scmi.c
@@ -404,6 +404,7 @@ static int scmi_clocks_probe(struct scmi_device *sdev)
 	const struct scmi_handle *handle = sdev->handle;
 	struct scmi_protocol_handle *ph;
 	const struct clk_ops *scmi_clk_ops_db[SCMI_MAX_CLK_OPS] = {};
+	struct scmi_clk *sclks;
 
 	if (!handle)
 		return -ENODEV;
@@ -430,18 +431,21 @@ static int scmi_clocks_probe(struct scmi_device *sdev)
 	transport_is_atomic = handle->is_transport_atomic(handle,
 							  &atomic_threshold_us);
 
+	sclks = devm_kcalloc(dev, count, sizeof(*sclks), GFP_KERNEL);
+	if (!sclks)
+		return -ENOMEM;
+
+	for (idx = 0; idx < count; idx++)
+		hws[idx] = &sclks[idx].hw;
+
 	for (idx = 0; idx < count; idx++) {
-		struct scmi_clk *sclk;
+		struct scmi_clk *sclk = &sclks[idx];
 		const struct clk_ops *scmi_ops;
 
-		sclk = devm_kzalloc(dev, sizeof(*sclk), GFP_KERNEL);
-		if (!sclk)
-			return -ENOMEM;
-
 		sclk->info = scmi_proto_clk_ops->info_get(ph, idx);
 		if (!sclk->info) {
 			dev_dbg(dev, "invalid clock info for idx %d\n", idx);
-			devm_kfree(dev, sclk);
+			hws[idx] = NULL;
 			continue;
 		}
 
@@ -479,13 +483,11 @@ static int scmi_clocks_probe(struct scmi_device *sdev)
 		if (err) {
 			dev_err(dev, "failed to register clock %d\n", idx);
 			devm_kfree(dev, sclk->parent_data);
-			devm_kfree(dev, sclk);
 			hws[idx] = NULL;
 		} else {
 			dev_dbg(dev, "Registered clock:%s%s\n",
 				sclk->info->name,
 				scmi_ops->enable ? " (atomic ops)" : "");
-			hws[idx] = &sclk->hw;
 		}
 	}
 

---
base-commit: 5abc7438f1e9d62e91ad775cc83c9594c48d2282
change-id: 20250604-clk-scmi-children-parent-fix-45a8912b8ba0

Best regards,
-- 
Sascha Hauer <s.hauer@pengutronix.de>

Re: [PATCH v2] clk: scmi: Fix children encountered before parents case

[Sudeep Holla]

On Thu, Jun 12, 2025 at 09:36:58AM +0200, Sascha Hauer wrote:
> When it comes to clocks with parents the SCMI clk driver assumes that
> parents are always initialized before their children which might not
> always be the case.
> 
> During initialization of the parent_data array we have:
> 
> 	sclk->parent_data[i].hw = hws[sclk->info->parents[i]];
> 
> hws[sclk->info->parents[i]] will not yet be initialized when children
> are encountered before their possible parents. Solve this by allocating
> all struct scmi_clk as an array first and populating all hws[] upfront.
> 

LGTM. I would like to add a note that we don't free individual scmi_clk
if for some reason it fails to setup. I can do that when I apply, just
checking if anyone has any objections. Please shout out if you have.

-- 
Regards,
Sudeep

Re: [PATCH v2] clk: scmi: Fix children encountered before parents case

[Sascha Hauer]

On Thu, Jun 12, 2025 at 09:29:16AM +0100, Sudeep Holla wrote:
> On Thu, Jun 12, 2025 at 09:36:58AM +0200, Sascha Hauer wrote:
> > When it comes to clocks with parents the SCMI clk driver assumes that
> > parents are always initialized before their children which might not
> > always be the case.
> > 
> > During initialization of the parent_data array we have:
> > 
> > 	sclk->parent_data[i].hw = hws[sclk->info->parents[i]];
> > 
> > hws[sclk->info->parents[i]] will not yet be initialized when children
> > are encountered before their possible parents. Solve this by allocating
> > all struct scmi_clk as an array first and populating all hws[] upfront.
> > 
> 
> LGTM. I would like to add a note that we don't free individual scmi_clk
> if for some reason it fails to setup. I can do that when I apply, just
> checking if anyone has any objections. Please shout out if you have.

Feel free to add that note. I should have added this myself since it's
not entirely obvious that the devm_kfree() has to be removed with this
patch.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2] clk: scmi: Fix children encountered before parents case

[Sudeep Holla]

On Thu, Jun 12, 2025 at 10:43:32AM +0200, Sascha Hauer wrote:
> On Thu, Jun 12, 2025 at 09:29:16AM +0100, Sudeep Holla wrote:
> > On Thu, Jun 12, 2025 at 09:36:58AM +0200, Sascha Hauer wrote:
> > > When it comes to clocks with parents the SCMI clk driver assumes that
> > > parents are always initialized before their children which might not
> > > always be the case.
> > > 
> > > During initialization of the parent_data array we have:
> > > 
> > > 	sclk->parent_data[i].hw = hws[sclk->info->parents[i]];
> > > 
> > > hws[sclk->info->parents[i]] will not yet be initialized when children
> > > are encountered before their possible parents. Solve this by allocating
> > > all struct scmi_clk as an array first and populating all hws[] upfront.
> > > 
> > 
> > LGTM. I would like to add a note that we don't free individual scmi_clk
> > if for some reason it fails to setup. I can do that when I apply, just
> > checking if anyone has any objections. Please shout out if you have.
> 
> Feel free to add that note. I should have added this myself since it's
> not entirely obvious that the devm_kfree() has to be removed with this
> patch.
> 

I did that and realised only bit later that I usually route SCMI clk driver
changes via clk tree.

Please repost with

Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>

I am sharing below the commit message update I did when I applied.

Regards,
Sudeep

-->8

clk: scmi: Handle case where child clocks are initialized before their parents

The SCMI clock driver currently assumes that parent clocks are always
initialized before their children. However, this assumption can fail if
a child clock is encountered before its parent during probe.

This leads to an issue during initialization of the parent_data array:

    sclk->parent_data[i].hw = hws[sclk->info->parents[i]];

If the parent clock's hardware structure has not been initialized yet,
this assignment results in invalid data.

To resolve this, allocate all struct scmi_clk instances as a contiguous
array at the beginning of the probe and populate the hws[] array upfront.
This ensures that any parent referenced later is already initialized,
regardless of the order in which clocks are processed.

Note that we can no longer free individual scmi_clk instances if
scmi_clk_ops_init() fails which shouldn't be a problem if the SCMI
platform has proper per-agent clock discovery.