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]

Message-ID: <CAPv3WKdVX1p3qmUufXE_cd=A0fuS8Eg9ELT0AYnXV6Z=cgJg-w@mail.gmail.com>
Date:   Tue, 30 Aug 2016 17:31:21 +0200
From:   Marcin Wojtas <mw@...ihalf.com>
To:     Stephen Boyd <sboyd@...eaurora.org>
Cc:     linux-kernel@...r.kernel.org,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>, linux-clk@...r.kernel.org,
        Michael Turquette <mturquette@...libre.com>,
        Sebastian Hesselbarth <sebastian.hesselbarth@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        Jason Cooper <jason@...edaemon.net>,
        Thomas Petazzoni <thomas.petazzoni@...e-electrons.com>,
        Gregory Clément 
        <gregory.clement@...e-electrons.com>, nadavh@...vell.com,
        Lior Amsalem <alior@...vell.com>,
        Tomasz Nowicki <tn@...ihalf.com>,
        Grzegorz Jaszczyk <jaz@...ihalf.com>
Subject: Re: [PATCH 2/2] clk: mvebu: dynamically allocate resources in Armada
 CP110 system controller

Hi Stephen,

2016-08-25 2:16 GMT+02:00 Stephen Boyd <sboyd@...eaurora.org>:
> 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.

Ok.

>
>> 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?

Will change to 'if (!cp110clks)'

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

Ok.

>
>> +                                   GFP_KERNEL);
>> +     if (IS_ERR(cp110_clk_data))
>
> Doesn't that return NULL on error?

Same as above, will change.

>
>> +             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().

Will try it. Shouldn't such change be placed in separate commit?

>
>>       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?

No, why? Just below there is a loop using it. Before it was taken from
global variable, which I got rid of.

Best regards,
Marcin