Re: [PATCH v3 10/13] cpuidle: psci: Prepare to use OS initiated suspend mode via PM domains

From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
To: Ulf Hansson <ulf.hansson@linaro.org>
Cc: Sudeep Holla <sudeep.holla@arm.com>,
	Rob Herring <robh+dt@kernel.org>,
	Linux PM <linux-pm@vger.kernel.org>,
	"Rafael J . Wysocki" <rjw@rjwysocki.net>,
	Daniel Lezcano <daniel.lezcano@linaro.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Lina Iyer <ilina@codeaurora.org>,
	Vincent Guittot <vincent.guittot@linaro.org>,
	Stephen Boyd <sboyd@kernel.org>, Andy Gross <agross@kernel.org>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	Kevin Hilman <khilman@kernel.org>,
	Linux ARM <linux-arm-kernel@lists.infradead.org>,
	linux-arm-msm <linux-arm-msm@vger.kernel.org>,
	Lina Iyer <lina.iyer@linaro.org>
Subject: Re: [PATCH v3 10/13] cpuidle: psci: Prepare to use OS initiated suspend mode via PM domains
Date: Fri, 6 Dec 2019 15:14:21 +0000	[thread overview]
Message-ID: <20191206151421.GA5288@e121166-lin.cambridge.arm.com> (raw)
In-Reply-To: <CAPDyKFq0gS2fasU3Yyh+wPC7Pjnucv6_+vDN234ks+yuiURKCw@mail.gmail.com>

On Fri, Dec 06, 2019 at 03:26:16PM +0100, Ulf Hansson wrote:

[...]

> > You can merge it as it is but that's how things stand and adding
> > a comment to the *code* would help understand its logic.
> 
> Okay, how about adding a comment along the lines of this:
> 
> "Using the deepest state for the CPU to trigger a potential selection
> of a shared state for the domain, assumes the domain states are all
> deeper states".

Just this it should be fine (I trimmed it a bit).

> > > So, unless I am missing your point, I think the above code does
> > > exactly what you want, no?
> > >
> > > In regards to the "arbitrary choice" of what cpuidle state to use,
> > > there are more details about why that is, in the changelog.
> > >
> > > >
> > > > This inizialization though does not belong in here, it is done at driver
> > > > level, it should not be done in this per-cpu path. IIUC the logic the
> > > > enter pointer should only be overridden if and only if all cpus managed
> > > > by the driver have a corresponding device associated.
> > >
> > > I think you have overlooked the fact that there are one cpuidle driver
> > > registered per CPU. The above doesn't make sense to me, sorry.
> >
> > You are calling psci_dt_cpu_init_idle() for every possibile cpu.
> >
> > Every time psci_dt_attach_cpu() is called, we check dev and override
> > the idle driver enter method. There is one driver, what I am saying
> > is that it is not correct to check dev and override the enter pointer
> > for *every* cpu that we try to attach to a power domain. This must
> > be done once for all by checking that *all* devices could be attached
> > to a power domain.
> 
> Ah, now I think get your point.
> 
> You want me to re-iterate through all the registered cpuidle drivers,
> which means one per CPU - and then override the enter callback for
> each of them, but only if all devices was successfully attached to a
> PM domain. Is that correct?
> 
> My only worries with this, is that we have already registered the
> cpuidle drivers and I don't think it's a good idea to update the enter
> callbacks, beyond that point.
> 
> Perhaps another option is to track whether the first CPU gets attached
> (and then update the enter callback), but after that require all the
> remaining CPUs to be attached as well - else bail out with an error
> code, failing to register all the driver instances.
> 
> What do you think about that?

I was confused - now we have one cpuidle driver per cpu so this
comment was bogus from this perspective (I was still reasoning
wit a *single* cpuidle driver across cpus. Apologies).

Sudeep will follow up on this but please forget this specific
comment - I was wrong.

Thanks,
Lorenzo