Re: [PATCH v13 07/10] firmware: psci: Implement vendor-specific resets as reboot-mode

[Shivendra Pratap <shivendra.pratap@oss.qualcomm.com>]

On 7/30/2025 8:53 PM, André Draszik wrote:
> On Wed, 2025-07-30 at 18:33 +0530, Shivendra Pratap wrote:
>>
>>
>> On 7/30/2025 2:14 PM, André Draszik wrote:
>>> On Sun, 2025-07-27 at 21:54 +0530, Shivendra Pratap wrote:
>>>> SoC vendors have different types of resets which are controlled
>>>> through various hardware registers. For instance, Qualcomm SoC
>>>> may have a requirement that reboot with “bootloader” command
>>>> should reboot the device to bootloader flashing mode and reboot
>>>> with “edl” should reboot the device into Emergency flashing mode.
>>>> Setting up such reboots on Qualcomm devices can be inconsistent
>>>> across SoC platforms and may require setting different HW
>>>> registers, where some of these registers may not be accessible to
>>>> HLOS. These knobs evolve over product generations and require
>>>> more drivers. PSCI spec defines, SYSTEM_RESET2, vendor-specific
>>>> reset which can help align this requirement. Add support for PSCI
>>>> SYSTEM_RESET2, vendor-specific resets and align the implementation
>>>> to allow user-space initiated reboots to trigger these resets.
>>>>
>>>> Introduce a late_initcall to register PSCI vendor-specific resets
>>>> as reboot modes. Implement a reboot-mode write function that sets
>>>> reset_type and cookie values during the reboot notifier callback.
>>>> Introduce a firmware-based call for SYSTEM_RESET2 vendor-specific
>>>> reset in the psci_sys_reset path, using reset_type and cookie if
>>>> supported by secure firmware.
>>>>
>>>> By using the above implementation, userspace will be able to issue
>>>> such resets using the reboot() system call with the "*arg"
>>>> parameter as a string based command. The commands can be defined
>>>> in PSCI device tree node as “reset-types” and are based on the
>>>> reboot-mode based commands.
>>>>
>>>> Signed-off-by: Shivendra Pratap <shivendra.pratap@oss.qualcomm.com>
>>>> ---
>>>>  drivers/firmware/psci/Kconfig |  2 ++
>>>>  drivers/firmware/psci/psci.c  | 57 ++++++++++++++++++++++++++++++++++++++++++-
>>>>  2 files changed, 58 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/firmware/psci/Kconfig b/drivers/firmware/psci/Kconfig
>>>> index 97944168b5e66aea1e38a7eb2d4ced8348fce64b..93ff7b071a0c364a376699733e6bc5654d56a17f 100644
>>>> --- a/drivers/firmware/psci/Kconfig
>>>> +++ b/drivers/firmware/psci/Kconfig
>>>> @@ -1,6 +1,8 @@
>>>>  # SPDX-License-Identifier: GPL-2.0-only
>>>>  config ARM_PSCI_FW
>>>>  	bool
>>>> +	select POWER_RESET
>>>> +	select REBOOT_MODE
>>>>  
>>>>  config ARM_PSCI_CHECKER
>>>>  	bool "ARM PSCI checker"
>>>> diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
>>>> index 38ca190d4a22d6e7e0f06420e8478a2b0ec2fe6f..e14bcdbec1750db8aa9297c8bcdb242f58cc420e 100644
>>>> --- a/drivers/firmware/psci/psci.c
>>>> +++ b/drivers/firmware/psci/psci.c
>>>> @@ -17,6 +17,7 @@
>>>>  #include <linux/printk.h>
>>>>  #include <linux/psci.h>
>>>>  #include <linux/reboot.h>
>>>> +#include <linux/reboot-mode.h>
>>>>  #include <linux/slab.h>
>>>>  #include <linux/suspend.h>
>>>>  
>>>> @@ -51,6 +52,14 @@ static int resident_cpu = -1;
>>>>  struct psci_operations psci_ops;
>>>>  static enum arm_smccc_conduit psci_conduit = SMCCC_CONDUIT_NONE;
>>>>  
>>>> +struct psci_vendor_sysreset2 {
>>>> +	u32 reset_type;
>>>> +	u32 cookie;
>>>> +	bool valid;
>>>> +};
>>>> +
>>>> +static struct psci_vendor_sysreset2 vendor_reset;
>>>> +
>>>>  bool psci_tos_resident_on(int cpu)
>>>>  {
>>>>  	return cpu == resident_cpu;
>>>> @@ -309,7 +318,10 @@ static int get_set_conduit_method(const struct device_node *np)
>>>>  static int psci_sys_reset(struct notifier_block *nb, unsigned long action,
>>>>  			  void *data)
>>>>  {
>>>> -	if ((reboot_mode == REBOOT_WARM || reboot_mode == REBOOT_SOFT) &&
>>>> +	if (vendor_reset.valid && psci_system_reset2_supported) {
>>>> +		invoke_psci_fn(PSCI_FN_NATIVE(1_1, SYSTEM_RESET2), vendor_reset.reset_type,
>>>> +			       vendor_reset.cookie, 0);
>>>> +	} else if ((reboot_mode == REBOOT_WARM || reboot_mode == REBOOT_SOFT) &&
>>>>  	    psci_system_reset2_supported) {
>>>>  		/*
>>>>  		 * reset_type[31] = 0 (architectural)
>>>
>>> I don't know the PSCI spec, but it looks like with this code it's not
>>> possible to set a reboot mode (in DT) and at the same time instruct
>>> the firmware whether a warm or a cold reboot was requested.
>>
>> The code added in this patch is kind of dead, until vendor_reset.valid is set to true.
>> It can be true, only when both below conditions are true.
>>  1. A SoC DT defines a psci->reboot-mode command say - "bootloader".
>>  2. reboot sys call is issued using LINUX_REBOOT_CMD_RESTART2 and the arg* as "bootloader".
>>       reboot(LINUX_REBOOT_MAGIC1, LINUX_REBOOT_MAGIC2, LINUX_REBOOT_CMD_RESTART2, "bootloader");
>>
>> With that in place, warm and cold reboot just work same as before until above conditions are true.
>> There is no effect on regular reboots or the reboots with a "command" which is not defined in
>> psci->reboot-mode DT.
>>
>> Now lets take a case below, where a SoC vendor wants a combination of psci->reboo-mode command and
>> warm/cold to work in combination. For ex. a requirement below:
>>  - reboot command say - "bootlaoder" should do a cold reboot along with some extra HW reg writes.
>>  - reboot command say - "edl" should do a warm reboot along with some extra HW reg writes.
>>
>> 1. For this, both commands will be defined in the psci->reboot-mode DT Node with the arguments that
>>    are defined and supported by the firmware.
>> 2. Further, such requirement will now be taken care by the underlying firmware that supports
>>    PSCI vendor-specific reset. When we call into firmware with vendor specific reset arguments,
>>    firmware will take care of the defined HW writes and warm/cold reset as per the mapping of the
>>    defined arguments. Firmware and the Linux kernel here are in agreement for executing the
>>    vendor-specific resets.
> 
> So that means
> 
>     echo warm > /sys/kernel/reboot/mode
>     reboot bootloader
> 
> and
> 
>     echo cold > /sys/kernel/reboot/mode
>     reboot bootloader
> 
> can not be distinguished.
> 
> The firmware can not know whether or not cold or warm reboot was
> requested in this case by the user.
> 
> More importantly, if e.g. an OOPS / panic happens after the reboot
> notifier has run (and set vendor_reset.valid because a reboot mode
> was requested), a panic handler changing reboot_mode to warm to
> retain RAM contents will have no effect, because the the original
> code above making those distinctions can not be reached anymore.
> 
> Above scenario with OOPS / panic after reboot notifier could e.g.
> happen as part of device_shutdown() - see kernel_shutdown_prepare()

We can handle the panic path by adding a panic_notifier in psci
and make vendor_reset.valid = false. Do you think adding this can clear
the panic scenario above or there can still be some leak?

thanks.

> 
> 
>>>
>>> Doing warm reboot is useful if e.g. RAM contents needs to be retained
>>> for crash recovery handling, or other reasons, while in normal cases
>>> doing a more secure cold reboot.
>>>
>>> On the other hand, of course it's useful to be able to specify the
>>> reboot target for normal reboots.
>>>
>>> Is this a problem with the PSCI spec or with this specific change
>>> geared at the Qcom implementation?
>>
>> SoC vendor should define a vendor-specific reset in psci DT only when they support them in their
>> firmware. 
>>
>> Do we still think we are breaking anything in the spec or in the warm or the cold
>> reset path? If so can we discuss such use-cases?
> 
> I don't know the spec, but see examples above.
> 
> Cheers,
> Andre'