Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230330084250.32600-1-quic_mkshah@quicinc.com>
 <20230330084250.32600-2-quic_mkshah@quicinc.com> <20230330093431.xn5wwiwqbne5owf7@bogus>
 <CAPDyKFpUmkF=pOwXNrva1k2R+RFBB39-Y4kA0Fve+-3NGbtDag@mail.gmail.com> <20230330131334.idb25zf4tdf3zqn3@bogus>
In-Reply-To: <20230330131334.idb25zf4tdf3zqn3@bogus>
From:   Wing Li <wingers@google.com>
Date:   Thu, 30 Mar 2023 18:46:33 -0700
Message-ID: <CADut4F0oh7kzv0GvuUV0CJnS_YxELoEzuZ_=tDKyKO86baCJNQ@mail.gmail.com>
Subject: Re: [PATCH v2 1/2] cpuidle: psci: Move enabling OSI mode after power
 domains creation
To:     Sudeep Holla <sudeep.holla@arm.com>
Cc:     Ulf Hansson <ulf.hansson@linaro.org>,
        Maulik Shah <quic_mkshah@quicinc.com>, andersson@kernel.org,
        dianders@chromium.org, swboyd@chromium.org,
        linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-pm@vger.kernel.org, jwerner@chromium.org,
        quic_lsrao@quicinc.com, quic_rjendra@quicinc.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Adding some clarifications.

On Thu, Mar 30, 2023 at 6:13=E2=80=AFAM Sudeep Holla <sudeep.holla@arm.com>=
 wrote:
>
> On Thu, Mar 30, 2023 at 02:06:19PM +0200, Ulf Hansson wrote:
> > On Thu, 30 Mar 2023 at 11:34, Sudeep Holla <sudeep.holla@arm.com> wrote=
:
> > >
> > > On Thu, Mar 30, 2023 at 02:12:49PM +0530, Maulik Shah wrote:
> > > > A switch from OSI to PC mode is only possible if all CPUs other tha=
n the
> > > > calling one are OFF, either through a call to CPU_OFF or not yet bo=
oted.
> > > >
> > >
> > > As per the spec, all cores are in one of the following states:
> > >  - Running
> > >  - OFF, either through a call to CPU_OFF or not yet booted
> > >  - Suspended, through a call to CPU_DEFAULT_SUSPEND
> > >
> > > Better to provide full information.

The spec quoted above only applies when switching from
platform-coordinated mode to OS-initiated mode.

For switching from OS-initiated to platform-coordinated, which is the
case Maulik is referring to, section 5.20.2 of the spec specifies:
"A switch from OS-initiated mode to platform-coordinated mode is only
possible if all cores other than the calling one are OFF, either
through a call to CPU_OFF or not yet booted."

> > >
> > > > Currently OSI mode is enabled before power domains are created. In =
cases
> > > > where CPUidle states are not using hierarchical CPU topology the ba=
il out
> > > > path tries to switch back to PC mode which gets denied by firmware =
since
> > > > other CPUs are online at this point and creates inconsistent state =
as
> > > > firmware is in OSI mode and Linux in PC mode.
> > > >
> > >
> > > OK what is the issue if the other cores are online ? As long as they =
are
> > > running, it is allowed in the spec, so your statement is incorrect.

The issue here is that the kernel prematurely enabled OSI mode based
on the condition that OSI mode is supported by the firmware, and is
unable to switch back to PC mode in the bail out path if hierarchical
CPU topology isn't used because the other CPUs at this point are now
online.

> > >
> > > Is CPUidle enabled before setting the OSI mode. I see only that can c=
ause
> > > issue as we don't use CPU_DEFAULT_SUSPEND. If idle is not yet enabled=
, it
> > > shouldn't be a problem.
> >
> > Sudeep, you may very well be correct here. Nevertheless, it looks like
> > the current public TF-A implementation doesn't work exactly like this,
> > as it reports an error in Maulik's case. We should fix it too, I
> > think.
> >
> > Although, to me it doesn't really matter as I think $subject patch
> > makes sense anyway. It's always nice to simplify code when it's
> > possible.
> >
>
> Agreed, I don't have any objection to the change. The wording the message
> worried me and wanted to check if there are any other issues because of t=
his.
> As such it doesn't look like there are but the commit message needs to be
> updated as it gives a different impression/understanding.

I think the commit message is accurate and we can keep it as is.

Best regards,
Wing

>
> --
> Regards,
> Sudeep