Re: [RESEND PATCH v2] firmware/psci: Print a warning if PSCI doesn't accept PC mode

[Dmitry Baryshkov <dmitry.baryshkov@linaro.org>]

On 20/10/2022 09:58, Johan Hovold wrote:
> On Wed, Oct 19, 2022 at 06:02:37PM +0300, Dmitry Baryshkov wrote:
>> On Wed, 19 Oct 2022 at 17:37, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>> On Wed, 19 Oct 2022 at 16:14, Johan Hovold <johan@kernel.org> wrote:
>>>> On Mon, Sep 26, 2022 at 02:02:49PM +0300, Dmitry Baryshkov wrote:
>>>>> The function psci_pd_try_set_osi_mode() will print an error if enabling
>>>>> OSI mode fails. To ease debugging PSCI issues print corresponding
>>>>> message if switching to PC mode fails too.
>>>>>
>>>>> Acked-by: Mark Rutland <mark.rutland@arm.com>
>>>>> Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
>>>>> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> 
>>>>> diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
>>>>> index 9fdcb6bff403..42cae0ba10e2 100644
>>>>> --- a/drivers/firmware/psci/psci.c
>>>>> +++ b/drivers/firmware/psci/psci.c
>>>>> @@ -164,6 +164,8 @@ int psci_set_osi_mode(bool enable)
>>>>>                        PSCI_1_0_SUSPEND_MODE_PC;
>>>>>
>>>>>        err = invoke_psci_fn(PSCI_1_0_FN_SET_SUSPEND_MODE, suspend_mode, 0, 0);
>>>>> +     if (err < 0)
>>>>> +             pr_warn("failed to set %s mode: %d\n", enable ? "OSI" : "PC", err);
>>>>>        return psci_to_linux_errno(err);
>>>>>   }
>>>>
>>>> When booting 6.1-rc1 I now see:
>>>>
>>>>          [    0.000000] psci: OSI mode supported.
>>>>          [    0.000000] psci: failed to set PC mode: -3
>>>>
>>>> on every boot with sc8280xp, while later enabling OSI mode still works:
>>>>
>>>>          [    0.227358] CPUidle PSCI: psci_pd_try_set_osi_mode - success
>>>>          [    0.227599] CPUidle PSCI: Initialized CPU PM domain topology
>>>>
>>>> Judging from Sudeep's comment on v1:
>>>>
>>>>          The platform must boot in PC mode, so even if it fails we ignore
>>>>          so not sure if the logging is of much help here IMO.
>>>>
>>>> perhaps logging this as an error is not a good idea?
>>
>> As far as I understand, some of the Qualcomm platforms booted in the
>> OSI mode, no matter what the spec says. I'm not sure about the
>> sc8280xp, but it definitely was the case previously. This was the
>> primary reason for adding a log here.
>>
>>> I think it's useful to get information that the FW doesn't fully
>>> conform to the spec (it should support PC mode), but that doesn't
>>> really mean that it can't work.
>>>
>>> To me, it sounds like we should move the logging to the debug level!?
>>
>> I'd say, either ignore the error, or move the warning to the info or
>> debug level.
> 
> "Ignore the error" as in removing the warning or as in telling every
> owner of an X13s Thinkpad to ignore it?
> 
> If there's nothing that can be done about it and this is nothing to
> worry about, then moving to debug level would seem like the right thing
> to do here. At least for the PC case.

Feel free to send the patch. I'd maybe use dev_info rather than dev_dbg, 
to let the user have a chance of seeing the warning.

Regarding telling every user to ignore the warning. This is not 
something new. E.g. each time I boot my ThinkPad, I see two or three 
warning messages related to TPM, despite having 'quiet' in kernel boot args.

> 
> Johan

-- 
With best wishes
Dmitry