linux-kernel - Re: [PATCH 14/26] clk: mvebu: Convert to clk

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20151015104326.6f7e907c@free-electrons.com>
Date:	Thu, 15 Oct 2015 10:43:26 +0200
From:	Thomas Petazzoni <thomas.petazzoni@...e-electrons.com>
To:	Stephen Boyd <sboyd@...eaurora.org>
Cc:	Mike Turquette <mturquette@...libre.com>,
	linux-kernel@...r.kernel.org, linux-clk@...r.kernel.org,
	Gregory CLEMENT <gregory.clement@...e-electrons.com>,
	Jason Cooper <jason@...edaemon.net>,
	Andrew Lunn <andrew@...n.ch>,
	Sebastian Hesselbarth <sebastian.hesselbarth@...il.com>
Subject: Re: [PATCH 14/26] clk: mvebu: Convert to clk_hw based provider APIs

Stephen,

On Wed, 14 Oct 2015 14:08:59 -0700, Stephen Boyd wrote:

> Here's that untested patch, which we can throw into clk-next for
> v4.4
> 
> -----8<----
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index b005f666e3a1..16b86a551bcb 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -3055,6 +3055,7 @@ const char *of_clk_get_parent_name(struct device_node *np, int index)
>  	u32 pv;
>  	int rc;
>  	int count;
> +	struct clk *clk;
>  
>  	if (index < 0)
>  		return NULL;
> @@ -3080,8 +3081,25 @@ const char *of_clk_get_parent_name(struct device_node *np, int index)
>  
>  	if (of_property_read_string_index(clkspec.np, "clock-output-names",
>  					  index,
> -					  &clk_name) < 0)
> -		clk_name = clkspec.np->name;
> +					  &clk_name) < 0) {
> +		/*
> +		 * Best effort to get the name if the clock has been
> +		 * registered with the framework. If the clock isn't
> +		 * registered, we return the node name as the name of
> +		 * the clock as long as #clock-cells = 0.
> +		 */
> +		clk = of_clk_get(np, index);
> +		if (IS_ERR(clk)) {
> +			if (clkspec.args_count == 0)
> +				clk_name = clkspec.np->name;
> +			else
> +				clk_name = NULL;
> +		} else {
> +			clk_name = __clk_get_name(clk);
> +			clk_put(clk);
> +		}
> +	}
> +
>  
>  	of_node_put(clkspec.np);
>  	return clk_name;

It almost worked, but not completely. The issue is that by the time you
call of_clk_get(np, index), index is no longer equal to the value
passed as argument to of_clk_get_parent_name(), it has been modified to
indicate the index of the *parent* clock.

So, after changing your patch to:

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 0ebcf44..60d2f62 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -3052,9 +3052,11 @@ const char *of_clk_get_parent_name(struct device_node *np, int index)
        struct property *prop;
        const char *clk_name;
        const __be32 *vp;
+       int parent_index;
        u32 pv;
        int rc;
        int count;
+       struct clk *clk;
 
        if (index < 0)
                return NULL;
@@ -3064,14 +3066,14 @@ const char *of_clk_get_parent_name(struct device_node *np, int index)
        if (rc)
                return NULL;
 
-       index = clkspec.args_count ? clkspec.args[0] : 0;
+       parent_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) {
+               if (parent_index == pv) {
                        index = count;
                        break;
                }
@@ -3079,9 +3081,26 @@ const char *of_clk_get_parent_name(struct device_node *np, int index)
        }
 
        if (of_property_read_string_index(clkspec.np, "clock-output-names",
-                                         index,
-                                         &clk_name) < 0)
-               clk_name = clkspec.np->name;
+                                         parent_index,
+                                         &clk_name) < 0) {
+               /*
+                * Best effort to get the name if the clock has been
+                * registered with the framework. If the clock isn't
+                * registered, we return the node name as the name of
+                * the clock as long as #clock-cells = 0.
+                */
+               clk = of_clk_get(np, index);
+               if (IS_ERR(clk)) {
+                       if (clkspec.args_count == 0)
+                               clk_name = clkspec.np->name;
+                       else
+                               clk_name = NULL;
+               } else {
+                       clk_name = __clk_get_name(clk);
+                       clk_put(clk);
+               }
+       }
+
 
        of_node_put(clkspec.np);
        return clk_name;

It does work properly for me, on top of 4.3-rc5, without any change to
my Device Tree files.

So, what is the plan now ?

 - Have the minimal fix in drivers/clk/mvebu/clk-cpu.c for 4.3.

 - Have the patch improving the of_clk_get_parent_name() logic merged
   in 4.4 + a revert of the fix done for 4.3 in clk-cpu.c. Unless maybe
   you don't want to clutter the core of the clock framework to handle
   this specific case?

 - Add the clock-output-names to the Marvell EBU Device Tree files, so
   that in a couple of kernel releases we can remove the hacks and rely
   on the generic logic of of_clk_get_parent_name().

Thanks,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/