linux-kernel - Re: [PATCH v2 08/12] soc: mediatek: pm-domains: Add subsystem clocks

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: <e9809057-20b6-b72e-0320-1131fc37fae8@collabora.com>
Date:   Mon, 26 Oct 2020 16:17:43 +0100
From:   Enric Balletbo i Serra <enric.balletbo@...labora.com>
To:     Matthias Brugger <matthias.bgg@...il.com>,
        linux-kernel@...r.kernel.org
Cc:     Collabora Kernel ML <kernel@...labora.com>, fparent@...libre.com,
        drinkcat@...omium.org, hsinyi@...omium.org, weiyi.lu@...iatek.com,
        Matthias Brugger <mbrugger@...e.com>,
        linux-arm-kernel@...ts.infradead.org,
        linux-mediatek@...ts.infradead.org
Subject: Re: [PATCH v2 08/12] soc: mediatek: pm-domains: Add subsystem clocks

Hi Matthias,

Thank you for your review.

On 2/10/20 11:04, Matthias Brugger wrote:
> 
> 
> On 01/10/2020 18:01, Enric Balletbo i Serra wrote:
>> From: Matthias Brugger <mbrugger@...e.com>
>>
>> For the bus protection operations, some subsystem clocks need to be enabled
>> before releasing the protection. This patch identifies the subsystem clocks
>> by it's name.
>>
>> Suggested-by: Weiyi Lu <weiyi.lu@...iatek.com>
>> [Adapted the patch to the mtk-pm-domains driver]
>> Signed-off-by: Matthias Brugger <mbrugger@...e.com>
>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@...labora.com>
>> ---
>>
>> Changes in v2:
>> - Use dev_err_probe if getting clocks fails, so an error is not printed
>>    if is deferred.
>>
>>   drivers/soc/mediatek/mtk-pm-domains.c | 89 ++++++++++++++++++++++-----
>>   1 file changed, 75 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/soc/mediatek/mtk-pm-domains.c
>> b/drivers/soc/mediatek/mtk-pm-domains.c
>> index e0a52d489fea..2075072f16da 100644
>> --- a/drivers/soc/mediatek/mtk-pm-domains.c
>> +++ b/drivers/soc/mediatek/mtk-pm-domains.c
> [...]
>>   +    for (i = 0; i < pd->num_subsys_clks; i++) {
>> +        struct clk *clk = of_clk_get(node, i + clk_ind);
>> +
>> +        if (IS_ERR(clk)) {
>> +            ret = PTR_ERR(clk);
>> +            dev_err_probe(scpsys->dev, PTR_ERR(clk),
>> +                      "%pOFn: failed to get clk at index %d: %d\n",
>> +                      node, i + clk_ind, ret);
>> +            goto err_put_subsys_clocks;
>> +        }
>> +
>> +        pd->subsys_clks[i].clk = clk;
>> +    }
>> +
>> +    ret = clk_bulk_prepare(pd->num_subsys_clks, pd->subsys_clks);
>> +    if (ret)
>> +        goto err_put_subsys_clocks;
>> +
> 
> Not sure if it really matters, but logically we should prepare the basic clocks
> before we prepare the subsystem clocks. And fix the error handling accordingly.
> 

Changed in next version.

> Regards,
> Matthias
> 
>>       ret = clk_bulk_prepare(pd->num_clks, pd->clks);
>>       if (ret)
>> -        goto err_put_clocks;
>> +        goto err_unprepare_subsys_clocks;
>>         /*
>>        * Initially turn on all domains to make the domains usable
>> @@ -462,6 +513,12 @@ static int scpsys_add_one_domain(struct scpsys *scpsys,
>> struct device_node *node
>>     err_unprepare_clocks:
>>       clk_bulk_unprepare(pd->num_clks, pd->clks);
>> +err_unprepare_subsys_clocks:
>> +    clk_bulk_unprepare(pd->num_subsys_clks, pd->subsys_clks);
>> +err_put_subsys_clocks:
>> +    clk_bulk_put(pd->num_subsys_clks, pd->subsys_clks);
>> +    devm_kfree(scpsys->dev, pd->subsys_clks);
>> +    pd->num_subsys_clks = 0;
>>   err_put_clocks:
>>       clk_bulk_put(pd->num_clks, pd->clks);
>>       devm_kfree(scpsys->dev, pd->clks);
>> @@ -541,6 +598,10 @@ static void scpsys_remove_one_domain(struct scpsys_domain
>> *pd)
>>       clk_bulk_unprepare(pd->num_clks, pd->clks);
>>       clk_bulk_put(pd->num_clks, pd->clks);
>>       pd->num_clks = 0;
>> +
>> +    clk_bulk_unprepare(pd->num_subsys_clks, pd->subsys_clks);
>> +    clk_bulk_put(pd->num_subsys_clks, pd->subsys_clks);
>> +    pd->num_subsys_clks = 0;
>>   }
>>     static void scpsys_domain_cleanup(struct scpsys *scpsys)
>>