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.
You're fixing a bug that is introduced in patch 1. That's not allowed.
These need to be squashed into one patch. I means the patch will be
slightly long and the commit message will be slightly long but I
feel like it's manageable.
> 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)
> {
> 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)
> return;
This is explained well in the commit message but the comment here needs
to be better. Also if scmi_protocol_msg_check() fails then we should
return. Let's rename the variable to "force_fastchannel".
ret = scmi_protocol_msg_check(ph, message_id, &attributes);
if (ret)
return;
/*
* Check if the MSG_ID supports fastchannel. The force_fastchannel
* quirk is necessary to work around a firmware bug. We can probably
* remove that quirk in 2030.
*/
if (!force_fastchannel && !MSG_SUPPORTS_FASTCHANNEL(attributes))
return;
>
> if (!p_addr) {
> diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
> index c7e5a34b254b..5b4559d0b054 100644
> --- a/drivers/firmware/arm_scmi/perf.c
> +++ b/drivers/firmware/arm_scmi/perf.c
> @@ -48,6 +48,10 @@ enum {
> PERF_FC_MAX,
> };
>
> +enum {
> + PERF_QUIRK_SKIP_FASTCHANNEL_LEVEL_GET,
> +};
Let's keep the FORCE_FASTCHANNEL naming. Normally we would do this like:
#define PERF_QUIRK_FORCE_FASTCHANNEL BIT(0)
> +
> struct scmi_opp {
> u32 perf;
> u32 power;
> @@ -183,6 +187,7 @@ struct scmi_perf_info {
> enum scmi_power_scale power_scale;
> u64 stats_addr;
> u32 stats_size;
> + u32 quirks;
> bool notify_lvl_cmd;
> bool notify_lim_cmd;
> struct perf_dom_info *dom_info;
> @@ -838,9 +843,10 @@ static int scmi_perf_level_limits_notify(const struct scmi_protocol_handle *ph,
> }
>
> static void scmi_perf_domain_init_fc(const struct scmi_protocol_handle *ph,
> - struct perf_dom_info *dom)
> + struct perf_dom_info *dom, u32 quirks)
> {
> struct scmi_fc_info *fc;
> + bool quirk_level_get = !!(quirks & BIT(PERF_QUIRK_SKIP_FASTCHANNEL_LEVEL_GET));
bool force_fastchannel = !!(quirks & PERF_QUIRK_FORCE_FASTCHANNEL);
>
> fc = devm_kcalloc(ph->dev, PERF_FC_MAX, sizeof(*fc), GFP_KERNEL);
> if (!fc)
regards,
dan carpenter
