On Wed, Feb 26, 2025 at 08:13:38AM +0530, Sibi Sankar wrote:
> The addition of per message-id fastchannel support check exposed
> a SCP firmware bug which leads to a device crash on X1E platforms.
> The X1E firmware supports fastchannel on LEVEL_GET but fails to
> have this set in the protocol message attributes and the fallback
> to regular messaging leads to a device crash since that implementation
> has a bug for all the X1E devices in the wild. Fix this by introducing
> a quirk that selectively skips the per message-id fastchannel check only
> for the LEVEL_GET message on X1E platforms.
>
> Signed-off-by: Sibi Sankar <quic_sibis@xxxxxxxxxxx>
> ---
> drivers/firmware/arm_scmi/driver.c | 5 +++--
> drivers/firmware/arm_scmi/perf.c | 30 +++++++++++++++++++++------
> drivers/firmware/arm_scmi/powercap.c | 8 +++----
> drivers/firmware/arm_scmi/protocols.h | 2 +-
> 4 files changed, 32 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> index 9313b9020fc1..b182fa8e8ccb 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c
> @@ -1903,7 +1903,8 @@ static void
> scmi_common_fastchannel_init(const struct scmi_protocol_handle *ph,
> u8 describe_id, u32 message_id, u32 valid_size,
> u32 domain, void __iomem **p_addr,
> - struct scmi_fc_db_info **p_db, u32 *rate_limit)
> + struct scmi_fc_db_info **p_db, u32 *rate_limit,
> + bool skip_check)
This does not look like it will scale.
> {
> int ret;
> u32 flags;
> @@ -1919,7 +1920,7 @@ scmi_common_fastchannel_init(const struct scmi_protocol_handle *ph,
>
> /* Check if the MSG_ID supports fastchannel */
> ret = scmi_protocol_msg_check(ph, message_id, &attributes);
> - if (!ret && !MSG_SUPPORTS_FASTCHANNEL(attributes))
> + if (!ret && !MSG_SUPPORTS_FASTCHANNEL(attributes) && !skip_check)
Why can't you just make sure that the bit is set in attributes as I
suggested? That seems like it should allow for a minimal implementation
of this.
> return;
>
> if (!p_addr) {
> @@ -1282,6 +1288,7 @@ static int scmi_perf_protocol_init(const struct scmi_protocol_handle *ph)
> {
> int domain, ret;
> u32 version;
> + struct device_node *np;
> struct scmi_perf_info *pinfo;
>
> ret = ph->xops->version_get(ph, &version);
> @@ -1297,6 +1304,17 @@ static int scmi_perf_protocol_init(const struct scmi_protocol_handle *ph)
>
> pinfo->version = version;
>
> + /*
> + * Some X1E devices support fastchannel for LEVEL_GET but erroneously
> + * says otherwise in the protocol message attributes. Add a quirk to
> + * force fastchannel on LEVEL_GET to prevent crashes on such devices.
> + */
> + np = of_find_compatible_node(NULL, NULL, "qcom,x1e80100");
Here you should use of_machine_is_compatible().
> + if (np) {
> + pinfo->quirks = BIT(PERF_QUIRK_SKIP_FASTCHANNEL_LEVEL_GET);
> + of_node_put(np);
> + }
> +
> ret = scmi_perf_attributes_get(ph, pinfo);
> if (ret)
> return ret;
Johan
