Re: [RFC PATCH v3 5/5] arm64: Use SYSTEM_OFF2 PSCI call to power off for hibernate

[Sudeep Holla]

On Fri, Mar 22, 2024 at 04:37:19PM +0000, Marc Zyngier wrote:
> On Fri, 22 Mar 2024 16:12:44 +0000,
> David Woodhouse <dwmw2@xxxxxxxxxxxxx> wrote:
> > 
> > On Fri, 2024-03-22 at 16:02 +0000, Marc Zyngier wrote:
> > > On Tue, 19 Mar 2024 12:59:06 +0000,
> > > David Woodhouse <dwmw2@xxxxxxxxxxxxx> wrote:
> > > 
> > > [...]
> > > 
> > > > +static void __init psci_init_system_off2(void)
> > > > +{
> > > > +       int ret;
> > > > +
> > > > +       ret = psci_features(PSCI_FN_NATIVE(1_3, SYSTEM_OFF2));
> > > > +
> > > > +       if (ret != PSCI_RET_NOT_SUPPORTED)
> > > > +               psci_system_off2_supported = true;
> > > 
> > > It'd be worth considering the (slightly broken) case where SYSTEM_OFF2
> > > is supported, but HIBERNATE_OFF is not set in the response, as the
> > > spec doesn't say that this bit is mandatory (it seems legal to
> > > implement SYSTEM_OFF2 without any hibernate type, making it similar to
> > > SYSTEM_OFF).
> > 
> > Such is not my understanding. If SYSTEM_OFF2 is supported, then
> > HIBERNATE_OFF *is* mandatory.
> > 
> > The next update to the spec is turning the PSCI_FEATURES response into
> > a *bitmap* of the available features, and I believe it will mandate
> > that bit zero is set.

Correct, but we add a extra check as well to be sure even if it is mandated
unless the spec relaxes in a way that psci_features(SYSTEM_OFF2) need not
return the mandatory types in the bitmask which I doubt.

Something like:
	if (ret != PSCI_RET_NOT_SUPPORTED &&
		(ret & BIT(PSCI_1_3_HIBERNATE_TYPE_OFF)))
		psci_system_off2_supported = true;

This will ensure the firmware will not randomly set bit[0]=0 if in the
future it support some newer types as well.

I understand the kernel is not conformance test for the spec but in
practice especially for such features and PSCI spec in particular, kernel
has become defacto conformance for firmware developers which is sad.
It some feature works in the kernel, the firmware is assumed to be
conformant to the spec w.r.t the feature.

--
Regards,
Sudeep