Re: [PATCH v2 2/3] firmware: psci: Add support for dt-supplied SYSTEM_RESET2 type

On 2/25/2020 3:03 AM, Sudeep Holla wrote:

On Mon, Feb 24, 2020 at 12:57:37PM -0800, Elliot Berman wrote:

quoted

@@ -493,6 +494,7 @@ typedef int (*psci_initcall_t)(const struct device_node *);
 static int __init psci_0_2_init(struct device_node *np)
 {
 	int err;
+	u32 param;

 	err = get_set_conduit_method(np);
 	if (err)

@@ -505,7 +507,19 @@ static int __init psci_0_2_init(struct device_node *np)
 	 * can be carried out according to the specific version reported
 	 * by firmware
 	 */
-	return psci_probe();
+	err = psci_probe();
+	if (err)
+		return err;
+
+	if (psci_system_reset2_supported &&
+	    !of_property_read_u32(np, "arm,psci-sys-reset2-param", &param)) {
+		if ((s32)param > 0)

What is the point on signed comparison here ? You are assuming all vendor
reset also as architecture by doing so which is wrong.

quoted

+			pr_warn("%08x is an invalid architectural reset type.\n",
+				param);

I thought the point was to have vendor reset here. Based on the 3/3 you
see to have vendor reset bit set, you ignore that by doing signed comparison
which is wrong and even the message is wrong. Specification defines only
one architectural reset(WARM RESET) and all others need to be vendor specific.

--
Regards,
Sudeep

I might've gone crazy, but all vendor-specific reset types would be
negative when cast as s32. Thus the check returns true only for an invalid
architectural reset type. I can switch to checking bits instead of using 
cast in v3 to avoid the confusion.

Alternatively, I could rename the DT property to
"arm,psci-sys-reset2-vendor-param" and then always set the 31st bit so that
it is impossible to provide an invalid architectural reset type in DT.

Let me know what is preferred.

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel