From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754494AbcGEJla (ORCPT ); Tue, 5 Jul 2016 05:41:30 -0400 Received: from mailout4.samsung.com ([203.254.224.34]:45262 "EHLO mailout4.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751598AbcGEJlD (ORCPT ); Tue, 5 Jul 2016 05:41:03 -0400 X-AuditID: cbfee68d-f79876d000001436-b1-577b80ac31fc Subject: Re: [RFC PATCH 2/2] rtc: s3c: Add s3c_rtc_{enable/disable}_clk in s3c_rtc_setfreq() To: Krzysztof Kozlowski , Alim Akhtar , rtc-linux@googlegroups.com, linux-kernel@vger.kernel.org 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> <577B762F.6020609@samsung.com> Cc: alexandre.belloni@free-electrons.com, javier@osg.samsung.com From: "pankaj.dubey" Message-id: <577B8131.2020108@samsung.com> Date: Tue, 05 Jul 2016 15:13:13 +0530 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.7.2 MIME-version: 1.0 In-reply-to: <577B762F.6020609@samsung.com> Content-type: text/plain; charset=windows-1252 Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFprHIsWRmVeSWpSXmKPExsWyRsSkTXdNQ3W4wZpmCYuOa4uZLB7M28Zm 8ebtGiaL1y8MLS7vmsNmsb+zg9GBzePJpouMHnsmnmTz2NJ/l92jb8sqRo/Pm+QCWKO4bFJS czLLUov07RK4Mua93MJcsEyo4tOeg0wNjI/5uhg5OSQETCRef/vADmGLSVy4t56ti5GLQ0hg BaPEtu2bmGGKTj5bzwKRmMUoseXPFWYI5zujxI2un2BVwgJxEmu3vWcFSYgITGOU6N/fxARR 9YhRYs2Uv2BLmAUcJFbPmwbWwSagLzH98TY2EJtXQEti8cWzjCA2i4CqxNRJX8FsUYEIidXr rjFD1AhK/Jh8jwXE5hTQlpi5+wlQLwfQTD2J+xe1IMbLS2xe8xbsOgmBc+wS07b+Y4aYKSDx bfIhFpB6CQFZiU0HoF6TlDi44gbLBEaxWUg2zEKYOgvJ1AWMzKsYRVMLkguKk9KLDPWKE3OL S/PS9ZLzczcxAqPt9L9nvTsYbx+wPsQowMGoxMN7YmFVuBBrYllxZe4hRlOgIyYyS4km5wNj Oq8k3tDYzMjC1MTU2Mjc0kxJnFdR6mewkEB6YklqdmpqQWpRfFFpTmrxIUYmDk6pBsYrTn+f ZHDfSFopt/3vHbNZjm+M028xyCzaX3iFW617pXdEtdKeB12erzz/+qaGbFti7bb/aVP7v7RL JibaArYK4ZFSlUrNV6PU6qOyIu3sL1e0Z7rOrspX87+7q5+BJZ7/i3M8z5X3PP+3bGHr4YlR Wy0R90/7f/2Drl82+zT/3r/D/dBdSYmlOCPRUIu5qDgRAKEpMEKxAgAA X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFnrNIsWRmVeSWpSXmKPExsVy+t9jQd01DdXhBn/2Clt0XFvMZPFg3jY2 izdv1zBZvH5haHF51xw2i/2dHYwObB5PNl1k9Ngz8SSbx5b+u+wefVtWMXp83iQXwBrVwGiT kZqYklqkkJqXnJ+SmZduq+QdHO8cb2pmYKhraGlhrqSQl5ibaqvk4hOg65aZA3SBkkJZYk4p UCggsbhYSd8O04TQEDddC5jGCF3fkCC4HiMDNJCwjjFj3sstzAXLhCo+7TnI1MD4mK+LkZND QsBE4uSz9SwQtpjEhXvr2boYuTiEBGYxSmz5c4UZwvnOKHGj6yczSJWwQJzE2m3vWUESIgLT GCX69zcxQVQ9YpRYM+UvO0gVs4CDxOp508A62AT0JaY/3sYGYvMKaEksvniWEcRmEVCVmDrp K5gtKhAhsXrdNWaIGkGJH5Pvgd3EKaAtMXP3E6BeDqCZehL3L2pBjJeX2LzmLfMERqA7ETpm IVTNQlK1gJF5FaNEakFyQXFSeq5hXmq5XnFibnFpXrpecn7uJkZwRD+T2sF4cJf7IUYBDkYl Ht6C+VXhQqyJZcWVuYcYJTiYlUR49Wqrw4V4UxIrq1KL8uOLSnNSiw8xmgK9MZFZSjQ5H5hs 8kriDY1NzIwsjcwsjEzMzZXEeR//XxcmJJCeWJKanZpakFoE08fEwSnVwLjwnda+SNdLHByd dhV6e+59PHY27FPjnEDnsmNLBH2nK008IJ3c8uTRBvU9DJMt+nLcFW5vmGG9a8ZM1rvcS8vN jFds3q//OujKQv5zltKO3I0ROy/FCx7Z++hXtNrdWbaF5SV+G7Inztr6rVU3nefIt8uWQjeX uywvze0+xrbzxTHG4q9J1yuUWIozEg21mIuKEwEw/ERa/gIAAA== DLP-Filter: Pass X-MTR: 20000000000000000@CPGS X-CFilter-Loop: Reflected Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Alim, On Tuesday 05 July 2016 02:26 PM, Krzysztof Kozlowski wrote: > 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: > > Reviewed-by: Krzysztof Kozlowski > After addressing Krzysztof's review comments, Please feel free to add Reviewed-by: Pankaj Dubey For testing on Exynos7420 Tested-by: Pankaj Dubey Thanks, Pankaj Dubey > Best regards, > Krzysztof > >