[PATCH v2] clk: let of_clk_get_parent_name() fail for invalid clock-indices

[PATCH v2] clk: let of_clk_get_parent_name() fail for invalid clock-indices

[Masahiro Yamada]

Currently, of_clk_get_parent_name() returns a wrong parent clock name
when "clock-indices" property exists and the target index is not
found in the property.  In this case, NULL should be returned.

For example,

        oscillator {
                compatible = "myclocktype";
                #clock-cells = <1>;
                clock-indices = <1>, <3>;
                clock-output-names = "clka", "clkb";
        };

        consumer {
                compatible = "myclockconsumer";
                clocks = <&oscillator 0>, <&oscillator 1>;
        };

Currently, of_clk_get_parent_name(consumer_np, 0) returns "clka"
(and of_clk_get_parent_name(consumer_np, 1) also returns "clka",
this is correct).   Because the "clock-indices" in the clock parent
does not contain <0>, of_clk_get_parent_name(consumer_np, 0) should
return NULL.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

Changes in v2:
  - Rephrase the git-log

 drivers/clk/clk.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 20d8e07..8698074 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -3054,12 +3054,9 @@ EXPORT_SYMBOL_GPL(of_clk_get_parent_count);
 const char *of_clk_get_parent_name(struct device_node *np, int index)
 {
 	struct of_phandle_args clkspec;
-	struct property *prop;
 	const char *clk_name;
-	const __be32 *vp;
-	u32 pv;
-	int rc;
-	int count;
+	const __be32 *list;
+	int rc, len, i;
 	struct clk *clk;
 
 	rc = of_parse_phandle_with_args(np, "clocks", "#clock-cells", index,
@@ -3068,17 +3065,20 @@ const char *of_clk_get_parent_name(struct device_node *np, int index)
 		return NULL;
 
 	index = clkspec.args_count ? clkspec.args[0] : 0;
-	count = 0;
 
 	/* if there is an indices property, use it to transfer the index
 	 * specified into an array offset for the clock-output-names property.
 	 */
-	of_property_for_each_u32(clkspec.np, "clock-indices", prop, vp, pv) {
-		if (index == pv) {
-			index = count;
-			break;
-		}
-		count++;
+	list = of_get_property(clkspec.np, "clock-indices", &len);
+	if (list) {
+		len /= sizeof(*list);
+		for (i = 0; i < len; i++)
+			if (index == be32_to_cpup(list++)) {
+				index = i;
+				break;
+			}
+		if (i == len)
+			return NULL;
 	}
 
 	if (of_property_read_string_index(clkspec.np, "clock-output-names",
-- 
1.9.1

Re: [PATCH v2] clk: let of_clk_get_parent_name() fail for invalid clock-indices

[Masahiro Yamada]

2015-11-30 17:46 GMT+09:00 Masahiro Yamada <yamada.masahiro@socionext.com>:
> Currently, of_clk_get_parent_name() returns a wrong parent clock name
> when "clock-indices" property exists and the target index is not
> found in the property.  In this case, NULL should be returned.
>
> For example,
>
>         oscillator {
>                 compatible = "myclocktype";
>                 #clock-cells = <1>;
>                 clock-indices = <1>, <3>;
>                 clock-output-names = "clka", "clkb";
>         };
>
>         consumer {
>                 compatible = "myclockconsumer";
>                 clocks = <&oscillator 0>, <&oscillator 1>;
>         };
>
> Currently, of_clk_get_parent_name(consumer_np, 0) returns "clka"
> (and of_clk_get_parent_name(consumer_np, 1) also returns "clka",
> this is correct).   Because the "clock-indices" in the clock parent
> does not contain <0>, of_clk_get_parent_name(consumer_np, 0) should
> return NULL.
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>



I've sent v2 to linux-gpio@vger.kernel.org by mistake.

My intention was to send it linux-clk@vger.kernel.org.


Sorry, Linus Walleij.




-- 
Best Regards
Masahiro Yamada

Re: [PATCH v2] clk: let of_clk_get_parent_name() fail for invalid clock-indices

[Stephen Boyd]

On 11/30, Masahiro Yamada wrote:
> Currently, of_clk_get_parent_name() returns a wrong parent clock name
> when "clock-indices" property exists and the target index is not
> found in the property.  In this case, NULL should be returned.
> 
> For example,
> 
>         oscillator {
>                 compatible = "myclocktype";
>                 #clock-cells = <1>;
>                 clock-indices = <1>, <3>;
>                 clock-output-names = "clka", "clkb";
>         };
> 
>         consumer {
>                 compatible = "myclockconsumer";
>                 clocks = <&oscillator 0>, <&oscillator 1>;
>         };
> 
> Currently, of_clk_get_parent_name(consumer_np, 0) returns "clka"
> (and of_clk_get_parent_name(consumer_np, 1) also returns "clka",
> this is correct).   Because the "clock-indices" in the clock parent
> does not contain <0>, of_clk_get_parent_name(consumer_np, 0) should
> return NULL.
> 
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>

Here's the proposed alternative, if you agree I will merge it
with the above commit text and attribution to you.

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index a66efc9d8bfc..f54583a9835a 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -3079,6 +3079,9 @@ const char *of_clk_get_parent_name(struct device_node *np, int index)
 		}
 		count++;
 	}
+	/* We went off the end of 'clock-indices' without finding it */
+	if (!vp && count)
+		return NULL;
 
 	if (of_property_read_string_index(clkspec.np, "clock-output-names",
 					  index,

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v2] clk: let of_clk_get_parent_name() fail for invalid clock-indices

[Masahiro Yamada]

Hi Stephen,


2015-12-01 9:58 GMT+09:00 Stephen Boyd <sboyd@codeaurora.org>:
> On 11/30, Masahiro Yamada wrote:
>> Currently, of_clk_get_parent_name() returns a wrong parent clock name
>> when "clock-indices" property exists and the target index is not
>> found in the property.  In this case, NULL should be returned.
>>
>> For example,
>>
>>         oscillator {
>>                 compatible = "myclocktype";
>>                 #clock-cells = <1>;
>>                 clock-indices = <1>, <3>;
>>                 clock-output-names = "clka", "clkb";
>>         };
>>
>>         consumer {
>>                 compatible = "myclockconsumer";
>>                 clocks = <&oscillator 0>, <&oscillator 1>;
>>         };
>>
>> Currently, of_clk_get_parent_name(consumer_np, 0) returns "clka"
>> (and of_clk_get_parent_name(consumer_np, 1) also returns "clka",
>> this is correct).   Because the "clock-indices" in the clock parent
>> does not contain <0>, of_clk_get_parent_name(consumer_np, 0) should
>> return NULL.
>>
>> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
>
> Here's the proposed alternative, if you agree I will merge it
> with the above commit text and attribution to you.
>
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index a66efc9d8bfc..f54583a9835a 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -3079,6 +3079,9 @@ const char *of_clk_get_parent_name(struct device_node *np, int index)
>                 }
>                 count++;
>         }
> +       /* We went off the end of 'clock-indices' without finding it */
> +       if (!vp && count)
> +               return NULL;
>
>         if (of_property_read_string_index(clkspec.np, "clock-output-names",
>                                           index,
>

No, again.
The existence of "clock-indices" should be checked
in order to omit the zero-length "clock-indices".


OK, let me guess the next alternative from you.


> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -3079,6 +3079,9 @@ const char *of_clk_get_parent_name(struct device_node *np, int index)
>                 }
>                 count++;
>         }
> +       /* We went off the end of 'clock-indices' without finding it */
> +       if (prop && !vp)
> +               return NULL;
>
>         if (of_property_read_string_index(clkspec.np, "clock-output-names",
>                                           index,
>

This works, but clumsy things are:

[1] If the "clock-indices" is missing, we can know it
     before looping around the of_property_for_each_u32().
     Checking the "prop" after the loop seems odd.

[2] "prop" and "vp" seem to be temporary storage that we should not
     know what they exactly are, like the auxiliary pointer in
list_for_each_safe().


Why do you insist on of_property_for_each_u32()?
It no longer fits in here.


-- 
Best Regards
Masahiro Yamada

Re: [PATCH v2] clk: let of_clk_get_parent_name() fail for invalid clock-indices

[Stephen Boyd]

On 12/01, Masahiro Yamada wrote:
> 2015-12-01 9:58 GMT+09:00 Stephen Boyd <sboyd@codeaurora.org>:
> > Here's the proposed alternative, if you agree I will merge it
> > with the above commit text and attribution to you.
> >
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index a66efc9d8bfc..f54583a9835a 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -3079,6 +3079,9 @@ const char *of_clk_get_parent_name(struct device_node *np, int index)
> >                 }
> >                 count++;
> >         }
> > +       /* We went off the end of 'clock-indices' without finding it */
> > +       if (!vp && count)
> > +               return NULL;
> >
> >         if (of_property_read_string_index(clkspec.np, "clock-output-names",
> >                                           index,
> >
> 
> No, again.
> The existence of "clock-indices" should be checked
> in order to omit the zero-length "clock-indices".
> 

Ah I missed that one. All these corner cases for broken DTs. Too
bad we don't have that DT validator.

> 
> OK, let me guess the next alternative from you.
> 
> 
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -3079,6 +3079,9 @@ const char *of_clk_get_parent_name(struct device_node *np, int index)
> >                 }
> >                 count++;
> >         }
> > +       /* We went off the end of 'clock-indices' without finding it */
> > +       if (prop && !vp)
> > +               return NULL;
> >
> >         if (of_property_read_string_index(clkspec.np, "clock-output-names",
> >                                           index,
> >

Cool, we can go faster now.

> 
> This works, but clumsy things are:
> 
> [1] If the "clock-indices" is missing, we can know it
>      before looping around the of_property_for_each_u32().
>      Checking the "prop" after the loop seems odd.

The of_property_for_each_u32 macro will do pretty much the same
work that you've done in your patch. So we're not going to go
around the loop at all in this case, we're just going to get the
property like you've done, and then of_prop_next_u32() is going
to return NULL and we'll never enter the loop.

Checking the property again is sort of odd, but we do similar
sorts of checks at the end of loops in other places in the
kernel, so it isn't out of the ordinary.

> 
> [2] "prop" and "vp" seem to be temporary storage that we should not
>      know what they exactly are, like the auxiliary pointer in
> list_for_each_safe().

True. In those cases, we check for list emptiness before
iterating over it with the list helper macros. So it sounds like
we should do that here as well.

> 
> 
> Why do you insist on of_property_for_each_u32()?
> It no longer fits in here.
> 

Sure. The other problem is be32_to_cpup() usage. I'd rather that
we use the style of looping that of_property_for_each_u32 does.
It doesn't make any assumptions about how the data is in memory.
Instead we call the iterator function and it gets the next value.

So here's another try, that open codes the macro so we can add
our count inside and check for a boolean property without
duplication.

----8<----

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index a66efc9d8bfc..a2112cdab191 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -3059,6 +3059,7 @@ const char *of_clk_get_parent_name(struct device_node *np, int index)
 	u32 pv;
 	int rc;
 	int count;
+	int len;
 	struct clk *clk;
 
 	rc = of_parse_phandle_with_args(np, "clocks", "#clock-cells", index,
@@ -3067,18 +3068,24 @@ const char *of_clk_get_parent_name(struct device_node *np, int index)
 		return NULL;
 
 	index = clkspec.args_count ? clkspec.args[0] : 0;
-	count = 0;
+	len = 0;
 
-	/* if there is an indices property, use it to transfer the index
+	/*
+	 * if there is an indices property, use it to transfer the index
 	 * specified into an array offset for the clock-output-names property.
 	 */
-	of_property_for_each_u32(clkspec.np, "clock-indices", prop, vp, pv) {
+	prop = of_find_property(clkspec.np, "clock-indices", &len);
+	for (vp = of_prop_next_u32(prop, NULL, &pv), count = 0;
+	     vp;
+	     vp = of_prop_next_u32(prop, vp, &pv), count++) {
 		if (index == pv) {
 			index = count;
 			break;
 		}
-		count++;
 	}
+	/* We went off the end of 'clock-indices' without finding it */
+	if (prop && count == len / sizeof(u32))
+		return NULL;
 
 	if (of_property_read_string_index(clkspec.np, "clock-output-names",
 					  index,
-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v2] clk: let of_clk_get_parent_name() fail for invalid clock-indices

[Masahiro Yamada]

Hi Stephen,


2015-12-01 17:40 GMT+09:00 Stephen Boyd <sboyd@codeaurora.org>:

>>
>>
>> Why do you insist on of_property_for_each_u32()?
>> It no longer fits in here.
>>
>
> Sure. The other problem is be32_to_cpup() usage. I'd rather that
> we use the style of looping that of_property_for_each_u32 does.
> It doesn't make any assumptions about how the data is in memory.
> Instead we call the iterator function and it gets the next value.

OK, let's avoid be32_to_cpup().


> So here's another try, that open codes the macro so we can add
> our count inside and check for a boolean property without
> duplication.
>
> ----8<----
>
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index a66efc9d8bfc..a2112cdab191 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -3059,6 +3059,7 @@ const char *of_clk_get_parent_name(struct device_node *np, int index)
>         u32 pv;
>         int rc;
>         int count;
> +       int len;
>         struct clk *clk;
>
>         rc = of_parse_phandle_with_args(np, "clocks", "#clock-cells", index,
> @@ -3067,18 +3068,24 @@ const char *of_clk_get_parent_name(struct device_node *np, int index)
>                 return NULL;
>
>         index = clkspec.args_count ? clkspec.args[0] : 0;
> -       count = 0;
> +       len = 0;
>
> -       /* if there is an indices property, use it to transfer the index
> +       /*
> +        * if there is an indices property, use it to transfer the index
>          * specified into an array offset for the clock-output-names property.
>          */
> -       of_property_for_each_u32(clkspec.np, "clock-indices", prop, vp, pv) {
> +       prop = of_find_property(clkspec.np, "clock-indices", &len);
> +       for (vp = of_prop_next_u32(prop, NULL, &pv), count = 0;
> +            vp;
> +            vp = of_prop_next_u32(prop, vp, &pv), count++) {
>                 if (index == pv) {
>                         index = count;
>                         break;
>                 }
> -               count++;
>         }
> +       /* We went off the end of 'clock-indices' without finding it */
> +       if (prop && count == len / sizeof(u32))
> +               return NULL;
>
>         if (of_property_read_string_index(clkspec.np, "clock-output-names",
>                                           index,


Hmm, the big "for" loop is the same as the expansion of
of_property_for_each_u32(),
this does not look cool to me.

OK, let's go back to of_property_for_each_u32() and choose the less
invasive fix.
I've sent v3.

BTW, I noticed that you had already applied the wrong version into the
clk-msm8996 branch.
(commit 6fe940981d)

It is wrong as I mentioned in my previous reply.
Please drop it.


-- 
Best Regards
Masahiro Yamada