Re: [RFC PATCH 2/2] rtc: s3c: Add s3c_rtc_{enable/disable}_clk in s3c_rtc_setfreq()

Path	csiph.com!news.mixmin.net!aioe.org!bofh.it!news.nic.it!robomod
From	Krzysztof Kozlowski <k.kozlowski@samsung.com>
Newsgroups	linux.kernel
Subject	Re: [RFC PATCH 2/2] rtc: s3c: Add s3c_rtc_{enable/disable}_clk in s3c_rtc_setfreq()
Date	Tue, 05 Jul 2016 11:00:02 +0200
Message-ID	<rRun0-ty-19@gated-at.bofh.it> (permalink)
References	<rR9Vg-4SC-11@gated-at.bofh.it> <rR9Vg-4SC-9@gated-at.bofh.it> <rRtKi-g7-29@gated-at.bofh.it> <rRun0-ty-21@gated-at.bofh.it>
X-Original-To	Alim Akhtar <alim.akhtar@samsung.com>, rtc-linux@googlegroups.com, linux-kernel@vger.kernel.org
X-Auditid	cbfec7f5-f792a6d000001302-dc-577b7630ff1b
User-Agent	Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.8.0
MIME-Version	1.0
Content-Type	text/plain; charset=windows-1252
Content-Transfer-Encoding	7bit
X-Brightmail-Tracker	H4sIAAAAAAAAA+NgFrrGLMWRmVeSWpSXmKPExsVy+t/xy7oGZdXhBs03bCw6ri1msngwbxub xZu3a5gsXr8wtLi8aw6bxaKtX9gt9nd2MDqwezzZdJHRY8/Ek2weW/rvsnv0bVnF6PF5k1wA axSXTUpqTmZZapG+XQJXxuJ/axgLVvNXLPpxir2BcT1PFyMnh4SAicSTl00sELaYxIV769m6 GLk4hASWMkqs+fqUGcJ5xiix/M4xJpAqYYE4ibXb3rN2MXJwiAikSrQ8NYSoOcgoMbvrKCNI DbNAjMSlDWuYQWw2AWOJzcuXsIHYvAJaEjcX3QOaw87BIqAqsRwsKioQITFr+w8miApBiR+T 77GATOcU0JZYOyccxGQW0JO4f1ELYra8xOY1b5knMArMQtIwC6FqFpKqBYzMqxhFU0uTC4qT 0nON9IoTc4tL89L1kvNzNzFCQvvrDsalx6wOMQpwMCrx8BbMrwoXYk0sK67MPcQowcGsJMJ7 uLg6XIg3JbGyKrUoP76oNCe1+BCjNAeLkjjvzF3vQ4QE0hNLUrNTUwtSi2CyTBycUg2MJ3jd +IN5zpv43LPSjTg5yekth8unR2flHJsrjCzq1e7OrJu3yyhgW179XlP2f9+V2spbe5UXzRT4 EjVv8qOpp3UfsbkHm/Ttyl+oUhx2hOHQl2PpR/mXuh2f3hbgHX5zh/Y/sZ0nFD5YiXZdO8Uo 0aI+7WWq9A6pA1Xys3vC263sheX3S1kosRRnJBpqMRcVJwIA4Ir4dWkCAAA=
Sender	robomod@news.nic.it
List-ID	<linux-kernel.vger.kernel.org>
X-Mailing-List	linux-kernel@vger.kernel.org
Approved	robomod@news.nic.it
Lines	52
Organization	linux.* mail to news gateway
X-Original-Cc	alexandre.belloni@free-electrons.com, javier@osg.samsung.com, pankaj.dubey@samsung.com
X-Original-Date	Tue, 05 Jul 2016 10:56:15 +0200
X-Original-Message-ID	<577B762F.6020609@samsung.com>
X-Original-References	<1467630195-6929-1-git-send-email-alim.akhtar@samsung.com> <1467630195-6929-2-git-send-email-alim.akhtar@samsung.com> <577B6CD9.80605@samsung.com> <577B7537.4080506@samsung.com>
X-Original-Sender	linux-kernel-owner@vger.kernel.org
Xref	csiph.com linux.kernel:1436849

