From: Geoff Levand <geoff@infradead.org> To: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> Cc: linux-arm-kernel@lists.infradead.org, linux-pm@vger.kernel.org, devicetree@vger.kernel.org, Mark Rutland <mark.rutland@arm.com>, Paul Walmsley <paul@pwsan.com>, Vincent Guittot <vincent.guittot@linaro.org>, Kevin Hilman <khilman@linaro.org>, Nicolas Pitre <nico@linaro.org>, Catalin Marinas <catalin.marinas@arm.com>, Peter De Schrijver <pdeschrijver@nvidia.com>, Daniel Lezcano <daniel.lezcano@linaro.org>, Stephen Boyd <sboyd@codeaurora.org>, Amit Kucheria <amit.kucheria@linaro.org>, Chander Kashyap <chander.kashyap@linaro.org>, Sebastian Capella <sebcape@gmail.com>, Rob Herring <robh+dt@kernel.org>, Santosh Shilimkar <santosh.shilimkar@ti.com>, Mark Brown <broonie@kernel.org>, Sudeep Holla <sudeep.holla@arm.com>, Grant Likely <grant.likely@linaro.org>, Tomasz Figa <t.figa@samsung.com>, Antti Miettinen <ananaza@iki.fi>, Charles Garcia Tobin <Charles.Garcia-Tobin@arm> Subject: Re: [PATCH v5 4/8] arm64: add PSCI CPU_SUSPEND based cpu_suspend support Date: Wed, 25 Jun 2014 13:52:00 -0700 [thread overview] Message-ID: <1403729520.11749.64.camel@smoke> (raw) In-Reply-To: <1403705421-17597-5-git-send-email-lorenzo.pieralisi@arm.com> 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? If drv->cpumask should only include 'psci' cpus, then should this be a BUG()? > + > + 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()? > #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. > +#ifdef CONFIG_ARM64_CPU_SUSPEND > + .cpu_suspend = cpu_psci_cpu_suspend, > +#endif > }; -Geoff
WARNING: multiple messages have this Message-ID (diff)
From: geoff@infradead.org (Geoff Levand) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH v5 4/8] arm64: add PSCI CPU_SUSPEND based cpu_suspend support Date: Wed, 25 Jun 2014 13:52:00 -0700 [thread overview] Message-ID: <1403729520.11749.64.camel@smoke> (raw) In-Reply-To: <1403705421-17597-5-git-send-email-lorenzo.pieralisi@arm.com> 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? If drv->cpumask should only include 'psci' cpus, then should this be a BUG()? > + > + 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()? > #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. > +#ifdef CONFIG_ARM64_CPU_SUSPEND > + .cpu_suspend = cpu_psci_cpu_suspend, > +#endif > }; -Geoff
next prev parent reply other threads:[~2014-06-25 20:52 UTC|newest] Thread overview: 70+ messages / expand[flat|nested] mbox.gz Atom feed top 2014-06-25 14:10 [PATCH v5 0/8] ARM generic idle states Lorenzo Pieralisi 2014-06-25 14:10 ` Lorenzo Pieralisi 2014-06-25 14:10 ` [PATCH v5 1/8] Documentation: arm: define DT idle states bindings Lorenzo Pieralisi 2014-06-25 14:10 ` Lorenzo Pieralisi 2014-06-25 14:58 ` Mark Rutland 2014-06-25 14:58 ` Mark Rutland 2014-06-25 17:37 ` Lorenzo Pieralisi 2014-06-25 17:37 ` Lorenzo Pieralisi 2014-06-26 18:32 ` Rob Herring 2014-06-26 18:32 ` Rob Herring 2014-06-27 10:53 ` Lorenzo Pieralisi 2014-06-27 10:53 ` Lorenzo Pieralisi 2014-06-25 15:56 ` Nicolas Pitre 2014-06-25 15:56 ` Nicolas Pitre 2014-06-26 10:17 ` Lorenzo Pieralisi 2014-06-26 10:17 ` Lorenzo Pieralisi 2014-06-26 19:30 ` Nicolas Pitre 2014-06-26 19:30 ` Nicolas Pitre 2014-06-25 14:10 ` [PATCH v5 2/8] Documentation: devicetree: psci: define CPU suspend parameter Lorenzo Pieralisi 2014-06-25 14:10 ` Lorenzo Pieralisi 2014-06-25 14:10 ` [PATCH v5 3/8] drivers: cpuidle: implement DT based idle states infrastructure Lorenzo Pieralisi 2014-06-25 14:10 ` Lorenzo Pieralisi 2014-06-25 15:59 ` Mark Rutland 2014-06-25 15:59 ` Mark Rutland 2014-06-26 16:01 ` Lorenzo Pieralisi 2014-06-26 16:01 ` Lorenzo Pieralisi [not found] ` <1403705421-17597-1-git-send-email-lorenzo.pieralisi-5wv7dgnIgG8@public.gmane.org> 2014-06-25 14:10 ` [PATCH v5 4/8] arm64: add PSCI CPU_SUSPEND based cpu_suspend support Lorenzo Pieralisi 2014-06-25 14:10 ` Lorenzo Pieralisi 2014-06-25 16:09 ` Mark Rutland 2014-06-25 16:09 ` Mark Rutland 2014-06-26 11:23 ` Lorenzo Pieralisi 2014-06-26 11:23 ` Lorenzo Pieralisi 2014-06-25 20:52 ` Geoff Levand [this message] 2014-06-25 20:52 ` Geoff Levand 2014-06-26 16:55 ` Lorenzo Pieralisi 2014-06-26 16:55 ` Lorenzo Pieralisi 2014-06-25 14:10 ` [PATCH v5 5/8] drivers: cpuidle: CPU idle ARM64 driver Lorenzo Pieralisi 2014-06-25 14:10 ` Lorenzo Pieralisi 2014-06-25 20:34 ` Geoff Levand 2014-06-25 20:34 ` Geoff Levand 2014-06-25 14:10 ` [PATCH v5 6/8] drivers: cpuidle: initialize big.LITTLE driver through DT Lorenzo Pieralisi 2014-06-25 14:10 ` Lorenzo Pieralisi 2014-06-25 15:06 ` Mark Rutland 2014-06-25 15:06 ` Mark Rutland 2014-06-25 16:44 ` Lorenzo Pieralisi 2014-06-25 16:44 ` Lorenzo Pieralisi 2014-06-25 14:10 ` [PATCH v5 7/8] drivers: cpuidle: initialize Exynos " Lorenzo Pieralisi 2014-06-25 14:10 ` Lorenzo Pieralisi 2014-06-25 15:13 ` Mark Rutland 2014-06-25 15:13 ` Mark Rutland 2014-06-25 16:58 ` Lorenzo Pieralisi 2014-06-25 16:58 ` Lorenzo Pieralisi 2014-06-25 15:23 ` Bartlomiej Zolnierkiewicz 2014-06-25 15:23 ` Bartlomiej Zolnierkiewicz 2014-06-26 15:16 ` Lorenzo Pieralisi 2014-06-26 15:16 ` Lorenzo Pieralisi 2014-07-17 14:20 ` Lorenzo Pieralisi 2014-07-17 14:20 ` Lorenzo Pieralisi 2014-07-18 8:45 ` Chander Kashyap 2014-07-18 8:45 ` Chander Kashyap 2014-07-18 16:10 ` Bartlomiej Zolnierkiewicz 2014-07-18 16:10 ` Bartlomiej Zolnierkiewicz 2014-06-25 14:10 ` [PATCH v5 8/8] arm64: boot: dts: update rtsm aemv8 dts with PSCI and idle states Lorenzo Pieralisi 2014-06-25 14:10 ` Lorenzo Pieralisi 2014-06-25 14:27 ` Mark Rutland 2014-06-25 14:27 ` Mark Rutland 2014-06-25 17:47 ` Lorenzo Pieralisi 2014-06-25 17:47 ` Lorenzo Pieralisi 2014-06-25 14:29 ` Sudeep Holla 2014-06-25 14:29 ` Sudeep Holla
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=1403729520.11749.64.camel@smoke \ --to=geoff@infradead.org \ --cc=Charles.Garcia-Tobin@arm \ --cc=amit.kucheria@linaro.org \ --cc=ananaza@iki.fi \ --cc=broonie@kernel.org \ --cc=catalin.marinas@arm.com \ --cc=chander.kashyap@linaro.org \ --cc=daniel.lezcano@linaro.org \ --cc=devicetree@vger.kernel.org \ --cc=grant.likely@linaro.org \ --cc=khilman@linaro.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-pm@vger.kernel.org \ --cc=lorenzo.pieralisi@arm.com \ --cc=mark.rutland@arm.com \ --cc=nico@linaro.org \ --cc=paul@pwsan.com \ --cc=pdeschrijver@nvidia.com \ --cc=robh+dt@kernel.org \ --cc=santosh.shilimkar@ti.com \ --cc=sboyd@codeaurora.org \ --cc=sebcape@gmail.com \ --cc=sudeep.holla@arm.com \ --cc=t.figa@samsung.com \ --cc=vincent.guittot@linaro.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.