linux-kernel - Re: [PATCH] drivers: dma: qcom: bam_dma: Manage clocks when controlled

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] [day] [month] [year] [list]

Message-ID: <16da38a6-36a2-2126-c8d5-6618fd9e4814@linaro.org>
Date:   Mon, 25 Jan 2021 08:33:15 -0500
From:   Thara Gopinath <thara.gopinath@...aro.org>
To:     Shawn Guo <shawn.guo@...aro.org>
Cc:     agross@...nel.org, bjorn.andersson@...aro.org,
        dan.j.williams@...el.com, vkoul@...nel.org,
        srinivas.kandagatla@...aro.org, linux-arm-msm@...r.kernel.org,
        dmaengine@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] drivers: dma: qcom: bam_dma: Manage clocks when
 controlled_remotely is set

Hi Shawn,

On 1/23/21 2:19 AM, Shawn Guo wrote:
> On Fri, Jan 22, 2021 at 10:44:09AM -0500, Thara Gopinath wrote:
>> Hi Shawn,
>>
>> Thanks for the review
>>
>> On 1/22/21 12:10 AM, Shawn Guo wrote:
>>> On Thu, Jan 21, 2021 at 09:52:51PM -0500, Thara Gopinath wrote:
>>>> When bam dma is "controlled remotely", thus far clocks were not controlled
>>>> from the Linux. In this scenario, Linux was disabling runtime pm in bam dma
>>>> driver and not doing any clock management in suspend/resume hooks.
>>>>
>>>> With introduction of crypto engine bam dma, the clock is a rpmh resource
>>>> that can be controlled from both Linux and TZ/remote side.  Now bam dma
>>>> clock is getting enabled during probe even though the bam dma can be
>>>> "controlled remotely". But due to clocks not being handled properly,
>>>> bam_suspend generates a unbalanced clk_unprepare warning during system
>>>> suspend.
>>>>
>>>> To fix the above issue and to enable proper clock-management, this patch
>>>> enables runtim-pm and handles bam dma clocks in suspend/resume hooks if
>>>> the clock node is present irrespective of controlled_remotely property.
>>>
>>> Shouldn't the following probe code need some update?  Now we have both
>>> controlled_remotely and clocks handle for cryptobam node.  For example,
>>> if devm_clk_get() returns -EPROBE_DEFER, we do not want to continue with
>>> bamclk forcing to be NULL, right?
>>
>> We still will have to set bdev->bamclk to NULL in certain scenarios. For eg
>> slimbus bam dma is controlled-remotely and the clocks are handled by the
>> remote s/w. Linux does not handle the clocks at all and  there is no clock
>> specified in the dt node.This is the norm for the devices that are also
>> controlled by remote s/w. Crypto bam dma is a special case where the clock
>> is actually a rpmh resource and hence can be independently handled from both
>> remote side and Linux by voting. In this case, the dma is controlled
>> remotely but clock can be turned off and on in Linux. Hence the need for
>> this patch.
> 
> So is it correct to say that clock is mandatory for !controlled-remotely
> BAM, while it's optional for controlled-remotely one.  If yes, maybe we
> can do something like below to make the code a bit easier to read?

Yes. Sure. I will change it to below.

> 
> 	if (controlled-remotely)
> 		bdev->bamclk = devm_clk_get_optional();
> 	else
> 		bdev->bamclk = devm_clk_get();
> 		
>> Yes, the probe code needs updating to handle -EPROBE_DEFER (esp if the clock
>> driver is built in as a module) I am not sure if the clock framework handles
>> -EPROBE_DEFER properly either. So that
>> might need updating too. This is a separate activity and not part of this
>> patch >
> 
> As the patch breaks the assumption that for controlled-remotely BAM
> there is no clock to be managed, the probe code becomes buggy right
> away.

mmm... not really. Either ways we don't handle -EPROBE_DEFER from clock 
code. That behavior is not worse because of this patch. I can send a 
separate patch to fix the -EPROBE_DEFER issue.

> 
> Shawn
> 

-- 
Warm Regards
Thara