Re: Re: [PATCH v2] clk: check for disabled clock-provider in of_clk_get_hw_from_clkspec

[Stephen Boyd <sboyd@kernel.org>]

Quoting Heiko Stübner (2025-02-27 06:36:39)
> Hi Stephen,
> 
> Am Mittwoch, 26. Februar 2025, 23:32:28 MEZ schrieb Stephen Boyd:
> > 
> > Applied to clk-next (unless this needs to fix something urgent?)
> 
> the area where this affects something for me is slated for 6.15, so
> personally I see no urge to have this in 6.14 - especially as the effect
> is present for so long already and nobody complained.
> 

Ok cool.

> 
> > I also wonder if we should use a
> > different return value so that we don't try to look up the clk by name
> > (see clk_core_fill_parent_index()). We could go even further and stop
> > trying to find the clk over and over again too. Maybe -ENODEV can
> > indicate that and we can cache that parent entry value so we stop
> > trying.
> 
> Pffff ... no clue :-)
> 
> I.e. in the case I have, we're coming from clk_get_optional() [0].
> which is supposed to just return NULL if the clock is not found, so at
> least for the consumer view, the internals are not fixed and we could have
> different "internal" error codes.
> 
> Not sure if more direct users of the of_clk_ functions would be affected
> though?
> 
> 
> In the case above, the optional clock is just a single one coming from a
> phy-block, which may probe later (needing defer) or never (if disabled).
> 
> As for caching the ENODEV, I'm not sure how often we'd experience that?
> 
> Like "normally" you have that one big clock-controller + maybe a number
> of smaller ones + maybe some blocks that expose one or two clocks
> to one specific user - in my case the hdmi-phy exposing its hdmi-pll
> for a nicer rate to the display controller when generating a hdmi output.
> 
> Does a case exist where some never-probed clock controller would have
> so many clock-consumers that caching that single of-property check
> would matter?

I don't know, nobody has measured it likely because this almost never
happens. I'm happy to not worry about the caching part until it's shown
to be a problem worth fixing.

Is there any point in searching by name in clk_core_fill_parent_index()
though? We've already found the reference in DT, and it isn't available,
so we know that the search by name is wrong. It may actually be harmful
if some parent can be found by name via the fallback path for a disabled
node. I'm thinking of this patch. I also noticed that we could have just
returned NULL from of_clk_get_hw_from_clkspec() and it would work the
same, but this is probably better so that we can build EPROBE_DEFER
logic on top.

----8<----
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 50faafbf5dda..17aa6e0a8ff7 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -475,11 +475,14 @@ of_clk_get_hw_from_clkspec(struct of_phandle_args *clkspec)
  *              #clock-cells = <1>;
  *      };
  *
- * Returns: -ENOENT when the provider can't be found or the clk doesn't
- * exist in the provider or the name can't be found in the DT node or
- * in a clkdev lookup. NULL when the provider knows about the clk but it
- * isn't provided on this system.
- * A valid clk_core pointer when the clk can be found in the provider.
+ * Return:
+ * * %-ENOENT  - The provider can't be found or the clk doesn't
+ *               exist in the provider or the name can't be found in the DT node or
+ *               in a clkdev lookup.
+ * * %-ENODEV  - The provider is disabled or in the failed state.
+ * * %NULL     - The provider knows about the clk but it isn't provided on this
+ *               system.
+ * * A valid clk_core pointer when the clk can be found in the provider.
  */
 static struct clk_core *clk_core_get(struct clk_core *core, u8 p_index)
 {
@@ -493,7 +496,14 @@ static struct clk_core *clk_core_get(struct clk_core *core, u8 p_index)
 
 	if (np && (name || index >= 0) &&
 	    !of_parse_clkspec(np, index, name, &clkspec)) {
-		hw = of_clk_get_hw_from_clkspec(&clkspec);
+		/*
+		 * Skip lookup by name in clk_core_fill_parent_index() if the
+		 * device node is unavailable.
+		 */
+		if (!of_device_is_available(clkspec.np))
+			hw = ERR_PTR(-ENODEV);
+		else
+			hw = of_clk_get_hw_from_clkspec(&clkspec);
 		of_node_put(clkspec.np);
 	} else if (name) {
 		/*