* [RFC PATCH 0/3] Introduce SCMI Quirks framework
@ 2025-04-01 12:25 Cristian Marussi
2025-04-01 12:25 ` [RFC PATCH 1/3] firmware: arm_scmi: Ensure that the message-id supports fastchannel Cristian Marussi
` (3 more replies)
0 siblings, 4 replies; 12+ messages in thread
From: Cristian Marussi @ 2025-04-01 12:25 UTC (permalink / raw)
To: linux-kernel, linux-arm-kernel, arm-scmi
Cc: sudeep.holla, james.quinlan, f.fainelli, vincent.guittot,
peng.fan, michal.simek, quic_sibis, dan.carpenter, maz, johan,
Cristian Marussi
Hi,
with the increasing adoption of SCMI across arm64 ecosystems, we have to
start considering how to deal and take care of out-of-spec SCMI firmware
platforms that are actively deployed in the wild, in a consistent manner.
This small series introduces a simple framework, based on static_keys,
that allows a user to:
- define a quirk and its matching conditions; quirks can match based on:
compatible / Vendor_ID / Sub_Vendor_ID / ImplVersion
from the longest matching sequence down to the shortest.
When the SCMI core stack boots it will enable the matching quirks
depending on the information gathered from the platform via Base
protocol: any NULL/0 match condition is ignored during matching and is
interpreted as ANY, so you can decide to match on a very specific
combination of compatibles and FW versions OR simply on a compatible.
- define a quirk code-block: simply a block of code, meant to play the
magic quirk trick, defined in the proximity of where it will be used
and gated by an implicit quirk static-key associated with the defined
quirk
Patch 1/3 in the series is really unrelated to the Quirk framework
itself: it is a slight variation on a fix posted previously by Sibi around
PERF FastChannels and it is included here for simplicity, since the example
quirk provided later in this series has to be applied exactly where 1/3
applies its modifications.
Patch 2/3 introduces support for SCMI quirks: support is default-n in
Kconfig as of this series. All the quirks found defined are stored in
an hashtable at module initialization time.
Later on, when the SCMI core stack probes and it has retrieved basic info
via Base protocol, all the matching quirks are enabled, which simply means
the related underlying specific quirks static-keys are enabled.
Patch 3/3 introduces an example Quirk for a known problem on a known
platform as reported by Johan, Marc and Sibi, BUT note that the matching
condition in tha patch MUST be properly completed in the patch (I was not
sure what to use...see my comments)
All of this as an RFC since:
- the provided macro-salad to enable quirks definitions is certainly
ugly and can be done better (and checkpatch is already complaining
a bit...)...or maybe I should just get rid of macros
- proper Documentation is missing...I wanted to have some feedback
before babbling out too much non-sense just in case all of this has
to go straight to the bin...
- macthing logic can be simplfied probably
Any feedback and testing is very much welcome.
Thanks,
Cristian
Cristian Marussi (2):
firmware: arm_scmi: Add Quirks framework
[NOT FOR UPSTREAM] firmware: arm_scmi: quirk: Ignore FC bit in
attributes
Sibi Sankar (1):
firmware: arm_scmi: Ensure that the message-id supports fastchannel
drivers/firmware/arm_scmi/Kconfig | 12 ++
drivers/firmware/arm_scmi/Makefile | 1 +
drivers/firmware/arm_scmi/base.c | 14 ++
drivers/firmware/arm_scmi/driver.c | 87 +++++++-----
drivers/firmware/arm_scmi/protocols.h | 2 +
drivers/firmware/arm_scmi/quirks.c | 194 ++++++++++++++++++++++++++
drivers/firmware/arm_scmi/quirks.h | 43 ++++++
7 files changed, 320 insertions(+), 33 deletions(-)
create mode 100644 drivers/firmware/arm_scmi/quirks.c
create mode 100644 drivers/firmware/arm_scmi/quirks.h
--
2.47.0
^ permalink raw reply [flat|nested] 12+ messages in thread
* [RFC PATCH 1/3] firmware: arm_scmi: Ensure that the message-id supports fastchannel
2025-04-01 12:25 [RFC PATCH 0/3] Introduce SCMI Quirks framework Cristian Marussi
@ 2025-04-01 12:25 ` Cristian Marussi
2025-04-03 8:15 ` Johan Hovold
2025-04-01 12:25 ` [RFC PATCH 2/3] firmware: arm_scmi: Add Quirks framework Cristian Marussi
` (2 subsequent siblings)
3 siblings, 1 reply; 12+ messages in thread
From: Cristian Marussi @ 2025-04-01 12:25 UTC (permalink / raw)
To: linux-kernel, linux-arm-kernel, arm-scmi
Cc: sudeep.holla, james.quinlan, f.fainelli, vincent.guittot,
peng.fan, michal.simek, quic_sibis, dan.carpenter, maz, johan,
stable, Johan Hovold, Cristian Marussi
From: Sibi Sankar <quic_sibis@quicinc.com>
Currently the perf and powercap protocol relies on the protocol domain
attributes, which just ensures that one fastchannel per domain, before
instantiating fastchannels for all possible message-ids. Fix this by
ensuring that each message-id supports fastchannel before initialization.
Logs:
scmi: Failed to get FC for protocol 13 [MSG_ID:6 / RES_ID:0] - ret:-95. Using regular messaging.
scmi: Failed to get FC for protocol 13 [MSG_ID:6 / RES_ID:1] - ret:-95. Using regular messaging.
scmi: Failed to get FC for protocol 13 [MSG_ID:6 / RES_ID:2] - ret:-95. Using regular messaging.
CC: stable@vger.kernel.org
Reported-by: Johan Hovold <johan+linaro@kernel.org>
Closes: https://lore.kernel.org/lkml/ZoQjAWse2YxwyRJv@hovoldconsulting.com/
Fixes: 6f9ea4dabd2d ("firmware: arm_scmi: Generalize the fast channel support")
Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
[Cristian: Modified the condition checked to establish support or not]
Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
Since PROTOCOL_MESSAGE_ATTRIBUTES, used to check if message_id is supported,
is a mandatory command, it cannot fail so we must bail-out NOT only if FC was
not supported for that command but also if the query fails as a whole; so the
condition checked for bailing out is modified to:
if (ret || !MSG_SUPPORTS_FASTCHANNEL(attributes)) {
Removed also Tested-by and Reviewed-by tags since I modified the logic.
---
drivers/firmware/arm_scmi/driver.c | 76 +++++++++++++++------------
drivers/firmware/arm_scmi/protocols.h | 2 +
2 files changed, 45 insertions(+), 33 deletions(-)
diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index bf2dc200604e..3855a9791f4a 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -1738,6 +1738,39 @@ static int scmi_common_get_max_msg_size(const struct scmi_protocol_handle *ph)
return info->desc->max_msg_size;
}
+/**
+ * scmi_protocol_msg_check - Check protocol message attributes
+ *
+ * @ph: A reference to the protocol handle.
+ * @message_id: The ID of the message to check.
+ * @attributes: A parameter to optionally return the retrieved message
+ * attributes, in case of Success.
+ *
+ * An helper to check protocol message attributes for a specific protocol
+ * and message pair.
+ *
+ * Return: 0 on SUCCESS
+ */
+static int scmi_protocol_msg_check(const struct scmi_protocol_handle *ph,
+ u32 message_id, u32 *attributes)
+{
+ int ret;
+ struct scmi_xfer *t;
+
+ ret = xfer_get_init(ph, PROTOCOL_MESSAGE_ATTRIBUTES,
+ sizeof(__le32), 0, &t);
+ if (ret)
+ return ret;
+
+ put_unaligned_le32(message_id, t->tx.buf);
+ ret = do_xfer(ph, t);
+ if (!ret && attributes)
+ *attributes = get_unaligned_le32(t->rx.buf);
+ xfer_put(ph, t);
+
+ return ret;
+}
+
/**
* struct scmi_iterator - Iterator descriptor
* @msg: A reference to the message TX buffer; filled by @prepare_message with
@@ -1879,6 +1912,7 @@ scmi_common_fastchannel_init(const struct scmi_protocol_handle *ph,
int ret;
u32 flags;
u64 phys_addr;
+ u32 attributes;
u8 size;
void __iomem *addr;
struct scmi_xfer *t;
@@ -1887,6 +1921,15 @@ scmi_common_fastchannel_init(const struct scmi_protocol_handle *ph,
struct scmi_msg_resp_desc_fc *resp;
const struct scmi_protocol_instance *pi = ph_to_pi(ph);
+ /* Check if the MSG_ID supports fastchannel */
+ ret = scmi_protocol_msg_check(ph, message_id, &attributes);
+ if (ret || !MSG_SUPPORTS_FASTCHANNEL(attributes)) {
+ dev_dbg(ph->dev,
+ "Skip FC init for 0x%02X/%d domain:%d - ret:%d\n",
+ pi->proto->id, message_id, domain, ret);
+ return;
+ }
+
if (!p_addr) {
ret = -EINVAL;
goto err_out;
@@ -2014,39 +2057,6 @@ static void scmi_common_fastchannel_db_ring(struct scmi_fc_db_info *db)
#endif
}
-/**
- * scmi_protocol_msg_check - Check protocol message attributes
- *
- * @ph: A reference to the protocol handle.
- * @message_id: The ID of the message to check.
- * @attributes: A parameter to optionally return the retrieved message
- * attributes, in case of Success.
- *
- * An helper to check protocol message attributes for a specific protocol
- * and message pair.
- *
- * Return: 0 on SUCCESS
- */
-static int scmi_protocol_msg_check(const struct scmi_protocol_handle *ph,
- u32 message_id, u32 *attributes)
-{
- int ret;
- struct scmi_xfer *t;
-
- ret = xfer_get_init(ph, PROTOCOL_MESSAGE_ATTRIBUTES,
- sizeof(__le32), 0, &t);
- if (ret)
- return ret;
-
- put_unaligned_le32(message_id, t->tx.buf);
- ret = do_xfer(ph, t);
- if (!ret && attributes)
- *attributes = get_unaligned_le32(t->rx.buf);
- xfer_put(ph, t);
-
- return ret;
-}
-
static const struct scmi_proto_helpers_ops helpers_ops = {
.extended_name_get = scmi_common_extended_name_get,
.get_max_msg_size = scmi_common_get_max_msg_size,
diff --git a/drivers/firmware/arm_scmi/protocols.h b/drivers/firmware/arm_scmi/protocols.h
index aaee57cdcd55..d62c4469d1fd 100644
--- a/drivers/firmware/arm_scmi/protocols.h
+++ b/drivers/firmware/arm_scmi/protocols.h
@@ -31,6 +31,8 @@
#define SCMI_PROTOCOL_VENDOR_BASE 0x80
+#define MSG_SUPPORTS_FASTCHANNEL(x) ((x) & BIT(0))
+
enum scmi_common_cmd {
PROTOCOL_VERSION = 0x0,
PROTOCOL_ATTRIBUTES = 0x1,
--
2.47.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [RFC PATCH 2/3] firmware: arm_scmi: Add Quirks framework
2025-04-01 12:25 [RFC PATCH 0/3] Introduce SCMI Quirks framework Cristian Marussi
2025-04-01 12:25 ` [RFC PATCH 1/3] firmware: arm_scmi: Ensure that the message-id supports fastchannel Cristian Marussi
@ 2025-04-01 12:25 ` Cristian Marussi
2025-04-01 12:25 ` [RFC PATCH 3/3] [NOT FOR UPSTREAM] firmware: arm_scmi: quirk: Ignore FC bit in attributes Cristian Marussi
2025-04-04 13:22 ` [RFC PATCH 0/3] Introduce SCMI Quirks framework Johan Hovold
3 siblings, 0 replies; 12+ messages in thread
From: Cristian Marussi @ 2025-04-01 12:25 UTC (permalink / raw)
To: linux-kernel, linux-arm-kernel, arm-scmi
Cc: sudeep.holla, james.quinlan, f.fainelli, vincent.guittot,
peng.fan, michal.simek, quic_sibis, dan.carpenter, maz, johan,
Cristian Marussi
Add a common framework to describe SCMI quirks and associate them with a
specific platform or a specific SCMI firmware version.
All the matching SCMI quirks will be enabled when the SCMI core stack
probes and afer all the needed SCMI firmware versioning information was
retrieved using Base protocol.
Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
drivers/firmware/arm_scmi/Kconfig | 12 ++
drivers/firmware/arm_scmi/Makefile | 1 +
drivers/firmware/arm_scmi/base.c | 14 +++
drivers/firmware/arm_scmi/driver.c | 3 +
drivers/firmware/arm_scmi/quirks.c | 191 +++++++++++++++++++++++++++++
drivers/firmware/arm_scmi/quirks.h | 40 ++++++
6 files changed, 261 insertions(+)
create mode 100644 drivers/firmware/arm_scmi/quirks.c
create mode 100644 drivers/firmware/arm_scmi/quirks.h
diff --git a/drivers/firmware/arm_scmi/Kconfig b/drivers/firmware/arm_scmi/Kconfig
index dabd874641d0..ddebba0e2fb0 100644
--- a/drivers/firmware/arm_scmi/Kconfig
+++ b/drivers/firmware/arm_scmi/Kconfig
@@ -69,6 +69,18 @@ config ARM_SCMI_DEBUG_COUNTERS
such useful debug counters. This can be helpful for debugging and
SCMI monitoring.
+config ARM_SCMI_QUIRKS
+ bool "Enable SCMI Quirks framework"
+ depends on JUMP_LABEL
+ help
+ Enables support for SCMI Quirks framework to workaround SCMI platform
+ firmware bugs on system already deployed in the wild.
+
+ The framework allows the definition of platform-specific code quirks
+ that will be associated and enabled only on the desired platforms
+ depending on the SCMI firmware advertised versions and/or machine
+ compatibles.
+
source "drivers/firmware/arm_scmi/transports/Kconfig"
source "drivers/firmware/arm_scmi/vendors/imx/Kconfig"
diff --git a/drivers/firmware/arm_scmi/Makefile b/drivers/firmware/arm_scmi/Makefile
index 9ac81adff567..780cd62b2f78 100644
--- a/drivers/firmware/arm_scmi/Makefile
+++ b/drivers/firmware/arm_scmi/Makefile
@@ -3,6 +3,7 @@ scmi-bus-y = bus.o
scmi-core-objs := $(scmi-bus-y)
scmi-driver-y = driver.o notify.o
+scmi-driver-$(CONFIG_ARM_SCMI_QUIRKS) += quirks.o
scmi-driver-$(CONFIG_ARM_SCMI_RAW_MODE_SUPPORT) += raw_mode.o
scmi-transport-$(CONFIG_ARM_SCMI_HAVE_SHMEM) = shmem.o
scmi-transport-$(CONFIG_ARM_SCMI_HAVE_MSG) += msg.o
diff --git a/drivers/firmware/arm_scmi/base.c b/drivers/firmware/arm_scmi/base.c
index 86b376c50a13..298820196e4e 100644
--- a/drivers/firmware/arm_scmi/base.c
+++ b/drivers/firmware/arm_scmi/base.c
@@ -8,10 +8,12 @@
#define pr_fmt(fmt) "SCMI Notifications BASE - " fmt
#include <linux/module.h>
+#include <linux/of.h>
#include <linux/scmi_protocol.h>
#include "common.h"
#include "notify.h"
+#include "quirks.h"
/* Updated only after ALL the mandatory features for that version are merged */
#define SCMI_PROTOCOL_SUPPORTED_VERSION 0x20001
@@ -376,8 +378,10 @@ static int scmi_base_protocol_init(const struct scmi_protocol_handle *ph)
int id, ret;
u8 *prot_imp;
u32 version;
+ const char *compatible = NULL;
char name[SCMI_SHORT_NAME_MAX_SIZE];
struct device *dev = ph->dev;
+ struct device_node *root;
struct scmi_revision_info *rev = scmi_revision_area_get(ph);
ret = ph->xops->version_get(ph, &version);
@@ -404,6 +408,16 @@ static int scmi_base_protocol_init(const struct scmi_protocol_handle *ph)
scmi_setup_protocol_implemented(ph, prot_imp);
+ root = of_find_node_by_path("/");
+ if (root) {
+ of_property_read_string(root, "compatible", &compatible);
+ of_node_put(root);
+ }
+
+ /* Enable applicable quirks */
+ scmi_quirks_enable(dev, compatible,
+ rev->vendor_id, rev->sub_vendor_id, rev->impl_ver);
+
dev_info(dev, "SCMI Protocol v%d.%d '%s:%s' Firmware version 0x%x\n",
rev->major_ver, rev->minor_ver, rev->vendor_id,
rev->sub_vendor_id, rev->impl_ver);
diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index 3855a9791f4a..4266ef852c48 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -38,6 +38,7 @@
#include "common.h"
#include "notify.h"
+#include "quirks.h"
#include "raw_mode.h"
@@ -3401,6 +3402,8 @@ static struct dentry *scmi_debugfs_init(void)
static int __init scmi_driver_init(void)
{
+ scmi_quirks_initialize();
+
/* Bail out if no SCMI transport was configured */
if (WARN_ON(!IS_ENABLED(CONFIG_ARM_SCMI_HAVE_TRANSPORT)))
return -EINVAL;
diff --git a/drivers/firmware/arm_scmi/quirks.c b/drivers/firmware/arm_scmi/quirks.c
new file mode 100644
index 000000000000..83798bc3b043
--- /dev/null
+++ b/drivers/firmware/arm_scmi/quirks.c
@@ -0,0 +1,191 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * System Control and Management Interface (SCMI) Message Protocol Quirks
+ *
+ * Copyright (C) 2025 ARM Ltd.
+ */
+
+#include <linux/ctype.h>
+#include <linux/device.h>
+#include <linux/export.h>
+#include <linux/hashtable.h>
+#include <linux/static_key.h>
+#include <linux/types.h>
+
+#include "quirks.h"
+
+#define SCMI_QUIRKS_HT_SZ 4
+
+struct scmi_quirk {
+ bool enabled;
+ const char *name;
+ char *compatible;
+ char *vendor;
+ char *sub_vendor_id;
+ u32 impl_ver;
+ struct static_key_false *key;
+ struct hlist_node hash;
+ unsigned int hkey;
+};
+
+#define __DEFINE_SCMI_QUIRK_ENTRY(_qn, _comp, _ven, _sub, _impl) \
+ static struct scmi_quirk scmi_quirk_entry_ ## _qn = { \
+ .name = __stringify(quirk_ ## _qn), \
+ .compatible = _comp, \
+ .vendor = _ven, \
+ .sub_vendor_id = _sub, \
+ .impl_ver = _impl, \
+ .key = &(scmi_quirk_ ## _qn), \
+ }
+
+#define __DECLARE_SCMI_QUIRK_ENTRY(_qn) (&(scmi_quirk_entry_ ## _qn))
+
+/*
+ * Define a quirk by name (_qn) and provide the matching tokens where:
+ *
+ * _comp : compatible string, NULL means any.
+ * _ven : SCMI Vendor ID string, NULL means any.
+ * _sub : SCMI SubVendor ID string, NULL means any.
+ * _impl : SCMI Implementation version, 0 means any.
+ *
+ * This implicitly define a properly named global static-key that will be
+ * used to dynamically enable the quirk at initialization time.
+ *
+ * Note that it is possible to associate multiple quirks to the same
+ * matching pattern, if your firmware quality is really astounding :P
+ */
+#define DEFINE_SCMI_QUIRK(_qn, _comp, _ven, _sub, _impl) \
+ DEFINE_STATIC_KEY_FALSE(scmi_quirk_ ## _qn); \
+ __DEFINE_SCMI_QUIRK_ENTRY(_qn, _comp, _ven, _sub, _impl)
+
+/*
+ * Same as DEFINE_SCMI_QUIRK but EXPORTED: this is meant to address quirks
+ * that possibly reside in code that is included in loadable kernel modules
+ * that needs to be able to access the global static keys at runtime to
+ * determine if enabled or not. (see SCMI_QUIRK to understand usage)
+ */
+#define DEFINE_SCMI_QUIRK_EXPORTED(_qn, _comp, _ven, _sub, _impl) \
+ DEFINE_STATIC_KEY_FALSE(scmi_quirk_ ## _qn); \
+ EXPORT_SYMBOL_GPL(scmi_quirk_ ## _qn); \
+ __DEFINE_SCMI_QUIRK_ENTRY(_qn, _comp, _ven, _sub, _impl)
+
+/* Global Quirks Definitions */
+
+/*
+ * Quirks Pointers Array
+ *
+ * This is filled at compile-time with the list of pointers to all the currently
+ * defined quirks descriptors.
+ */
+static struct scmi_quirk *scmi_quirks_table[] = {
+ NULL
+};
+
+/*
+ * Quirks HashTable
+ *
+ * A run-time populated hashtable containing all the defined quirks descriptors
+ * hashed by matching pattern.
+ */
+static DEFINE_READ_MOSTLY_HASHTABLE(scmi_quirks_ht, SCMI_QUIRKS_HT_SZ);
+
+/* TODO
+ * This is brutally copied from vendor modules matching logic, should be
+ * refactored and unified.
+ */
+static unsigned int
+scmi_quirk_signature(const char *compat, const char *vend, const char *sub_vend,
+ u32 impl)
+{
+ char *signature, *p;
+ unsigned int hash32;
+ unsigned long hash = 0;
+
+ /* vendor_id/sub_vendor_id guaranteed <= SCMI_SHORT_NAME_MAX_SIZE */
+ signature = kasprintf(GFP_KERNEL, "%s|%s|%s|0x%08X",
+ compat ?: "", vend ?: "", sub_vend ?: "", impl);
+ if (!signature)
+ return 0;
+
+ pr_debug("SCMI Quirk Signature >>>%s<<<\n", signature);
+
+ p = signature;
+ while (*p)
+ hash = partial_name_hash(tolower(*p++), hash);
+ hash32 = end_name_hash(hash);
+
+ kfree(signature);
+
+ return hash32;
+}
+
+void scmi_quirks_initialize(void)
+{
+ struct scmi_quirk *quirk;
+ int i;
+
+ for (i = 0, quirk = scmi_quirks_table[0]; quirk;
+ i++, quirk = scmi_quirks_table[i]) {
+ quirk->hkey = scmi_quirk_signature(quirk->compatible,
+ quirk->vendor,
+ quirk->sub_vendor_id,
+ quirk->impl_ver);
+
+ hash_add(scmi_quirks_ht, &quirk->hash, quirk->hkey);
+
+ pr_debug("Registered SCMI [%s] -- %p - Key [0x%08X] - %s/%s/%s/0x%08X\n",
+ quirk->name, quirk, quirk->hkey, quirk->compatible,
+ quirk->vendor, quirk->sub_vendor_id, quirk->impl_ver);
+ }
+
+ pr_debug("SCMI Quirks initialized\n");
+}
+
+void scmi_quirks_enable(struct device *dev, const char *compat,
+ const char *vend, const char *subv, const u32 impl)
+{
+ dev_info(dev, "Looking for quirks matching: %s/%s/%s/0x%08X\n",
+ compat, vend, subv, impl);
+
+ /*
+ * TODO Lookup logic is cumbersome/over-engineered and can be simplified
+ *
+ * Lookup into scmi_quirks_ht using 2 loops: with/without compatible
+ */
+ for (int k = 1; k >= 0 ; k--) {
+ const char *compat_sel = k > 0 ? compat : NULL;
+
+ for (int i = 4; i > 0; i--) {
+ struct scmi_quirk *quirk;
+ unsigned int hkey;
+ int j = 0;
+
+ if (!compat_sel && i <= 1)
+ break;
+
+ hkey = scmi_quirk_signature(compat_sel,
+ i > 1 ? vend : NULL,
+ i > 2 ? subv : NULL,
+ i > 3 ? impl : 0);
+
+ /*
+ * Note that there could be multiple matches so we
+ * will enable multiple quirk part of an hash collision
+ * domain...BUT we cannot assume that ALL quirks on the
+ * same collision domain are a full match.
+ */
+ hash_for_each_possible(scmi_quirks_ht, quirk, hash, hkey) {
+ if (quirk->enabled || quirk->hkey != hkey)
+ continue;
+
+ dev_info(dev,
+ "[%d] Enabling [%s] - %s/%s/%s/0x%08X\n",
+ j++, quirk->name, quirk->compatible, quirk->vendor,
+ quirk->sub_vendor_id, quirk->impl_ver);
+
+ static_branch_enable(quirk->key);
+ quirk->enabled = true;
+ }
+ }
+ }
+}
diff --git a/drivers/firmware/arm_scmi/quirks.h b/drivers/firmware/arm_scmi/quirks.h
new file mode 100644
index 000000000000..0f1a14b13ba5
--- /dev/null
+++ b/drivers/firmware/arm_scmi/quirks.h
@@ -0,0 +1,40 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * System Control and Management Interface (SCMI) Message Protocol Quirks
+ *
+ * Copyright (C) 2025 ARM Ltd.
+ */
+#ifndef _SCMI_QUIRKS_H
+#define _SCMI_QUIRKS_H
+
+#include <linux/static_key.h>
+#include <linux/types.h>
+
+#ifdef CONFIG_ARM_SCMI_QUIRKS
+
+#define DECLARE_SCMI_QUIRK(_qn) \
+ DECLARE_STATIC_KEY_FALSE(scmi_quirk_ ## _qn)
+
+#define SCMI_QUIRK(_qn, _blk) \
+ do { \
+ if (static_branch_unlikely(&(scmi_quirk_ ## _qn))) \
+ (_blk); \
+ } while (0)
+
+void scmi_quirks_initialize(void);
+void scmi_quirks_enable(struct device *dev, const char *compat,
+ const char *vend, const char *subv, const u32 impl);
+
+#else
+
+#define DECLARE_SCMI_QUIRK(_qn)
+#define SCMI_QUIRK(_qn, _blk)
+
+static inline void scmi_quirks_initialize(void) { }
+static inline void scmi_quirks_enable(struct device *dev, const char *compat,
+ const char *vend, const char *sub_vend,
+ const u32 impl) { }
+
+#endif /* CONFIG_ARM_SCMI_QUIRKS */
+
+#endif /* _SCMI_QUIRKS_H */
--
2.47.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [RFC PATCH 3/3] [NOT FOR UPSTREAM] firmware: arm_scmi: quirk: Ignore FC bit in attributes
2025-04-01 12:25 [RFC PATCH 0/3] Introduce SCMI Quirks framework Cristian Marussi
2025-04-01 12:25 ` [RFC PATCH 1/3] firmware: arm_scmi: Ensure that the message-id supports fastchannel Cristian Marussi
2025-04-01 12:25 ` [RFC PATCH 2/3] firmware: arm_scmi: Add Quirks framework Cristian Marussi
@ 2025-04-01 12:25 ` Cristian Marussi
2025-04-03 8:25 ` Johan Hovold
2025-04-04 13:22 ` [RFC PATCH 0/3] Introduce SCMI Quirks framework Johan Hovold
3 siblings, 1 reply; 12+ messages in thread
From: Cristian Marussi @ 2025-04-01 12:25 UTC (permalink / raw)
To: linux-kernel, linux-arm-kernel, arm-scmi
Cc: sudeep.holla, james.quinlan, f.fainelli, vincent.guittot,
peng.fan, michal.simek, quic_sibis, dan.carpenter, maz, johan,
Cristian Marussi
Some platform misreported the support of FastChannel when queried: ignore
that bit on selected platforms.
Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
Match features has to be set-up properly before upstreaming this.
Ideally the out-of-spec firmware should be matched with a quirk matching
pattern based on Vendor/SubVendor/ImplVersion....but it is NOT clear if the
platform at hand will ship with future fixed firmwares where the ImplVersion
field is properly handled.
If we cannot be sure about that, we should fallback to a compatible match.
---
drivers/firmware/arm_scmi/driver.c | 8 ++++++++
drivers/firmware/arm_scmi/quirks.c | 3 +++
drivers/firmware/arm_scmi/quirks.h | 3 +++
3 files changed, 14 insertions(+)
diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index 4266ef852c48..212456305bc9 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -1904,6 +1904,13 @@ struct scmi_msg_resp_desc_fc {
__le32 db_preserve_hmask;
};
+#define QUIRK_PERF_FC_FORCE \
+ ({ \
+ if (pi->proto->id == SCMI_PROTOCOL_PERF || \
+ message_id == 0x5 /* PERF_LEVEL_GET */) \
+ attributes |= BIT(0); \
+ })
+
static void
scmi_common_fastchannel_init(const struct scmi_protocol_handle *ph,
u8 describe_id, u32 message_id, u32 valid_size,
@@ -1924,6 +1931,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);
+ SCMI_QUIRK(perf_level_get_fc_force, QUIRK_PERF_FC_FORCE);
if (ret || !MSG_SUPPORTS_FASTCHANNEL(attributes)) {
dev_dbg(ph->dev,
"Skip FC init for 0x%02X/%d domain:%d - ret:%d\n",
diff --git a/drivers/firmware/arm_scmi/quirks.c b/drivers/firmware/arm_scmi/quirks.c
index 83798bc3b043..78d51bd0e5b5 100644
--- a/drivers/firmware/arm_scmi/quirks.c
+++ b/drivers/firmware/arm_scmi/quirks.c
@@ -70,6 +70,8 @@ struct scmi_quirk {
__DEFINE_SCMI_QUIRK_ENTRY(_qn, _comp, _ven, _sub, _impl)
/* Global Quirks Definitions */
+DEFINE_SCMI_QUIRK(perf_level_get_fc_force,
+ "your-bad-compatible", NULL, NULL, 0x0);
/*
* Quirks Pointers Array
@@ -78,6 +80,7 @@ struct scmi_quirk {
* defined quirks descriptors.
*/
static struct scmi_quirk *scmi_quirks_table[] = {
+ __DECLARE_SCMI_QUIRK_ENTRY(perf_level_get_fc_force),
NULL
};
diff --git a/drivers/firmware/arm_scmi/quirks.h b/drivers/firmware/arm_scmi/quirks.h
index 0f1a14b13ba5..3968eba375cf 100644
--- a/drivers/firmware/arm_scmi/quirks.h
+++ b/drivers/firmware/arm_scmi/quirks.h
@@ -37,4 +37,7 @@ static inline void scmi_quirks_enable(struct device *dev, const char *compat,
#endif /* CONFIG_ARM_SCMI_QUIRKS */
+/* Quirk delarations */
+DECLARE_SCMI_QUIRK(perf_level_get_fc_force);
+
#endif /* _SCMI_QUIRKS_H */
--
2.47.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [RFC PATCH 1/3] firmware: arm_scmi: Ensure that the message-id supports fastchannel
2025-04-01 12:25 ` [RFC PATCH 1/3] firmware: arm_scmi: Ensure that the message-id supports fastchannel Cristian Marussi
@ 2025-04-03 8:15 ` Johan Hovold
0 siblings, 0 replies; 12+ messages in thread
From: Johan Hovold @ 2025-04-03 8:15 UTC (permalink / raw)
To: Cristian Marussi
Cc: linux-kernel, linux-arm-kernel, arm-scmi, sudeep.holla,
james.quinlan, f.fainelli, vincent.guittot, peng.fan,
michal.simek, quic_sibis, dan.carpenter, maz, stable,
Johan Hovold
On Tue, Apr 01, 2025 at 01:25:43PM +0100, Cristian Marussi wrote:
> From: Sibi Sankar <quic_sibis@quicinc.com>
>
> Currently the perf and powercap protocol relies on the protocol domain
> attributes, which just ensures that one fastchannel per domain, before
> instantiating fastchannels for all possible message-ids. Fix this by
> ensuring that each message-id supports fastchannel before initialization.
>
> Logs:
> scmi: Failed to get FC for protocol 13 [MSG_ID:6 / RES_ID:0] - ret:-95. Using regular messaging.
> scmi: Failed to get FC for protocol 13 [MSG_ID:6 / RES_ID:1] - ret:-95. Using regular messaging.
> scmi: Failed to get FC for protocol 13 [MSG_ID:6 / RES_ID:2] - ret:-95. Using regular messaging.
>
> CC: stable@vger.kernel.org
> Reported-by: Johan Hovold <johan+linaro@kernel.org>
> Closes: https://lore.kernel.org/lkml/ZoQjAWse2YxwyRJv@hovoldconsulting.com/
> Fixes: 6f9ea4dabd2d ("firmware: arm_scmi: Generalize the fast channel support")
> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
> [Cristian: Modified the condition checked to establish support or not]
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> ---
> Since PROTOCOL_MESSAGE_ATTRIBUTES, used to check if message_id is supported,
> is a mandatory command, it cannot fail so we must bail-out NOT only if FC was
> not supported for that command but also if the query fails as a whole; so the
> condition checked for bailing out is modified to:
>
> if (ret || !MSG_SUPPORTS_FASTCHANNEL(attributes)) {
>
> Removed also Tested-by and Reviewed-by tags since I modified the logic.
This still works as intended:
Reviewed-by: Johan Hovold <johan+linaro@kernel.org>
Tested-by: Johan Hovold <johan+linaro@kernel.org>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC PATCH 3/3] [NOT FOR UPSTREAM] firmware: arm_scmi: quirk: Ignore FC bit in attributes
2025-04-01 12:25 ` [RFC PATCH 3/3] [NOT FOR UPSTREAM] firmware: arm_scmi: quirk: Ignore FC bit in attributes Cristian Marussi
@ 2025-04-03 8:25 ` Johan Hovold
2025-04-03 9:08 ` Cristian Marussi
0 siblings, 1 reply; 12+ messages in thread
From: Johan Hovold @ 2025-04-03 8:25 UTC (permalink / raw)
To: Cristian Marussi
Cc: linux-kernel, linux-arm-kernel, arm-scmi, sudeep.holla,
james.quinlan, f.fainelli, vincent.guittot, peng.fan,
michal.simek, quic_sibis, dan.carpenter, maz
On Tue, Apr 01, 2025 at 01:25:45PM +0100, Cristian Marussi wrote:
> Some platform misreported the support of FastChannel when queried: ignore
> that bit on selected platforms.
>
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> ---
> Match features has to be set-up properly before upstreaming this.
> Ideally the out-of-spec firmware should be matched with a quirk matching
> pattern based on Vendor/SubVendor/ImplVersion....but it is NOT clear if the
> platform at hand will ship with future fixed firmwares where the ImplVersion
> field is properly handled.
> If we cannot be sure about that, we should fallback to a compatible match.
> ---
> drivers/firmware/arm_scmi/driver.c | 8 ++++++++
> drivers/firmware/arm_scmi/quirks.c | 3 +++
> drivers/firmware/arm_scmi/quirks.h | 3 +++
> 3 files changed, 14 insertions(+)
>
> diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> index 4266ef852c48..212456305bc9 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c
> @@ -1904,6 +1904,13 @@ struct scmi_msg_resp_desc_fc {
> __le32 db_preserve_hmask;
> };
>
> +#define QUIRK_PERF_FC_FORCE \
> + ({ \
> + if (pi->proto->id == SCMI_PROTOCOL_PERF || \
> + message_id == 0x5 /* PERF_LEVEL_GET */) \
This should be logical AND and PERF_LEVEL_GET is 0x8 (currently
fastchannel is enabled for all PERF messages).
> + attributes |= BIT(0); \
> + })
> +
> static void
> scmi_common_fastchannel_init(const struct scmi_protocol_handle *ph,
> u8 describe_id, u32 message_id, u32 valid_size,
> @@ -1924,6 +1931,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);
> + SCMI_QUIRK(perf_level_get_fc_force, QUIRK_PERF_FC_FORCE);
This is cool and I assume can be used to minimise overhead in hot paths.
Perhaps you can have concerns about readability and remembering to
update the quirk implementation if the code here changes.
Does it even get compile tested if SCMI_QUIRKS is disabled?
> if (ret || !MSG_SUPPORTS_FASTCHANNEL(attributes)) {
> dev_dbg(ph->dev,
> "Skip FC init for 0x%02X/%d domain:%d - ret:%d\n",
> diff --git a/drivers/firmware/arm_scmi/quirks.c b/drivers/firmware/arm_scmi/quirks.c
> index 83798bc3b043..78d51bd0e5b5 100644
> --- a/drivers/firmware/arm_scmi/quirks.c
> +++ b/drivers/firmware/arm_scmi/quirks.c
> @@ -70,6 +70,8 @@ struct scmi_quirk {
> __DEFINE_SCMI_QUIRK_ENTRY(_qn, _comp, _ven, _sub, _impl)
>
> /* Global Quirks Definitions */
> +DEFINE_SCMI_QUIRK(perf_level_get_fc_force,
> + "your-bad-compatible", NULL, NULL, 0x0);
At first I tried matching on the SoC (fallback) compatible without
success until I noticed you need to put the primary machine compatible
here. For the SoC at hand, that would mean adding 10 or so entries since
all current commercial devices would be affected by this.
Matching on vendor and protocol works.
> /*
> * Quirks Pointers Array
> @@ -78,6 +80,7 @@ struct scmi_quirk {
> * defined quirks descriptors.
> */
> static struct scmi_quirk *scmi_quirks_table[] = {
> + __DECLARE_SCMI_QUIRK_ENTRY(perf_level_get_fc_force),
> NULL
> };
Johan
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC PATCH 3/3] [NOT FOR UPSTREAM] firmware: arm_scmi: quirk: Ignore FC bit in attributes
2025-04-03 8:25 ` Johan Hovold
@ 2025-04-03 9:08 ` Cristian Marussi
2025-04-03 10:05 ` Johan Hovold
0 siblings, 1 reply; 12+ messages in thread
From: Cristian Marussi @ 2025-04-03 9:08 UTC (permalink / raw)
To: Johan Hovold
Cc: Cristian Marussi, linux-kernel, linux-arm-kernel, arm-scmi,
sudeep.holla, james.quinlan, f.fainelli, vincent.guittot,
peng.fan, michal.simek, quic_sibis, dan.carpenter, maz
On Thu, Apr 03, 2025 at 10:25:21AM +0200, Johan Hovold wrote:
> On Tue, Apr 01, 2025 at 01:25:45PM +0100, Cristian Marussi wrote:
> > Some platform misreported the support of FastChannel when queried: ignore
> > that bit on selected platforms.
> >
Hi Johan,
thanks for the review.
> > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> > ---
> > Match features has to be set-up properly before upstreaming this.
> > Ideally the out-of-spec firmware should be matched with a quirk matching
> > pattern based on Vendor/SubVendor/ImplVersion....but it is NOT clear if the
> > platform at hand will ship with future fixed firmwares where the ImplVersion
> > field is properly handled.
> > If we cannot be sure about that, we should fallback to a compatible match.
> > ---
> > drivers/firmware/arm_scmi/driver.c | 8 ++++++++
> > drivers/firmware/arm_scmi/quirks.c | 3 +++
> > drivers/firmware/arm_scmi/quirks.h | 3 +++
> > 3 files changed, 14 insertions(+)
> >
> > diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> > index 4266ef852c48..212456305bc9 100644
> > --- a/drivers/firmware/arm_scmi/driver.c
> > +++ b/drivers/firmware/arm_scmi/driver.c
> > @@ -1904,6 +1904,13 @@ struct scmi_msg_resp_desc_fc {
> > __le32 db_preserve_hmask;
> > };
> >
> > +#define QUIRK_PERF_FC_FORCE \
> > + ({ \
> > + if (pi->proto->id == SCMI_PROTOCOL_PERF || \
> > + message_id == 0x5 /* PERF_LEVEL_GET */) \
>
> This should be logical AND and PERF_LEVEL_GET is 0x8 (currently
> fastchannel is enabled for all PERF messages).
...right...not sure how I botched this condition completely...my bad...
(even the comment is wrong :P...)
...I experimented with multiple version of this...so I suppose this is why...
...I will post a fixed V1 once I hacve a bit more feedback on the list...
>
> > + attributes |= BIT(0); \
> > + })
> > +
> > static void
> > scmi_common_fastchannel_init(const struct scmi_protocol_handle *ph,
> > u8 describe_id, u32 message_id, u32 valid_size,
> > @@ -1924,6 +1931,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);
> > + SCMI_QUIRK(perf_level_get_fc_force, QUIRK_PERF_FC_FORCE);
>
> This is cool and I assume can be used to minimise overhead in hot paths.
> Perhaps you can have concerns about readability and remembering to
> update the quirk implementation if the code here changes.
My main aim here was to be able to define the quirk code as much as
possible in the proximity of where it is used...so that is clear what it
does and dont get lost in some general common table....and the macro was
a way to uniform the treatment of the static keys...
...but I am still not sure if all of these macros just degrade visibility
and we could get rid of them...would be really cool to somehow break the
build if the code "sorrounding" the SCMI_QUIRK changes and you dont update
(somehow) the quirk too...so as to be sure that the quirk is taking care of
and maintained...but I doubt that is feasible, because, really, how do you
even deternine which code changes are in proximity enough to the quirk to
justify a break...same block ? same functions ? you cannot really know
semantically where some changes can impact this part of the code...
..I supppose reviews and testing is the key and the only possible answer
to this..
>
> Does it even get compile tested if SCMI_QUIRKS is disabled?
It evaluates to nothing when CONFIG_ARM_SCMI_QUIRKS is disabled...
...so maybe I could add a Kconfig dep on COMPILE_TEST ....if this is what
you mean..
>
> > if (ret || !MSG_SUPPORTS_FASTCHANNEL(attributes)) {
> > dev_dbg(ph->dev,
> > "Skip FC init for 0x%02X/%d domain:%d - ret:%d\n",
> > diff --git a/drivers/firmware/arm_scmi/quirks.c b/drivers/firmware/arm_scmi/quirks.c
> > index 83798bc3b043..78d51bd0e5b5 100644
> > --- a/drivers/firmware/arm_scmi/quirks.c
> > +++ b/drivers/firmware/arm_scmi/quirks.c
> > @@ -70,6 +70,8 @@ struct scmi_quirk {
> > __DEFINE_SCMI_QUIRK_ENTRY(_qn, _comp, _ven, _sub, _impl)
> >
> > /* Global Quirks Definitions */
> > +DEFINE_SCMI_QUIRK(perf_level_get_fc_force,
> > + "your-bad-compatible", NULL, NULL, 0x0);
>
> At first I tried matching on the SoC (fallback) compatible without
> success until I noticed you need to put the primary machine compatible
> here. For the SoC at hand, that would mean adding 10 or so entries since
> all current commercial devices would be affected by this.
>
Ah right...I tested on a number of combinations BUT assumed only one
compatible was to be found...you can potentially add dozens of this
definitions for a number of platforms associating the same quirk to all
of them and let the match logic enabling only the proper on...BUT this
clearly does NOT scale indeed and you will have to endlessly add new
platform if fw does NOT get fixed ever...
> Matching on vendor and protocol works.
>
That is abosutely the preferred way, BUT the match should be on
Vendor/SubVendor/ImplVersion ... if the platform properly uses
ImplementationVersion to differentiate between firmware builds...
...if not you will end up applying the quirk on ANY current and future
FW from this Vendor...maybe not an issue in this case...BUT they should
seriously thinking about using ImplementationVersion properly in their
future FW releases...especially if, as of now, no new fixed FW release
has ever been released...
> > /*
> > * Quirks Pointers Array
> > @@ -78,6 +80,7 @@ struct scmi_quirk {
> > * defined quirks descriptors.
> > */
> > static struct scmi_quirk *scmi_quirks_table[] = {
> > + __DECLARE_SCMI_QUIRK_ENTRY(perf_level_get_fc_force),
> > NULL
> > };
>
> Johan
Thanks for having had a look !
Cristian
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC PATCH 3/3] [NOT FOR UPSTREAM] firmware: arm_scmi: quirk: Ignore FC bit in attributes
2025-04-03 9:08 ` Cristian Marussi
@ 2025-04-03 10:05 ` Johan Hovold
2025-04-04 13:33 ` Cristian Marussi
0 siblings, 1 reply; 12+ messages in thread
From: Johan Hovold @ 2025-04-03 10:05 UTC (permalink / raw)
To: Cristian Marussi
Cc: linux-kernel, linux-arm-kernel, arm-scmi, sudeep.holla,
james.quinlan, f.fainelli, vincent.guittot, peng.fan,
michal.simek, quic_sibis, dan.carpenter, maz
On Thu, Apr 03, 2025 at 10:08:41AM +0100, Cristian Marussi wrote:
> On Thu, Apr 03, 2025 at 10:25:21AM +0200, Johan Hovold wrote:
> > On Tue, Apr 01, 2025 at 01:25:45PM +0100, Cristian Marussi wrote:
> > > +#define QUIRK_PERF_FC_FORCE \
> > > + ({ \
> > > + if (pi->proto->id == SCMI_PROTOCOL_PERF || \
> > > + message_id == 0x5 /* PERF_LEVEL_GET */) \
> >
> > This should be logical AND and PERF_LEVEL_GET is 0x8 (currently
> > fastchannel is enabled for all PERF messages).
>
> ...right...not sure how I botched this condition completely...my bad...
> (even the comment is wrong :P...)
The PERF_LEVEL_GET comment? That one is correct, right? :)
> > > + attributes |= BIT(0); \
> > > + })
> > > +
> > > static void
> > > scmi_common_fastchannel_init(const struct scmi_protocol_handle *ph,
> > > u8 describe_id, u32 message_id, u32 valid_size,
> > > @@ -1924,6 +1931,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);
> > > + SCMI_QUIRK(perf_level_get_fc_force, QUIRK_PERF_FC_FORCE);
> >
> > This is cool and I assume can be used to minimise overhead in hot paths.
> > Perhaps you can have concerns about readability and remembering to
> > update the quirk implementation if the code here changes.
>
> My main aim here was to be able to define the quirk code as much as
> possible in the proximity of where it is used...so that is clear what it
> does and dont get lost in some general common table....and the macro was
> a way to uniform the treatment of the static keys...
>
> ...but I am still not sure if all of these macros just degrade visibility
> and we could get rid of them...would be really cool to somehow break the
> build if the code "sorrounding" the SCMI_QUIRK changes and you dont update
> (somehow) the quirk too...so as to be sure that the quirk is taking care of
> and maintained...but I doubt that is feasible, because, really, how do you
> even deternine which code changes are in proximity enough to the quirk to
> justify a break...same block ? same functions ? you cannot really know
> semantically where some changes can impact this part of the code...
> ..I supppose reviews and testing is the key and the only possible answer
> to this..
Yeah, it goes both ways. Getting the quirk implementation out of the way
makes it easier to follow the normal flow, but also makes it a bit
harder to review the quirk. Your implementation may be a good trade-off.
> > Does it even get compile tested if SCMI_QUIRKS is disabled?
>
> It evaluates to nothing when CONFIG_ARM_SCMI_QUIRKS is disabled...
> ...so maybe I could add a Kconfig dep on COMPILE_TEST ....if this is what
> you mean..
Perhaps there's some way to get the quirk code always compiled but
discarded when CONFIG_ARM_SCMI_QUIRKS is disabled (e.g. by using
IS_ENABLED() in the macro)?
CONFIG_ARM_SCMI_QUIRKS may also need to be enabled by default as it can
be hard to track down random crashes to a missing quirk.
> > > /* Global Quirks Definitions */
> > > +DEFINE_SCMI_QUIRK(perf_level_get_fc_force,
> > > + "your-bad-compatible", NULL, NULL, 0x0);
> >
> > At first I tried matching on the SoC (fallback) compatible without
> > success until I noticed you need to put the primary machine compatible
> > here. For the SoC at hand, that would mean adding 10 or so entries since
> > all current commercial devices would be affected by this.
> >
>
> Ah right...I tested on a number of combinations BUT assumed only one
> compatible was to be found...you can potentially add dozens of this
> definitions for a number of platforms associating the same quirk to all
> of them and let the match logic enabling only the proper on...BUT this
> clearly does NOT scale indeed and you will have to endlessly add new
> platform if fw does NOT get fixed ever...
>
> > Matching on vendor and protocol works.
> >
>
> That is abosutely the preferred way, BUT the match should be on
> Vendor/SubVendor/ImplVersion ... if the platform properly uses
> ImplementationVersion to differentiate between firmware builds...
We don't seem to have a subvendor here and if IIUC the version has not
been bumped (yet) after fixing the FC issue.
> ...if not you will end up applying the quirk on ANY current and future
> FW from this Vendor...maybe not an issue in this case...BUT they should
> seriously thinking about using ImplementationVersion properly in their
> future FW releases...especially if, as of now, no new fixed FW release
> has ever been released...
Right, in this case it would probably be OK.
But what if the version is bumped for some other reason (e.g. before a
bug has been identified)? Then you'd currently need an entry for each
affected revision or does the implementation assume it applies to
anything <= ImplVersion? Do we want to add support for version ranges?
Johan
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC PATCH 0/3] Introduce SCMI Quirks framework
2025-04-01 12:25 [RFC PATCH 0/3] Introduce SCMI Quirks framework Cristian Marussi
` (2 preceding siblings ...)
2025-04-01 12:25 ` [RFC PATCH 3/3] [NOT FOR UPSTREAM] firmware: arm_scmi: quirk: Ignore FC bit in attributes Cristian Marussi
@ 2025-04-04 13:22 ` Johan Hovold
2025-04-04 13:30 ` Cristian Marussi
2025-04-15 13:55 ` Cristian Marussi
3 siblings, 2 replies; 12+ messages in thread
From: Johan Hovold @ 2025-04-04 13:22 UTC (permalink / raw)
To: Cristian Marussi
Cc: linux-kernel, linux-arm-kernel, arm-scmi, sudeep.holla,
james.quinlan, f.fainelli, vincent.guittot, peng.fan,
michal.simek, quic_sibis, dan.carpenter, maz
Hi Cristian,
On Tue, Apr 01, 2025 at 01:25:42PM +0100, Cristian Marussi wrote:
> with the increasing adoption of SCMI across arm64 ecosystems, we have to
> start considering how to deal and take care of out-of-spec SCMI firmware
> platforms that are actively deployed in the wild, in a consistent manner.
>
> This small series introduces a simple framework, based on static_keys,
> that allows a user to:
>
> - define a quirk and its matching conditions; quirks can match based on:
>
> compatible / Vendor_ID / Sub_Vendor_ID / ImplVersion
>
> from the longest matching sequence down to the shortest.
>
> When the SCMI core stack boots it will enable the matching quirks
> depending on the information gathered from the platform via Base
> protocol: any NULL/0 match condition is ignored during matching and is
> interpreted as ANY, so you can decide to match on a very specific
> combination of compatibles and FW versions OR simply on a compatible.
>
> - define a quirk code-block: simply a block of code, meant to play the
> magic quirk trick, defined in the proximity of where it will be used
> and gated by an implicit quirk static-key associated with the defined
> quirk
> Any feedback and testing is very much welcome.
I'm hitting the below lockdep splat when running with this series and
the FC quirk enabled.
Johan
[ 8.399581] ======================================================
[ 8.399582] WARNING: possible circular locking dependency detected
[ 8.399583] 6.14.0 #103 Not tainted
[ 8.399584] ------------------------------------------------------
[ 8.399584] kworker/u49:5/165 is trying to acquire lock:
[ 8.399586] ffff65d004d36320 (&info->protocols_mtx){+.+.}-{4:4}, at: scmi_get_protocol_instance+0x4c/0x5c0
[ 8.399596]
but task is already holding lock:
[ 8.399596] ffff65d00a895348 (&ni->pending_mtx){+.+.}-{4:4}, at: __scmi_event_handler_get_ops+0x70/0x47c
[ 8.399600]
which lock already depends on the new lock.
[ 8.399601]
the existing dependency chain (in reverse order) is:
[ 8.399601]
-> #4 (&ni->pending_mtx){+.+.}-{4:4}:
[ 8.399603] __mutex_lock+0xa8/0x514
[ 8.399606] mutex_lock_nested+0x24/0x30
[ 8.399608] __scmi_event_handler_get_ops+0x70/0x47c
[ 8.399610] scmi_notifier_register+0x58/0x29c
[ 8.399612] scmi_cpufreq_init+0x30c/0x4d0
[ 8.399615] cpufreq_online+0x610/0xb58
[ 8.399616] cpufreq_add_dev+0xcc/0xe4
[ 8.399618] subsys_interface_register+0xfc/0x118
[ 8.399622] cpufreq_register_driver+0x178/0x2ec
[ 8.399623] scmi_cpufreq_probe+0x88/0x11c
[ 8.399625] scmi_dev_probe+0x28/0x3c
[ 8.399626] really_probe+0xc0/0x38c
[ 8.399627] __driver_probe_device+0x7c/0x160
[ 8.399629] driver_probe_device+0x40/0x110
[ 8.399630] __device_attach_driver+0xbc/0x158
[ 8.399631] bus_for_each_drv+0x84/0xe0
[ 8.399633] __device_attach+0xa8/0x1d4
[ 8.399634] device_initial_probe+0x14/0x20
[ 8.399636] bus_probe_device+0xb0/0xb4
[ 8.399637] device_add+0x57c/0x784
[ 8.399639] device_register+0x20/0x30
[ 8.399641] __scmi_device_create.part.0+0xf0/0x224
[ 8.399642] scmi_device_create+0x184/0x1c8
[ 8.399644] scmi_create_protocol_devices+0x50/0xb4
[ 8.399646] scmi_probe+0x854/0x91c
[ 8.399647] platform_probe+0x68/0xd8
[ 8.399649] really_probe+0xc0/0x38c
[ 8.399650] __driver_probe_device+0x7c/0x160
[ 8.399651] driver_probe_device+0x40/0x110
[ 8.399653] __device_attach_driver+0xbc/0x158
[ 8.399654] bus_for_each_drv+0x84/0xe0
[ 8.399656] __device_attach+0xa8/0x1d4
[ 8.399657] device_initial_probe+0x14/0x20
[ 8.399658] bus_probe_device+0xb0/0xb4
[ 8.399659] device_add+0x57c/0x784
[ 8.399661] platform_device_add+0x110/0x274
[ 8.399663] scmi_mailbox_probe+0xcc/0x110
[ 8.399664] platform_probe+0x68/0xd8
[ 8.399666] really_probe+0xc0/0x38c
[ 8.399667] __driver_probe_device+0x7c/0x160
[ 8.399668] driver_probe_device+0x40/0x110
[ 8.399669] __device_attach_driver+0xbc/0x158
[ 8.399671] bus_for_each_drv+0x84/0xe0
[ 8.399673] __device_attach+0xa8/0x1d4
[ 8.399674] device_initial_probe+0x14/0x20
[ 8.399675] bus_probe_device+0xb0/0xb4
[ 8.399676] deferred_probe_work_func+0xa0/0xf4
[ 8.399677] process_one_work+0x20c/0x610
[ 8.399682] worker_thread+0x23c/0x378
[ 8.399684] kthread+0x13c/0x20c
[ 8.399685] ret_from_fork+0x10/0x20
[ 8.399687]
-> #3 (&policy->rwsem){+.+.}-{4:4}:
[ 8.399689] down_write+0x50/0xe8
[ 8.399690] cpufreq_online+0x5cc/0xb58
[ 8.399692] cpufreq_add_dev+0xcc/0xe4
[ 8.399693] subsys_interface_register+0xfc/0x118
[ 8.399695] cpufreq_register_driver+0x178/0x2ec
[ 8.399697] scmi_cpufreq_probe+0x88/0x11c
[ 8.399699] scmi_dev_probe+0x28/0x3c
[ 8.399700] really_probe+0xc0/0x38c
[ 8.399701] __driver_probe_device+0x7c/0x160
[ 8.399702] driver_probe_device+0x40/0x110
[ 8.399704] __device_attach_driver+0xbc/0x158
[ 8.399705] bus_for_each_drv+0x84/0xe0
[ 8.399707] __device_attach+0xa8/0x1d4
[ 8.399708] device_initial_probe+0x14/0x20
[ 8.399709] bus_probe_device+0xb0/0xb4
[ 8.399710] device_add+0x57c/0x784
[ 8.399712] device_register+0x20/0x30
[ 8.399714] __scmi_device_create.part.0+0xf0/0x224
[ 8.877044] scmi_device_create+0x184/0x1c8
[ 8.882444] scmi_create_protocol_devices+0x50/0xb4
[ 8.888617] scmi_probe+0x854/0x91c
[ 8.893302] platform_probe+0x68/0xd8
[ 8.898172] really_probe+0xc0/0x38c
[ 8.902943] __driver_probe_device+0x7c/0x160
[ 8.908525] driver_probe_device+0x40/0x110
[ 8.913933] __device_attach_driver+0xbc/0x158
[ 8.919608] bus_for_each_drv+0x84/0xe0
[ 8.924657] __device_attach+0xa8/0x1d4
[ 8.929708] device_initial_probe+0x14/0x20
[ 8.935124] bus_probe_device+0xb0/0xb4
[ 8.940178] device_add+0x57c/0x784
[ 8.944870] platform_device_add+0x110/0x274
[ 8.950370] scmi_mailbox_probe+0xcc/0x110
[ 8.955699] platform_probe+0x68/0xd8
[ 8.960582] really_probe+0xc0/0x38c
[ 8.965375] __driver_probe_device+0x7c/0x160
[ 8.970971] driver_probe_device+0x40/0x110
[ 8.976390] __device_attach_driver+0xbc/0x158
[ 8.982076] bus_for_each_drv+0x84/0xe0
[ 8.987148] __device_attach+0xa8/0x1d4
[ 8.992212] device_initial_probe+0x14/0x20
[ 8.997637] bus_probe_device+0xb0/0xb4
[ 9.002699] deferred_probe_work_func+0xa0/0xf4
[ 9.008480] process_one_work+0x20c/0x610
[ 9.013783] worker_thread+0x23c/0x378
[ 9.018775] kthread+0x13c/0x20c
[ 9.023223] ret_from_fork+0x10/0x20
[ 9.028031]
-> #2 (subsys mutex#2){+.+.}-{4:4}:
[ 9.035379] __mutex_lock+0xa8/0x514
[ 9.040197] mutex_lock_nested+0x24/0x30
[ 9.045366] subsys_interface_register+0x50/0x118
[ 9.051338] cpufreq_register_driver+0x178/0x2ec
[ 9.057223] scmi_cpufreq_probe+0x88/0x11c
[ 9.062585] scmi_dev_probe+0x28/0x3c
[ 9.067497] really_probe+0xc0/0x38c
[ 9.072317] __driver_probe_device+0x7c/0x160
[ 9.077941] driver_probe_device+0x40/0x110
[ 9.083392] __device_attach_driver+0xbc/0x158
[ 9.089104] bus_for_each_drv+0x84/0xe0
[ 9.094199] __device_attach+0xa8/0x1d4
[ 9.099292] device_initial_probe+0x14/0x20
[ 9.104749] bus_probe_device+0xb0/0xb4
[ 9.109843] device_add+0x57c/0x784
[ 9.114582] device_register+0x20/0x30
[ 9.119585] __scmi_device_create.part.0+0xf0/0x224
[ 9.125750] scmi_device_create+0x184/0x1c8
[ 9.131210] scmi_create_protocol_devices+0x50/0xb4
[ 9.137380] scmi_probe+0x854/0x91c
[ 9.142118] platform_probe+0x68/0xd8
[ 9.147091] really_probe+0xc0/0x38c
[ 9.151924] __driver_probe_device+0x7c/0x160
[ 9.157556] driver_probe_device+0x40/0x110
[ 9.163014] __device_attach_driver+0xbc/0x158
[ 9.168733] bus_for_each_drv+0x84/0xe0
[ 9.173836] __device_attach+0xa8/0x1d4
[ 9.178932] device_initial_probe+0x14/0x20
[ 9.184381] bus_probe_device+0xb0/0xb4
[ 9.189478] device_add+0x57c/0x784
[ 9.194213] platform_device_add+0x110/0x274
[ 9.199758] scmi_mailbox_probe+0xcc/0x110
[ 9.205172] platform_probe+0x68/0xd8
[ 9.210097] really_probe+0xc0/0x38c
[ 9.214930] __driver_probe_device+0x7c/0x160
[ 9.220558] driver_probe_device+0x40/0x110
[ 9.226017] __device_attach_driver+0xbc/0x158
[ 9.231739] bus_for_each_drv+0x84/0xe0
[ 9.236838] __device_attach+0xa8/0x1d4
[ 9.241937] device_initial_probe+0x14/0x20
[ 9.247396] bus_probe_device+0xb0/0xb4
[ 9.252490] deferred_probe_work_func+0xa0/0xf4
[ 9.258298] process_one_work+0x20c/0x610
[ 9.263631] worker_thread+0x23c/0x378
[ 9.268644] kthread+0x13c/0x20c
[ 9.273118] ret_from_fork+0x10/0x20
[ 9.277954]
-> #1 (cpu_hotplug_lock){++++}-{0:0}:
[ 9.285522] percpu_down_read.constprop.0+0x54/0x168
[ 9.291789] cpus_read_lock+0x10/0x1c
[ 9.296770] static_key_enable+0x18/0x34
[ 9.301961] scmi_quirks_enable+0xec/0x19c
[ 9.307327] scmi_base_protocol_init+0x374/0x4ec
[ 9.313232] scmi_get_protocol_instance+0x198/0x5c0
[ 9.319411] scmi_probe+0x3cc/0x91c
[ 9.324156] platform_probe+0x68/0xd8
[ 9.329083] really_probe+0xc0/0x38c
[ 9.333918] __driver_probe_device+0x7c/0x160
[ 9.339557] driver_probe_device+0x40/0x110
[ 9.345020] __device_attach_driver+0xbc/0x158
[ 9.350753] bus_for_each_drv+0x84/0xe0
[ 9.355858] __device_attach+0xa8/0x1d4
[ 9.360964] device_initial_probe+0x14/0x20
[ 9.366425] bus_probe_device+0xb0/0xb4
[ 9.371532] device_add+0x57c/0x784
[ 9.376273] platform_device_add+0x110/0x274
[ 9.381824] scmi_mailbox_probe+0xcc/0x110
[ 9.387193] platform_probe+0x68/0xd8
[ 9.392121] really_probe+0xc0/0x38c
[ 9.397017] __driver_probe_device+0x7c/0x160
[ 9.402656] driver_probe_device+0x40/0x110
[ 9.408113] __device_attach_driver+0xbc/0x158
[ 9.413892] bus_for_each_drv+0x84/0xe0
[ 9.418997] __device_attach+0xa8/0x1d4
[ 9.424096] device_initial_probe+0x14/0x20
[ 9.429561] bus_probe_device+0xb0/0xb4
[ 9.434663] deferred_probe_work_func+0xa0/0xf4
[ 9.440483] process_one_work+0x20c/0x610
[ 9.445767] worker_thread+0x23c/0x378
[ 9.450786] kthread+0x13c/0x20c
[ 9.455318] ret_from_fork+0x10/0x20
[ 9.460153]
-> #0 (&info->protocols_mtx){+.+.}-{4:4}:
[ 9.468090] __lock_acquire+0x1344/0x20e8
[ 9.473371] lock_acquire+0x1c8/0x354
[ 9.478295] __mutex_lock+0xa8/0x514
[ 9.483129] mutex_lock_nested+0x24/0x30
[ 9.488375] scmi_get_protocol_instance+0x4c/0x5c0
[ 9.494464] scmi_protocol_acquire+0x10/0x24
[ 9.500014] __scmi_event_handler_get_ops+0x1c0/0x47c
[ 9.506367] scmi_notifier_register+0x58/0x29c
[ 9.512096] scmi_cpufreq_init+0x30c/0x4d0
[ 9.517471] cpufreq_online+0x610/0xb58
[ 9.522580] cpufreq_add_dev+0xcc/0xe4
[ 9.527602] subsys_interface_register+0xfc/0x118
[ 9.533614] cpufreq_register_driver+0x178/0x2ec
[ 9.539526] scmi_cpufreq_probe+0x88/0x11c
[ 9.544897] scmi_dev_probe+0x28/0x3c
[ 9.549825] really_probe+0xc0/0x38c
[ 9.554656] __driver_probe_device+0x7c/0x160
[ 9.560297] driver_probe_device+0x40/0x110
[ 9.565764] __device_attach_driver+0xbc/0x158
[ 9.571494] bus_for_each_drv+0x84/0xe0
[ 9.576601] __device_attach+0xa8/0x1d4
[ 9.581710] device_initial_probe+0x14/0x20
[ 9.587178] bus_probe_device+0xb0/0xb4
[ 9.592282] device_add+0x57c/0x784
[ 9.597085] device_register+0x20/0x30
[ 9.602095] __scmi_device_create.part.0+0xf0/0x224
[ 9.608269] scmi_device_create+0x184/0x1c8
[ 9.613786] scmi_create_protocol_devices+0x50/0xb4
[ 9.619965] scmi_probe+0x854/0x91c
[ 9.624718] platform_probe+0x68/0xd8
[ 9.629647] really_probe+0xc0/0x38c
[ 9.634486] __driver_probe_device+0x7c/0x160
[ 9.640125] driver_probe_device+0x40/0x110
[ 9.645593] __device_attach_driver+0xbc/0x158
[ 9.651328] bus_for_each_drv+0x84/0xe0
[ 9.656437] __device_attach+0xa8/0x1d4
[ 9.661541] device_initial_probe+0x14/0x20
[ 9.667006] bus_probe_device+0xb0/0xb4
[ 9.672161] device_add+0x57c/0x784
[ 9.676913] platform_device_add+0x110/0x274
[ 9.682467] scmi_mailbox_probe+0xcc/0x110
[ 9.687843] platform_probe+0x68/0xd8
[ 9.692770] really_probe+0xc0/0x38c
[ 9.697607] __driver_probe_device+0x7c/0x160
[ 9.703250] driver_probe_device+0x40/0x110
[ 9.708718] __device_attach_driver+0xbc/0x158
[ 9.714454] bus_for_each_drv+0x84/0xe0
[ 9.719568] __device_attach+0xa8/0x1d4
[ 9.724670] device_initial_probe+0x14/0x20
[ 9.730193] bus_probe_device+0xb0/0xb4
[ 9.735302] deferred_probe_work_func+0xa0/0xf4
[ 9.741128] process_one_work+0x20c/0x610
[ 9.746414] worker_thread+0x23c/0x378
[ 9.751437] kthread+0x13c/0x20c
[ 9.755912] ret_from_fork+0x10/0x20
[ 9.760746]
other info that might help us debug this:
[ 9.770784] Chain exists of:
&info->protocols_mtx --> &policy->rwsem --> &ni->pending_mtx
[ 9.784150] Possible unsafe locking scenario:
[ 9.791482] CPU0 CPU1
[ 9.796815] ---- ----
[ 9.802092] lock(&ni->pending_mtx);
[ 9.806468] lock(&policy->rwsem);
[ 9.813262] lock(&ni->pending_mtx);
[ 9.820230] lock(&info->protocols_mtx);
[ 9.824981]
*** DEADLOCK ***
[ 9.832879] 11 locks held by kworker/u49:5/165:
[ 9.838145] #0: ffff65d000010948 ((wq_completion)events_unbound){+.+.}-{0:0}, at: process_one_work+0x190/0x610
[ 9.849165] #1: ffff800081eb3dd0 (deferred_probe_work){+.+.}-{0:0}, at: process_one_work+0x1b8/0x610
[ 9.859312] #2: ffff65d0010b48f8 (&dev->mutex){....}-{4:4}, at: __device_attach+0x38/0x1d4
[ 9.868580] #3: ffff65d001e758f8 (&dev->mutex){....}-{4:4}, at: __device_attach+0x38/0x1d4
[ 9.877839] #4: ffff65d004d36460 (&info->devreq_mtx){+.+.}-{4:4}, at: scmi_create_protocol_devices+0x3c/0xb4
[ 9.888790] #5: ffffc981b6c603c8 (scmi_requested_devices_mtx){+.+.}-{4:4}, at: scmi_device_create+0xc8/0x1c8
[ 9.899701] #6: ffff65d0092ca8f8 (&dev->mutex){....}-{4:4}, at: __device_attach+0x38/0x1d4
[ 9.909022] #7: ffffc981b6af2148 (cpu_hotplug_lock){++++}-{0:0}, at: cpus_read_lock+0x10/0x1c
[ 9.918629] #8: ffff65d0009ee518 (subsys mutex#2){+.+.}-{4:4}, at: subsys_interface_register+0x50/0x118
[ 9.929143] #9: ffff65d011fc3b80 (&policy->rwsem){+.+.}-{4:4}, at: cpufreq_online+0x5cc/0xb58
[ 9.938832] #10: ffff65d00a895348 (&ni->pending_mtx){+.+.}-{4:4}, at: __scmi_event_handler_get_ops+0x70/0x47c
[ 9.949921]
stack backtrace:
[ 9.956066] CPU: 0 UID: 0 PID: 165 Comm: kworker/u49:5 Not tainted 6.14.0 #103
[ 9.956068] Hardware name: Qualcomm CRD, BIOS 6.0.241007.BOOT.MXF.2.4-00534.1-HAMOA-1 10/ 7/2024
[ 9.956069] Workqueue: events_unbound deferred_probe_work_func
[ 9.956071] Call trace:
[ 9.956072] show_stack+0x18/0x24 (C)
[ 9.956074] dump_stack_lvl+0x90/0xd0
[ 9.956077] dump_stack+0x18/0x24
[ 9.956078] print_circular_bug+0x298/0x37c
[ 9.956080] check_noncircular+0x15c/0x170
[ 9.956081] __lock_acquire+0x1344/0x20e8
[ 9.956082] lock_acquire+0x1c8/0x354
[ 9.956083] __mutex_lock+0xa8/0x514
[ 9.956084] mutex_lock_nested+0x24/0x30
[ 9.956085] scmi_get_protocol_instance+0x4c/0x5c0
[ 9.956087] scmi_protocol_acquire+0x10/0x24
[ 9.956089] __scmi_event_handler_get_ops+0x1c0/0x47c
[ 9.956091] scmi_notifier_register+0x58/0x29c
[ 9.956093] scmi_cpufreq_init+0x30c/0x4d0
[ 9.956095] cpufreq_online+0x610/0xb58
[ 9.956096] cpufreq_add_dev+0xcc/0xe4
[ 9.956098] subsys_interface_register+0xfc/0x118
[ 9.956100] cpufreq_register_driver+0x178/0x2ec
[ 9.956101] scmi_cpufreq_probe+0x88/0x11c
[ 9.956103] scmi_dev_probe+0x28/0x3c
[ 9.956104] really_probe+0xc0/0x38c
[ 9.956105] __driver_probe_device+0x7c/0x160
[ 9.956107] driver_probe_device+0x40/0x110
[ 9.956108] __device_attach_driver+0xbc/0x158
[ 9.956109] bus_for_each_drv+0x84/0xe0
[ 9.956111] __device_attach+0xa8/0x1d4
[ 9.956112] device_initial_probe+0x14/0x20
[ 9.956113] bus_probe_device+0xb0/0xb4
[ 9.956114] device_add+0x57c/0x784
[ 9.956116] device_register+0x20/0x30
[ 9.956118] __scmi_device_create.part.0+0xf0/0x224
[ 9.956120] scmi_device_create+0x184/0x1c8
[ 9.956121] scmi_create_protocol_devices+0x50/0xb4
[ 9.956123] scmi_probe+0x854/0x91c
[ 9.956124] platform_probe+0x68/0xd8
[ 9.956126] really_probe+0xc0/0x38c
[ 9.956127] __driver_probe_device+0x7c/0x160
[ 9.956128] driver_probe_device+0x40/0x110
[ 9.956129] __device_attach_driver+0xbc/0x158
[ 9.956131] bus_for_each_drv+0x84/0xe0
[ 9.956133] __device_attach+0xa8/0x1d4
[ 9.956134] device_initial_probe+0x14/0x20
[ 9.956135] bus_probe_device+0xb0/0xb4
[ 9.956136] device_add+0x57c/0x784
[ 9.956138] platform_device_add+0x110/0x274
[ 9.956140] scmi_mailbox_probe+0xcc/0x110
[ 9.956141] platform_probe+0x68/0xd8
[ 9.956142] really_probe+0xc0/0x38c
[ 9.956144] __driver_probe_device+0x7c/0x160
[ 9.956145] driver_probe_device+0x40/0x110
[ 9.956146] __device_attach_driver+0xbc/0x158
[ 9.956147] bus_for_each_drv+0x84/0xe0
[ 9.956149] __device_attach+0xa8/0x1d4
[ 9.956151] device_initial_probe+0x14/0x20
[ 9.956152] bus_probe_device+0xb0/0xb4
[ 9.956153] deferred_probe_work_func+0xa0/0xf4
[ 9.956154] process_one_work+0x20c/0x610
[ 9.956156] worker_thread+0x23c/0x378
[ 9.956157] kthread+0x13c/0x20c
[ 9.956159] ret_from_fork+0x10/0x20
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC PATCH 0/3] Introduce SCMI Quirks framework
2025-04-04 13:22 ` [RFC PATCH 0/3] Introduce SCMI Quirks framework Johan Hovold
@ 2025-04-04 13:30 ` Cristian Marussi
2025-04-15 13:55 ` Cristian Marussi
1 sibling, 0 replies; 12+ messages in thread
From: Cristian Marussi @ 2025-04-04 13:30 UTC (permalink / raw)
To: Johan Hovold
Cc: Cristian Marussi, linux-kernel, linux-arm-kernel, arm-scmi,
sudeep.holla, james.quinlan, f.fainelli, vincent.guittot,
peng.fan, michal.simek, quic_sibis, dan.carpenter, maz
On Fri, Apr 04, 2025 at 03:22:57PM +0200, Johan Hovold wrote:
> Hi Cristian,
>
> On Tue, Apr 01, 2025 at 01:25:42PM +0100, Cristian Marussi wrote:
>
> > with the increasing adoption of SCMI across arm64 ecosystems, we have to
> > start considering how to deal and take care of out-of-spec SCMI firmware
> > platforms that are actively deployed in the wild, in a consistent manner.
> >
> > This small series introduces a simple framework, based on static_keys,
> > that allows a user to:
> >
> > - define a quirk and its matching conditions; quirks can match based on:
> >
> > compatible / Vendor_ID / Sub_Vendor_ID / ImplVersion
> >
> > from the longest matching sequence down to the shortest.
> >
> > When the SCMI core stack boots it will enable the matching quirks
> > depending on the information gathered from the platform via Base
> > protocol: any NULL/0 match condition is ignored during matching and is
> > interpreted as ANY, so you can decide to match on a very specific
> > combination of compatibles and FW versions OR simply on a compatible.
> >
> > - define a quirk code-block: simply a block of code, meant to play the
> > magic quirk trick, defined in the proximity of where it will be used
> > and gated by an implicit quirk static-key associated with the defined
> > quirk
>
> > Any feedback and testing is very much welcome.
>
> I'm hitting the below lockdep splat when running with this series and
> the FC quirk enabled.
>
Thanks, I'll have a look...
Cristian
> Johan
>
>
> [ 8.399581] ======================================================
> [ 8.399582] WARNING: possible circular locking dependency detected
> [ 8.399583] 6.14.0 #103 Not tainted
> [ 8.399584] ------------------------------------------------------
> [ 8.399584] kworker/u49:5/165 is trying to acquire lock:
> [ 8.399586] ffff65d004d36320 (&info->protocols_mtx){+.+.}-{4:4}, at: scmi_get_protocol_instance+0x4c/0x5c0
> [ 8.399596]
> but task is already holding lock:
> [ 8.399596] ffff65d00a895348 (&ni->pending_mtx){+.+.}-{4:4}, at: __scmi_event_handler_get_ops+0x70/0x47c
> [ 8.399600]
> which lock already depends on the new lock.
>
> [ 8.399601]
> the existing dependency chain (in reverse order) is:
> [ 8.399601]
> -> #4 (&ni->pending_mtx){+.+.}-{4:4}:
> [ 8.399603] __mutex_lock+0xa8/0x514
> [ 8.399606] mutex_lock_nested+0x24/0x30
> [ 8.399608] __scmi_event_handler_get_ops+0x70/0x47c
> [ 8.399610] scmi_notifier_register+0x58/0x29c
> [ 8.399612] scmi_cpufreq_init+0x30c/0x4d0
> [ 8.399615] cpufreq_online+0x610/0xb58
> [ 8.399616] cpufreq_add_dev+0xcc/0xe4
> [ 8.399618] subsys_interface_register+0xfc/0x118
> [ 8.399622] cpufreq_register_driver+0x178/0x2ec
> [ 8.399623] scmi_cpufreq_probe+0x88/0x11c
> [ 8.399625] scmi_dev_probe+0x28/0x3c
> [ 8.399626] really_probe+0xc0/0x38c
> [ 8.399627] __driver_probe_device+0x7c/0x160
> [ 8.399629] driver_probe_device+0x40/0x110
> [ 8.399630] __device_attach_driver+0xbc/0x158
> [ 8.399631] bus_for_each_drv+0x84/0xe0
> [ 8.399633] __device_attach+0xa8/0x1d4
> [ 8.399634] device_initial_probe+0x14/0x20
> [ 8.399636] bus_probe_device+0xb0/0xb4
> [ 8.399637] device_add+0x57c/0x784
> [ 8.399639] device_register+0x20/0x30
> [ 8.399641] __scmi_device_create.part.0+0xf0/0x224
> [ 8.399642] scmi_device_create+0x184/0x1c8
> [ 8.399644] scmi_create_protocol_devices+0x50/0xb4
> [ 8.399646] scmi_probe+0x854/0x91c
> [ 8.399647] platform_probe+0x68/0xd8
> [ 8.399649] really_probe+0xc0/0x38c
> [ 8.399650] __driver_probe_device+0x7c/0x160
> [ 8.399651] driver_probe_device+0x40/0x110
> [ 8.399653] __device_attach_driver+0xbc/0x158
> [ 8.399654] bus_for_each_drv+0x84/0xe0
> [ 8.399656] __device_attach+0xa8/0x1d4
> [ 8.399657] device_initial_probe+0x14/0x20
> [ 8.399658] bus_probe_device+0xb0/0xb4
> [ 8.399659] device_add+0x57c/0x784
> [ 8.399661] platform_device_add+0x110/0x274
> [ 8.399663] scmi_mailbox_probe+0xcc/0x110
> [ 8.399664] platform_probe+0x68/0xd8
> [ 8.399666] really_probe+0xc0/0x38c
> [ 8.399667] __driver_probe_device+0x7c/0x160
> [ 8.399668] driver_probe_device+0x40/0x110
> [ 8.399669] __device_attach_driver+0xbc/0x158
> [ 8.399671] bus_for_each_drv+0x84/0xe0
> [ 8.399673] __device_attach+0xa8/0x1d4
> [ 8.399674] device_initial_probe+0x14/0x20
> [ 8.399675] bus_probe_device+0xb0/0xb4
> [ 8.399676] deferred_probe_work_func+0xa0/0xf4
> [ 8.399677] process_one_work+0x20c/0x610
> [ 8.399682] worker_thread+0x23c/0x378
> [ 8.399684] kthread+0x13c/0x20c
> [ 8.399685] ret_from_fork+0x10/0x20
> [ 8.399687]
> -> #3 (&policy->rwsem){+.+.}-{4:4}:
> [ 8.399689] down_write+0x50/0xe8
> [ 8.399690] cpufreq_online+0x5cc/0xb58
> [ 8.399692] cpufreq_add_dev+0xcc/0xe4
> [ 8.399693] subsys_interface_register+0xfc/0x118
> [ 8.399695] cpufreq_register_driver+0x178/0x2ec
> [ 8.399697] scmi_cpufreq_probe+0x88/0x11c
> [ 8.399699] scmi_dev_probe+0x28/0x3c
> [ 8.399700] really_probe+0xc0/0x38c
> [ 8.399701] __driver_probe_device+0x7c/0x160
> [ 8.399702] driver_probe_device+0x40/0x110
> [ 8.399704] __device_attach_driver+0xbc/0x158
> [ 8.399705] bus_for_each_drv+0x84/0xe0
> [ 8.399707] __device_attach+0xa8/0x1d4
> [ 8.399708] device_initial_probe+0x14/0x20
> [ 8.399709] bus_probe_device+0xb0/0xb4
> [ 8.399710] device_add+0x57c/0x784
> [ 8.399712] device_register+0x20/0x30
> [ 8.399714] __scmi_device_create.part.0+0xf0/0x224
> [ 8.877044] scmi_device_create+0x184/0x1c8
> [ 8.882444] scmi_create_protocol_devices+0x50/0xb4
> [ 8.888617] scmi_probe+0x854/0x91c
> [ 8.893302] platform_probe+0x68/0xd8
> [ 8.898172] really_probe+0xc0/0x38c
> [ 8.902943] __driver_probe_device+0x7c/0x160
> [ 8.908525] driver_probe_device+0x40/0x110
> [ 8.913933] __device_attach_driver+0xbc/0x158
> [ 8.919608] bus_for_each_drv+0x84/0xe0
> [ 8.924657] __device_attach+0xa8/0x1d4
> [ 8.929708] device_initial_probe+0x14/0x20
> [ 8.935124] bus_probe_device+0xb0/0xb4
> [ 8.940178] device_add+0x57c/0x784
> [ 8.944870] platform_device_add+0x110/0x274
> [ 8.950370] scmi_mailbox_probe+0xcc/0x110
> [ 8.955699] platform_probe+0x68/0xd8
> [ 8.960582] really_probe+0xc0/0x38c
> [ 8.965375] __driver_probe_device+0x7c/0x160
> [ 8.970971] driver_probe_device+0x40/0x110
> [ 8.976390] __device_attach_driver+0xbc/0x158
> [ 8.982076] bus_for_each_drv+0x84/0xe0
> [ 8.987148] __device_attach+0xa8/0x1d4
> [ 8.992212] device_initial_probe+0x14/0x20
> [ 8.997637] bus_probe_device+0xb0/0xb4
> [ 9.002699] deferred_probe_work_func+0xa0/0xf4
> [ 9.008480] process_one_work+0x20c/0x610
> [ 9.013783] worker_thread+0x23c/0x378
> [ 9.018775] kthread+0x13c/0x20c
> [ 9.023223] ret_from_fork+0x10/0x20
> [ 9.028031]
> -> #2 (subsys mutex#2){+.+.}-{4:4}:
> [ 9.035379] __mutex_lock+0xa8/0x514
> [ 9.040197] mutex_lock_nested+0x24/0x30
> [ 9.045366] subsys_interface_register+0x50/0x118
> [ 9.051338] cpufreq_register_driver+0x178/0x2ec
> [ 9.057223] scmi_cpufreq_probe+0x88/0x11c
> [ 9.062585] scmi_dev_probe+0x28/0x3c
> [ 9.067497] really_probe+0xc0/0x38c
> [ 9.072317] __driver_probe_device+0x7c/0x160
> [ 9.077941] driver_probe_device+0x40/0x110
> [ 9.083392] __device_attach_driver+0xbc/0x158
> [ 9.089104] bus_for_each_drv+0x84/0xe0
> [ 9.094199] __device_attach+0xa8/0x1d4
> [ 9.099292] device_initial_probe+0x14/0x20
> [ 9.104749] bus_probe_device+0xb0/0xb4
> [ 9.109843] device_add+0x57c/0x784
> [ 9.114582] device_register+0x20/0x30
> [ 9.119585] __scmi_device_create.part.0+0xf0/0x224
> [ 9.125750] scmi_device_create+0x184/0x1c8
> [ 9.131210] scmi_create_protocol_devices+0x50/0xb4
> [ 9.137380] scmi_probe+0x854/0x91c
> [ 9.142118] platform_probe+0x68/0xd8
> [ 9.147091] really_probe+0xc0/0x38c
> [ 9.151924] __driver_probe_device+0x7c/0x160
> [ 9.157556] driver_probe_device+0x40/0x110
> [ 9.163014] __device_attach_driver+0xbc/0x158
> [ 9.168733] bus_for_each_drv+0x84/0xe0
> [ 9.173836] __device_attach+0xa8/0x1d4
> [ 9.178932] device_initial_probe+0x14/0x20
> [ 9.184381] bus_probe_device+0xb0/0xb4
> [ 9.189478] device_add+0x57c/0x784
> [ 9.194213] platform_device_add+0x110/0x274
> [ 9.199758] scmi_mailbox_probe+0xcc/0x110
> [ 9.205172] platform_probe+0x68/0xd8
> [ 9.210097] really_probe+0xc0/0x38c
> [ 9.214930] __driver_probe_device+0x7c/0x160
> [ 9.220558] driver_probe_device+0x40/0x110
> [ 9.226017] __device_attach_driver+0xbc/0x158
> [ 9.231739] bus_for_each_drv+0x84/0xe0
> [ 9.236838] __device_attach+0xa8/0x1d4
> [ 9.241937] device_initial_probe+0x14/0x20
> [ 9.247396] bus_probe_device+0xb0/0xb4
> [ 9.252490] deferred_probe_work_func+0xa0/0xf4
> [ 9.258298] process_one_work+0x20c/0x610
> [ 9.263631] worker_thread+0x23c/0x378
> [ 9.268644] kthread+0x13c/0x20c
> [ 9.273118] ret_from_fork+0x10/0x20
> [ 9.277954]
> -> #1 (cpu_hotplug_lock){++++}-{0:0}:
> [ 9.285522] percpu_down_read.constprop.0+0x54/0x168
> [ 9.291789] cpus_read_lock+0x10/0x1c
> [ 9.296770] static_key_enable+0x18/0x34
> [ 9.301961] scmi_quirks_enable+0xec/0x19c
> [ 9.307327] scmi_base_protocol_init+0x374/0x4ec
> [ 9.313232] scmi_get_protocol_instance+0x198/0x5c0
> [ 9.319411] scmi_probe+0x3cc/0x91c
> [ 9.324156] platform_probe+0x68/0xd8
> [ 9.329083] really_probe+0xc0/0x38c
> [ 9.333918] __driver_probe_device+0x7c/0x160
> [ 9.339557] driver_probe_device+0x40/0x110
> [ 9.345020] __device_attach_driver+0xbc/0x158
> [ 9.350753] bus_for_each_drv+0x84/0xe0
> [ 9.355858] __device_attach+0xa8/0x1d4
> [ 9.360964] device_initial_probe+0x14/0x20
> [ 9.366425] bus_probe_device+0xb0/0xb4
> [ 9.371532] device_add+0x57c/0x784
> [ 9.376273] platform_device_add+0x110/0x274
> [ 9.381824] scmi_mailbox_probe+0xcc/0x110
> [ 9.387193] platform_probe+0x68/0xd8
> [ 9.392121] really_probe+0xc0/0x38c
> [ 9.397017] __driver_probe_device+0x7c/0x160
> [ 9.402656] driver_probe_device+0x40/0x110
> [ 9.408113] __device_attach_driver+0xbc/0x158
> [ 9.413892] bus_for_each_drv+0x84/0xe0
> [ 9.418997] __device_attach+0xa8/0x1d4
> [ 9.424096] device_initial_probe+0x14/0x20
> [ 9.429561] bus_probe_device+0xb0/0xb4
> [ 9.434663] deferred_probe_work_func+0xa0/0xf4
> [ 9.440483] process_one_work+0x20c/0x610
> [ 9.445767] worker_thread+0x23c/0x378
> [ 9.450786] kthread+0x13c/0x20c
> [ 9.455318] ret_from_fork+0x10/0x20
> [ 9.460153]
> -> #0 (&info->protocols_mtx){+.+.}-{4:4}:
> [ 9.468090] __lock_acquire+0x1344/0x20e8
> [ 9.473371] lock_acquire+0x1c8/0x354
> [ 9.478295] __mutex_lock+0xa8/0x514
> [ 9.483129] mutex_lock_nested+0x24/0x30
> [ 9.488375] scmi_get_protocol_instance+0x4c/0x5c0
> [ 9.494464] scmi_protocol_acquire+0x10/0x24
> [ 9.500014] __scmi_event_handler_get_ops+0x1c0/0x47c
> [ 9.506367] scmi_notifier_register+0x58/0x29c
> [ 9.512096] scmi_cpufreq_init+0x30c/0x4d0
> [ 9.517471] cpufreq_online+0x610/0xb58
> [ 9.522580] cpufreq_add_dev+0xcc/0xe4
> [ 9.527602] subsys_interface_register+0xfc/0x118
> [ 9.533614] cpufreq_register_driver+0x178/0x2ec
> [ 9.539526] scmi_cpufreq_probe+0x88/0x11c
> [ 9.544897] scmi_dev_probe+0x28/0x3c
> [ 9.549825] really_probe+0xc0/0x38c
> [ 9.554656] __driver_probe_device+0x7c/0x160
> [ 9.560297] driver_probe_device+0x40/0x110
> [ 9.565764] __device_attach_driver+0xbc/0x158
> [ 9.571494] bus_for_each_drv+0x84/0xe0
> [ 9.576601] __device_attach+0xa8/0x1d4
> [ 9.581710] device_initial_probe+0x14/0x20
> [ 9.587178] bus_probe_device+0xb0/0xb4
> [ 9.592282] device_add+0x57c/0x784
> [ 9.597085] device_register+0x20/0x30
> [ 9.602095] __scmi_device_create.part.0+0xf0/0x224
> [ 9.608269] scmi_device_create+0x184/0x1c8
> [ 9.613786] scmi_create_protocol_devices+0x50/0xb4
> [ 9.619965] scmi_probe+0x854/0x91c
> [ 9.624718] platform_probe+0x68/0xd8
> [ 9.629647] really_probe+0xc0/0x38c
> [ 9.634486] __driver_probe_device+0x7c/0x160
> [ 9.640125] driver_probe_device+0x40/0x110
> [ 9.645593] __device_attach_driver+0xbc/0x158
> [ 9.651328] bus_for_each_drv+0x84/0xe0
> [ 9.656437] __device_attach+0xa8/0x1d4
> [ 9.661541] device_initial_probe+0x14/0x20
> [ 9.667006] bus_probe_device+0xb0/0xb4
> [ 9.672161] device_add+0x57c/0x784
> [ 9.676913] platform_device_add+0x110/0x274
> [ 9.682467] scmi_mailbox_probe+0xcc/0x110
> [ 9.687843] platform_probe+0x68/0xd8
> [ 9.692770] really_probe+0xc0/0x38c
> [ 9.697607] __driver_probe_device+0x7c/0x160
> [ 9.703250] driver_probe_device+0x40/0x110
> [ 9.708718] __device_attach_driver+0xbc/0x158
> [ 9.714454] bus_for_each_drv+0x84/0xe0
> [ 9.719568] __device_attach+0xa8/0x1d4
> [ 9.724670] device_initial_probe+0x14/0x20
> [ 9.730193] bus_probe_device+0xb0/0xb4
> [ 9.735302] deferred_probe_work_func+0xa0/0xf4
> [ 9.741128] process_one_work+0x20c/0x610
> [ 9.746414] worker_thread+0x23c/0x378
> [ 9.751437] kthread+0x13c/0x20c
> [ 9.755912] ret_from_fork+0x10/0x20
> [ 9.760746]
> other info that might help us debug this:
>
> [ 9.770784] Chain exists of:
> &info->protocols_mtx --> &policy->rwsem --> &ni->pending_mtx
>
> [ 9.784150] Possible unsafe locking scenario:
>
> [ 9.791482] CPU0 CPU1
> [ 9.796815] ---- ----
> [ 9.802092] lock(&ni->pending_mtx);
> [ 9.806468] lock(&policy->rwsem);
> [ 9.813262] lock(&ni->pending_mtx);
> [ 9.820230] lock(&info->protocols_mtx);
> [ 9.824981]
> *** DEADLOCK ***
>
> [ 9.832879] 11 locks held by kworker/u49:5/165:
> [ 9.838145] #0: ffff65d000010948 ((wq_completion)events_unbound){+.+.}-{0:0}, at: process_one_work+0x190/0x610
> [ 9.849165] #1: ffff800081eb3dd0 (deferred_probe_work){+.+.}-{0:0}, at: process_one_work+0x1b8/0x610
> [ 9.859312] #2: ffff65d0010b48f8 (&dev->mutex){....}-{4:4}, at: __device_attach+0x38/0x1d4
> [ 9.868580] #3: ffff65d001e758f8 (&dev->mutex){....}-{4:4}, at: __device_attach+0x38/0x1d4
> [ 9.877839] #4: ffff65d004d36460 (&info->devreq_mtx){+.+.}-{4:4}, at: scmi_create_protocol_devices+0x3c/0xb4
> [ 9.888790] #5: ffffc981b6c603c8 (scmi_requested_devices_mtx){+.+.}-{4:4}, at: scmi_device_create+0xc8/0x1c8
> [ 9.899701] #6: ffff65d0092ca8f8 (&dev->mutex){....}-{4:4}, at: __device_attach+0x38/0x1d4
> [ 9.909022] #7: ffffc981b6af2148 (cpu_hotplug_lock){++++}-{0:0}, at: cpus_read_lock+0x10/0x1c
> [ 9.918629] #8: ffff65d0009ee518 (subsys mutex#2){+.+.}-{4:4}, at: subsys_interface_register+0x50/0x118
> [ 9.929143] #9: ffff65d011fc3b80 (&policy->rwsem){+.+.}-{4:4}, at: cpufreq_online+0x5cc/0xb58
> [ 9.938832] #10: ffff65d00a895348 (&ni->pending_mtx){+.+.}-{4:4}, at: __scmi_event_handler_get_ops+0x70/0x47c
> [ 9.949921]
> stack backtrace:
> [ 9.956066] CPU: 0 UID: 0 PID: 165 Comm: kworker/u49:5 Not tainted 6.14.0 #103
> [ 9.956068] Hardware name: Qualcomm CRD, BIOS 6.0.241007.BOOT.MXF.2.4-00534.1-HAMOA-1 10/ 7/2024
> [ 9.956069] Workqueue: events_unbound deferred_probe_work_func
> [ 9.956071] Call trace:
> [ 9.956072] show_stack+0x18/0x24 (C)
> [ 9.956074] dump_stack_lvl+0x90/0xd0
> [ 9.956077] dump_stack+0x18/0x24
> [ 9.956078] print_circular_bug+0x298/0x37c
> [ 9.956080] check_noncircular+0x15c/0x170
> [ 9.956081] __lock_acquire+0x1344/0x20e8
> [ 9.956082] lock_acquire+0x1c8/0x354
> [ 9.956083] __mutex_lock+0xa8/0x514
> [ 9.956084] mutex_lock_nested+0x24/0x30
> [ 9.956085] scmi_get_protocol_instance+0x4c/0x5c0
> [ 9.956087] scmi_protocol_acquire+0x10/0x24
> [ 9.956089] __scmi_event_handler_get_ops+0x1c0/0x47c
> [ 9.956091] scmi_notifier_register+0x58/0x29c
> [ 9.956093] scmi_cpufreq_init+0x30c/0x4d0
> [ 9.956095] cpufreq_online+0x610/0xb58
> [ 9.956096] cpufreq_add_dev+0xcc/0xe4
> [ 9.956098] subsys_interface_register+0xfc/0x118
> [ 9.956100] cpufreq_register_driver+0x178/0x2ec
> [ 9.956101] scmi_cpufreq_probe+0x88/0x11c
> [ 9.956103] scmi_dev_probe+0x28/0x3c
> [ 9.956104] really_probe+0xc0/0x38c
> [ 9.956105] __driver_probe_device+0x7c/0x160
> [ 9.956107] driver_probe_device+0x40/0x110
> [ 9.956108] __device_attach_driver+0xbc/0x158
> [ 9.956109] bus_for_each_drv+0x84/0xe0
> [ 9.956111] __device_attach+0xa8/0x1d4
> [ 9.956112] device_initial_probe+0x14/0x20
> [ 9.956113] bus_probe_device+0xb0/0xb4
> [ 9.956114] device_add+0x57c/0x784
> [ 9.956116] device_register+0x20/0x30
> [ 9.956118] __scmi_device_create.part.0+0xf0/0x224
> [ 9.956120] scmi_device_create+0x184/0x1c8
> [ 9.956121] scmi_create_protocol_devices+0x50/0xb4
> [ 9.956123] scmi_probe+0x854/0x91c
> [ 9.956124] platform_probe+0x68/0xd8
> [ 9.956126] really_probe+0xc0/0x38c
> [ 9.956127] __driver_probe_device+0x7c/0x160
> [ 9.956128] driver_probe_device+0x40/0x110
> [ 9.956129] __device_attach_driver+0xbc/0x158
> [ 9.956131] bus_for_each_drv+0x84/0xe0
> [ 9.956133] __device_attach+0xa8/0x1d4
> [ 9.956134] device_initial_probe+0x14/0x20
> [ 9.956135] bus_probe_device+0xb0/0xb4
> [ 9.956136] device_add+0x57c/0x784
> [ 9.956138] platform_device_add+0x110/0x274
> [ 9.956140] scmi_mailbox_probe+0xcc/0x110
> [ 9.956141] platform_probe+0x68/0xd8
> [ 9.956142] really_probe+0xc0/0x38c
> [ 9.956144] __driver_probe_device+0x7c/0x160
> [ 9.956145] driver_probe_device+0x40/0x110
> [ 9.956146] __device_attach_driver+0xbc/0x158
> [ 9.956147] bus_for_each_drv+0x84/0xe0
> [ 9.956149] __device_attach+0xa8/0x1d4
> [ 9.956151] device_initial_probe+0x14/0x20
> [ 9.956152] bus_probe_device+0xb0/0xb4
> [ 9.956153] deferred_probe_work_func+0xa0/0xf4
> [ 9.956154] process_one_work+0x20c/0x610
> [ 9.956156] worker_thread+0x23c/0x378
> [ 9.956157] kthread+0x13c/0x20c
> [ 9.956159] ret_from_fork+0x10/0x20
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC PATCH 3/3] [NOT FOR UPSTREAM] firmware: arm_scmi: quirk: Ignore FC bit in attributes
2025-04-03 10:05 ` Johan Hovold
@ 2025-04-04 13:33 ` Cristian Marussi
0 siblings, 0 replies; 12+ messages in thread
From: Cristian Marussi @ 2025-04-04 13:33 UTC (permalink / raw)
To: Johan Hovold
Cc: Cristian Marussi, linux-kernel, linux-arm-kernel, arm-scmi,
sudeep.holla, james.quinlan, f.fainelli, vincent.guittot,
peng.fan, michal.simek, quic_sibis, dan.carpenter, maz
On Thu, Apr 03, 2025 at 12:05:30PM +0200, Johan Hovold wrote:
> On Thu, Apr 03, 2025 at 10:08:41AM +0100, Cristian Marussi wrote:
> > On Thu, Apr 03, 2025 at 10:25:21AM +0200, Johan Hovold wrote:
> > > On Tue, Apr 01, 2025 at 01:25:45PM +0100, Cristian Marussi wrote:
>
> > > > +#define QUIRK_PERF_FC_FORCE \
> > > > + ({ \
> > > > + if (pi->proto->id == SCMI_PROTOCOL_PERF || \
> > > > + message_id == 0x5 /* PERF_LEVEL_GET */) \
> > >
> > > This should be logical AND and PERF_LEVEL_GET is 0x8 (currently
> > > fastchannel is enabled for all PERF messages).
> >
> > ...right...not sure how I botched this condition completely...my bad...
> > (even the comment is wrong :P...)
>
> The PERF_LEVEL_GET comment? That one is correct, right? :)
yes...but not attached to a message_id == 0x5 :P
>
> > > > + attributes |= BIT(0); \
> > > > + })
> > > > +
> > > > static void
> > > > scmi_common_fastchannel_init(const struct scmi_protocol_handle *ph,
> > > > u8 describe_id, u32 message_id, u32 valid_size,
> > > > @@ -1924,6 +1931,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);
> > > > + SCMI_QUIRK(perf_level_get_fc_force, QUIRK_PERF_FC_FORCE);
> > >
> > > This is cool and I assume can be used to minimise overhead in hot paths.
> > > Perhaps you can have concerns about readability and remembering to
> > > update the quirk implementation if the code here changes.
> >
> > My main aim here was to be able to define the quirk code as much as
> > possible in the proximity of where it is used...so that is clear what it
> > does and dont get lost in some general common table....and the macro was
> > a way to uniform the treatment of the static keys...
> >
> > ...but I am still not sure if all of these macros just degrade visibility
> > and we could get rid of them...would be really cool to somehow break the
> > build if the code "sorrounding" the SCMI_QUIRK changes and you dont update
> > (somehow) the quirk too...so as to be sure that the quirk is taking care of
> > and maintained...but I doubt that is feasible, because, really, how do you
> > even deternine which code changes are in proximity enough to the quirk to
> > justify a break...same block ? same functions ? you cannot really know
> > semantically where some changes can impact this part of the code...
> > ..I supppose reviews and testing is the key and the only possible answer
> > to this..
>
> Yeah, it goes both ways. Getting the quirk implementation out of the way
> makes it easier to follow the normal flow, but also makes it a bit
> harder to review the quirk. Your implementation may be a good trade-off.
>
> > > Does it even get compile tested if SCMI_QUIRKS is disabled?
> >
> > It evaluates to nothing when CONFIG_ARM_SCMI_QUIRKS is disabled...
> > ...so maybe I could add a Kconfig dep on COMPILE_TEST ....if this is what
> > you mean..
>
> Perhaps there's some way to get the quirk code always compiled but
> discarded when CONFIG_ARM_SCMI_QUIRKS is disabled (e.g. by using
> IS_ENABLED() in the macro)?
>
I'll think about it.
> CONFIG_ARM_SCMI_QUIRKS may also need to be enabled by default as it can
> be hard to track down random crashes to a missing quirk.
>
Done for V1
> > > > /* Global Quirks Definitions */
> > > > +DEFINE_SCMI_QUIRK(perf_level_get_fc_force,
> > > > + "your-bad-compatible", NULL, NULL, 0x0);
> > >
> > > At first I tried matching on the SoC (fallback) compatible without
> > > success until I noticed you need to put the primary machine compatible
> > > here. For the SoC at hand, that would mean adding 10 or so entries since
> > > all current commercial devices would be affected by this.
> > >
> >
> > Ah right...I tested on a number of combinations BUT assumed only one
> > compatible was to be found...you can potentially add dozens of this
> > definitions for a number of platforms associating the same quirk to all
> > of them and let the match logic enabling only the proper on...BUT this
> > clearly does NOT scale indeed and you will have to endlessly add new
> > platform if fw does NOT get fixed ever...
> >
> > > Matching on vendor and protocol works.
> > >
> >
> > That is abosutely the preferred way, BUT the match should be on
> > Vendor/SubVendor/ImplVersion ... if the platform properly uses
> > ImplementationVersion to differentiate between firmware builds...
>
> We don't seem to have a subvendor here and if IIUC the version has not
> been bumped (yet) after fixing the FC issue.
>
Ok.
> > ...if not you will end up applying the quirk on ANY current and future
> > FW from this Vendor...maybe not an issue in this case...BUT they should
> > seriously thinking about using ImplementationVersion properly in their
> > future FW releases...especially if, as of now, no new fixed FW release
> > has ever been released...
>
> Right, in this case it would probably be OK.
>
> But what if the version is bumped for some other reason (e.g. before a
> bug has been identified)? Then you'd currently need an entry for each
> affected revision or does the implementation assume it applies to
> anything <= ImplVersion? Do we want to add support for version ranges?
>
Right good point...we cannot assume anything really...quirks can be
needed on a single version or on a range...we need ranges...
I will rework the logic for V1.
Thanks of the review and the feedback
Cristian
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC PATCH 0/3] Introduce SCMI Quirks framework
2025-04-04 13:22 ` [RFC PATCH 0/3] Introduce SCMI Quirks framework Johan Hovold
2025-04-04 13:30 ` Cristian Marussi
@ 2025-04-15 13:55 ` Cristian Marussi
1 sibling, 0 replies; 12+ messages in thread
From: Cristian Marussi @ 2025-04-15 13:55 UTC (permalink / raw)
To: Johan Hovold
Cc: Cristian Marussi, linux-kernel, linux-arm-kernel, arm-scmi,
sudeep.holla, james.quinlan, f.fainelli, vincent.guittot,
peng.fan, michal.simek, quic_sibis, dan.carpenter, maz
On Fri, Apr 04, 2025 at 03:22:57PM +0200, Johan Hovold wrote:
> Hi Cristian,
>
> On Tue, Apr 01, 2025 at 01:25:42PM +0100, Cristian Marussi wrote:
>
> > with the increasing adoption of SCMI across arm64 ecosystems, we have to
> > start considering how to deal and take care of out-of-spec SCMI firmware
> > platforms that are actively deployed in the wild, in a consistent manner.
> >
> > This small series introduces a simple framework, based on static_keys,
> > that allows a user to:
> >
> > - define a quirk and its matching conditions; quirks can match based on:
> >
> > compatible / Vendor_ID / Sub_Vendor_ID / ImplVersion
> >
> > from the longest matching sequence down to the shortest.
> >
> > When the SCMI core stack boots it will enable the matching quirks
> > depending on the information gathered from the platform via Base
> > protocol: any NULL/0 match condition is ignored during matching and is
> > interpreted as ANY, so you can decide to match on a very specific
> > combination of compatibles and FW versions OR simply on a compatible.
> >
> > - define a quirk code-block: simply a block of code, meant to play the
> > magic quirk trick, defined in the proximity of where it will be used
> > and gated by an implicit quirk static-key associated with the defined
> > quirk
>
> > Any feedback and testing is very much welcome.
>
> I'm hitting the below lockdep splat when running with this series and
> the FC quirk enabled.
>
> Johan
>
>
> [ 8.399581] ======================================================
> [ 8.399582] WARNING: possible circular locking dependency detected
> [ 8.399583] 6.14.0 #103 Not tainted
> [ 8.399584] ------------------------------------------------------
> [ 8.399584] kworker/u49:5/165 is trying to acquire lock:
> [ 8.399586] ffff65d004d36320 (&info->protocols_mtx){+.+.}-{4:4}, at: scmi_get_protocol_instance+0x4c/0x5c0
> [ 8.399596]
> but task is already holding lock:
> [ 8.399596] ffff65d00a895348 (&ni->pending_mtx){+.+.}-{4:4}, at: __scmi_event_handler_get_ops+0x70/0x47c
> [ 8.399600]
> which lock already depends on the new lock.
>
> [ 8.399601]
> the existing dependency chain (in reverse order) is:
> [ 8.399601]
> -> #4 (&ni->pending_mtx){+.+.}-{4:4}:
Hi Johan,
I have reproduced this when running with LOCKDEP and quirks enabled:
it was due to the fact that static_branch_enable() takes a cpu_lock
internally and I was triggering all of this from Base protocol init
which takes a number of other mutexes...
I moved SCMI quirk initialiation after Base protocol initialization
in scmi_probe and it is gone...and indeed was not needed anyway so
early.
I'll post a new V1 for quirks in a short while including this change.
Thanks,
Cristian
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2025-04-15 14:05 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-04-01 12:25 [RFC PATCH 0/3] Introduce SCMI Quirks framework Cristian Marussi
2025-04-01 12:25 ` [RFC PATCH 1/3] firmware: arm_scmi: Ensure that the message-id supports fastchannel Cristian Marussi
2025-04-03 8:15 ` Johan Hovold
2025-04-01 12:25 ` [RFC PATCH 2/3] firmware: arm_scmi: Add Quirks framework Cristian Marussi
2025-04-01 12:25 ` [RFC PATCH 3/3] [NOT FOR UPSTREAM] firmware: arm_scmi: quirk: Ignore FC bit in attributes Cristian Marussi
2025-04-03 8:25 ` Johan Hovold
2025-04-03 9:08 ` Cristian Marussi
2025-04-03 10:05 ` Johan Hovold
2025-04-04 13:33 ` Cristian Marussi
2025-04-04 13:22 ` [RFC PATCH 0/3] Introduce SCMI Quirks framework Johan Hovold
2025-04-04 13:30 ` Cristian Marussi
2025-04-15 13:55 ` Cristian Marussi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).