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

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

[...]

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.

Thanks,
Lorenzo

I'm not sure how best to define the behavior if user sets
reboot_mode = REBOOT_WARM and then user does "reboot bootloader". IMO,
"REBOOT_WARM" is at odds with the "bootloader" command. We can have one
take precedence over the other. I chose for the vendor resets to take
priority, since if userspace is providing specific command at reboot
time, that's probably what they want.

Let me know your thoughts and I'm happy to change the behavior around.


Thanks,
Elliot