[PATCH] clk: Add fault tolerance to of_clk_hw_onecell_get()

[PATCH] clk: Add fault tolerance to of_clk_hw_onecell_get()

[Chuan Liu via B4 Relay]

From: Chuan Liu <chuan.liu@amlogic.com>

In specific cases, even a clk_provider managing only a single clock may
reference of_clk_hw_onecell_get() to access its member clocks, as seen
in implementations like clk-scmi.

For a clk_provider with only one clock, when calling
of_parse_phandle_with_args(), the phandle_args->args[] members are not
assigned. In this case, the reference to phandle_args->args[0] in
of_clk_hw_onecell_get() becomes invalid. If phandle_args->args[0]
initially contains a non-zero value, this will trigger an error.

Signed-off-by: Chuan Liu <chuan.liu@amlogic.com>
---
Error conditions observed:

scmi_clk: protocol@14 {
	reg = <0x14>;
	#clock-cells = <0>;
};

phandle1: clock-controller@1 {
	#clock-cells = <1>;
}

clock-consumer@2 {
	assigned-clocks = <&phandle1 1>,
			  <&scmi_clk>;
	assigned-clock-rates = <xxx>,
			       <xxx>;
}

Under these conditions, executing of_clk_set_defaults() triggers the
error: 'of_clk_hw_onecell_get: invalid index 1'.

