linux-kernel - Re: [PATCH v2 1/2] i2c: designware: Fix clock issue when PM is disabled

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: <f659afe0-6506-470b-be7e-99bd5ab03299@socionext.com>
Date: Wed, 20 Aug 2025 19:07:44 +0900
From: Kunihiko Hayashi <hayashi.kunihiko@...ionext.com>
To: Andi Shyti <andi.shyti@...nel.org>
Cc: Jarkko Nikula <jarkko.nikula@...ux.intel.com>,
 Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
 Mika Westerberg <mika.westerberg@...ux.intel.com>,
 Jan Dabros <jsd@...ihalf.com>, ito.kohei@...ionext.com,
 linux-i2c@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/2] i2c: designware: Fix clock issue when PM is
 disabled

Hi Andi,

Thank you for your comment.

On 2025/08/20 7:06, Andi Shyti wrote:
> Hi Kunihiko,
> 
> On Thu, Jul 24, 2025 at 01:22:10PM +0900, Kunihiko Hayashi wrote:
>> When removing the driver, enable the clocks once by calling
>> pm_runtime_get_sync(), and call pm_runtime_put_sync() to disable
>> the clocks.
> 
> can we rephrase this to something like:
> 
>    When the driver is removed, the clocks are first enabled by
>    calling pm_runtime_get_sync(), and then disabled with
>    pm_runtime_put_sync().
> 
> Does it work?

Yes, your description is better.

> 
>> If CONFIG_PM=y, clocks for this controller are disabled when it's in
>> the idle state. So the clocks are properly disabled when the driver
>> exits.
>>
>> Othewise, the clocks are always enabled and the PM functions have
>> no effect. Therefore, the driver exits without disabling the clocks.
>>
>>      # cat /sys/kernel/debug/clk/clk-pclk/clk_enable_count
>>      18
>>      # echo 1214a000.i2c > /sys/bus/platform/drivers/i2c_designware/bind
>>      # cat /sys/kernel/debug/clk/clk-pclk/clk_enable_count
>>      20
>>      # echo 1214a000.i2c >
> /sys/bus/platform/drivers/i2c_designware/unbind
>>      # cat /sys/kernel/debug/clk/clk-pclk/clk_enable_count
>>      20
>>
>> To ensure that the clocks can be disabled correctly even without
>> CONFIG_PM=y, should add the following fixes:
>>
>> - Replace with pm_runtime_put_noidle(), which only decrements the
> runtime
>>    PM usage count.
>> - Call i2c_dw_prepare_clk(false) to explicitly disable the clocks.
>>
>> Fixes: 7272194ed391f ("i2c-designware: add minimal support for runtime
> PM")
> 
> This commit doesn't look quite right to me, although it's quite
> difficult to find the culprit, as the clk api's have changed a
> lot over time.
> 
> Do you think this is better:
> 
> Fixes: b33af11de236 ("i2c: designware: Do not require clock when SSCN and
> FFCN are provided")

Surely, the code that handles PM and clocks has changed some time,
so it's difficult to identify the commit with "Fixes" tag.
I think the first commit that called pm_runtime_put_sync() was the cause.

> It doesn't matter much as it won't apply in any of the two
> versions> 
> The Fixes tag should be added in both the patches, but there is
> no need to resend, I will apply them once we agree on the commit
> message.

Okay, I agree with you.

Thank you,

---
Best Regards
Kunihiko Hayashi