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

[Ulf Hansson <ulf.hansson@linaro.org>]

On Thu, 20 Oct 2022 at 08:58, Johan Hovold <johan@kernel.org> 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.

Yes.

For the OSI case, I think a better option is to extend the print of
"Initialized CPU PM domain topology\n", in psci_cpuidle_domain_probe()
with information about whether OSI is being used or not.

Kind regards
Uffe