While the root cause lies in invalid input data to
of_clk_hw_onecell_get(), checking phandle->args_count for data validity
improves driver fault tolerance.
---
 drivers/clk/clk.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 0565c87656cf..4994c551befe 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -4966,7 +4966,7 @@ EXPORT_SYMBOL_GPL(of_clk_hw_simple_get);
 struct clk *of_clk_src_onecell_get(struct of_phandle_args *clkspec, void *data)
 {
 	struct clk_onecell_data *clk_data = data;
-	unsigned int idx = clkspec->args[0];
+	unsigned int idx = clkspec->args_count ? clkspec->args[0] : 0;
 
 	if (idx >= clk_data->clk_num) {
 		pr_err("%s: invalid clock index %u\n", __func__, idx);
@@ -4981,7 +4981,7 @@ struct clk_hw *
 of_clk_hw_onecell_get(struct of_phandle_args *clkspec, void *data)
 {
 	struct clk_hw_onecell_data *hw_data = data;
-	unsigned int idx = clkspec->args[0];
+	unsigned int idx = clkspec->args_count ? clkspec->args[0] : 0;
 
 	if (idx >= hw_data->num) {
 		pr_err("%s: invalid index %u\n", __func__, idx);

---
base-commit: 58abdca0eb653c1a2e755ba9ba406ee475d87636
change-id: 20250730-add_fault_tolerance_to_of_clk_hw_onecell_get-fdc81f7e35a5

Best regards,
-- 
Chuan Liu <chuan.liu@amlogic.com>

Re: [PATCH] clk: Add fault tolerance to of_clk_hw_onecell_get()

[Stephen Boyd]

Quoting Chuan Liu via B4 Relay (2025-07-31 05:39:58)
> From: Chuan Liu <chuan.liu@amlogic.com>
> 
> In specific cases, even a clk_provider managing only a single clock may
> reference of_clk_hw_onecell_get() to access its member clocks, as seen
> in implementations like clk-scmi.
> 
> For a clk_provider with only one clock, when calling
> of_parse_phandle_with_args(), the phandle_args->args[] members are not
> assigned. In this case, the reference to phandle_args->args[0] in
> of_clk_hw_onecell_get() becomes invalid. If phandle_args->args[0]
> initially contains a non-zero value, this will trigger an error.
> 
> Signed-off-by: Chuan Liu <chuan.liu@amlogic.com>
> ---
> Error conditions observed:
> 
> scmi_clk: protocol@14 {
>         reg = <0x14>;
>         #clock-cells = <0>;
> };
> 
> phandle1: clock-controller@1 {
>         #clock-cells = <1>;
> }
> 
> clock-consumer@2 {
>         assigned-clocks = <&phandle1 1>,
>                           <&scmi_clk>;
>         assigned-clock-rates = <xxx>,
>                                <xxx>;
> }
> 
> Under these conditions, executing of_clk_set_defaults() triggers the
> error: 'of_clk_hw_onecell_get: invalid index 1'.

Please write a KUnit test.

Re: [PATCH] clk: Add fault tolerance to of_clk_hw_onecell_get()

[Chuan Liu]

Hi Stephen:

   Thanks for the quick feedback. I'll improve the KUnit tests and send v2.


On 8/1/2025 12:45 AM, Stephen Boyd wrote:
> [ EXTERNAL EMAIL ]
>
> Quoting Chuan Liu via B4 Relay (2025-07-31 05:39:58)
>> From: Chuan Liu <chuan.liu@amlogic.com>
>>
>> In specific cases, even a clk_provider managing only a single clock may
>> reference of_clk_hw_onecell_get() to access its member clocks, as seen
>> in implementations like clk-scmi.
>>
>> For a clk_provider with only one clock, when calling
>> of_parse_phandle_with_args(), the phandle_args->args[] members are not
>> assigned. In this case, the reference to phandle_args->args[0] in
>> of_clk_hw_onecell_get() becomes invalid. If phandle_args->args[0]
>> initially contains a non-zero value, this will trigger an error.
>>
>> Signed-off-by: Chuan Liu <chuan.liu@amlogic.com>
>> ---
>> Error conditions observed:
>>
>> scmi_clk: protocol@14 {
>>          reg = <0x14>;
>>          #clock-cells = <0>;
>> };
>>
>> phandle1: clock-controller@1 {
>>          #clock-cells = <1>;
>> }
>>
>> clock-consumer@2 {
>>          assigned-clocks = <&phandle1 1>,
>>                            <&scmi_clk>;
>>          assigned-clock-rates = <xxx>,
>>                                 <xxx>;
>> }
>>
>> Under these conditions, executing of_clk_set_defaults() triggers the
>> error: 'of_clk_hw_onecell_get: invalid index 1'.
> Please write a KUnit test.

Re: [PATCH] clk: Add fault tolerance to of_clk_hw_onecell_get()

[Chuan Liu]

Hi Stephen:

     After reviewing all CCF drivers referencing of_clk_hw_onecell_get(),
I confirmed their corresponding binding *.yaml files define
'#clock-cells' as 'const: 1'.

Therefore, the case I previously described is currently disallowed.
Please ignore this patch. Thanks.


On 8/1/2025 12:45 AM, Stephen Boyd wrote:
> [ EXTERNAL EMAIL ]
>
> Quoting Chuan Liu via B4 Relay (2025-07-31 05:39:58)
>> From: Chuan Liu <chuan.liu@amlogic.com>
>>
>> In specific cases, even a clk_provider managing only a single clock may
>> reference of_clk_hw_onecell_get() to access its member clocks, as seen
>> in implementations like clk-scmi.
>>
>> For a clk_provider with only one clock, when calling
>> of_parse_phandle_with_args(), the phandle_args->args[] members are not
>> assigned. In this case, the reference to phandle_args->args[0] in
>> of_clk_hw_onecell_get() becomes invalid. If phandle_args->args[0]
>> initially contains a non-zero value, this will trigger an error.
>>
>> Signed-off-by: Chuan Liu <chuan.liu@amlogic.com>
>> ---
>> Error conditions observed:
>>
>> scmi_clk: protocol@14 {
>>          reg = <0x14>;
>>          #clock-cells = <0>;
>> };
>>
>> phandle1: clock-controller@1 {
>>          #clock-cells = <1>;
>> }
>>
>> clock-consumer@2 {
>>          assigned-clocks = <&phandle1 1>,
>>                            <&scmi_clk>;
>>          assigned-clock-rates = <xxx>,
>>                                 <xxx>;
>> }
>>
>> Under these conditions, executing of_clk_set_defaults() triggers the
>> error: 'of_clk_hw_onecell_get: invalid index 1'.
> Please write a KUnit test.