From: "pankaj.dubey" <pankaj.dubey@samsung.com>
To: Alim Akhtar <alim.akhtar@samsung.com>,
Krzysztof Kozlowski <k.kozlowski@samsung.com>,
rtc-linux@googlegroups.com, linux-kernel@vger.kernel.org
Cc: alexandre.belloni@free-electrons.com, javier@osg.samsung.com
Subject: [rtc-linux] Re: [RFC PATCH 2/2] rtc: s3c: Add s3c_rtc_{enable/disable}_clk in s3c_rtc_setfreq()
Date: Tue, 05 Jul 2016 15:14:06 +0530 [thread overview]
Message-ID: <577B8166.8060208@samsung.com> (raw)
In-Reply-To: <577B7537.4080506@samsung.com>
Hi Alim,
On Tuesday 05 July 2016 02:22 PM, 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.
>
After addressing Krzysztof's review comments, Please feel free to add
Reviewed-by: Pankaj Dubey <pankaj.dubey@samsung.com>
For testing on Exynos7420
Tested-by: Pankaj Dubey <pankaj.dubey@samsung.com>
Thanks,
Pankaj Dubey
>
>> Best regards,
>> Krzysztof
>>
>
--
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
---
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
WARNING: multiple messages have this Message-ID (diff)
From: "pankaj.dubey" <pankaj.dubey@samsung.com>
To: Alim Akhtar <alim.akhtar@samsung.com>,
Krzysztof Kozlowski <k.kozlowski@samsung.com>,
rtc-linux@googlegroups.com, linux-kernel@vger.kernel.org
Cc: alexandre.belloni@free-electrons.com, javier@osg.samsung.com
Subject: Re: [RFC PATCH 2/2] rtc: s3c: Add s3c_rtc_{enable/disable}_clk in s3c_rtc_setfreq()
Date: Tue, 05 Jul 2016 15:14:06 +0530 [thread overview]
Message-ID: <577B8166.8060208@samsung.com> (raw)
In-Reply-To: <577B7537.4080506@samsung.com>
Hi Alim,
On Tuesday 05 July 2016 02:22 PM, 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.
>
After addressing Krzysztof's review comments, Please feel free to add
Reviewed-by: Pankaj Dubey <pankaj.dubey@samsung.com>
For testing on Exynos7420
Tested-by: Pankaj Dubey <pankaj.dubey@samsung.com>
Thanks,
Pankaj Dubey
>
>> Best regards,
>> Krzysztof
>>
>
next prev parent reply other threads:[~2016-07-05 9:41 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-07-04 11:03 [rtc-linux] [RFC PATCH 1/2] rtc: s3c: Remove unnecessary call to disable already disabled clock Alim Akhtar
2016-07-04 11:03 ` Alim Akhtar
2016-07-04 11:03 ` [rtc-linux] [RFC PATCH 2/2] rtc: s3c: Add s3c_rtc_{enable/disable}_clk in s3c_rtc_setfreq() Alim Akhtar
2016-07-04 11:03 ` Alim Akhtar
2016-07-05 8:16 ` [rtc-linux] " Krzysztof Kozlowski
2016-07-05 8:16 ` Krzysztof Kozlowski
2016-07-05 8:52 ` [rtc-linux] " Alim Akhtar
2016-07-05 8:52 ` Alim Akhtar
2016-07-05 8:56 ` [rtc-linux] " Krzysztof Kozlowski
2016-07-05 8:56 ` Krzysztof Kozlowski
2016-07-05 9:43 ` [rtc-linux] " pankaj.dubey
2016-07-05 9:43 ` pankaj.dubey
2016-07-05 9:44 ` pankaj.dubey [this message]
2016-07-05 9:44 ` pankaj.dubey
2016-07-05 7:18 ` [rtc-linux] Re: [RFC PATCH 1/2] rtc: s3c: Remove unnecessary call to disable already disabled clock Krzysztof Kozlowski
2016-07-05 7:18 ` Krzysztof Kozlowski
2016-07-05 8:46 ` [rtc-linux] " Alim Akhtar
2016-07-05 8:46 ` Alim Akhtar
2016-07-05 8:50 ` [rtc-linux] " Krzysztof Kozlowski
2016-07-05 8:50 ` Krzysztof Kozlowski
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=577B8166.8060208@samsung.com \
--to=pankaj.dubey@samsung.com \
--cc=alexandre.belloni@free-electrons.com \
--cc=alim.akhtar@samsung.com \
--cc=javier@osg.samsung.com \
--cc=k.kozlowski@samsung.com \
--cc=linux-kernel@vger.kernel.org \
--cc=rtc-linux@googlegroups.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.