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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
