[PATCH] drivers: psci: make PSCI 1.0 functions initialization version dependent

[Sudeep Holla]

On 23/10/15 15:46, Lorenzo Pieralisi wrote:
> The PSCI specifications [1] and the SMC calling convention mandate
> that unimplemented functions ids must return NOT_SUPPORTED (0xffffffff)
> if a function id is called but it is not implemented.
>
> Consequently, PSCI 1.0 function ids that require the 1.0 PSCI_FEATURES
> call to be initialized:
>
> CPU_SUSPEND (psci_init_cpu_suspend())
> SYSTEM_SUSPEND (psci_init_system_suspend())
>
> call the PSCI_FEATURES function id independently of the detected
> PSCI firmware version, since, if the PSCI_FEATURES function id is not
> implemented, it must return NOT_SUPPORTED according to the PSCI
> specifications, causing the initialization functions to fail as expected.
>
> Some existing PSCI implementations (ie Qemu PSCI emulation), do not
> comply with the SMC calling convention and fail if function ids that are
> not implemented are called from the OS, causing boot failures.
>
> To solve this issue, this patch adds code that checks the PSCI firmware
> version before calling PSCI 1.0 initialization functions so that the
> OS makes sure that it is calling 1.0 functions only if the firmware
> version detected is 1.0 or greater, therefore avoiding PSCI calls
> that are bound to fail and might cause system boot failures owing
> to non-compliant PSCI firmware implementations.
>
> [1] http://infocenter.arm.com/help/topic/com.arm.doc.den0022c/DEN0022C_Power_State_Coordination_Interface.pdf
>
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi at arm.com>
> Cc: Arnd Bergmann <arnd at arndb.de>
> Cc: Kevin Hilman <khilman at kernel.org>
> Cc: Sudeep Holla <sudeep.holla at arm.com>

Looks good to me(however have alternate thought, see below)
Acked-by: Sudeep Holla <sudeep.holla at arm.com>

> Cc: Olof Johansson <olof at lixom.net>
> Cc: Mark Rutland <mark.rutland at arm.com>
> ---
> Arnd, Kevin, Olof,
>
> this applies to current arm-soc drivers/psci branch, and solves the
> issue Kevin detected through kernelci with Qemu emulation:
>
> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-October/380304.html
>
> Tested on:
>
> - Juno host
> - AMD Seattle host
> - kvmtool arm64 guest (on Juno arm64 defconfig host)
> - Qemu x86 host (aarch64 emulation)
>
> A run on kernelci and consequent tested-by tags would be much appreciated,
> thanks for spotting this and for your help.
>
> Thanks,
> Lorenzo
>
>   drivers/firmware/psci.c | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c
> index 492db42..d24f35d 100644
> --- a/drivers/firmware/psci.c
> +++ b/drivers/firmware/psci.c
> @@ -347,9 +347,10 @@ static int __init psci_probe(void)
>
>   	psci_init_migrate();
>
> -	psci_init_cpu_suspend();
> -
> -	psci_init_system_suspend();
> +	if (PSCI_VERSION_MAJOR(ver) >= 1) {

Alternatively we can just add this check in psci_features function and
return PSCI_RET_NOT_SUPPORTED if PSCI_VERSION_MAJOR(ver) < 1. New
callers of psci_features are also protected and need not add checks at 
call sites. Thoughts ?

I am fine with this patch as is and don't have a strong opinion.

> +		psci_init_cpu_suspend();
> +		psci_init_system_suspend();
> +	}
>
>   	return 0;
>   }
>

-- 
Regards,
Sudeep