linux-kernel - Re: [PATCH 2/2] clk: mvebu: dynamically allocate resources in Armada CP110 system controller

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]

Date:   Wed, 24 Aug 2016 17:16:13 -0700
From:   Stephen Boyd <sboyd@...eaurora.org>
To:     Marcin Wojtas <mw@...ihalf.com>
Cc:     linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-clk@...r.kernel.org, mturquette@...libre.com,
        sebastian.hesselbarth@...il.com, andrew@...n.ch,
        jason@...edaemon.net, thomas.petazzoni@...e-electrons.com,
        gregory.clement@...e-electrons.com, nadavh@...vell.com,
        alior@...vell.com, tn@...ihalf.com, jaz@...ihalf.com
Subject: Re: [PATCH 2/2] clk: mvebu: dynamically allocate resources in Armada
 CP110 system controller

On 08/23, Marcin Wojtas wrote:
> Original commit, which added support for Armada CP110 system controller
> used global variables for storing all clock information. It worked
> fine for Armada 7k SoC, with single CP110 block. After dual-CP110 Armada 8k
> was introduced, the data got overwritten and corrupted.
> 
> This patch fixes the issue by allocating resources dynamically in the
> driver probe and storing it as platform drvdata.
> 
> Fixes: d3da3eaef7f4 ("clk: mvebu: new driver for Armada CP110 system ...")
> 

Please drop the space between fixes tag and the signoff.

> Signed-off-by: Marcin Wojtas <mw@...ihalf.com>
> ---
>  drivers/clk/mvebu/cp110-system-controller.c | 29 ++++++++++++++++++++---------
>  1 file changed, 20 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/clk/mvebu/cp110-system-controller.c b/drivers/clk/mvebu/cp110-system-controller.c
> index 0835e1d..2bd87d2 100644
> --- a/drivers/clk/mvebu/cp110-system-controller.c
> +++ b/drivers/clk/mvebu/cp110-system-controller.c
> @@ -81,13 +81,6 @@ enum {
>  #define CP110_GATE_EIP150		25
>  #define CP110_GATE_EIP197		26
>  
> -static struct clk *cp110_clks[CP110_CLK_NUM];
> -
> -static struct clk_onecell_data cp110_clk_data = {
> -	.clks = cp110_clks,
> -	.clk_num = CP110_CLK_NUM,
> -};
> -
>  struct cp110_gate_clk {
>  	struct clk_hw hw;
>  	struct regmap *regmap;
> @@ -195,7 +188,8 @@ static int cp110_syscon_clk_probe(struct platform_device *pdev)
>  	struct regmap *regmap;
>  	struct device_node *np = pdev->dev.of_node;
>  	const char *ppv2_name, *apll_name, *core_name, *eip_name, *nand_name;
> -	struct clk *clk;
> +	struct clk_onecell_data *cp110_clk_data;
> +	struct clk *clk, **cp110_clks;
>  	u32 nand_clk_ctrl;
>  	int i, ret;
>  
> @@ -208,6 +202,20 @@ static int cp110_syscon_clk_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> +	cp110_clks = devm_kcalloc(&pdev->dev, sizeof(struct clk *),
> +				  CP110_CLK_NUM, GFP_KERNEL);
> +	if (IS_ERR(cp110_clks))

Doesn't that return NULL on error?

> +		return PTR_ERR(cp110_clks);
> +
> +	cp110_clk_data = devm_kzalloc(&pdev->dev,
> +				      sizeof(struct clk_onecell_data),

sizeof(*cp110_clk_data) please

> +				      GFP_KERNEL);
> +	if (IS_ERR(cp110_clk_data))

Doesn't that return NULL on error?

> +		return PTR_ERR(cp110_clk_data);
> +
> +	cp110_clk_data->clks = cp110_clks;
> +	cp110_clk_data->clk_num = CP110_CLK_NUM;
> +
>  	/* Register the APLL which is the root of the clk tree */
>  	of_property_read_string_index(np, "core-clock-output-names",
>  				      CP110_CORE_APLL, &apll_name);
> @@ -335,10 +343,12 @@ static int cp110_syscon_clk_probe(struct platform_device *pdev)
>  		cp110_clks[CP110_MAX_CORE_CLOCKS + i] = clk;
>  	}
>  
> -	ret = of_clk_add_provider(np, cp110_of_clk_get, &cp110_clk_data);
> +	ret = of_clk_add_provider(np, cp110_of_clk_get, cp110_clk_data);

It would be nice if this could be converted to
of_clk_add_hw_provider().

>  	if (ret)
>  		goto fail_clk_add;
>  
> +	platform_set_drvdata(pdev, cp110_clks);
> +
>  	return 0;
>  
>  fail_clk_add:
> @@ -365,6 +375,7 @@ fail0:
>  
>  static int cp110_syscon_clk_remove(struct platform_device *pdev)
>  {
> +	struct clk **cp110_clks = platform_get_drvdata(pdev);

Is this variable unused now?

>  	int i;
>  
>  	of_clk_del_provider(pdev->dev.of_node);
> -- 
> 1.8.3.1
> 

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