linux-kernel - Re: [PATCH] spi: rockchip: Avoid redundant clock disable in pm operation

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: <c5726963-69a6-47d8-a576-86cebd481ef2@rock-chips.com>
Date: Wed, 28 Aug 2024 15:09:08 +0800
From: Jon Lin <jon.lin@...k-chips.com>
To: Brian Norris <briannorris@...omium.org>
Cc: broonie@...nel.org, linux-rockchip@...ts.infradead.org,
 linux-kernel@...r.kernel.org, heiko@...ech.de,
 linux-arm-kernel@...ts.infradead.org, linux-spi@...r.kernel.org
Subject: Re: [PATCH] spi: rockchip: Avoid redundant clock disable in pm
 operation

On 2024/8/27 10:59, Brian Norris wrote:
> On Mon, Aug 26, 2024 at 6:33 PM Jon Lin <jon.lin@...k-chips.com> wrote:
>> On 2024/8/27 6:27, Brian Norris wrote:
>>> It seems like you'd really be served well by
>>> pm_runtime_force_{suspend,resume}() here, and in fact, that's what this
>>> driver used to use before the breaking change (commit
>>> e882575efc77). Why aren't you just going back to using it? (This is the
>>> kind of thing I might expect in your commit message -- reasoning as to
>>> why you're doing what you're doing.)
>>>
>>> And in fact, I already submitted a patch that resolves the above problem
>>> and does exactly that:
>>>
>>> https://lore.kernel.org/all/20240823214235.1718769-1-briannorris@chromium.org/
>>> [PATCH] spi: rockchip: Resolve unbalanced runtime PM / system PM handling
>>>
>>> Do you see any problem with it?
>>>
>>
>> I have reviewed your submission and although the code has been
>> simplified, the execution efficiency has decreased. So although it is a
>> commonly used processing solution for SPI Upstream, I still hope to
>> retain a more efficiency approach as I submitted.
> 
> What do you mean by "efficiency"? You mean because there's
> indirection, via the PM runtime framework? If so, I doubt that's a
> priority for this piece of functionality -- simplicity is more
> important than a function call or two when talking about system
> suspend.
> 

Your code is fine, and I have tested it locally. The interface 
simplification is indeed in line with the direction of community 
development, and your solution can be used as a solution.

> Additionally, simplicity has additional benefits -- it heads off
> questions that your more complex code doesn't address. For example,
> are runtime PM and system PM mutually exclusive? Do we have to
> coordinate with any pending autosuspend? (Reading through
> https://docs.kernel.org/power/runtime_pm.html, I believe these are not
> actually concerns, but it's really not obvious and takes a bit of
> reading.) But your patch makes it more likely that runtime and system
> PM states get out of sync.
> 
> Anyway, if the patches really are equivalent, I suppose it can be the
> maintainer's choice as to which is preferable.
> 
> Brian
>