Re: [PATCH v3 08/13] cpuidle: psci: Add a helper to attach a CPU to its PM domain

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>
Subject: Re: [PATCH v3 08/13] cpuidle: psci: Add a helper to attach a CPU to its PM domain
Date: Thu, 28 Nov 2019 18:31:43 +0000	[thread overview]
Message-ID: <20191128183143.GA15793@e121166-lin.cambridge.arm.com> (raw)
In-Reply-To: <CAPDyKFr1GTAkNyTmLvvC1ovm4yOtMGLOVWA-e-wjGJz2ENdN1g@mail.gmail.com>

On Thu, Nov 28, 2019 at 06:21:03PM +0100, Ulf Hansson wrote:
> On Thu, 28 Nov 2019 at 15:15, Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com> wrote:
> >
> > On Wed, Nov 27, 2019 at 11:29:09AM +0100, Ulf Hansson wrote:
> >
> > [...]
> >
> > > +struct device *psci_dt_attach_cpu(int cpu)
> > > +{
> > > +     struct device *dev;
> > > +
> > > +     /* Currently limit the hierarchical topology to be used in OSI mode. */
> > > +     if (!psci_has_osi_support())
> > > +             return NULL;
> > > +
> > > +     dev = dev_pm_domain_attach_by_name(get_cpu_device(cpu), "psci");
> > > +     if (IS_ERR_OR_NULL(dev))
> > > +             return dev;
> > > +
> > > +     pm_runtime_irq_safe(dev);
> > > +     if (cpu_online(cpu))
> >
> > It is unclear to me how we handle (or rather we don't) CPU hotplug
> > with this series - it does not look OK unless genpd code manages
> > that automatically.
> 
> The series doesn't handle CPU hotplug at the moment, simply because I
> am targeting to get the basic support, upstream first.

Basic support must work and that includes CPU hotplug - I don't want
to merge code that work with assumptions that aren't valid.

> For a functionality point of view, this isn't a problem in my opinion.
> Simply because the consequence is only that the idle states for the
> "cluster" will not be reached if there is a CPU brought offline.
> 
> As we talked about at LPC and as also told Sudeep for the v2 series,
> CPU hotplug is going to be implemented by using a CPU HP notifier.

Yes, it should be part of the series.

> That should be fine, right?

Yes, hopefully but it has to be part of the series.

Thanks,
Lorenzo