Re: [PATCH] firmware: psci: handle unsupported SET_SUSPEND_MODE in PC mode

[Patrick DELAUNAY]

Hi,

On 11/12/26 11:06, Sudeep Holla wrote:
> On Fri, Oct 30, 2026 at 11:04:51AM +0100, Patrick DELAUNAY wrote:
> > Hi,
> >
> > On 10/30/26 09:32, Sudeep Holla wrote:
> > > On Thu, Oct 29, 2026 at 07:16:25PM +0100, Patrick Delaunay wrote:
> > > > The PSCI specification makes PSCI_SET_SUSPEND_MODE optional. If it
> > > > returns NOT_SUPPORTED when Linux requests platform-coordinated (PC)
> > > > mode, Linux can assume that PC mode is in effect. Treat this response
> > > > as success so PSCI power-domain providers can initialize when firmware
> > > > does not support OS-initiated (OSI) mode.
> > > >
> > > > Continue to report an error when OSI mode is requested but
> > > > SET_SUSPEND_MODE is not supported.
> > > So, your platform advertises the presence/support for OSI mode and yet
> > > opted for not implementing SET_SUSPEND_MODE ? If so, it is indeed a
> > > firmware bug as reported.
> > I think it is no realy a platform bug but a too strict management in Linux
> > kernel, for PSCI power domain.
> > The OSI mode is not yet supported by the STM32MP25 platform in Linux at
> > upstream but I introduce PSCI domains in device tree (but without idle power
> > domain) just to prepare this activation in next steps.
> Fair enough, that shouldn't be a problem.
>
> > As the PSCI support is not activated in Firmware (TF-A BL31), the firmware
> > correctly expose its capacities (PSCI_OS_INIT_MODE =0) => PSCI feature
> > indicate that OSI is not supported (FF_SUPPORTS_OS_INIT_MODE = 0) =>
> > PSCI_SET_SUSPEND_MODE function is not supported (optional when OSI
> > is not supported)
> >
> > in Linux side, the power domain driver
> > (drivers/cpuidle/cpuidle-psci-domain.c)
> >
> > correctly check the support of OSI:
> >      bool use_osi = psci_has_osi_support();
> >
> > in STM32MP25 platform use_osi = false, so the driver request
> > Plafrom Cooordinated mode (PC mode) as expected
> > But here  the GENPD driver try to use optional function to set mode:
> >
> >      /* let's try to enable OSI. */
> >      ret = psci_set_osi_mode(use_osi);
> >      if (ret)
> >          goto remove_pd;
> IMO we should make this psci_set_osi_mode() call only when use_osi == true
> above. If it is passing false, it is setting PC mode but as comments states
> it may be assuming OSI mode and proceeding further which is completely wrong.
> I would bail out of psci_cpuidle_domain_probe() early if use_osi is false.
>
> > And the problem occurs here because the optional function is called but the
> > normal use case is not handled (OSI mode not supported / PSCI_SET_SUSPEND_MODE
> > not supported)
> Agreed, but I would check before calling here. The proposed fix hides
> PSCI_RET_NOT_SUPPORTED error for PC-mode setting. So clear NACK for that.
>
> > With the proposed patch, the Linux kernel can have PSCI domain in device
> > tree even when PSCI OSI support is deactivated in PSCI firmware (or not yet
> > activated): the domain are always ON (GENPD_FLAG_ALWAYS_ON).
> Do you see any issue failing in psci_cpuidle_domain_probe() instead ?
>
> > NB: I hesitate between the current patch and a patch in PSCI power domain
> > driver:
> >
> >      /* let's try to enable OSI. */
> >      ret = psci_set_osi_mode(use_osi);
> > + if (ret == -EOPNOTSUPP && !use_osi)
> > +    ret = 0;
> >
> >      if (ret)
> >          goto remove_pd;
> See above, if the driver is assuming OSI mode is set after the call like the
> comment states, it better check if use_osi = true.
>
> > but I patch PSCI firmware driver because I think it is a more generic place
> > to handle optional PSCI function.
> If we have to handle it in the psci, I would rather not make a call to the
> firmware if PSCI_FEATURES(PSCI_1_0_FN_SET_SUSPEND_MODE) returns not supported.
> That is the expectation for any ABI the firmware said it doesn't support.
> Making unsupported ABI call to the firmware may result in unexpected behaviour
> and must be avoided.

Ok, it is clear

Thanks for the review

you can drop this patch and I will propose an other patch in PCSI CPU idle

driver to no more request "PC mode" with psci_set_osi_mode()

when use_osi= false


Patrick