On 2/25/2020 3:03 AM, Sudeep Holla wrote:
> On Mon, Feb 24, 2020 at 12:57:37PM -0800, Elliot Berman wrote:
>> @@ -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", ¶m)) {
>> + 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.
>
>> + 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