Show key headers only | View raw

On 07/05/2016 10:52 AM, Alim Akhtar wrote:
> 
> 
> On 07/05/2016 01:46 PM, Krzysztof Kozlowski wrote:
>> On 07/04/2016 01:03 PM, Alim Akhtar wrote:
>>> As per code flow it is possible that s3c_rtc_setfreq() might get called
>>> with rtc clock disabled and in set_freq we perform h/w registers
>>> read/write,
>>> which might results in a kernel crash while probing rtc driver.
>>> Below is one such case:
>>> s3c_rtc_probe()
>>>      clk_prepare_enable(info->rtc_clk) // rtc clock enabled
>>>        s3c_rtc_gettime() // will enable clk if not done, and disable
>>> it upon exit
>>>          s3c_rtc_setfreq() //then this will be called with clk disabled
>>
>> The indentation suggests levels of calls (chain) not sequence. This
>> should be:
>> s3c_rtc_probe()
>>    clk_prepare_enable(info->rtc_clk) // rtc clock enabled
>>    s3c_rtc_gettime() // will enable clk if not done, and disable it
>> upon exit
>>    s3c_rtc_setfreq() //then this will be called with clk disabled
>>
>>>
>>> This patch take cares of such issue by adding
>>> s3c_rtc_{enable/disable}_clk in
>>> s3c_rtc_setfreq().
>>
>> What I don't get is that you wrote "it is *possible* that
>> s3c_rtc_setfreq() *might* get called". From my understanding this will
>> happen always because src_rtc_gettime() always disables the clocks.
>>
>> Why it does not happen always?
>>
> 
> Yes, you are right, it is always disabled when reaches s3c_rtc_setfreq().
> And I observed a kernel crash while testing on exynos7 platform in
> s3c_rtc_setfreq() because clock was always disabled at this point. And
> this patches fixes this issue.

Ok, thanks! Could you adjust the commit message mentioning that it
always causes crash on Exynos 7 platform and adding:

Fixes: 24e1455493da ("drivers/rtc/rtc-s3c.c: delete duplicate clock
control")
Cc: <stable@vger.kernel.org>

Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>

Best regards,
Krzysztof

Thread

[RFC PATCH 2/2] rtc: s3c: Add s3c_rtc_{enable/disable}_clk in  s3c_rtc_setfreq() Alim Akhtar <alim.akhtar@samsung.com> - 2016-07-04 13:10 +0200
  Re: [RFC PATCH 2/2] rtc: s3c: Add s3c_rtc_{enable/disable}_clk in  s3c_rtc_setfreq() Krzysztof Kozlowski <k.kozlowski@samsung.com> - 2016-07-05 10:20 +0200
    Re: [RFC PATCH 2/2] rtc: s3c: Add s3c_rtc_{enable/disable}_clk in  s3c_rtc_setfreq() Krzysztof Kozlowski <k.kozlowski@samsung.com> - 2016-07-05 11:00 +0200
      Re: [RFC PATCH 2/2] rtc: s3c: Add s3c_rtc_{enable/disable}_clk in  s3c_rtc_setfreq() "pankaj.dubey" <pankaj.dubey@samsung.com> - 2016-07-05 11:50 +0200
    Re: [RFC PATCH 2/2] rtc: s3c: Add s3c_rtc_{enable/disable}_clk in  s3c_rtc_setfreq() Alim Akhtar <alim.akhtar@samsung.com> - 2016-07-05 11:00 +0200
      Re: [RFC PATCH 2/2] rtc: s3c: Add s3c_rtc_{enable/disable}_clk in  s3c_rtc_setfreq() "pankaj.dubey" <pankaj.dubey@samsung.com> - 2016-07-05 11:50 +0200

csiph-web