All of lore.kernel.org
 help / color / mirror / Atom feed
From: Daniel Lezcano <daniel.lezcano@linaro.org>
To: ning.n.jiang@gmail.com
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
Subject: Re: [PATCH] ARM: timer: Shutdown clock event device when stopping local timer
Date: Mon, 01 Apr 2013 02:06:17 +0200	[thread overview]
Message-ID: <5158CF79.2060702@linaro.org> (raw)
In-Reply-To: <1364549049-29278-1-git-send-email-ning.n.jiang@gmail.com>

On 03/29/2013 10:24 AM, ning.n.jiang@gmail.com wrote:
> From: Ning Jiang <ning.n.jiang@gmail.com>
> 
> 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.
> 
> 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.
> 
> Signed-off-by: Ning Jiang <ning.n.jiang@gmail.com>
> ---


Don't you see a kernel BUG with this patch ?

In the clockevents code, there is:

/**
 * clockevents_notify - notification about relevant events
 */
void clockevents_notify(unsigned long reason, void *arg)
{
	...
	case CLOCK_EVT_NOTIFY_CPU_DEAD:
	...
                cpu = *((int *)arg);
                list_for_each_entry_safe(dev, tmp, &clockevent_devices,
					list) {
                       if (cpumask_test_cpu(cpu, dev->cpumask) &&
                            cpumask_weight(dev->cpumask) == 1 &&
                            !tick_is_broadcast_device(dev)) {
                                BUG_ON(dev->mode !=
					CLOCK_EVT_MODE_UNUSED);

				^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

                                list_del(&dev->list);
                        }
                }
                break;
	...
}

This is called triggered from hrtimer_cpu_notify with the CPU_DEAD event.


-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

WARNING: multiple messages have this Message-ID (diff)
From: daniel.lezcano@linaro.org (Daniel Lezcano)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH] ARM: timer: Shutdown clock event device when stopping local timer
Date: Mon, 01 Apr 2013 02:06:17 +0200	[thread overview]
Message-ID: <5158CF79.2060702@linaro.org> (raw)
In-Reply-To: <1364549049-29278-1-git-send-email-ning.n.jiang@gmail.com>

On 03/29/2013 10:24 AM, ning.n.jiang at gmail.com wrote:
> From: Ning Jiang <ning.n.jiang@gmail.com>
> 
> 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.
> 
> 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.
> 
> Signed-off-by: Ning Jiang <ning.n.jiang@gmail.com>
> ---


Don't you see a kernel BUG with this patch ?

In the clockevents code, there is:

/**
 * clockevents_notify - notification about relevant events
 */
void clockevents_notify(unsigned long reason, void *arg)
{
	...
	case CLOCK_EVT_NOTIFY_CPU_DEAD:
	...
                cpu = *((int *)arg);
                list_for_each_entry_safe(dev, tmp, &clockevent_devices,
					list) {
                       if (cpumask_test_cpu(cpu, dev->cpumask) &&
                            cpumask_weight(dev->cpumask) == 1 &&
                            !tick_is_broadcast_device(dev)) {
                                BUG_ON(dev->mode !=
					CLOCK_EVT_MODE_UNUSED);

				^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

                                list_del(&dev->list);
                        }
                }
                break;
	...
}

This is called triggered from hrtimer_cpu_notify with the CPU_DEAD event.


-- 
 <http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

  parent reply	other threads:[~2013-04-01  0:06 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-03-29  9:24 [PATCH] ARM: timer: Shutdown clock event device when stopping local timer ning.n.jiang
2013-03-29  9:24 ` ning.n.jiang
2013-03-29  9:24 ` ning.n.jiang at gmail.com
2013-03-29 18:32 ` Stephen Boyd
2013-03-29 18:32   ` Stephen Boyd
2013-03-30  9:57   ` Ning Jiang
2013-03-30  9:57     ` Ning Jiang
2013-03-30 10:04     ` Russell King - ARM Linux
2013-03-30 10:04       ` Russell King - ARM Linux
2013-03-30 11:56       ` Ning Jiang
2013-03-30 11:56         ` Ning Jiang
2013-03-31 13:11         ` Ning Jiang
2013-03-31 13:11           ` Ning Jiang
2013-03-31 23:41 ` Daniel Lezcano
2013-03-31 23:41   ` Daniel Lezcano
2013-04-01  1:56   ` Ning Jiang
2013-04-01  1:56     ` Ning Jiang
2013-04-02 12:01     ` Ning Jiang
2013-04-02 12:01       ` Ning Jiang
2013-04-01  0:06 ` Daniel Lezcano [this message]
2013-04-01  0:06   ` Daniel Lezcano
2013-04-01  2:05   ` Ning Jiang
2013-04-01  2:05     ` Ning Jiang

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=5158CF79.2060702@linaro.org \
    --to=daniel.lezcano@linaro.org \
    --cc=arnd@arndb.de \
    --cc=bryanh@codeaurora.org \
    --cc=davidb@codeaurora.org \
    --cc=dwalker@fifo99.com \
    --cc=john.stultz@linaro.org \
    --cc=kgene.kim@samsung.com \
    --cc=linus.walleij@linaro.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-samsung-soc@vger.kernel.org \
    --cc=linux@arm.linux.org.uk \
    --cc=ning.n.jiang@gmail.com \
    --cc=rob.herring@calxeda.com \
    --cc=shawn.guo@linaro.org \
    --cc=tglx@linutronix.de \
    /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.