Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
In-Reply-To: <6ec0ca8d-85c7-53d6-acf2-22c4ac13e805@codeaurora.org>
References: <20210323224336.1311783-1-swboyd@chromium.org> <6ec0ca8d-85c7-53d6-acf2-22c4ac13e805@codeaurora.org>
Subject: Re: [PATCH v2] firmware: qcom_scm: Only compile legacy calls on ARM
From:   Stephen Boyd <swboyd@chromium.org>
Cc:     linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org,
        Brian Masney <masneyb@onstation.org>,
        Stephan Gerhold <stephan@gerhold.net>,
        Jeffrey Hugo <jhugo@codeaurora.org>,
        Douglas Anderson <dianders@chromium.org>
To:     Andy Gross <agross@kernel.org>,
        Bjorn Andersson <bjorn.andersson@linaro.org>,
        Elliot Berman <eberman@codeaurora.org>
Date:   Thu, 01 Apr 2021 23:58:48 -0700
Message-ID: <161734672825.2260335.8472441215895199196@swboyd.mtv.corp.google.com>
User-Agent: alot/0.9.1
Precedence: bulk

Quoting Elliot Berman (2021-04-01 18:12:14)
>=20
> It might be a good idea to wrap these lines from qcom_scm_call with #if=20
> IS_ENABLED(CONFIG_ARM), and the corresponding ones in qcom_scm_call_atomi=
c:
>=20
>    case SMC_CONVENTION_LEGACY:
>        return scm_legacy_call(dev, desc, res);
>=20
> If something is wrong with loaded firmware and LEGACY convention is=20
> incorrectly selected, you would get a better hint about the problem:=20
> "Unknown current SCM calling convention." You would still get the hint=20
> earlier from __get_convention, but that may not be obvious to someone=20
> unfamiliar with the SCM driver.
>=20
> I'll defer to your/Bjorn's preference:
>=20
> Acked-by: Elliot Berman <eberman@codeaurora.org>
>=20
> with or without modifying qcom_scm_call{_atomic}.
>=20

Maybe it would be better to catch that problem at the source and force
arm64 calling convention to be safe? I'm thinking of this patch, but it
could be even more fine tuned and probably the sc7180 check could be
removed in the process if the rest of this email makes sense.

If I understand correctly CONFIG_ARM64=3Dy should never use legacy
convention (and never the 32-bit one either?), so we can figure that out
pretty easily and just force it to use 64-bit if the architecture is
arm64. If only the 64-bit convention is supported on arm64 then we
really don't even need to call into the firmware to figure it out on
arm64. We can do this convention detection stuff on arm32 (CONFIG_ARM)
and always assume 64-bit convention on CONFIG_ARM64. Is that right?

---8<---
diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index 747808a8ddf4..22187ebc1678 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -140,7 +140,13 @@ static enum qcom_scm_convention __get_convention(void)
 	if (!ret && res.result[0] =3D=3D 1)
 		goto found;
=20
-	probed_convention =3D SMC_CONVENTION_LEGACY;
+	if (IS_ENABLED(CONFIG_ARM)) {
+		probed_convention =3D SMC_CONVENTION_LEGACY;
+	} else {
+		probed_convention =3D SMC_CONVENTION_ARM_64;
+		forced =3D true;
+		WARN(1, "qcom_scm: Detected legacy convention on arm64; firmware is brok=
en\n");
+	}
 found:
 	spin_lock_irqsave(&scm_query_lock, flags);
 	if (probed_convention !=3D qcom_scm_convention) {