| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <285738930.THBQOWvsah@vostro.rjw.lan>
Date: Thu, 09 Apr 2015 23:19:59 +0200
From: "Rafael J. Wysocki" <rjw@...ysocki.net>
To: Tomeu Vizoso <tomeu.vizoso@...labora.com>
Cc: Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
"linux-tegra@...r.kernel.org" <linux-tegra@...r.kernel.org>,
"linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
"Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
Russell King <linux@....linux.org.uk>,
Stephen Warren <swarren@...dotorg.org>,
Thierry Reding <thierry.reding@...il.com>,
Alexandre Courbot <gnurou@...il.com>,
Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>,
Kyungmin Park <kyungmin.park@...sung.com>,
Daniel Lezcano <daniel.lezcano@...aro.org>,
Kukjin Kim <kgene.kim@...sung.com>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] ARM: tegra: cpuidle: implement cpuidle_state.enter_freeze()
On Thursday, April 09, 2015 11:18:25 AM Tomeu Vizoso wrote:
> On 04/08/2015 01:55 PM, Lorenzo Pieralisi wrote:
> > On Wed, Apr 08, 2015 at 11:54:38AM +0100, Tomeu Vizoso wrote:
> >> This callback is expected to do the same as enter() only that all
> >> non-wakeup IRQs are expected to be disabled.
> >
> > This is not true or at least it is misworded. The enter_freeze() function
> > is expected to return from the state with IRQs disabled at CPU level, or
> > put it differently it must not re-enable IRQs while executing since the
> > tick is frozen.
>
> True, only that it mentions interrupts in general, not just IRQs (I
> don't know if the terminology used in the base code matches the one in
> ARM's documentation).
>
> /*
> * CPUs execute ->enter_freeze with the local tick or entire timekeeping
> * suspended, so it must not re-enable interrupts at any point (even
> * temporarily) or attempt to change states of clock event devices.
> */
This means interrupts on the local CPU (ie. the thing done by local_irq_disable()).
> >> It will be called when the system goes to suspend-to-idle and will
> >> reduce power usage because CPUs won't be awaken for unnecessary IRQs.
> >>
> >> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@...labora.com>
> >> Cc: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> >> ---
> >> arch/arm/mach-tegra/cpuidle-tegra114.c | 31 ++++++++++++++++++++++++-------
> >> 1 file changed, 24 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/arch/arm/mach-tegra/cpuidle-tegra114.c b/arch/arm/mach-tegra/cpuidle-tegra114.c
> >> index f2b586d..ef06001 100644
> >> --- a/arch/arm/mach-tegra/cpuidle-tegra114.c
> >> +++ b/arch/arm/mach-tegra/cpuidle-tegra114.c
> >> @@ -39,28 +39,44 @@ static int tegra114_idle_power_down(struct cpuidle_device *dev,
> >> struct cpuidle_driver *drv,
> >> int index)
> >> {
> >> - local_fiq_disable();
> >> -
> >> tegra_set_cpu_in_lp2();
> >> cpu_pm_enter();
> >>
> >> - clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &dev->cpu);
> >> -
> >> call_firmware_op(prepare_idle);
> >>
> >> /* Do suspend by ourselves if the firmware does not implement it */
> >> if (call_firmware_op(do_idle, 0) == -ENOSYS)
> >> cpu_suspend(0, tegra30_sleep_cpu_secondary_finish);
> >>
> >> - clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &dev->cpu);
> >> -
> >> cpu_pm_exit();
> >> tegra_clear_cpu_in_lp2();
> >>
> >> + return index;
> >> +}
> >> +
> >> +static int tegra114_idle_enter(struct cpuidle_device *dev,
> >> + struct cpuidle_driver *drv,
> >> + int index)
> >> +{
> >> + local_fiq_disable();
> >> +
> >> + clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &dev->cpu);
> >> +
> >> + index = tegra114_idle_power_down(dev, drv, index);
> >> +
> >> + clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &dev->cpu);
> >> +
> >> local_fiq_enable();
> >>
> >> return index;
> >> }
> >> +
> >> +static void tegra114_idle_enter_freeze(struct cpuidle_device *dev,
> >> + struct cpuidle_driver *drv,
> >> + int index)
> >> +{
> >> + tegra114_idle_power_down(dev, drv, index);
> >
> > Cool. So if the problem is FIQs, you don't disabled them on entry
> > which means you enter the "frozen" state with FIQs enabled and tick frozen,
> > unless I am missing something.
>
> I have gone a bit deeper in the code and that's correct, AFAICS.
>
> > The question here is: are we allowed to enable FIQs before returning
> > from an enter_freeze() call (and to enter it with FIQs enabled) ?
> >
> > If we are not your code here certainly does not solve the issue, since
> > it does _not_ disable FIQs upon enter_freeze call anyway.
>
> I think doing that would go against the wording of the comment I quoted
> above, so I see two ways of fixing this:
>
> * Change the wording to refer to normal IRQs and leave the task of
> enabling and disabling FIQs to the enter_freeze implementation (ugly and
> I don't see any good reason for this)
>
> * Have FIQs already disabled when enter_freeze gets called, probably by
> having arch_cpu_idle_enter do it on ARM (and the inverse in
> arch_cpu_idle_exit)?
>
> Rafael, what's your opinion on this?
I don't know what FIQs are. :-)
->enter_freeze is entered with interrupts disabled on the local CPU. It is
not supposed to re-enable them. That is, while in the ->enter_freeze callback
routine, the CPU must not be interrupted aby anything other than NMI.
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists