From patchwork Sat Mar 30 09:57:38 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: ning.n.jiang@gmail.com X-Patchwork-Id: 2367281 Return-Path: X-Original-To: patchwork-linux-samsung-soc@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork1.kernel.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by patchwork1.kernel.org (Postfix) with ESMTP id DF7C83FD40 for ; Sat, 30 Mar 2013 09:57:40 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756609Ab3C3J5k (ORCPT ); Sat, 30 Mar 2013 05:57:40 -0400 Received: from mail-ve0-f173.google.com ([209.85.128.173]:57555 "EHLO mail-ve0-f173.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756615Ab3C3J5j (ORCPT ); Sat, 30 Mar 2013 05:57:39 -0400 Received: by mail-ve0-f173.google.com with SMTP id cy12so1204781veb.32 for ; Sat, 30 Mar 2013 02:57:38 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:x-received:in-reply-to:references:date:message-id :subject:from:to:cc:content-type; bh=awoSt2XMHxl056Om7LEfe1wKL8SwupAnuCJ1a9bgxXw=; b=oBb6prNaAlytrWi+CwnoVu9c5+pygf2TatGHdtQWM6E0Tlg03W+Tv9W/8zGWsMtb5V FDE2zQmCV7GasRe6LeGJi1d073g1KwBP9fH7ycTHb/RKPRwj9U4M9v7SpUmHFLPQiF9/ WRq296BxjN42x8wpyU/xyZv6ML9MmwXyM9T28hp44ngWeBbOgVzyHxE4Cc6c9cE3SXE3 b3B62V55wZXWF2VVRT434MGnpmBJlRw6c2GHMzlyj6rHlrDxIhNgoVY5EJ6VWATsqKtt gKe8ymex1vjmRsfa2QDnoDgfCTuRhXnEWSazxlU4S6tb0R1bcbrfFsElXRRBE1FO8gXJ xdkA== MIME-Version: 1.0 X-Received: by 10.58.173.131 with SMTP id bk3mr3954793vec.48.1364637458139; Sat, 30 Mar 2013 02:57:38 -0700 (PDT) Received: by 10.58.32.103 with HTTP; Sat, 30 Mar 2013 02:57:38 -0700 (PDT) In-Reply-To: <5155DE43.4000601@codeaurora.org> References: <1364549049-29278-1-git-send-email-ning.n.jiang@gmail.com> <5155DE43.4000601@codeaurora.org> Date: Sat, 30 Mar 2013 17:57:38 +0800 Message-ID: Subject: Re: [PATCH] ARM: timer: Shutdown clock event device when stopping local timer From: Ning Jiang To: Stephen Boyd Cc: linux@arm.linux.org.uk, kgene.kim@samsung.com, davidb@codeaurora.org, dwalker@fifo99.com, bryanh@codeaurora.org, john.stultz@linaro.org, tglx@linutronix.de, linus.walleij@linaro.org, shawn.guo@linaro.org, rob.herring@calxeda.com, arnd@arndb.de, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-samsung-soc@vger.kernel.org, linux-arm-msm@vger.kernel.org Sender: linux-samsung-soc-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-samsung-soc@vger.kernel.org 2013/3/30 Stephen Boyd : > On 03/29/13 02:24, ning.n.jiang@gmail.com wrote: >> From: Ning Jiang >> >> Currently there are two problems when we try to stop local timer. >> First, it calls set_mode function directly so mode state is not >> updated for the clock event device. Second, it makes the device >> unused instead of shutdown. > > What device is this a problem on? I believe this only matters to drivers > which enable their timer in their set_next_event() callback? But even > then, does anything actually happen because the interrupt should have > been disabled in the local timer stop callback. > Right. Drivers which enable timer in set_next_event() will have this problem. It will not have functional problem in my case. But my device cannot enter low power mode with a pending interrupt even if it is disabled. >> >> A subtle error will happen because of it. When a cpu is plugged out >> it will stop the local timer. It will call tick_nohz_idle_enter() >> in idle thread afterwards. It will cancel the sched timer and try >> to reprogram the next event. This is wrong since the local timer >> is supposed to be stopped. >> >> The right way to stop the local timer is to shutdown it by calling >> clockevents_set_mode(). Thus when we try to reprogram the clock >> event device, it will return directly without doing anything since >> the clock mode is CLOCK_EVT_MODE_SHUTDOWN. > > While this prevents the set_next_event() callback from being called on a > dying CPU, wouldn't it be better to fix this problem in the core code > once instead of fixing it many times in each local timer driver? It > doesn't seem to make much sense to program an event on a CPU that is > about to die, so why do we do that? > Actually, I was trying to fix it in the core code like this, but I thought it is not that good and we still need to fix the local timer driver problem even with this fix. if (dev->mode == CLOCK_EVT_MODE_SHUTDOWN) > -- > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, > hosted by The Linux Foundation > --- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/kernel/time/clockevents.c b/kernel/time/clockevents.c index c6d6400..e22e268 100644 --- a/kernel/time/clockevents.c +++ b/kernel/time/clockevents.c @@ -210,6 +210,9 @@ int clockevents_program_event(struct clock_event_device *dev, ktime_t expires, return -ETIME; } + if (cpu_is_offline(smp_processor_id())) + return 0; + dev->next_event = expires;