Re: [PATCH v2 7/7] ARM: smp: Add runtime PM support for CPU hotplug

[Russell King - ARM Linux <linux@arm.linux.org.uk>]

On Fri, Sep 04, 2015 at 09:12:02AM -0600, Lina Iyer wrote:
> On Fri, Sep 04 2015 at 03:27 -0600, Russell King - ARM Linux wrote:
> >On Fri, Sep 04, 2015 at 10:17:35AM +0100, Russell King - ARM Linux wrote:
> >>On Thu, Sep 03, 2015 at 01:58:34PM -0600, Lina Iyer wrote:
> >>> @@ -401,6 +412,11 @@ asmlinkage void secondary_start_kernel(void)
> >>>  	local_irq_enable();
> >>>  	local_fiq_enable();
> >>>
> >>> +	/* We are running, enable runtime PM for the CPU. */
> >>> +	cpu_dev = get_cpu_device(cpu);
> >>> +	if (cpu_dev)
> >>> +		pm_runtime_get_sync(cpu_dev);
> >>> +
> >>
> >>Please no.  This is fragile.
> >>
> >>pm_runtime_get_sync() can sleep, and this path is used when the system
> >>is fully up and running.  Locks may be held, which cause this to sleep.
> >>However, we're calling it from a context where we can't sleep - which
> >>is the general rule for any part of system initialisation where we
> >>have not entered the idle thread yet (the idle thread is what we run
> >>when sleeping.)
> >>
> More explanation below.
> 
> Another patch (3/7) in this series defines CPU devices as IRQ safe and
> therefore the dev->power.lock would be spinlock'd before calling the
> rest of the PM runtime code and the domain. The CPU PM domain would also
> be an IRQ safe domain (patch 2/7 makes the genpd framework usable in IRQ
> safe context).
> 
> >>NAK on this.
> >
> >There's another issue here.  This is obviously wrong.  If the CPU is
> >inside the PM domain, then don't you need the PM domain powered up for
> >the CPU to start executing instructions?
> >
> This would not be the case for CPU PM domains. The CPU PM domains AFAIK,
> are triggered by the same wake up interrupt that brings the CPU out of
> its power downs state. The domain hardware has to be awake before the
> CPU executes its first instruction. Thefore the hw power up is a logical
> OR of the wakeup signals to any of the cores.

You're taking the behaviour of the hardware you have in front of you
and claiming that it's true everywhere, and shoving that into generic
code.

I know, for example on OMAP, you have to power up the CPU first before
you can "wake" it.

I wouldn't be surprised if other SoCs are like that: where they require
the CPU core to be powered and held in reset, before releasing the reset
to then allow them to start executing the secondary core bringup.

Relying on hardware to do this sounds really fragile and bad design to
me.

If you want to persue your current design, don't make it generic code,
because it's got no right to be generic with assumptions like that.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.