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

[Patrick DELAUNAY <patrick.delaunay@foss.st.com>]

Hi,

On 11/12/26 11:36, Patrick DELAUNAY wrote:
> 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


Patch replaced by PSCI CPUIdle domain patch:

[PATCH] cpuidle: psci: Skip SET_SUSPEND_MODE when OSI is unsupported

https://lore.kernel.org/all/20261112172241.1.Ibe6dddf06a4b7e5b24af24dc85d053e36f940549@changeid/#r

https://patchwork.kernel.org/project/linux-arm-kernel/patch/20261112172241.1.Ibe6dddf06a4b7e5b24af24dc85d053e36f940549@changeid/


>
> _______________________________________________
> Linux-stm32 mailing list
> Linux-stm32@st-md-mailman.stormreply.com
> https://st-md-mailman.stormreply.com/mailman/listinfo/linux-stm32