Re: [PATCH v5 3/4] firmware: psci: Read and use vendor reset types

On 8/15/2024 11:35 PM, Elliot Berman wrote:

On Thu, Aug 15, 2024 at 04:40:55PM +0200, Lorenzo Pieralisi wrote:

quoted

On Mon, Aug 12, 2024 at 11:46:08PM +0530, Shivendra Pratap wrote:

quoted

On 8/9/2024 10:28 PM, Elliot Berman wrote:

quoted

On Fri, Aug 09, 2024 at 03:30:38PM +0200, Lorenzo Pieralisi wrote:

quoted

On Wed, Aug 07, 2024 at 11:10:50AM -0700, Elliot Berman wrote:

[...]

quoted

quoted

quoted

+static void psci_vendor_sys_reset2(unsigned long action, void *data)

'action' is unused and therefore it is not really needed.

quoted

+{
+	const char *cmd = data;
+	unsigned long ret;
+	size_t i;
+
+	for (i = 0; i < num_psci_reset_params; i++) {
+		if (!strcmp(psci_reset_params[i].mode, cmd)) {
+			ret = invoke_psci_fn(PSCI_FN_NATIVE(1_1, SYSTEM_RESET2),
+					     psci_reset_params[i].reset_type,
+					     psci_reset_params[i].cookie, 0);
+			pr_err("failed to perform reset \"%s\": %ld\n",
+				cmd, (long)ret);
+		}
+	}
+}
+
 static int psci_sys_reset(struct notifier_block *nb, unsigned long action,
 			  void *data)
 {
+	if (data && num_psci_reset_params)

So, reboot_mode here is basically ignored; if there is a vendor defined
reset, we fire it off.

I think Mark mentioned his concerns earlier related to REBOOT_* mode and
reset type (granted, the context was different):

https://lore.kernel.org/all/20200320120105.GA36658@C02TD0UTHF1T.local/ (local)

I would like to understand if this is the right thing to do before
accepting this patchset.

I don't have any concerns to move this part below checking reboot_mode.
Or, I could add reboot_mode == REBOOT_COLD check.

The question is how can we map vendor specific reboot magic to Linux
reboot modes sensibly in generic PSCI code - that's by definition
vendor specific.

I don't think it's a reasonable thing to do. "reboot bootloader" or
"reboot edl" don't make sense to the Linux reboot modes.

I believe the Linux reboot modes enum is oriented to perspective of
Linux itself and the vendor resets are oriented towards behavior of the
SoC.

Thanks,
Elliot

Agree.

from perspective of linux reboot modes, kernel's current
implementation in reset path is like:

__
#1 If reboot_mode is WARM/SOFT and PSCI_SYSRESET2 is supported 
    Call PSCI - SYSTEM_RESET2 - ARCH RESET
#2 ELSE
    Call PSCI - SYSTEM_RESET COLD RESET
___

ARM SPECS for PSCI SYSTEM_RESET2
This function extends SYSTEM_RESET. It provides:
• ARCH RESET: set Bit[31] to 0               = > This is already in place in condition #1.
• vendor-specific resets: set Bit[31] to 1.  = > current patchset adds this part before kernel's reboot_mode reset at #0.


In current patchset, we see a condition added at
#0-psci_vendor_reset2 being called before kernel’s current
reboot_mode condition and it can take any action only if all below
conditions are satisfied.
- PSCI SYSTEM_RESET2 is supported.
- psci dt node defines an entry "bootloader" as a reboot-modes.
- User issues reboot with a command say - (reboot bootloader).
- If vendor reset fails, default reboot mode will execute as is.

Don't see if we will skip or break the kernel reboot_mode flow with
this patch.  Also if user issues reboot <cmd> and <cmd> is supported
on SOC vendor reset psci node, should cmd take precedence over
kernel reboot mode enum? may be yes? 

Please wrap lines when replying.

sure. will try to take care.

quoted

I don't think it is a matter of precedence. reboot_mode and the reboot
command passed to the reboot() syscall are there for different (?)
reasons.

What I am asking is whether it is always safe to execute a PSCI vendor
reset irrispective of the reboot_mode value.

Valid point, but it depends on how we configure reboot mode and vendor reset.
If the configuration is conflicting in DT, then reboot_mode and vendor reset
may conflict and show non-predictable results.
For instance, on qcs6490, we have have nvmem-reboot-mode driver
which supports "reboot mode bootloader" function via its current DT as the PMIC
registers are accessible for write on this soc. If we enable nvmem-reboot-mode
and then configure vendor_reset2(mode-bootloader) to perform a different
function on reboot, they will conflict and may result in a non-predictable
behavior. The developer or soc vendor has to take care of this in any
case so this may be a invalid scenario?

May be vendor_reset2 gives more flexibility here on how a soc vendor may 
implement reboot modes and other vendor reset types. In case soc vendor
wants to keep some reboot mode register as open access, they can still
use reboot_mode driver and then others reboot/reset modes can be configured
via vendor_reset2.

For instance, on qcs6490, we can use nvmem-reboot-mode driver for 
"reboot mode bootloader" and use the current patch-vendor_reset2 for
"reboot mode edl". This can be configured via DT. Now even if we
enable both current-patch-vendor_reset2(reboot mode bootloader) 
and nvmem-reboot-mode (mode-bootloader) at same time on this soc,
they are harmless to each other and work as desired as both(DT entries)
align with each other and the PMIC registers are accessible to kernel. The
same thing can conflict, if we enable both drivers at same time and configure
them with conflicting parameters in DT for (reboot mode bootloader).

The only way I see it to be unsafe is we need some other driver using
the reboot_mode to configure something and then the PSCI vendor reset
being incompatible with whatever that other driver did. I don't see that
happens today, so it is up to us to decide what the policy ought to be.
The PSCI spec doesn't help us here because the reboot_mode enum is
totally a Linux construct. In my opinion, firmware should be able to
deal with whatever the driver did or (less ideal) the driver need to be
aware of the PSCI vendor resets. Thus, it would be always safe to
execute a PSCI vendor reset regardless of the reboot_mode value.

Thanks,
Elliot