From mboxrd@z Thu Jan 1 00:00:00 1970 From: Lorenzo Pieralisi Subject: Re: [PATCH v5 4/8] arm64: add PSCI CPU_SUSPEND based cpu_suspend support Date: Thu, 26 Jun 2014 17:55:05 +0100 Message-ID: <20140626165505.GD27364@e102568-lin.cambridge.arm.com> References: <1403705421-17597-1-git-send-email-lorenzo.pieralisi@arm.com> <1403705421-17597-5-git-send-email-lorenzo.pieralisi@arm.com> <1403729520.11749.64.camel@smoke> Mime-Version: 1.0 Content-Type: text/plain; charset=WINDOWS-1252 Content-Transfer-Encoding: 8BIT Return-path: In-Reply-To: <1403729520.11749.64.camel@smoke> Content-Disposition: inline Sender: linux-pm-owner@vger.kernel.org To: Geoff Levand Cc: "linux-arm-kernel@lists.infradead.org" , "linux-pm@vger.kernel.org" , "devicetree@vger.kernel.org" , Mark Rutland , Paul Walmsley , Vincent Guittot , Kevin Hilman , Nicolas Pitre , Catalin Marinas , Peter De Schrijver , Daniel Lezcano , Stephen Boyd , Amit Kucheria , Chander Kashyap , Sebastian Capella , Rob Herring , Santosh Shilimkar , Mark Brown , Sudeep Holla , "grant.likely@linaro.org" List-Id: devicetree@vger.kernel.org Hi Geoff, On Wed, Jun 25, 2014 at 09:52:00PM +0100, Geoff Levand wrote: > Hi Lorenzo, > > On Wed, 2014-06-25 at 15:10 +0100, Lorenzo Pieralisi wrote: > > diff --git a/arch/arm64/kernel/psci.c b/arch/arm64/kernel/psci.c > > > + /* > > + * This is belt-and-braces: make sure that if the idle > > + * specified protocol is psci, the cpu_ops have been > > + * initialized to psci operations. Anything else is > > + * a recipe for mayhem. > > + */ > > + for_each_cpu(cpu, drv->cpumask) { > > + cpu_ops_ptr = cpu_ops[cpu]; > > + if (WARN_ON(!cpu_ops_ptr || strcmp(cpu_ops_ptr->name, "psci"))) > > + return -EOPNOTSUPP; > > + } > > I'm not sure how drv->cpumask is setup, but if a system has mixed enable > methods, say some cpus 'spin-table' and some 'psci', will this give a > false error? I do not think that's a false error. If the idle states specify an entry-method == psci, and cpu_ops for some cpus are not set to PSCI, obviously because the enable-method specified that, that's a firmware bug. > If drv->cpumask should only include 'psci' cpus, then should this be a > BUG()? Ok, if we got here, it is because the idle-states entry-method was set to PSCI. Now, if any of the CPUs in the driver mask has a cpu_ops pointer != PSCI, we have a problem and we should warn on that. I do not think that justifies a BUG_ON, but that's one of those things, it is debatable. Question is whether the check should also be carried out at cpu_ops initialization (ie to check for mixed cpu_ops), for certain if the idle states entry-method is PSCI and cpu_ops != PSCI we should WARN/BUG on that. Or embed this idle state parameters initialization at cpu_ops init (see other thread you started) so that we can kill two birds with one stone. > > + > > + psci_states = kcalloc(drv->state_count, sizeof(*psci_states), > > + GFP_KERNEL); > > + > > + if (!psci_states) { > > + pr_warn("psci idle state allocation failed\n"); > > + return -ENOMEM; > > + } > > + > > + for_each_cpu(cpu, drv->cpumask) { > > + if (per_cpu(psci_power_state, cpu)) { > > + pr_warn("idle states already initialized on cpu %u\n", > > + cpu); > > This seems like an implementation problem, if so, shouldn't this be > pr_debug()? Maybe, I will give it some thought. > > #endif > > > > +#ifdef CONFIG_ARM64_CPU_SUSPEND > > +static int cpu_psci_cpu_suspend(unsigned long index) > > +{ > > + struct psci_power_state *state = __get_cpu_var(psci_power_state); > > + > > + if (!state) > > + return -EOPNOTSUPP; > > + > > + return psci_ops.cpu_suspend(state[index], virt_to_phys(cpu_resume)); > > +} > > +#endif > > Why not put a __maybe_unused attribute on cpu_psci_cpu_suspend() and > remove the preprocessor conditional. That way this code will always be > compiled, and with therefor always get a build test. The linker should > strip out the unused code when CONFIG_ARM64_CPU_SUSPEND=n and the code > below is not compiled. It can make sense, yes. Thanks, Lorenzo