From 63b282f172717609136b51571bcc1f74d92d2be6 Mon Sep 17 00:00:00 2001 From: Cristian Marussi Date: Tue, 3 Aug 2021 14:10:10 +0100 Subject: [PATCH 01/18] firmware: arm_scmi: Add support for type handling in common functions Add SCMI type handling to pack/unpack_scmi_header common helper functions. Initialize hdr.type properly when initializing a command xfer. Link: https://lore.kernel.org/r/20210803131024.40280-2-cristian.marussi@arm.com Signed-off-by: Cristian Marussi Signed-off-by: Sudeep Holla --- drivers/firmware/arm_scmi/common.h | 6 +++++- drivers/firmware/arm_scmi/driver.c | 1 + 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h index 8685619d38f9..7c2b9fd7e929 100644 --- a/drivers/firmware/arm_scmi/common.h +++ b/drivers/firmware/arm_scmi/common.h @@ -70,6 +70,7 @@ struct scmi_msg_resp_prot_version { * * @id: The identifier of the message being sent * @protocol_id: The identifier of the protocol used to send @id message + * @type: The SCMI type for this message * @seq: The token to identify the message. When a message returns, the * platform returns the whole message header unmodified including the * token @@ -80,6 +81,7 @@ struct scmi_msg_resp_prot_version { struct scmi_msg_hdr { u8 id; u8 protocol_id; + u8 type; u16 seq; u32 status; bool poll_completion; @@ -89,13 +91,14 @@ struct scmi_msg_hdr { * pack_scmi_header() - packs and returns 32-bit header * * @hdr: pointer to header containing all the information on message id, - * protocol id and sequence id. + * protocol id, sequence id and type. * * Return: 32-bit packed message header to be sent to the platform. */ static inline u32 pack_scmi_header(struct scmi_msg_hdr *hdr) { return FIELD_PREP(MSG_ID_MASK, hdr->id) | + FIELD_PREP(MSG_TYPE_MASK, hdr->type) | FIELD_PREP(MSG_TOKEN_ID_MASK, hdr->seq) | FIELD_PREP(MSG_PROTOCOL_ID_MASK, hdr->protocol_id); } @@ -110,6 +113,7 @@ static inline void unpack_scmi_header(u32 msg_hdr, struct scmi_msg_hdr *hdr) { hdr->id = MSG_XTRACT_ID(msg_hdr); hdr->protocol_id = MSG_XTRACT_PROT_ID(msg_hdr); + hdr->type = MSG_XTRACT_TYPE(msg_hdr); } /** diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c index 9b2e8d42a992..a7a789c5d101 100644 --- a/drivers/firmware/arm_scmi/driver.c +++ b/drivers/firmware/arm_scmi/driver.c @@ -566,6 +566,7 @@ static int xfer_get_init(const struct scmi_protocol_handle *ph, xfer->tx.len = tx_size; xfer->rx.len = rx_size ? : info->desc->max_msg_size; + xfer->hdr.type = MSG_TYPE_COMMAND; xfer->hdr.id = msg_id; xfer->hdr.poll_completion = false; From 3669032514be157d48acc4aa01728815d035d0c3 Mon Sep 17 00:00:00 2001 From: Cristian Marussi Date: Tue, 3 Aug 2021 14:10:11 +0100 Subject: [PATCH 02/18] firmware: arm_scmi: Remove scmi_dump_header_dbg() helper Being a while that we have SCMI trace events in the SCMI stack, remove this debug helper and its call sites. Link: https://lore.kernel.org/r/20210803131024.40280-3-cristian.marussi@arm.com Signed-off-by: Cristian Marussi Signed-off-by: Sudeep Holla --- drivers/firmware/arm_scmi/driver.c | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c index a7a789c5d101..bd7f81b2b46b 100644 --- a/drivers/firmware/arm_scmi/driver.c +++ b/drivers/firmware/arm_scmi/driver.c @@ -172,19 +172,6 @@ static inline int scmi_to_linux_errno(int errno) return -EIO; } -/** - * scmi_dump_header_dbg() - Helper to dump a message header. - * - * @dev: Device pointer corresponding to the SCMI entity - * @hdr: pointer to header. - */ -static inline void scmi_dump_header_dbg(struct device *dev, - struct scmi_msg_hdr *hdr) -{ - dev_dbg(dev, "Message ID: %x Sequence ID: %x Protocol: %x\n", - hdr->id, hdr->seq, hdr->protocol_id); -} - void scmi_notification_instance_data_set(const struct scmi_handle *handle, void *priv) { @@ -288,7 +275,6 @@ static void scmi_handle_notification(struct scmi_chan_info *cinfo, u32 msg_hdr) } unpack_scmi_header(msg_hdr, &xfer->hdr); - scmi_dump_header_dbg(dev, &xfer->hdr); info->desc->ops->fetch_notification(cinfo, info->desc->max_msg_size, xfer); scmi_notify(cinfo->handle, xfer->hdr.protocol_id, @@ -339,8 +325,6 @@ static void scmi_handle_response(struct scmi_chan_info *cinfo, if (msg_type == MSG_TYPE_DELAYED_RESP) xfer->rx.len = info->desc->max_msg_size; - scmi_dump_header_dbg(dev, &xfer->hdr); - info->desc->ops->fetch_response(cinfo, xfer); trace_scmi_rx_done(xfer->transfer_id, xfer->hdr.id, From ceac257db055b8d13aef6fa83e9f2b6733c23532 Mon Sep 17 00:00:00 2001 From: Cristian Marussi Date: Tue, 3 Aug 2021 14:10:12 +0100 Subject: [PATCH 03/18] firmware: arm_scmi: Add optional transport_init/exit support Some SCMI transport could need to perform some transport specific setup before they can be used by the SCMI core transport layer: typically this early setup consists in registering with some other kernel subsystem. Add the optional capability for a transport to provide a couple of init and exit functions that are assured to be called early during the SCMI core initialization phase, well before the SCMI core probing step. [ Peter: Adapted RFC patch by Cristian for submission to upstream. ] Link: https://lore.kernel.org/r/20210803131024.40280-4-cristian.marussi@arm.com Signed-off-by: Peter Hilber [ Cristian: Fixed scmi_transports_exit point of invocation ] Signed-off-by: Cristian Marussi Signed-off-by: Sudeep Holla --- drivers/firmware/arm_scmi/common.h | 8 +++++ drivers/firmware/arm_scmi/driver.c | 57 ++++++++++++++++++++++++++++++ 2 files changed, 65 insertions(+) diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h index 7c2b9fd7e929..9454488021ea 100644 --- a/drivers/firmware/arm_scmi/common.h +++ b/drivers/firmware/arm_scmi/common.h @@ -321,6 +321,12 @@ struct scmi_device *scmi_child_dev_find(struct device *parent, /** * struct scmi_desc - Description of SoC integration * + * @transport_init: An optional function that a transport can provide to + * initialize some transport-specific setup during SCMI core + * initialization, so ahead of SCMI core probing. + * @transport_exit: An optional function that a transport can provide to + * de-initialize some transport-specific setup during SCMI core + * de-initialization, so after SCMI core removal. * @ops: Pointer to the transport specific ops structure * @max_rx_timeout_ms: Timeout for communication with SoC (in Milliseconds) * @max_msg: Maximum number of messages that can be pending @@ -328,6 +334,8 @@ struct scmi_device *scmi_child_dev_find(struct device *parent, * @max_msg_size: Maximum size of data per message that can be handled. */ struct scmi_desc { + int (*transport_init)(void); + void (*transport_exit)(void); const struct scmi_transport_ops *ops; int max_rx_timeout_ms; int max_msg; diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c index bd7f81b2b46b..374bf97a7b4a 100644 --- a/drivers/firmware/arm_scmi/driver.c +++ b/drivers/firmware/arm_scmi/driver.c @@ -1581,10 +1581,65 @@ static struct platform_driver scmi_driver = { .remove = scmi_remove, }; +/** + * __scmi_transports_setup - Common helper to call transport-specific + * .init/.exit code if provided. + * + * @init: A flag to distinguish between init and exit. + * + * Note that, if provided, we invoke .init/.exit functions for all the + * transports currently compiled in. + * + * Return: 0 on Success. + */ +static inline int __scmi_transports_setup(bool init) +{ + int ret = 0; + const struct of_device_id *trans; + + for (trans = scmi_of_match; trans->data; trans++) { + const struct scmi_desc *tdesc = trans->data; + + if ((init && !tdesc->transport_init) || + (!init && !tdesc->transport_exit)) + continue; + + if (init) + ret = tdesc->transport_init(); + else + tdesc->transport_exit(); + + if (ret) { + pr_err("SCMI transport %s FAILED initialization!\n", + trans->compatible); + break; + } + } + + return ret; +} + +static int __init scmi_transports_init(void) +{ + return __scmi_transports_setup(true); +} + +static void __exit scmi_transports_exit(void) +{ + __scmi_transports_setup(false); +} + static int __init scmi_driver_init(void) { + int ret; + scmi_bus_init(); + /* Initialize any compiled-in transport which provided an init/exit */ + ret = scmi_transports_init(); + if (ret) + return ret; + scmi_base_register(); scmi_clock_register(); @@ -1613,6 +1668,8 @@ static void __exit scmi_driver_exit(void) scmi_bus_exit(); + scmi_transports_exit(); + platform_driver_unregister(&scmi_driver); } module_exit(scmi_driver_exit); From 9ca5a1838e593d96d5d1727eed150ed4f7c179e2 Mon Sep 17 00:00:00 2001 From: Cristian Marussi Date: Tue, 3 Aug 2021 14:10:13 +0100 Subject: [PATCH 04/18] firmware: arm_scmi: Introduce monotonically increasing tokens Tokens are sequence numbers embedded in the each SCMI message header: they are used to correlate commands with responses (and delayed responses), but their usage and policy of selection is entirely up to the caller (usually the OSPM agent), while they are completely opaque to the callee (i.e. SCMI platform) which merely copies them back from the command into the response message header. This also means that the platform does not, can not and should not enforce any kind of policy on received messages depending on the contained sequence number: platform can perfectly handle concurrent requests carrying the same identifiying token if that should happen. Moreover the platform is not required to produce in-order responses to agent requests, the only constraint in these regards is that in case of an asynchronous message the delayed response must be sent after the immediate response for the synchronous part of the command transaction. Currenly the SCMI stack of the OSPM agent selects a token for the egressing commands picking the lowest possible number which is not already in use by an existing in-flight transaction, which means, in other words, that we immediately reuse any token after its transaction has completed or it has timed out: this policy indeed does simplify management and lookup of tokens and associated xfers. Under the above assumptions and constraints, since there is really no state shared between the agent and the platform to let the platform know when a token and its associated message has timed out, the current policy of early reuse of tokens can easily lead to the situation in which a spurious or late received response (or delayed_response), related to an old stale and timed out transaction, can be wrongly associated to a newer valid in-flight xfer that just happens to have reused the same token. This misbehaviour on such late/spurious responses is more easily exposed on those transports that naturally have an higher level of parallelism in processing multiple concurrent in-flight messages. This commit introduces a new policy of selection of tokens for the OSPM agent: each new command transfer now gets the next available, monotonically increasing token, until tokens are exhausted and the counter rolls over. Such new policy mitigates the above issues with late/spurious responses since the tokens are now reused as late as possible (when they roll back ideally) and so it is much easier to identify such late/spurious responses to stale timed out transactions: this also helps in simplifying the specific transports implementation since stale transport messages can be easily identified and discarded early on in the rx path without the need to cross check their actual state with the core transport layer. This mitigation is even more effective when, as is usually the case, the maximum number of pending messages is capped by the platform to a much lower number than the whole possible range of tokens values (2^10). This internal policy change in the core SCMI transport layer is fully transparent to the specific transports so it has not and should not have any impact on the transports implementation. Link: https://lore.kernel.org/r/20210803131024.40280-5-cristian.marussi@arm.com Signed-off-by: Cristian Marussi Signed-off-by: Sudeep Holla --- drivers/firmware/arm_scmi/common.h | 32 ++++ drivers/firmware/arm_scmi/driver.c | 254 +++++++++++++++++++++++++---- 2 files changed, 251 insertions(+), 35 deletions(-) diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h index 9454488021ea..651af0c7bed9 100644 --- a/drivers/firmware/arm_scmi/common.h +++ b/drivers/firmware/arm_scmi/common.h @@ -14,6 +14,8 @@ #include #include #include +#include +#include #include #include #include @@ -65,6 +67,16 @@ struct scmi_msg_resp_prot_version { #define MSG_XTRACT_TOKEN(hdr) FIELD_GET(MSG_TOKEN_ID_MASK, (hdr)) #define MSG_TOKEN_MAX (MSG_XTRACT_TOKEN(MSG_TOKEN_ID_MASK) + 1) +/* + * Size of @pending_xfers hashtable included in @scmi_xfers_info; ideally, in + * order to minimize space and collisions, this should equal max_msg, i.e. the + * maximum number of in-flight messages on a specific platform, but such value + * is only available at runtime while kernel hashtables are statically sized: + * pick instead as a fixed static size the maximum number of entries that can + * fit the whole table into one 4k page. + */ +#define SCMI_PENDING_XFERS_HT_ORDER_SZ 9 + /** * struct scmi_msg_hdr - Message(Tx/Rx) header * @@ -138,6 +150,9 @@ struct scmi_msg { * buffer for the rx path as we use for the tx path. * @done: command message transmit completion event * @async_done: pointer to delayed response message received event completion + * @pending: True for xfers added to @pending_xfers hashtable + * @node: An hlist_node reference used to store this xfer, alternatively, on + * the free list @free_xfers or in the @pending_xfers hashtable */ struct scmi_xfer { int transfer_id; @@ -146,8 +161,25 @@ struct scmi_xfer { struct scmi_msg rx; struct completion done; struct completion *async_done; + bool pending; + struct hlist_node node; }; +/* + * An helper macro to lookup an xfer from the @pending_xfers hashtable + * using the message sequence number token as a key. + */ +#define XFER_FIND(__ht, __k) \ +({ \ + typeof(__k) k_ = __k; \ + struct scmi_xfer *xfer_ = NULL; \ + \ + hash_for_each_possible((__ht), xfer_, node, k_) \ + if (xfer_->hdr.seq == k_) \ + break; \ + xfer_; \ +}) + struct scmi_xfer_ops; /** diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c index 374bf97a7b4a..3c99390f9415 100644 --- a/drivers/firmware/arm_scmi/driver.c +++ b/drivers/firmware/arm_scmi/driver.c @@ -21,6 +21,7 @@ #include #include #include +#include #include #include #include @@ -67,16 +68,21 @@ struct scmi_requested_dev { /** * struct scmi_xfers_info - Structure to manage transfer information * - * @xfer_block: Preallocated Message array * @xfer_alloc_table: Bitmap table for allocated messages. * Index of this bitmap table is also used for message * sequence identifier. * @xfer_lock: Protection for message allocation + * @free_xfers: A free list for available to use xfers. It is initialized with + * a number of xfers equal to the maximum allowed in-flight + * messages. + * @pending_xfers: An hashtable, indexed by msg_hdr.seq, used to keep all the + * currently in-flight messages. */ struct scmi_xfers_info { - struct scmi_xfer *xfer_block; unsigned long *xfer_alloc_table; spinlock_t xfer_lock; + struct hlist_head free_xfers; + DECLARE_HASHTABLE(pending_xfers, SCMI_PENDING_XFERS_HT_ORDER_SZ); }; /** @@ -191,46 +197,185 @@ void *scmi_notification_instance_data_get(const struct scmi_handle *handle) return info->notify_priv; } +/** + * scmi_xfer_token_set - Reserve and set new token for the xfer at hand + * + * @minfo: Pointer to Tx/Rx Message management info based on channel type + * @xfer: The xfer to act upon + * + * Pick the next unused monotonically increasing token and set it into + * xfer->hdr.seq: picking a monotonically increasing value avoids immediate + * reuse of freshly completed or timed-out xfers, thus mitigating the risk + * of incorrect association of a late and expired xfer with a live in-flight + * transaction, both happening to re-use the same token identifier. + * + * Since platform is NOT required to answer our request in-order we should + * account for a few rare but possible scenarios: + * + * - exactly 'next_token' may be NOT available so pick xfer_id >= next_token + * using find_next_zero_bit() starting from candidate next_token bit + * + * - all tokens ahead upto (MSG_TOKEN_ID_MASK - 1) are used in-flight but we + * are plenty of free tokens at start, so try a second pass using + * find_next_zero_bit() and starting from 0. + * + * X = used in-flight + * + * Normal + * ------ + * + * |- xfer_id picked + * -----------+---------------------------------------------------------- + * | | |X|X|X| | | | | | ... ... ... ... ... ... ... ... ... ... ...|X|X| + * ---------------------------------------------------------------------- + * ^ + * |- next_token + * + * Out-of-order pending at start + * ----------------------------- + * + * |- xfer_id picked, last_token fixed + * -----+---------------------------------------------------------------- + * |X|X| | | | |X|X| ... ... ... ... ... ... ... ... ... ... ... ...|X| | + * ---------------------------------------------------------------------- + * ^ + * |- next_token + * + * + * Out-of-order pending at end + * --------------------------- + * + * |- xfer_id picked, last_token fixed + * -----+---------------------------------------------------------------- + * |X|X| | | | |X|X| ... ... ... ... ... ... ... ... ... ... |X|X|X||X|X| + * ---------------------------------------------------------------------- + * ^ + * |- next_token + * + * Context: Assumes to be called with @xfer_lock already acquired. + * + * Return: 0 on Success or error + */ +static int scmi_xfer_token_set(struct scmi_xfers_info *minfo, + struct scmi_xfer *xfer) +{ + unsigned long xfer_id, next_token; + + /* + * Pick a candidate monotonic token in range [0, MSG_TOKEN_MAX - 1] + * using the pre-allocated transfer_id as a base. + * Note that the global transfer_id is shared across all message types + * so there could be holes in the allocated set of monotonic sequence + * numbers, but that is going to limit the effectiveness of the + * mitigation only in very rare limit conditions. + */ + next_token = (xfer->transfer_id & (MSG_TOKEN_MAX - 1)); + + /* Pick the next available xfer_id >= next_token */ + xfer_id = find_next_zero_bit(minfo->xfer_alloc_table, + MSG_TOKEN_MAX, next_token); + if (xfer_id == MSG_TOKEN_MAX) { + /* + * After heavily out-of-order responses, there are no free + * tokens ahead, but only at start of xfer_alloc_table so + * try again from the beginning. + */ + xfer_id = find_next_zero_bit(minfo->xfer_alloc_table, + MSG_TOKEN_MAX, 0); + /* + * Something is wrong if we got here since there can be a + * maximum number of (MSG_TOKEN_MAX - 1) in-flight messages + * but we have not found any free token [0, MSG_TOKEN_MAX - 1]. + */ + if (WARN_ON_ONCE(xfer_id == MSG_TOKEN_MAX)) + return -ENOMEM; + } + + /* Update +/- last_token accordingly if we skipped some hole */ + if (xfer_id != next_token) + atomic_add((int)(xfer_id - next_token), &transfer_last_id); + + /* Set in-flight */ + set_bit(xfer_id, minfo->xfer_alloc_table); + xfer->hdr.seq = (u16)xfer_id; + + return 0; +} + +/** + * scmi_xfer_token_clear - Release the token + * + * @minfo: Pointer to Tx/Rx Message management info based on channel type + * @xfer: The xfer to act upon + */ +static inline void scmi_xfer_token_clear(struct scmi_xfers_info *minfo, + struct scmi_xfer *xfer) +{ + clear_bit(xfer->hdr.seq, minfo->xfer_alloc_table); +} + /** * scmi_xfer_get() - Allocate one message * * @handle: Pointer to SCMI entity handle * @minfo: Pointer to Tx/Rx Message management info based on channel type + * @set_pending: If true a monotonic token is picked and the xfer is added to + * the pending hash table. * * Helper function which is used by various message functions that are * exposed to clients of this driver for allocating a message traffic event. * - * This function can sleep depending on pending requests already in the system - * for the SCMI entity. Further, this also holds a spinlock to maintain - * integrity of internal data structures. + * Picks an xfer from the free list @free_xfers (if any available) and, if + * required, sets a monotonically increasing token and stores the inflight xfer + * into the @pending_xfers hashtable for later retrieval. + * + * The successfully initialized xfer is refcounted. + * + * Context: Holds @xfer_lock while manipulating @xfer_alloc_table and + * @free_xfers. * * Return: 0 if all went fine, else corresponding error. */ static struct scmi_xfer *scmi_xfer_get(const struct scmi_handle *handle, - struct scmi_xfers_info *minfo) + struct scmi_xfers_info *minfo, + bool set_pending) { - u16 xfer_id; + int ret; + unsigned long flags; struct scmi_xfer *xfer; - unsigned long flags, bit_pos; - struct scmi_info *info = handle_to_scmi_info(handle); - /* Keep the locked section as small as possible */ spin_lock_irqsave(&minfo->xfer_lock, flags); - bit_pos = find_first_zero_bit(minfo->xfer_alloc_table, - info->desc->max_msg); - if (bit_pos == info->desc->max_msg) { + if (hlist_empty(&minfo->free_xfers)) { spin_unlock_irqrestore(&minfo->xfer_lock, flags); return ERR_PTR(-ENOMEM); } - set_bit(bit_pos, minfo->xfer_alloc_table); - spin_unlock_irqrestore(&minfo->xfer_lock, flags); - xfer_id = bit_pos; + /* grab an xfer from the free_list */ + xfer = hlist_entry(minfo->free_xfers.first, struct scmi_xfer, node); + hlist_del_init(&xfer->node); - xfer = &minfo->xfer_block[xfer_id]; - xfer->hdr.seq = xfer_id; + /* + * Allocate transfer_id early so that can be used also as base for + * monotonic sequence number generation if needed. + */ xfer->transfer_id = atomic_inc_return(&transfer_last_id); + if (set_pending) { + /* Pick and set monotonic token */ + ret = scmi_xfer_token_set(minfo, xfer); + if (!ret) { + hash_add(minfo->pending_xfers, &xfer->node, + xfer->hdr.seq); + xfer->pending = true; + } else { + dev_err(handle->dev, + "Failed to get monotonic token %d\n", ret); + hlist_add_head(&xfer->node, &minfo->free_xfers); + xfer = ERR_PTR(ret); + } + } + spin_unlock_irqrestore(&minfo->xfer_lock, flags); + return xfer; } @@ -240,6 +385,9 @@ static struct scmi_xfer *scmi_xfer_get(const struct scmi_handle *handle, * @minfo: Pointer to Tx/Rx Message management info based on channel type * @xfer: message that was reserved by scmi_xfer_get * + * After refcount check, possibly release an xfer, clearing the token slot, + * removing xfer from @pending_xfers and putting it back into free_xfers. + * * This holds a spinlock to maintain integrity of internal data structures. */ static void @@ -247,16 +395,39 @@ __scmi_xfer_put(struct scmi_xfers_info *minfo, struct scmi_xfer *xfer) { unsigned long flags; - /* - * Keep the locked section as small as possible - * NOTE: we might escape with smp_mb and no lock here.. - * but just be conservative and symmetric. - */ spin_lock_irqsave(&minfo->xfer_lock, flags); - clear_bit(xfer->hdr.seq, minfo->xfer_alloc_table); + if (xfer->pending) { + scmi_xfer_token_clear(minfo, xfer); + hash_del(&xfer->node); + xfer->pending = false; + } + hlist_add_head(&xfer->node, &minfo->free_xfers); spin_unlock_irqrestore(&minfo->xfer_lock, flags); } +/** + * scmi_xfer_lookup_unlocked - Helper to lookup an xfer_id + * + * @minfo: Pointer to Tx/Rx Message management info based on channel type + * @xfer_id: Token ID to lookup in @pending_xfers + * + * Refcounting is untouched. + * + * Context: Assumes to be called with @xfer_lock already acquired. + * + * Return: A valid xfer on Success or error otherwise + */ +static struct scmi_xfer * +scmi_xfer_lookup_unlocked(struct scmi_xfers_info *minfo, u16 xfer_id) +{ + struct scmi_xfer *xfer = NULL; + + if (test_bit(xfer_id, minfo->xfer_alloc_table)) + xfer = XFER_FIND(minfo->pending_xfers, xfer_id); + + return xfer ?: ERR_PTR(-EINVAL); +} + static void scmi_handle_notification(struct scmi_chan_info *cinfo, u32 msg_hdr) { struct scmi_xfer *xfer; @@ -266,7 +437,7 @@ static void scmi_handle_notification(struct scmi_chan_info *cinfo, u32 msg_hdr) ktime_t ts; ts = ktime_get_boottime(); - xfer = scmi_xfer_get(cinfo->handle, minfo); + xfer = scmi_xfer_get(cinfo->handle, minfo, false); if (IS_ERR(xfer)) { dev_err(dev, "failed to get free message slot (%ld)\n", PTR_ERR(xfer)); @@ -292,19 +463,22 @@ static void scmi_handle_notification(struct scmi_chan_info *cinfo, u32 msg_hdr) static void scmi_handle_response(struct scmi_chan_info *cinfo, u16 xfer_id, u8 msg_type) { + unsigned long flags; struct scmi_xfer *xfer; struct device *dev = cinfo->dev; struct scmi_info *info = handle_to_scmi_info(cinfo->handle); struct scmi_xfers_info *minfo = &info->tx_minfo; /* Are we even expecting this? */ - if (!test_bit(xfer_id, minfo->xfer_alloc_table)) { + spin_lock_irqsave(&minfo->xfer_lock, flags); + xfer = scmi_xfer_lookup_unlocked(minfo, xfer_id); + spin_unlock_irqrestore(&minfo->xfer_lock, flags); + if (IS_ERR(xfer)) { dev_err(dev, "message for %d is not expected!\n", xfer_id); info->desc->ops->clear_channel(cinfo); return; } - xfer = &minfo->xfer_block[xfer_id]; /* * Even if a response was indeed expected on this slot at this point, * a buggy platform could wrongly reply feeding us an unexpected @@ -541,7 +715,7 @@ static int xfer_get_init(const struct scmi_protocol_handle *ph, tx_size > info->desc->max_msg_size) return -ERANGE; - xfer = scmi_xfer_get(pi->handle, minfo); + xfer = scmi_xfer_get(pi->handle, minfo, true); if (IS_ERR(xfer)) { ret = PTR_ERR(xfer); dev_err(dev, "failed to get free message slot(%d)\n", ret); @@ -1018,18 +1192,25 @@ static int __scmi_xfer_info_init(struct scmi_info *sinfo, return -EINVAL; } - info->xfer_block = devm_kcalloc(dev, desc->max_msg, - sizeof(*info->xfer_block), GFP_KERNEL); - if (!info->xfer_block) - return -ENOMEM; + hash_init(info->pending_xfers); - info->xfer_alloc_table = devm_kcalloc(dev, BITS_TO_LONGS(desc->max_msg), + /* Allocate a bitmask sized to hold MSG_TOKEN_MAX tokens */ + info->xfer_alloc_table = devm_kcalloc(dev, BITS_TO_LONGS(MSG_TOKEN_MAX), sizeof(long), GFP_KERNEL); if (!info->xfer_alloc_table) return -ENOMEM; - /* Pre-initialize the buffer pointer to pre-allocated buffers */ - for (i = 0, xfer = info->xfer_block; i < desc->max_msg; i++, xfer++) { + /* + * Preallocate a number of xfers equal to max inflight messages, + * pre-initialize the buffer pointer to pre-allocated buffers and + * attach all of them to the free list + */ + INIT_HLIST_HEAD(&info->free_xfers); + for (i = 0; i < desc->max_msg; i++) { + xfer = devm_kzalloc(dev, sizeof(*xfer), GFP_KERNEL); + if (!xfer) + return -ENOMEM; + xfer->rx.buf = devm_kcalloc(dev, sizeof(u8), desc->max_msg_size, GFP_KERNEL); if (!xfer->rx.buf) @@ -1037,6 +1218,9 @@ static int __scmi_xfer_info_init(struct scmi_info *sinfo, xfer->tx.buf = xfer->rx.buf; init_completion(&xfer->done); + + /* Add initialized xfer to the free list */ + hlist_add_head(&xfer->node, &info->free_xfers); } spin_lock_init(&info->xfer_lock); From ed7c04c1fea3b027c1b2efcf57b50c98ef05840b Mon Sep 17 00:00:00 2001 From: Cristian Marussi Date: Tue, 3 Aug 2021 14:10:14 +0100 Subject: [PATCH 05/18] firmware: arm_scmi: Handle concurrent and out-of-order messages Even though in case of asynchronous commands an SCMI platform is constrained to emit the delayed response message only after the related message response has been sent, the configured underlying transport could still deliver such messages together or in inverted order, causing races due to the concurrent or out-of-order access to the underlying xfer. Introduce a mechanism to grant exclusive access to an xfer in order to properly serialize concurrent accesses to the same xfer originating from multiple correlated messages. Add additional state information to xfer descriptors so as to be able to identify out-of-order message deliveries and act accordingly: - when a delayed response is expected but delivered before the related response, the synchronous response is considered as successfully received and the delayed response processing is carried on as usual. - when/if the missing synchronous response is subsequently received, it is discarded as not congruent with the current state of the xfer, or simply, because the xfer has been already released and so, now, the monotonically increasing sequence number carried by the late response is stale. Link: https://lore.kernel.org/r/20210803131024.40280-6-cristian.marussi@arm.com Signed-off-by: Cristian Marussi Signed-off-by: Sudeep Holla --- drivers/firmware/arm_scmi/common.h | 30 +++- drivers/firmware/arm_scmi/driver.c | 257 ++++++++++++++++++++++++----- 2 files changed, 246 insertions(+), 41 deletions(-) diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h index 651af0c7bed9..dd19ec2e0105 100644 --- a/drivers/firmware/arm_scmi/common.h +++ b/drivers/firmware/arm_scmi/common.h @@ -17,7 +17,9 @@ #include #include #include +#include #include +#include #include #include @@ -153,6 +155,23 @@ struct scmi_msg { * @pending: True for xfers added to @pending_xfers hashtable * @node: An hlist_node reference used to store this xfer, alternatively, on * the free list @free_xfers or in the @pending_xfers hashtable + * @users: A refcount to track the active users for this xfer. + * This is meant to protect against the possibility that, when a command + * transaction times out concurrently with the reception of a valid + * response message, the xfer could be finally put on the TX path, and + * so vanish, while on the RX path scmi_rx_callback() is still + * processing it: in such a case this refcounting will ensure that, even + * though the timed-out transaction will anyway cause the command + * request to be reported as failed by time-out, the underlying xfer + * cannot be discarded and possibly reused until the last one user on + * the RX path has released it. + * @busy: An atomic flag to ensure exclusive write access to this xfer + * @state: The current state of this transfer, with states transitions deemed + * valid being: + * - SCMI_XFER_SENT_OK -> SCMI_XFER_RESP_OK [ -> SCMI_XFER_DRESP_OK ] + * - SCMI_XFER_SENT_OK -> SCMI_XFER_DRESP_OK + * (Missing synchronous response is assumed OK and ignored) + * @lock: A spinlock to protect state and busy fields. */ struct scmi_xfer { int transfer_id; @@ -163,6 +182,16 @@ struct scmi_xfer { struct completion *async_done; bool pending; struct hlist_node node; + refcount_t users; +#define SCMI_XFER_FREE 0 +#define SCMI_XFER_BUSY 1 + atomic_t busy; +#define SCMI_XFER_SENT_OK 0 +#define SCMI_XFER_RESP_OK 1 +#define SCMI_XFER_DRESP_OK 2 + int state; + /* A lock to protect state and busy fields */ + spinlock_t lock; }; /* @@ -399,5 +428,4 @@ bool shmem_poll_done(struct scmi_shared_mem __iomem *shmem, void scmi_notification_instance_data_set(const struct scmi_handle *handle, void *priv); void *scmi_notification_instance_data_get(const struct scmi_handle *handle); - #endif /* _SCMI_COMMON_H */ diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c index 3c99390f9415..bfc35d7c7dbd 100644 --- a/drivers/firmware/arm_scmi/driver.c +++ b/drivers/firmware/arm_scmi/driver.c @@ -374,6 +374,11 @@ static struct scmi_xfer *scmi_xfer_get(const struct scmi_handle *handle, xfer = ERR_PTR(ret); } } + + if (!IS_ERR(xfer)) { + refcount_set(&xfer->users, 1); + atomic_set(&xfer->busy, SCMI_XFER_FREE); + } spin_unlock_irqrestore(&minfo->xfer_lock, flags); return xfer; @@ -396,12 +401,14 @@ __scmi_xfer_put(struct scmi_xfers_info *minfo, struct scmi_xfer *xfer) unsigned long flags; spin_lock_irqsave(&minfo->xfer_lock, flags); - if (xfer->pending) { - scmi_xfer_token_clear(minfo, xfer); - hash_del(&xfer->node); - xfer->pending = false; + if (refcount_dec_and_test(&xfer->users)) { + if (xfer->pending) { + scmi_xfer_token_clear(minfo, xfer); + hash_del(&xfer->node); + xfer->pending = false; + } + hlist_add_head(&xfer->node, &minfo->free_xfers); } - hlist_add_head(&xfer->node, &minfo->free_xfers); spin_unlock_irqrestore(&minfo->xfer_lock, flags); } @@ -428,6 +435,171 @@ scmi_xfer_lookup_unlocked(struct scmi_xfers_info *minfo, u16 xfer_id) return xfer ?: ERR_PTR(-EINVAL); } +/** + * scmi_msg_response_validate - Validate message type against state of related + * xfer + * + * @cinfo: A reference to the channel descriptor. + * @msg_type: Message type to check + * @xfer: A reference to the xfer to validate against @msg_type + * + * This function checks if @msg_type is congruent with the current state of + * a pending @xfer; if an asynchronous delayed response is received before the + * related synchronous response (Out-of-Order Delayed Response) the missing + * synchronous response is assumed to be OK and completed, carrying on with the + * Delayed Response: this is done to address the case in which the underlying + * SCMI transport can deliver such out-of-order responses. + * + * Context: Assumes to be called with xfer->lock already acquired. + * + * Return: 0 on Success, error otherwise + */ +static inline int scmi_msg_response_validate(struct scmi_chan_info *cinfo, + u8 msg_type, + struct scmi_xfer *xfer) +{ + /* + * Even if a response was indeed expected on this slot at this point, + * a buggy platform could wrongly reply feeding us an unexpected + * delayed response we're not prepared to handle: bail-out safely + * blaming firmware. + */ + if (msg_type == MSG_TYPE_DELAYED_RESP && !xfer->async_done) { + dev_err(cinfo->dev, + "Delayed Response for %d not expected! Buggy F/W ?\n", + xfer->hdr.seq); + return -EINVAL; + } + + switch (xfer->state) { + case SCMI_XFER_SENT_OK: + if (msg_type == MSG_TYPE_DELAYED_RESP) { + /* + * Delayed Response expected but delivered earlier. + * Assume message RESPONSE was OK and skip state. + */ + xfer->hdr.status = SCMI_SUCCESS; + xfer->state = SCMI_XFER_RESP_OK; + complete(&xfer->done); + dev_warn(cinfo->dev, + "Received valid OoO Delayed Response for %d\n", + xfer->hdr.seq); + } + break; + case SCMI_XFER_RESP_OK: + if (msg_type != MSG_TYPE_DELAYED_RESP) + return -EINVAL; + break; + case SCMI_XFER_DRESP_OK: + /* No further message expected once in SCMI_XFER_DRESP_OK */ + return -EINVAL; + } + + return 0; +} + +/** + * scmi_xfer_state_update - Update xfer state + * + * @xfer: A reference to the xfer to update + * @msg_type: Type of message being processed. + * + * Note that this message is assumed to have been already successfully validated + * by @scmi_msg_response_validate(), so here we just update the state. + * + * Context: Assumes to be called on an xfer exclusively acquired using the + * busy flag. + */ +static inline void scmi_xfer_state_update(struct scmi_xfer *xfer, u8 msg_type) +{ + xfer->hdr.type = msg_type; + + /* Unknown command types were already discarded earlier */ + if (xfer->hdr.type == MSG_TYPE_COMMAND) + xfer->state = SCMI_XFER_RESP_OK; + else + xfer->state = SCMI_XFER_DRESP_OK; +} + +static bool scmi_xfer_acquired(struct scmi_xfer *xfer) +{ + int ret; + + ret = atomic_cmpxchg(&xfer->busy, SCMI_XFER_FREE, SCMI_XFER_BUSY); + + return ret == SCMI_XFER_FREE; +} + +/** + * scmi_xfer_command_acquire - Helper to lookup and acquire a command xfer + * + * @cinfo: A reference to the channel descriptor. + * @msg_hdr: A message header to use as lookup key + * + * When a valid xfer is found for the sequence number embedded in the provided + * msg_hdr, reference counting is properly updated and exclusive access to this + * xfer is granted till released with @scmi_xfer_command_release. + * + * Return: A valid @xfer on Success or error otherwise. + */ +static inline struct scmi_xfer * +scmi_xfer_command_acquire(struct scmi_chan_info *cinfo, u32 msg_hdr) +{ + int ret; + unsigned long flags; + struct scmi_xfer *xfer; + struct scmi_info *info = handle_to_scmi_info(cinfo->handle); + struct scmi_xfers_info *minfo = &info->tx_minfo; + u8 msg_type = MSG_XTRACT_TYPE(msg_hdr); + u16 xfer_id = MSG_XTRACT_TOKEN(msg_hdr); + + /* Are we even expecting this? */ + spin_lock_irqsave(&minfo->xfer_lock, flags); + xfer = scmi_xfer_lookup_unlocked(minfo, xfer_id); + if (IS_ERR(xfer)) { + dev_err(cinfo->dev, + "Message for %d type %d is not expected!\n", + xfer_id, msg_type); + spin_unlock_irqrestore(&minfo->xfer_lock, flags); + return xfer; + } + refcount_inc(&xfer->users); + spin_unlock_irqrestore(&minfo->xfer_lock, flags); + + spin_lock_irqsave(&xfer->lock, flags); + ret = scmi_msg_response_validate(cinfo, msg_type, xfer); + /* + * If a pending xfer was found which was also in a congruent state with + * the received message, acquire exclusive access to it setting the busy + * flag. + * Spins only on the rare limit condition of concurrent reception of + * RESP and DRESP for the same xfer. + */ + if (!ret) { + spin_until_cond(scmi_xfer_acquired(xfer)); + scmi_xfer_state_update(xfer, msg_type); + } + spin_unlock_irqrestore(&xfer->lock, flags); + + if (ret) { + dev_err(cinfo->dev, + "Invalid message type:%d for %d - HDR:0x%X state:%d\n", + msg_type, xfer_id, msg_hdr, xfer->state); + /* On error the refcount incremented above has to be dropped */ + __scmi_xfer_put(minfo, xfer); + xfer = ERR_PTR(-EINVAL); + } + + return xfer; +} + +static inline void scmi_xfer_command_release(struct scmi_info *info, + struct scmi_xfer *xfer) +{ + atomic_set(&xfer->busy, SCMI_XFER_FREE); + __scmi_xfer_put(&info->tx_minfo, xfer); +} + static void scmi_handle_notification(struct scmi_chan_info *cinfo, u32 msg_hdr) { struct scmi_xfer *xfer; @@ -460,57 +632,35 @@ static void scmi_handle_notification(struct scmi_chan_info *cinfo, u32 msg_hdr) info->desc->ops->clear_channel(cinfo); } -static void scmi_handle_response(struct scmi_chan_info *cinfo, - u16 xfer_id, u8 msg_type) +static void scmi_handle_response(struct scmi_chan_info *cinfo, u32 msg_hdr) { - unsigned long flags; struct scmi_xfer *xfer; - struct device *dev = cinfo->dev; struct scmi_info *info = handle_to_scmi_info(cinfo->handle); - struct scmi_xfers_info *minfo = &info->tx_minfo; - /* Are we even expecting this? */ - spin_lock_irqsave(&minfo->xfer_lock, flags); - xfer = scmi_xfer_lookup_unlocked(minfo, xfer_id); - spin_unlock_irqrestore(&minfo->xfer_lock, flags); + xfer = scmi_xfer_command_acquire(cinfo, msg_hdr); if (IS_ERR(xfer)) { - dev_err(dev, "message for %d is not expected!\n", xfer_id); info->desc->ops->clear_channel(cinfo); return; } - /* - * Even if a response was indeed expected on this slot at this point, - * a buggy platform could wrongly reply feeding us an unexpected - * delayed response we're not prepared to handle: bail-out safely - * blaming firmware. - */ - if (unlikely(msg_type == MSG_TYPE_DELAYED_RESP && !xfer->async_done)) { - dev_err(dev, - "Delayed Response for %d not expected! Buggy F/W ?\n", - xfer_id); - info->desc->ops->clear_channel(cinfo); - /* It was unexpected, so nobody will clear the xfer if not us */ - __scmi_xfer_put(minfo, xfer); - return; - } - /* rx.len could be shrunk in the sync do_xfer, so reset to maxsz */ - if (msg_type == MSG_TYPE_DELAYED_RESP) + if (xfer->hdr.type == MSG_TYPE_DELAYED_RESP) xfer->rx.len = info->desc->max_msg_size; info->desc->ops->fetch_response(cinfo, xfer); trace_scmi_rx_done(xfer->transfer_id, xfer->hdr.id, xfer->hdr.protocol_id, xfer->hdr.seq, - msg_type); + xfer->hdr.type); - if (msg_type == MSG_TYPE_DELAYED_RESP) { + if (xfer->hdr.type == MSG_TYPE_DELAYED_RESP) { info->desc->ops->clear_channel(cinfo); complete(xfer->async_done); } else { complete(&xfer->done); } + + scmi_xfer_command_release(info, xfer); } /** @@ -527,7 +677,6 @@ static void scmi_handle_response(struct scmi_chan_info *cinfo, */ void scmi_rx_callback(struct scmi_chan_info *cinfo, u32 msg_hdr) { - u16 xfer_id = MSG_XTRACT_TOKEN(msg_hdr); u8 msg_type = MSG_XTRACT_TYPE(msg_hdr); switch (msg_type) { @@ -536,7 +685,7 @@ void scmi_rx_callback(struct scmi_chan_info *cinfo, u32 msg_hdr) break; case MSG_TYPE_COMMAND: case MSG_TYPE_DELAYED_RESP: - scmi_handle_response(cinfo, xfer_id, msg_type); + scmi_handle_response(cinfo, msg_hdr); break; default: WARN_ONCE(1, "received unknown msg_type:%d\n", msg_type); @@ -548,7 +697,7 @@ void scmi_rx_callback(struct scmi_chan_info *cinfo, u32 msg_hdr) * xfer_put() - Release a transmit message * * @ph: Pointer to SCMI protocol handle - * @xfer: message that was reserved by scmi_xfer_get + * @xfer: message that was reserved by xfer_get_init */ static void xfer_put(const struct scmi_protocol_handle *ph, struct scmi_xfer *xfer) @@ -566,7 +715,12 @@ static bool scmi_xfer_done_no_timeout(struct scmi_chan_info *cinfo, { struct scmi_info *info = handle_to_scmi_info(cinfo->handle); + /* + * Poll also on xfer->done so that polling can be forcibly terminated + * in case of out-of-order receptions of delayed responses + */ return info->desc->ops->poll_done(cinfo, xfer) || + try_wait_for_completion(&xfer->done) || ktime_after(ktime_get(), stop); } @@ -606,6 +760,16 @@ static int do_xfer(const struct scmi_protocol_handle *ph, xfer->hdr.protocol_id, xfer->hdr.seq, xfer->hdr.poll_completion); + xfer->state = SCMI_XFER_SENT_OK; + /* + * Even though spinlocking is not needed here since no race is possible + * on xfer->state due to the monotonically increasing tokens allocation, + * we must anyway ensure xfer->state initialization is not re-ordered + * after the .send_message() to be sure that on the RX path an early + * ISR calling scmi_rx_callback() cannot see an old stale xfer->state. + */ + smp_mb(); + ret = info->desc->ops->send_message(cinfo, xfer); if (ret < 0) { dev_dbg(dev, "Failed to send message %d\n", ret); @@ -617,10 +781,22 @@ static int do_xfer(const struct scmi_protocol_handle *ph, spin_until_cond(scmi_xfer_done_no_timeout(cinfo, xfer, stop)); - if (ktime_before(ktime_get(), stop)) - info->desc->ops->fetch_response(cinfo, xfer); - else + if (ktime_before(ktime_get(), stop)) { + unsigned long flags; + + /* + * Do not fetch_response if an out-of-order delayed + * response is being processed. + */ + spin_lock_irqsave(&xfer->lock, flags); + if (xfer->state == SCMI_XFER_SENT_OK) { + info->desc->ops->fetch_response(cinfo, xfer); + xfer->state = SCMI_XFER_RESP_OK; + } + spin_unlock_irqrestore(&xfer->lock, flags); + } else { ret = -ETIMEDOUT; + } } else { /* And we wait for the response. */ timeout = msecs_to_jiffies(info->desc->max_rx_timeout_ms); @@ -1218,6 +1394,7 @@ static int __scmi_xfer_info_init(struct scmi_info *sinfo, xfer->tx.buf = xfer->rx.buf; init_completion(&xfer->done); + spin_lock_init(&xfer->lock); /* Add initialized xfer to the free list */ hlist_add_head(&xfer->node, &info->free_xfers); From e9b21c96181c36343597ce789e386d296f93a23b Mon Sep 17 00:00:00 2001 From: Cristian Marussi Date: Tue, 3 Aug 2021 14:10:15 +0100 Subject: [PATCH 06/18] firmware: arm_scmi: Make .clear_channel optional Make transport operation .clear_channel optional since some transports do not need it and so avoid to have them implement dummy callbacks. Link: https://lore.kernel.org/r/20210803131024.40280-7-cristian.marussi@arm.com Signed-off-by: Cristian Marussi Signed-off-by: Sudeep Holla --- drivers/firmware/arm_scmi/driver.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c index bfc35d7c7dbd..6c77fcc1bcb7 100644 --- a/drivers/firmware/arm_scmi/driver.c +++ b/drivers/firmware/arm_scmi/driver.c @@ -600,6 +600,13 @@ static inline void scmi_xfer_command_release(struct scmi_info *info, __scmi_xfer_put(&info->tx_minfo, xfer); } +static inline void scmi_clear_channel(struct scmi_info *info, + struct scmi_chan_info *cinfo) +{ + if (info->desc->ops->clear_channel) + info->desc->ops->clear_channel(cinfo); +} + static void scmi_handle_notification(struct scmi_chan_info *cinfo, u32 msg_hdr) { struct scmi_xfer *xfer; @@ -613,7 +620,7 @@ static void scmi_handle_notification(struct scmi_chan_info *cinfo, u32 msg_hdr) if (IS_ERR(xfer)) { dev_err(dev, "failed to get free message slot (%ld)\n", PTR_ERR(xfer)); - info->desc->ops->clear_channel(cinfo); + scmi_clear_channel(info, cinfo); return; } @@ -629,7 +636,7 @@ static void scmi_handle_notification(struct scmi_chan_info *cinfo, u32 msg_hdr) __scmi_xfer_put(minfo, xfer); - info->desc->ops->clear_channel(cinfo); + scmi_clear_channel(info, cinfo); } static void scmi_handle_response(struct scmi_chan_info *cinfo, u32 msg_hdr) @@ -639,7 +646,7 @@ static void scmi_handle_response(struct scmi_chan_info *cinfo, u32 msg_hdr) xfer = scmi_xfer_command_acquire(cinfo, msg_hdr); if (IS_ERR(xfer)) { - info->desc->ops->clear_channel(cinfo); + scmi_clear_channel(info, cinfo); return; } @@ -654,7 +661,7 @@ static void scmi_handle_response(struct scmi_chan_info *cinfo, u32 msg_hdr) xfer->hdr.type); if (xfer->hdr.type == MSG_TYPE_DELAYED_RESP) { - info->desc->ops->clear_channel(cinfo); + scmi_clear_channel(info, cinfo); complete(xfer->async_done); } else { complete(&xfer->done); From 2930abcffd9f0b36e8fd4df01f6311eede686817 Mon Sep 17 00:00:00 2001 From: Cristian Marussi Date: Tue, 3 Aug 2021 14:10:16 +0100 Subject: [PATCH 07/18] firmware: arm_scmi: Make polling mode optional Add a check for the presence of .poll_done transport operation so that transports that do not need to support polling mode have no need to provide a dummy .poll_done callback either and polling mode can be disabled in the SCMI core for that tranport. Link: https://lore.kernel.org/r/20210803131024.40280-8-cristian.marussi@arm.com Signed-off-by: Cristian Marussi Signed-off-by: Sudeep Holla --- drivers/firmware/arm_scmi/driver.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c index 6c77fcc1bcb7..5ff0bcbb4db4 100644 --- a/drivers/firmware/arm_scmi/driver.c +++ b/drivers/firmware/arm_scmi/driver.c @@ -751,6 +751,12 @@ static int do_xfer(const struct scmi_protocol_handle *ph, struct device *dev = info->dev; struct scmi_chan_info *cinfo; + if (xfer->hdr.poll_completion && !info->desc->ops->poll_done) { + dev_warn_once(dev, + "Polling mode is not supported by transport.\n"); + return -EINVAL; + } + /* * Initialise protocol id now from protocol handle to avoid it being * overridden by mistake (or malice) by the protocol code mangling with @@ -787,7 +793,6 @@ static int do_xfer(const struct scmi_protocol_handle *ph, ktime_t stop = ktime_add_ns(ktime_get(), SCMI_MAX_POLL_TO_NS); spin_until_cond(scmi_xfer_done_no_timeout(cinfo, xfer, stop)); - if (ktime_before(ktime_get(), stop)) { unsigned long flags; From e8419c24bacee45bfe3504814e91fc89ff8c23de Mon Sep 17 00:00:00 2001 From: Cristian Marussi Date: Tue, 3 Aug 2021 14:10:17 +0100 Subject: [PATCH 08/18] firmware: arm_scmi: Make SCMI transports configurable Add configuration options to be able to select which SCMI transports have to be compiled into the SCMI stack. Mailbox and SMC are by default enabled if their related dependencies are satisfied. While doing that move all SCMI related config options in their own dedicated submenu. Link: https://lore.kernel.org/r/20210803131024.40280-9-cristian.marussi@arm.com Signed-off-by: Cristian Marussi Signed-off-by: Sudeep Holla --- drivers/firmware/Kconfig | 34 +-------------- drivers/firmware/arm_scmi/Kconfig | 70 ++++++++++++++++++++++++++++++ drivers/firmware/arm_scmi/Makefile | 4 +- drivers/firmware/arm_scmi/common.h | 4 +- drivers/firmware/arm_scmi/driver.c | 6 ++- 5 files changed, 80 insertions(+), 38 deletions(-) create mode 100644 drivers/firmware/arm_scmi/Kconfig diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig index 1db738d5b301..8d41f73f5395 100644 --- a/drivers/firmware/Kconfig +++ b/drivers/firmware/Kconfig @@ -6,39 +6,7 @@ menu "Firmware Drivers" -config ARM_SCMI_PROTOCOL - tristate "ARM System Control and Management Interface (SCMI) Message Protocol" - depends on ARM || ARM64 || COMPILE_TEST - depends on MAILBOX || HAVE_ARM_SMCCC_DISCOVERY - help - ARM System Control and Management Interface (SCMI) protocol is a - set of operating system-independent software interfaces that are - used in system management. SCMI is extensible and currently provides - interfaces for: Discovery and self-description of the interfaces - it supports, Power domain management which is the ability to place - a given device or domain into the various power-saving states that - it supports, Performance management which is the ability to control - the performance of a domain that is composed of compute engines - such as application processors and other accelerators, Clock - management which is the ability to set and inquire rates on platform - managed clocks and Sensor management which is the ability to read - sensor data, and be notified of sensor value. - - This protocol library provides interface for all the client drivers - making use of the features offered by the SCMI. - -config ARM_SCMI_POWER_DOMAIN - tristate "SCMI power domain driver" - depends on ARM_SCMI_PROTOCOL || (COMPILE_TEST && OF) - default y - select PM_GENERIC_DOMAINS if PM - help - This enables support for the SCMI power domains which can be - enabled or disabled via the SCP firmware - - This driver can also be built as a module. If so, the module - will be called scmi_pm_domain. Note this may needed early in boot - before rootfs may be available. +source "drivers/firmware/arm_scmi/Kconfig" config ARM_SCPI_PROTOCOL tristate "ARM System Control and Power Interface (SCPI) Message Protocol" diff --git a/drivers/firmware/arm_scmi/Kconfig b/drivers/firmware/arm_scmi/Kconfig new file mode 100644 index 000000000000..a6fcd06b2232 --- /dev/null +++ b/drivers/firmware/arm_scmi/Kconfig @@ -0,0 +1,70 @@ +# SPDX-License-Identifier: GPL-2.0-only +menu "ARM System Control and Management Interface Protocol" + +config ARM_SCMI_PROTOCOL + tristate "ARM System Control and Management Interface (SCMI) Message Protocol" + depends on ARM || ARM64 || COMPILE_TEST + help + ARM System Control and Management Interface (SCMI) protocol is a + set of operating system-independent software interfaces that are + used in system management. SCMI is extensible and currently provides + interfaces for: Discovery and self-description of the interfaces + it supports, Power domain management which is the ability to place + a given device or domain into the various power-saving states that + it supports, Performance management which is the ability to control + the performance of a domain that is composed of compute engines + such as application processors and other accelerators, Clock + management which is the ability to set and inquire rates on platform + managed clocks and Sensor management which is the ability to read + sensor data, and be notified of sensor value. + + This protocol library provides interface for all the client drivers + making use of the features offered by the SCMI. + +if ARM_SCMI_PROTOCOL + +config ARM_SCMI_HAVE_TRANSPORT + bool + help + This declares whether at least one SCMI transport has been configured. + Used to trigger a build bug when trying to build SCMI without any + configured transport. + +config ARM_SCMI_TRANSPORT_MAILBOX + bool "SCMI transport based on Mailbox" + depends on MAILBOX + select ARM_SCMI_HAVE_TRANSPORT + default y + help + Enable mailbox based transport for SCMI. + + If you want the ARM SCMI PROTOCOL stack to include support for a + transport based on mailboxes, answer Y. + +config ARM_SCMI_TRANSPORT_SMC + bool "SCMI transport based on SMC" + depends on HAVE_ARM_SMCCC_DISCOVERY + select ARM_SCMI_HAVE_TRANSPORT + default y + help + Enable SMC based transport for SCMI. + + If you want the ARM SCMI PROTOCOL stack to include support for a + transport based on SMC, answer Y. + +endif #ARM_SCMI_PROTOCOL + +config ARM_SCMI_POWER_DOMAIN + tristate "SCMI power domain driver" + depends on ARM_SCMI_PROTOCOL || (COMPILE_TEST && OF) + default y + select PM_GENERIC_DOMAINS if PM + help + This enables support for the SCMI power domains which can be + enabled or disabled via the SCP firmware + + This driver can also be built as a module. If so, the module + will be called scmi_pm_domain. Note this may needed early in boot + before rootfs may be available. + +endmenu diff --git a/drivers/firmware/arm_scmi/Makefile b/drivers/firmware/arm_scmi/Makefile index 6a2ef63306d6..38163d6991b3 100644 --- a/drivers/firmware/arm_scmi/Makefile +++ b/drivers/firmware/arm_scmi/Makefile @@ -2,8 +2,8 @@ scmi-bus-y = bus.o scmi-driver-y = driver.o notify.o scmi-transport-y = shmem.o -scmi-transport-$(CONFIG_MAILBOX) += mailbox.o -scmi-transport-$(CONFIG_HAVE_ARM_SMCCC_DISCOVERY) += smc.o +scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_MAILBOX) += mailbox.o +scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_SMC) += smc.o scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o voltage.o scmi-module-objs := $(scmi-bus-y) $(scmi-driver-y) $(scmi-protocols-y) \ $(scmi-transport-y) diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h index dd19ec2e0105..204cde53a9a7 100644 --- a/drivers/firmware/arm_scmi/common.h +++ b/drivers/firmware/arm_scmi/common.h @@ -403,8 +403,10 @@ struct scmi_desc { int max_msg_size; }; +#ifdef CONFIG_ARM_SCMI_TRANSPORT_MAILBOX extern const struct scmi_desc scmi_mailbox_desc; -#ifdef CONFIG_HAVE_ARM_SMCCC_DISCOVERY +#endif +#ifdef CONFIG_ARM_SCMI_TRANSPORT_SMC extern const struct scmi_desc scmi_smc_desc; #endif diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c index 5ff0bcbb4db4..d20e6760b488 100644 --- a/drivers/firmware/arm_scmi/driver.c +++ b/drivers/firmware/arm_scmi/driver.c @@ -1933,10 +1933,10 @@ ATTRIBUTE_GROUPS(versions); /* Each compatible listed below must have descriptor associated with it */ static const struct of_device_id scmi_of_match[] = { -#ifdef CONFIG_MAILBOX +#ifdef CONFIG_ARM_SCMI_TRANSPORT_MAILBOX { .compatible = "arm,scmi", .data = &scmi_mailbox_desc }, #endif -#ifdef CONFIG_HAVE_ARM_SMCCC_DISCOVERY +#ifdef CONFIG_ARM_SCMI_TRANSPORT_SMC { .compatible = "arm,scmi-smc", .data = &scmi_smc_desc}, #endif { /* Sentinel */ }, @@ -2008,6 +2008,8 @@ static int __init scmi_driver_init(void) scmi_bus_init(); + BUILD_BUG_ON(!IS_ENABLED(CONFIG_ARM_SCMI_HAVE_TRANSPORT)); + /* Initialize any compiled-in transport which provided an init/exit */ ret = scmi_transports_init(); if (ret) From a7b1138b921dc19f859296b2eb3daa7c5e22c354 Mon Sep 17 00:00:00 2001 From: Igor Skalkin Date: Tue, 3 Aug 2021 14:10:18 +0100 Subject: [PATCH 09/18] firmware: arm_scmi: Make shmem support optional for transports Upcoming new SCMI transports won't need any kind of shared memory support. Compile shmem.c only if a shmem based transport is selected. Link: https://lore.kernel.org/r/20210803131024.40280-10-cristian.marussi@arm.com Co-developed-by: Peter Hilber Signed-off-by: Igor Skalkin [ Peter: Adapted patch for submission to upstream. ] Signed-off-by: Peter Hilber [ Cristian: Adapted patch/commit_msg to new SCMI Kconfig layout ] Signed-off-by: Cristian Marussi Signed-off-by: Sudeep Holla --- drivers/firmware/arm_scmi/Kconfig | 8 ++++++++ drivers/firmware/arm_scmi/Makefile | 2 +- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/drivers/firmware/arm_scmi/Kconfig b/drivers/firmware/arm_scmi/Kconfig index a6fcd06b2232..cd84e5f2ee02 100644 --- a/drivers/firmware/arm_scmi/Kconfig +++ b/drivers/firmware/arm_scmi/Kconfig @@ -30,10 +30,17 @@ config ARM_SCMI_HAVE_TRANSPORT Used to trigger a build bug when trying to build SCMI without any configured transport. +config ARM_SCMI_HAVE_SHMEM + bool + help + This declares whether a shared memory based transport for SCMI is + available. + config ARM_SCMI_TRANSPORT_MAILBOX bool "SCMI transport based on Mailbox" depends on MAILBOX select ARM_SCMI_HAVE_TRANSPORT + select ARM_SCMI_HAVE_SHMEM default y help Enable mailbox based transport for SCMI. @@ -45,6 +52,7 @@ config ARM_SCMI_TRANSPORT_SMC bool "SCMI transport based on SMC" depends on HAVE_ARM_SMCCC_DISCOVERY select ARM_SCMI_HAVE_TRANSPORT + select ARM_SCMI_HAVE_SHMEM default y help Enable SMC based transport for SCMI. diff --git a/drivers/firmware/arm_scmi/Makefile b/drivers/firmware/arm_scmi/Makefile index 38163d6991b3..e0e6bd3dba9e 100644 --- a/drivers/firmware/arm_scmi/Makefile +++ b/drivers/firmware/arm_scmi/Makefile @@ -1,7 +1,7 @@ # SPDX-License-Identifier: GPL-2.0-only scmi-bus-y = bus.o scmi-driver-y = driver.o notify.o -scmi-transport-y = shmem.o +scmi-transport-$(CONFIG_ARM_SCMI_HAVE_SHMEM) = shmem.o scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_MAILBOX) += mailbox.o scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_SMC) += smc.o scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o voltage.o From c92c3e382ebd2382b26a41e312a266a40c4fb05c Mon Sep 17 00:00:00 2001 From: Igor Skalkin Date: Tue, 3 Aug 2021 14:10:19 +0100 Subject: [PATCH 10/18] firmware: arm_scmi: Add method to override max message number The maximum number of simultaneously pending messages is a transport specific quantity that is usually described statically in struct scmi_desc. Some transports, though, can calculate such number only at run-time after some initial transport specific setup and probing is completed; moreover the resulting max message numbers could also be different between rx and tx channels. Add an optional get_max_msg() operation so that a transport can report more accurate max message numbers for each channel type. The value in scmi_desc.max_msg is still used as default when transport does not provide any get_max_msg() method. Link: https://lore.kernel.org/r/20210803131024.40280-11-cristian.marussi@arm.com Co-developed-by: Peter Hilber Co-developed-by: Cristian Marussi Signed-off-by: Igor Skalkin [ Peter: Adapted patch for submission to upstream. ] Signed-off-by: Peter Hilber [ Cristian: refactored how get_max_msg() is used to minimize core changes ] Signed-off-by: Cristian Marussi Signed-off-by: Sudeep Holla --- drivers/firmware/arm_scmi/common.h | 9 +++++-- drivers/firmware/arm_scmi/driver.c | 40 +++++++++++++++++++++++++++--- 2 files changed, 43 insertions(+), 6 deletions(-) diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h index 204cde53a9a7..6320345865e8 100644 --- a/drivers/firmware/arm_scmi/common.h +++ b/drivers/firmware/arm_scmi/common.h @@ -351,6 +351,9 @@ struct scmi_chan_info { * @chan_available: Callback to check if channel is available or not * @chan_setup: Callback to allocate and setup a channel * @chan_free: Callback to free a channel + * @get_max_msg: Optional callback to provide max_msg dynamically + * Returns the maximum number of messages for the channel type + * (tx or rx) that can be pending simultaneously in the system * @send_message: Callback to send a message * @mark_txdone: Callback to mark tx as done * @fetch_response: Callback to fetch response @@ -363,6 +366,7 @@ struct scmi_transport_ops { int (*chan_setup)(struct scmi_chan_info *cinfo, struct device *dev, bool tx); int (*chan_free)(int id, void *p, void *data); + unsigned int (*get_max_msg)(struct scmi_chan_info *base_cinfo); int (*send_message)(struct scmi_chan_info *cinfo, struct scmi_xfer *xfer); void (*mark_txdone)(struct scmi_chan_info *cinfo, int ret); @@ -390,8 +394,9 @@ struct scmi_device *scmi_child_dev_find(struct device *parent, * de-initialization, so after SCMI core removal. * @ops: Pointer to the transport specific ops structure * @max_rx_timeout_ms: Timeout for communication with SoC (in Milliseconds) - * @max_msg: Maximum number of messages that can be pending - * simultaneously in the system + * @max_msg: Maximum number of messages for a channel type (tx or rx) that can + * be pending simultaneously in the system. May be overridden by the + * get_max_msg op. * @max_msg_size: Maximum size of data per message that can be handled. */ struct scmi_desc { diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c index d20e6760b488..a907f3fe4c08 100644 --- a/drivers/firmware/arm_scmi/driver.c +++ b/drivers/firmware/arm_scmi/driver.c @@ -72,6 +72,7 @@ struct scmi_requested_dev { * Index of this bitmap table is also used for message * sequence identifier. * @xfer_lock: Protection for message allocation + * @max_msg: Maximum number of messages that can be pending * @free_xfers: A free list for available to use xfers. It is initialized with * a number of xfers equal to the maximum allowed in-flight * messages. @@ -81,6 +82,7 @@ struct scmi_requested_dev { struct scmi_xfers_info { unsigned long *xfer_alloc_table; spinlock_t xfer_lock; + int max_msg; struct hlist_head free_xfers; DECLARE_HASHTABLE(pending_xfers, SCMI_PENDING_XFERS_HT_ORDER_SZ); }; @@ -1373,10 +1375,10 @@ static int __scmi_xfer_info_init(struct scmi_info *sinfo, const struct scmi_desc *desc = sinfo->desc; /* Pre-allocated messages, no more than what hdr.seq can support */ - if (WARN_ON(!desc->max_msg || desc->max_msg > MSG_TOKEN_MAX)) { + if (WARN_ON(!info->max_msg || info->max_msg > MSG_TOKEN_MAX)) { dev_err(dev, "Invalid maximum messages %d, not in range [1 - %lu]\n", - desc->max_msg, MSG_TOKEN_MAX); + info->max_msg, MSG_TOKEN_MAX); return -EINVAL; } @@ -1394,7 +1396,7 @@ static int __scmi_xfer_info_init(struct scmi_info *sinfo, * attach all of them to the free list */ INIT_HLIST_HEAD(&info->free_xfers); - for (i = 0; i < desc->max_msg; i++) { + for (i = 0; i < info->max_msg; i++) { xfer = devm_kzalloc(dev, sizeof(*xfer), GFP_KERNEL); if (!xfer) return -ENOMEM; @@ -1417,10 +1419,40 @@ static int __scmi_xfer_info_init(struct scmi_info *sinfo, return 0; } +static int scmi_channels_max_msg_configure(struct scmi_info *sinfo) +{ + const struct scmi_desc *desc = sinfo->desc; + + if (!desc->ops->get_max_msg) { + sinfo->tx_minfo.max_msg = desc->max_msg; + sinfo->rx_minfo.max_msg = desc->max_msg; + } else { + struct scmi_chan_info *base_cinfo; + + base_cinfo = idr_find(&sinfo->tx_idr, SCMI_PROTOCOL_BASE); + if (!base_cinfo) + return -EINVAL; + sinfo->tx_minfo.max_msg = desc->ops->get_max_msg(base_cinfo); + + /* RX channel is optional so can be skipped */ + base_cinfo = idr_find(&sinfo->rx_idr, SCMI_PROTOCOL_BASE); + if (base_cinfo) + sinfo->rx_minfo.max_msg = + desc->ops->get_max_msg(base_cinfo); + } + + return 0; +} + static int scmi_xfer_info_init(struct scmi_info *sinfo) { - int ret = __scmi_xfer_info_init(sinfo, &sinfo->tx_minfo); + int ret; + ret = scmi_channels_max_msg_configure(sinfo); + if (ret) + return ret; + + ret = __scmi_xfer_info_init(sinfo, &sinfo->tx_minfo); if (!ret && idr_find(&sinfo->rx_idr, SCMI_PROTOCOL_BASE)) ret = __scmi_xfer_info_init(sinfo, &sinfo->rx_minfo); From f301bba0ca7392d16a6ea4f1d264a91f1fadea1a Mon Sep 17 00:00:00 2001 From: Peter Hilber Date: Tue, 3 Aug 2021 14:10:20 +0100 Subject: [PATCH 11/18] firmware: arm_scmi: Add message passing abstractions for transports Add abstractions for future transports using message passing, such as virtio. Derive the abstractions from the shared memory abstractions. Abstract the transport SDU through the opaque struct scmi_msg_payld. Also enable the transport to determine all other required information about the transport SDU. Link: https://lore.kernel.org/r/20210803131024.40280-12-cristian.marussi@arm.com Signed-off-by: Peter Hilber [ Cristian: Adapted to new SCMI Kconfig layout, updated Copyrights ] Signed-off-by: Cristian Marussi Signed-off-by: Sudeep Holla --- drivers/firmware/arm_scmi/Kconfig | 6 ++ drivers/firmware/arm_scmi/Makefile | 1 + drivers/firmware/arm_scmi/common.h | 15 ++++ drivers/firmware/arm_scmi/msg.c | 111 +++++++++++++++++++++++++++++ 4 files changed, 133 insertions(+) create mode 100644 drivers/firmware/arm_scmi/msg.c diff --git a/drivers/firmware/arm_scmi/Kconfig b/drivers/firmware/arm_scmi/Kconfig index cd84e5f2ee02..24fed705b02c 100644 --- a/drivers/firmware/arm_scmi/Kconfig +++ b/drivers/firmware/arm_scmi/Kconfig @@ -36,6 +36,12 @@ config ARM_SCMI_HAVE_SHMEM This declares whether a shared memory based transport for SCMI is available. +config ARM_SCMI_HAVE_MSG + bool + help + This declares whether a message passing based transport for SCMI is + available. + config ARM_SCMI_TRANSPORT_MAILBOX bool "SCMI transport based on Mailbox" depends on MAILBOX diff --git a/drivers/firmware/arm_scmi/Makefile b/drivers/firmware/arm_scmi/Makefile index e0e6bd3dba9e..aaad9f6589aa 100644 --- a/drivers/firmware/arm_scmi/Makefile +++ b/drivers/firmware/arm_scmi/Makefile @@ -4,6 +4,7 @@ scmi-driver-y = driver.o notify.o scmi-transport-$(CONFIG_ARM_SCMI_HAVE_SHMEM) = shmem.o scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_MAILBOX) += mailbox.o scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_SMC) += smc.o +scmi-transport-$(CONFIG_ARM_SCMI_HAVE_MSG) += msg.o scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o voltage.o scmi-module-objs := $(scmi-bus-y) $(scmi-driver-y) $(scmi-protocols-y) \ $(scmi-transport-y) diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h index 6320345865e8..4da6cecc0a1c 100644 --- a/drivers/firmware/arm_scmi/common.h +++ b/drivers/firmware/arm_scmi/common.h @@ -432,6 +432,21 @@ void shmem_clear_channel(struct scmi_shared_mem __iomem *shmem); bool shmem_poll_done(struct scmi_shared_mem __iomem *shmem, struct scmi_xfer *xfer); +/* declarations for message passing transports */ +struct scmi_msg_payld; + +/* Maximum overhead of message w.r.t. struct scmi_desc.max_msg_size */ +#define SCMI_MSG_MAX_PROT_OVERHEAD (2 * sizeof(__le32)) + +size_t msg_response_size(struct scmi_xfer *xfer); +size_t msg_command_size(struct scmi_xfer *xfer); +void msg_tx_prepare(struct scmi_msg_payld *msg, struct scmi_xfer *xfer); +u32 msg_read_header(struct scmi_msg_payld *msg); +void msg_fetch_response(struct scmi_msg_payld *msg, size_t len, + struct scmi_xfer *xfer); +void msg_fetch_notification(struct scmi_msg_payld *msg, size_t len, + size_t max_len, struct scmi_xfer *xfer); + void scmi_notification_instance_data_set(const struct scmi_handle *handle, void *priv); void *scmi_notification_instance_data_get(const struct scmi_handle *handle); diff --git a/drivers/firmware/arm_scmi/msg.c b/drivers/firmware/arm_scmi/msg.c new file mode 100644 index 000000000000..d33a704e5814 --- /dev/null +++ b/drivers/firmware/arm_scmi/msg.c @@ -0,0 +1,111 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * For transports using message passing. + * + * Derived from shm.c. + * + * Copyright (C) 2019-2021 ARM Ltd. + * Copyright (C) 2020-2021 OpenSynergy GmbH + */ + +#include + +#include "common.h" + +/* + * struct scmi_msg_payld - Transport SDU layout + * + * The SCMI specification requires all parameters, message headers, return + * arguments or any protocol data to be expressed in little endian format only. + */ +struct scmi_msg_payld { + __le32 msg_header; + __le32 msg_payload[]; +}; + +/** + * msg_command_size() - Actual size of transport SDU for command. + * + * @xfer: message which core has prepared for sending + * + * Return: transport SDU size. + */ +size_t msg_command_size(struct scmi_xfer *xfer) +{ + return sizeof(struct scmi_msg_payld) + xfer->tx.len; +} + +/** + * msg_response_size() - Maximum size of transport SDU for response. + * + * @xfer: message which core has prepared for sending + * + * Return: transport SDU size. + */ +size_t msg_response_size(struct scmi_xfer *xfer) +{ + return sizeof(struct scmi_msg_payld) + sizeof(__le32) + xfer->rx.len; +} + +/** + * msg_tx_prepare() - Set up transport SDU for command. + * + * @msg: transport SDU for command + * @xfer: message which is being sent + */ +void msg_tx_prepare(struct scmi_msg_payld *msg, struct scmi_xfer *xfer) +{ + msg->msg_header = cpu_to_le32(pack_scmi_header(&xfer->hdr)); + if (xfer->tx.buf) + memcpy(msg->msg_payload, xfer->tx.buf, xfer->tx.len); +} + +/** + * msg_read_header() - Read SCMI header from transport SDU. + * + * @msg: transport SDU + * + * Return: SCMI header + */ +u32 msg_read_header(struct scmi_msg_payld *msg) +{ + return le32_to_cpu(msg->msg_header); +} + +/** + * msg_fetch_response() - Fetch response SCMI payload from transport SDU. + * + * @msg: transport SDU with response + * @len: transport SDU size + * @xfer: message being responded to + */ +void msg_fetch_response(struct scmi_msg_payld *msg, size_t len, + struct scmi_xfer *xfer) +{ + size_t prefix_len = sizeof(*msg) + sizeof(msg->msg_payload[0]); + + xfer->hdr.status = le32_to_cpu(msg->msg_payload[0]); + xfer->rx.len = min_t(size_t, xfer->rx.len, + len >= prefix_len ? len - prefix_len : 0); + + /* Take a copy to the rx buffer.. */ + memcpy(xfer->rx.buf, &msg->msg_payload[1], xfer->rx.len); +} + +/** + * msg_fetch_notification() - Fetch notification payload from transport SDU. + * + * @msg: transport SDU with notification + * @len: transport SDU size + * @max_len: maximum SCMI payload size to fetch + * @xfer: notification message + */ +void msg_fetch_notification(struct scmi_msg_payld *msg, size_t len, + size_t max_len, struct scmi_xfer *xfer) +{ + xfer->rx.len = min_t(size_t, max_len, + len >= sizeof(*msg) ? len - sizeof(*msg) : 0); + + /* Take a copy to the rx buffer.. */ + memcpy(xfer->rx.buf, msg->msg_payload, xfer->rx.len); +} From 7885281260f9b952dc66b67182a2218b01e7859f Mon Sep 17 00:00:00 2001 From: Peter Hilber Date: Tue, 3 Aug 2021 14:10:21 +0100 Subject: [PATCH 12/18] firmware: arm_scmi: Add optional link_supplier() transport op Some transports are also effectively registered with other kernel subsystem in order to be properly probed and initialized; as a consequence such kind of transports, and their related devices, might still not have been probed and initialized at the time the main SCMI core driver is probed. Add an optional .link_supplier() transport operation which can be used by the core SCMI stack to dynamically check if the transport is ready and dynamically link its device to the SCMI platform instance device. Link: https://lore.kernel.org/r/20210803131024.40280-13-cristian.marussi@arm.com Signed-off-by: Peter Hilber [ Cristian: reworded commit message ] Signed-off-by: Cristian Marussi Signed-off-by: Sudeep Holla --- drivers/firmware/arm_scmi/common.h | 2 ++ drivers/firmware/arm_scmi/driver.c | 6 ++++++ 2 files changed, 8 insertions(+) diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h index 4da6cecc0a1c..024d97fbf97b 100644 --- a/drivers/firmware/arm_scmi/common.h +++ b/drivers/firmware/arm_scmi/common.h @@ -348,6 +348,7 @@ struct scmi_chan_info { /** * struct scmi_transport_ops - Structure representing a SCMI transport ops * + * @link_supplier: Optional callback to add link to a supplier device * @chan_available: Callback to check if channel is available or not * @chan_setup: Callback to allocate and setup a channel * @chan_free: Callback to free a channel @@ -362,6 +363,7 @@ struct scmi_chan_info { * @poll_done: Callback to poll transfer status */ struct scmi_transport_ops { + int (*link_supplier)(struct device *dev); bool (*chan_available)(struct device *dev, int idx); int (*chan_setup)(struct scmi_chan_info *cinfo, struct device *dev, bool tx); diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c index a907f3fe4c08..7ae4ec8f8d1f 100644 --- a/drivers/firmware/arm_scmi/driver.c +++ b/drivers/firmware/arm_scmi/driver.c @@ -1814,6 +1814,12 @@ static int scmi_probe(struct platform_device *pdev) handle->devm_protocol_get = scmi_devm_protocol_get; handle->devm_protocol_put = scmi_devm_protocol_put; + if (desc->ops->link_supplier) { + ret = desc->ops->link_supplier(dev); + if (ret) + return ret; + } + ret = scmi_txrx_setup(info, dev, SCMI_PROTOCOL_BASE); if (ret) return ret; From 60625667c439e6e1945d464b6eb296d144c5cb2a Mon Sep 17 00:00:00 2001 From: Igor Skalkin Date: Tue, 3 Aug 2021 14:10:22 +0100 Subject: [PATCH 13/18] dt-bindings: arm: Add virtio transport for SCMI Document the properties for arm,scmi-virtio compatible nodes. The backing virtio SCMI device is described in patch [1]. While doing that, make shmem property required only for pre-existing mailbox and smc transports, since virtio-scmi does not need it. [1] https://lists.oasis-open.org/archives/virtio-comment/202102/msg00018.html Link: https://lore.kernel.org/r/20210803131024.40280-14-cristian.marussi@arm.com Co-developed-by: Peter Hilber Reviewed-by: Rob Herring Signed-off-by: Igor Skalkin [ Peter: Adapted patch for submission to upstream. ] Signed-off-by: Peter Hilber [ Cristian: converted to yaml format, moved shmen required property. ] Signed-off-by: Cristian Marussi Signed-off-by: Sudeep Holla --- Documentation/devicetree/bindings/firmware/arm,scmi.yaml | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml index cebf6ffe70d5..5c4c6782e052 100644 --- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml +++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml @@ -34,6 +34,10 @@ properties: - description: SCMI compliant firmware with ARM SMC/HVC transport items: - const: arm,scmi-smc + - description: SCMI compliant firmware with SCMI Virtio transport. + The virtio transport only supports a single device. + items: + - const: arm,scmi-virtio interrupts: description: @@ -172,6 +176,7 @@ patternProperties: Each sub-node represents a protocol supported. If the platform supports a dedicated communication channel for a particular protocol, then the corresponding transport properties must be present. + The virtio transport does not support a dedicated communication channel. properties: reg: @@ -195,7 +200,6 @@ patternProperties: required: - compatible - - shmem if: properties: @@ -209,6 +213,7 @@ then: required: - mboxes + - shmem else: if: @@ -219,6 +224,7 @@ else: then: required: - arm,smc-id + - shmem examples: - | From 13fba878ccdd15b1c2fdce424995744dc40eaf16 Mon Sep 17 00:00:00 2001 From: Cristian Marussi Date: Tue, 3 Aug 2021 14:10:23 +0100 Subject: [PATCH 14/18] firmware: arm_scmi: Add priv parameter to scmi_rx_callback Add a new opaque void *priv parameter to scmi_rx_callback which can be optionally provided by the transport layer when invoking scmi_rx_callback and that will be passed back to the transport layer in xfer->priv. This can be used by transports that needs to keep track of their specific data structures together with the valid xfers. Link: https://lore.kernel.org/r/20210803131024.40280-15-cristian.marussi@arm.com Signed-off-by: Cristian Marussi Signed-off-by: Sudeep Holla --- drivers/firmware/arm_scmi/common.h | 4 +++- drivers/firmware/arm_scmi/driver.c | 17 ++++++++++++----- drivers/firmware/arm_scmi/mailbox.c | 2 +- drivers/firmware/arm_scmi/smc.c | 3 ++- 4 files changed, 18 insertions(+), 8 deletions(-) diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h index 024d97fbf97b..7864c21269b0 100644 --- a/drivers/firmware/arm_scmi/common.h +++ b/drivers/firmware/arm_scmi/common.h @@ -172,6 +172,7 @@ struct scmi_msg { * - SCMI_XFER_SENT_OK -> SCMI_XFER_DRESP_OK * (Missing synchronous response is assumed OK and ignored) * @lock: A spinlock to protect state and busy fields. + * @priv: A pointer for transport private usage. */ struct scmi_xfer { int transfer_id; @@ -192,6 +193,7 @@ struct scmi_xfer { int state; /* A lock to protect state and busy fields */ spinlock_t lock; + void *priv; }; /* @@ -417,7 +419,7 @@ extern const struct scmi_desc scmi_mailbox_desc; extern const struct scmi_desc scmi_smc_desc; #endif -void scmi_rx_callback(struct scmi_chan_info *cinfo, u32 msg_hdr); +void scmi_rx_callback(struct scmi_chan_info *cinfo, u32 msg_hdr, void *priv); void scmi_free_channel(struct scmi_chan_info *cinfo, struct idr *idr, int id); /* shmem related declarations */ diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c index 7ae4ec8f8d1f..aaca01a4d752 100644 --- a/drivers/firmware/arm_scmi/driver.c +++ b/drivers/firmware/arm_scmi/driver.c @@ -609,7 +609,8 @@ static inline void scmi_clear_channel(struct scmi_info *info, info->desc->ops->clear_channel(cinfo); } -static void scmi_handle_notification(struct scmi_chan_info *cinfo, u32 msg_hdr) +static void scmi_handle_notification(struct scmi_chan_info *cinfo, + u32 msg_hdr, void *priv) { struct scmi_xfer *xfer; struct device *dev = cinfo->dev; @@ -627,6 +628,8 @@ static void scmi_handle_notification(struct scmi_chan_info *cinfo, u32 msg_hdr) } unpack_scmi_header(msg_hdr, &xfer->hdr); + if (priv) + xfer->priv = priv; info->desc->ops->fetch_notification(cinfo, info->desc->max_msg_size, xfer); scmi_notify(cinfo->handle, xfer->hdr.protocol_id, @@ -641,7 +644,8 @@ static void scmi_handle_notification(struct scmi_chan_info *cinfo, u32 msg_hdr) scmi_clear_channel(info, cinfo); } -static void scmi_handle_response(struct scmi_chan_info *cinfo, u32 msg_hdr) +static void scmi_handle_response(struct scmi_chan_info *cinfo, + u32 msg_hdr, void *priv) { struct scmi_xfer *xfer; struct scmi_info *info = handle_to_scmi_info(cinfo->handle); @@ -656,6 +660,8 @@ static void scmi_handle_response(struct scmi_chan_info *cinfo, u32 msg_hdr) if (xfer->hdr.type == MSG_TYPE_DELAYED_RESP) xfer->rx.len = info->desc->max_msg_size; + if (priv) + xfer->priv = priv; info->desc->ops->fetch_response(cinfo, xfer); trace_scmi_rx_done(xfer->transfer_id, xfer->hdr.id, @@ -677,6 +683,7 @@ static void scmi_handle_response(struct scmi_chan_info *cinfo, u32 msg_hdr) * * @cinfo: SCMI channel info * @msg_hdr: Message header + * @priv: Transport specific private data. * * Processes one received message to appropriate transfer information and * signals completion of the transfer. @@ -684,17 +691,17 @@ static void scmi_handle_response(struct scmi_chan_info *cinfo, u32 msg_hdr) * NOTE: This function will be invoked in IRQ context, hence should be * as optimal as possible. */ -void scmi_rx_callback(struct scmi_chan_info *cinfo, u32 msg_hdr) +void scmi_rx_callback(struct scmi_chan_info *cinfo, u32 msg_hdr, void *priv) { u8 msg_type = MSG_XTRACT_TYPE(msg_hdr); switch (msg_type) { case MSG_TYPE_NOTIFICATION: - scmi_handle_notification(cinfo, msg_hdr); + scmi_handle_notification(cinfo, msg_hdr, priv); break; case MSG_TYPE_COMMAND: case MSG_TYPE_DELAYED_RESP: - scmi_handle_response(cinfo, msg_hdr); + scmi_handle_response(cinfo, msg_hdr, priv); break; default: WARN_ONCE(1, "received unknown msg_type:%d\n", msg_type); diff --git a/drivers/firmware/arm_scmi/mailbox.c b/drivers/firmware/arm_scmi/mailbox.c index e3dcb58314ae..e09eb12bf421 100644 --- a/drivers/firmware/arm_scmi/mailbox.c +++ b/drivers/firmware/arm_scmi/mailbox.c @@ -43,7 +43,7 @@ static void rx_callback(struct mbox_client *cl, void *m) { struct scmi_mailbox *smbox = client_to_scmi_mailbox(cl); - scmi_rx_callback(smbox->cinfo, shmem_read_header(smbox->shmem)); + scmi_rx_callback(smbox->cinfo, shmem_read_header(smbox->shmem), NULL); } static bool mailbox_chan_available(struct device *dev, int idx) diff --git a/drivers/firmware/arm_scmi/smc.c b/drivers/firmware/arm_scmi/smc.c index bed5596c7209..4effecc3bb46 100644 --- a/drivers/firmware/arm_scmi/smc.c +++ b/drivers/firmware/arm_scmi/smc.c @@ -154,7 +154,8 @@ static int smc_send_message(struct scmi_chan_info *cinfo, if (scmi_info->irq) wait_for_completion(&scmi_info->tx_complete); - scmi_rx_callback(scmi_info->cinfo, shmem_read_header(scmi_info->shmem)); + scmi_rx_callback(scmi_info->cinfo, + shmem_read_header(scmi_info->shmem), NULL); mutex_unlock(&scmi_info->shmem_lock); From 46abe13b5e3db187e52cd0de06c07bbce010726c Mon Sep 17 00:00:00 2001 From: Igor Skalkin Date: Tue, 3 Aug 2021 14:10:24 +0100 Subject: [PATCH 15/18] firmware: arm_scmi: Add virtio transport This transport enables communications with an SCMI platform through virtio; the SCMI platform will be represented by a virtio device. Implement an SCMI virtio driver according to the virtio SCMI device spec [1]. Virtio device id 32 has been reserved for the SCMI device [2]. The virtio transport has one Tx channel (virtio cmdq, A2P channel) and at most one Rx channel (virtio eventq, P2A channel). The following feature bit defined in [1] is not implemented: VIRTIO_SCMI_F_SHARED_MEMORY. The number of messages which can be pending simultaneously is restricted according to the virtqueue capacity negotiated at probing time. As soon as Rx channel message buffers are allocated or have been read out by the arm-scmi driver, feed them back to the virtio device. Since some virtio devices may not have the short response time exhibited by SCMI platforms using other transports, set a generous response timeout. SCMI polling mode is not supported by this virtio transport since deemed meaningless: polling mode operation is offered by the SCMI core to those transports that could not provide a completion interrupt on the TX path, which is never the case for virtio whose core callbacks can easily call into core scmi_rx_callback upon messages reception. [1] https://github.com/oasis-tcs/virtio-spec/blob/master/virtio-scmi.tex [2] https://www.oasis-open.org/committees/ballot.php?id=3496 Link: https://lore.kernel.org/r/20210803131024.40280-16-cristian.marussi@arm.com Cc: "Michael S. Tsirkin" Cc: Jason Wang Co-developed-by: Peter Hilber Co-developed-by: Cristian Marussi Signed-off-by: Igor Skalkin [ Peter: Adapted patch for submission to upstream. ] Signed-off-by: Peter Hilber [ Cristian: simplified driver logic, changed link_supplier and channel available/setup logic, removed dummy callbacks ] Signed-off-by: Cristian Marussi Signed-off-by: Sudeep Holla --- MAINTAINERS | 1 + drivers/firmware/arm_scmi/Kconfig | 11 + drivers/firmware/arm_scmi/Makefile | 1 + drivers/firmware/arm_scmi/common.h | 3 + drivers/firmware/arm_scmi/driver.c | 3 + drivers/firmware/arm_scmi/virtio.c | 491 +++++++++++++++++++++++++++++ include/uapi/linux/virtio_ids.h | 1 + include/uapi/linux/virtio_scmi.h | 24 ++ 8 files changed, 535 insertions(+) create mode 100644 drivers/firmware/arm_scmi/virtio.c create mode 100644 include/uapi/linux/virtio_scmi.h diff --git a/MAINTAINERS b/MAINTAINERS index a61f4f3b78a9..db1c7b74642e 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -17940,6 +17940,7 @@ F: drivers/regulator/scmi-regulator.c F: drivers/reset/reset-scmi.c F: include/linux/sc[mp]i_protocol.h F: include/trace/events/scmi.h +F: include/uapi/linux/virtio_scmi.h SYSTEM RESET/SHUTDOWN DRIVERS M: Sebastian Reichel diff --git a/drivers/firmware/arm_scmi/Kconfig b/drivers/firmware/arm_scmi/Kconfig index 24fed705b02c..7f4d2435503b 100644 --- a/drivers/firmware/arm_scmi/Kconfig +++ b/drivers/firmware/arm_scmi/Kconfig @@ -66,6 +66,17 @@ config ARM_SCMI_TRANSPORT_SMC If you want the ARM SCMI PROTOCOL stack to include support for a transport based on SMC, answer Y. +config ARM_SCMI_TRANSPORT_VIRTIO + bool "SCMI transport based on VirtIO" + depends on VIRTIO + select ARM_SCMI_HAVE_TRANSPORT + select ARM_SCMI_HAVE_MSG + help + This enables the virtio based transport for SCMI. + + If you want the ARM SCMI PROTOCOL stack to include support for a + transport based on VirtIO, answer Y. + endif #ARM_SCMI_PROTOCOL config ARM_SCMI_POWER_DOMAIN diff --git a/drivers/firmware/arm_scmi/Makefile b/drivers/firmware/arm_scmi/Makefile index aaad9f6589aa..1dcf123d64ab 100644 --- a/drivers/firmware/arm_scmi/Makefile +++ b/drivers/firmware/arm_scmi/Makefile @@ -5,6 +5,7 @@ scmi-transport-$(CONFIG_ARM_SCMI_HAVE_SHMEM) = shmem.o scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_MAILBOX) += mailbox.o scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_SMC) += smc.o scmi-transport-$(CONFIG_ARM_SCMI_HAVE_MSG) += msg.o +scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_VIRTIO) += virtio.o scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o voltage.o scmi-module-objs := $(scmi-bus-y) $(scmi-driver-y) $(scmi-protocols-y) \ $(scmi-transport-y) diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h index 7864c21269b0..dea1bfbe1052 100644 --- a/drivers/firmware/arm_scmi/common.h +++ b/drivers/firmware/arm_scmi/common.h @@ -418,6 +418,9 @@ extern const struct scmi_desc scmi_mailbox_desc; #ifdef CONFIG_ARM_SCMI_TRANSPORT_SMC extern const struct scmi_desc scmi_smc_desc; #endif +#ifdef CONFIG_ARM_SCMI_TRANSPORT_VIRTIO +extern const struct scmi_desc scmi_virtio_desc; +#endif void scmi_rx_callback(struct scmi_chan_info *cinfo, u32 msg_hdr, void *priv); void scmi_free_channel(struct scmi_chan_info *cinfo, struct idr *idr, int id); diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c index aaca01a4d752..00fcacd06562 100644 --- a/drivers/firmware/arm_scmi/driver.c +++ b/drivers/firmware/arm_scmi/driver.c @@ -1983,6 +1983,9 @@ static const struct of_device_id scmi_of_match[] = { #endif #ifdef CONFIG_ARM_SCMI_TRANSPORT_SMC { .compatible = "arm,scmi-smc", .data = &scmi_smc_desc}, +#endif +#ifdef CONFIG_ARM_SCMI_TRANSPORT_VIRTIO + { .compatible = "arm,scmi-virtio", .data = &scmi_virtio_desc}, #endif { /* Sentinel */ }, }; diff --git a/drivers/firmware/arm_scmi/virtio.c b/drivers/firmware/arm_scmi/virtio.c new file mode 100644 index 000000000000..3dacf794b177 --- /dev/null +++ b/drivers/firmware/arm_scmi/virtio.c @@ -0,0 +1,491 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Virtio Transport driver for Arm System Control and Management Interface + * (SCMI). + * + * Copyright (C) 2020-2021 OpenSynergy. + * Copyright (C) 2021 ARM Ltd. + */ + +/** + * DOC: Theory of Operation + * + * The scmi-virtio transport implements a driver for the virtio SCMI device. + * + * There is one Tx channel (virtio cmdq, A2P channel) and at most one Rx + * channel (virtio eventq, P2A channel). Each channel is implemented through a + * virtqueue. Access to each virtqueue is protected by spinlocks. + */ + +#include +#include +#include +#include + +#include +#include + +#include "common.h" + +#define VIRTIO_SCMI_MAX_MSG_SIZE 128 /* Value may be increased. */ +#define VIRTIO_SCMI_MAX_PDU_SIZE \ + (VIRTIO_SCMI_MAX_MSG_SIZE + SCMI_MSG_MAX_PROT_OVERHEAD) +#define DESCRIPTORS_PER_TX_MSG 2 + +/** + * struct scmi_vio_channel - Transport channel information + * + * @vqueue: Associated virtqueue + * @cinfo: SCMI Tx or Rx channel + * @free_list: List of unused scmi_vio_msg, maintained for Tx channels only + * @is_rx: Whether channel is an Rx channel + * @ready: Whether transport user is ready to hear about channel + * @max_msg: Maximum number of pending messages for this channel. + * @lock: Protects access to all members except ready. + * @ready_lock: Protects access to ready. If required, it must be taken before + * lock. + */ +struct scmi_vio_channel { + struct virtqueue *vqueue; + struct scmi_chan_info *cinfo; + struct list_head free_list; + bool is_rx; + bool ready; + unsigned int max_msg; + /* lock to protect access to all members except ready. */ + spinlock_t lock; + /* lock to rotects access to ready flag. */ + spinlock_t ready_lock; +}; + +/** + * struct scmi_vio_msg - Transport PDU information + * + * @request: SDU used for commands + * @input: SDU used for (delayed) responses and notifications + * @list: List which scmi_vio_msg may be part of + * @rx_len: Input SDU size in bytes, once input has been received + */ +struct scmi_vio_msg { + struct scmi_msg_payld *request; + struct scmi_msg_payld *input; + struct list_head list; + unsigned int rx_len; +}; + +/* Only one SCMI VirtIO device can possibly exist */ +static struct virtio_device *scmi_vdev; + +static bool scmi_vio_have_vq_rx(struct virtio_device *vdev) +{ + return virtio_has_feature(vdev, VIRTIO_SCMI_F_P2A_CHANNELS); +} + +static int scmi_vio_feed_vq_rx(struct scmi_vio_channel *vioch, + struct scmi_vio_msg *msg) +{ + struct scatterlist sg_in; + int rc; + unsigned long flags; + + sg_init_one(&sg_in, msg->input, VIRTIO_SCMI_MAX_PDU_SIZE); + + spin_lock_irqsave(&vioch->lock, flags); + + rc = virtqueue_add_inbuf(vioch->vqueue, &sg_in, 1, msg, GFP_ATOMIC); + if (rc) + dev_err_once(vioch->cinfo->dev, + "failed to add to virtqueue (%d)\n", rc); + else + virtqueue_kick(vioch->vqueue); + + spin_unlock_irqrestore(&vioch->lock, flags); + + return rc; +} + +static void scmi_finalize_message(struct scmi_vio_channel *vioch, + struct scmi_vio_msg *msg) +{ + if (vioch->is_rx) { + scmi_vio_feed_vq_rx(vioch, msg); + } else { + unsigned long flags; + + spin_lock_irqsave(&vioch->lock, flags); + list_add(&msg->list, &vioch->free_list); + spin_unlock_irqrestore(&vioch->lock, flags); + } +} + +static void scmi_vio_complete_cb(struct virtqueue *vqueue) +{ + unsigned long ready_flags; + unsigned long flags; + unsigned int length; + struct scmi_vio_channel *vioch; + struct scmi_vio_msg *msg; + bool cb_enabled = true; + + if (WARN_ON_ONCE(!vqueue->vdev->priv)) + return; + vioch = &((struct scmi_vio_channel *)vqueue->vdev->priv)[vqueue->index]; + + for (;;) { + spin_lock_irqsave(&vioch->ready_lock, ready_flags); + + if (!vioch->ready) { + if (!cb_enabled) + (void)virtqueue_enable_cb(vqueue); + goto unlock_ready_out; + } + + spin_lock_irqsave(&vioch->lock, flags); + if (cb_enabled) { + virtqueue_disable_cb(vqueue); + cb_enabled = false; + } + msg = virtqueue_get_buf(vqueue, &length); + if (!msg) { + if (virtqueue_enable_cb(vqueue)) + goto unlock_out; + cb_enabled = true; + } + spin_unlock_irqrestore(&vioch->lock, flags); + + if (msg) { + msg->rx_len = length; + scmi_rx_callback(vioch->cinfo, + msg_read_header(msg->input), msg); + + scmi_finalize_message(vioch, msg); + } + + spin_unlock_irqrestore(&vioch->ready_lock, ready_flags); + } + +unlock_out: + spin_unlock_irqrestore(&vioch->lock, flags); +unlock_ready_out: + spin_unlock_irqrestore(&vioch->ready_lock, ready_flags); +} + +static const char *const scmi_vio_vqueue_names[] = { "tx", "rx" }; + +static vq_callback_t *scmi_vio_complete_callbacks[] = { + scmi_vio_complete_cb, + scmi_vio_complete_cb +}; + +static unsigned int virtio_get_max_msg(struct scmi_chan_info *base_cinfo) +{ + struct scmi_vio_channel *vioch = base_cinfo->transport_info; + + return vioch->max_msg; +} + +static int virtio_link_supplier(struct device *dev) +{ + if (!scmi_vdev) { + dev_notice_once(dev, + "Deferring probe after not finding a bound scmi-virtio device\n"); + return -EPROBE_DEFER; + } + + if (!device_link_add(dev, &scmi_vdev->dev, + DL_FLAG_AUTOREMOVE_CONSUMER)) { + dev_err(dev, "Adding link to supplier virtio device failed\n"); + return -ECANCELED; + } + + return 0; +} + +static bool virtio_chan_available(struct device *dev, int idx) +{ + struct scmi_vio_channel *channels, *vioch = NULL; + + if (WARN_ON_ONCE(!scmi_vdev)) + return false; + + channels = (struct scmi_vio_channel *)scmi_vdev->priv; + + switch (idx) { + case VIRTIO_SCMI_VQ_TX: + vioch = &channels[VIRTIO_SCMI_VQ_TX]; + break; + case VIRTIO_SCMI_VQ_RX: + if (scmi_vio_have_vq_rx(scmi_vdev)) + vioch = &channels[VIRTIO_SCMI_VQ_RX]; + break; + default: + return false; + } + + return vioch && !vioch->cinfo ? true : false; +} + +static int virtio_chan_setup(struct scmi_chan_info *cinfo, struct device *dev, + bool tx) +{ + unsigned long flags; + struct scmi_vio_channel *vioch; + int index = tx ? VIRTIO_SCMI_VQ_TX : VIRTIO_SCMI_VQ_RX; + int i; + + if (!scmi_vdev) + return -EPROBE_DEFER; + + vioch = &((struct scmi_vio_channel *)scmi_vdev->priv)[index]; + + for (i = 0; i < vioch->max_msg; i++) { + struct scmi_vio_msg *msg; + + msg = devm_kzalloc(cinfo->dev, sizeof(*msg), GFP_KERNEL); + if (!msg) + return -ENOMEM; + + if (tx) { + msg->request = devm_kzalloc(cinfo->dev, + VIRTIO_SCMI_MAX_PDU_SIZE, + GFP_KERNEL); + if (!msg->request) + return -ENOMEM; + } + + msg->input = devm_kzalloc(cinfo->dev, VIRTIO_SCMI_MAX_PDU_SIZE, + GFP_KERNEL); + if (!msg->input) + return -ENOMEM; + + if (tx) { + spin_lock_irqsave(&vioch->lock, flags); + list_add_tail(&msg->list, &vioch->free_list); + spin_unlock_irqrestore(&vioch->lock, flags); + } else { + scmi_vio_feed_vq_rx(vioch, msg); + } + } + + spin_lock_irqsave(&vioch->lock, flags); + cinfo->transport_info = vioch; + /* Indirectly setting channel not available any more */ + vioch->cinfo = cinfo; + spin_unlock_irqrestore(&vioch->lock, flags); + + spin_lock_irqsave(&vioch->ready_lock, flags); + vioch->ready = true; + spin_unlock_irqrestore(&vioch->ready_lock, flags); + + return 0; +} + +static int virtio_chan_free(int id, void *p, void *data) +{ + unsigned long flags; + struct scmi_chan_info *cinfo = p; + struct scmi_vio_channel *vioch = cinfo->transport_info; + + spin_lock_irqsave(&vioch->ready_lock, flags); + vioch->ready = false; + spin_unlock_irqrestore(&vioch->ready_lock, flags); + + scmi_free_channel(cinfo, data, id); + + spin_lock_irqsave(&vioch->lock, flags); + vioch->cinfo = NULL; + spin_unlock_irqrestore(&vioch->lock, flags); + + return 0; +} + +static int virtio_send_message(struct scmi_chan_info *cinfo, + struct scmi_xfer *xfer) +{ + struct scmi_vio_channel *vioch = cinfo->transport_info; + struct scatterlist sg_out; + struct scatterlist sg_in; + struct scatterlist *sgs[DESCRIPTORS_PER_TX_MSG] = { &sg_out, &sg_in }; + unsigned long flags; + int rc; + struct scmi_vio_msg *msg; + + spin_lock_irqsave(&vioch->lock, flags); + + if (list_empty(&vioch->free_list)) { + spin_unlock_irqrestore(&vioch->lock, flags); + return -EBUSY; + } + + msg = list_first_entry(&vioch->free_list, typeof(*msg), list); + list_del(&msg->list); + + msg_tx_prepare(msg->request, xfer); + + sg_init_one(&sg_out, msg->request, msg_command_size(xfer)); + sg_init_one(&sg_in, msg->input, msg_response_size(xfer)); + + rc = virtqueue_add_sgs(vioch->vqueue, sgs, 1, 1, msg, GFP_ATOMIC); + if (rc) { + list_add(&msg->list, &vioch->free_list); + dev_err_once(vioch->cinfo->dev, + "%s() failed to add to virtqueue (%d)\n", __func__, + rc); + } else { + virtqueue_kick(vioch->vqueue); + } + + spin_unlock_irqrestore(&vioch->lock, flags); + + return rc; +} + +static void virtio_fetch_response(struct scmi_chan_info *cinfo, + struct scmi_xfer *xfer) +{ + struct scmi_vio_msg *msg = xfer->priv; + + if (msg) { + msg_fetch_response(msg->input, msg->rx_len, xfer); + xfer->priv = NULL; + } +} + +static void virtio_fetch_notification(struct scmi_chan_info *cinfo, + size_t max_len, struct scmi_xfer *xfer) +{ + struct scmi_vio_msg *msg = xfer->priv; + + if (msg) { + msg_fetch_notification(msg->input, msg->rx_len, max_len, xfer); + xfer->priv = NULL; + } +} + +static const struct scmi_transport_ops scmi_virtio_ops = { + .link_supplier = virtio_link_supplier, + .chan_available = virtio_chan_available, + .chan_setup = virtio_chan_setup, + .chan_free = virtio_chan_free, + .get_max_msg = virtio_get_max_msg, + .send_message = virtio_send_message, + .fetch_response = virtio_fetch_response, + .fetch_notification = virtio_fetch_notification, +}; + +static int scmi_vio_probe(struct virtio_device *vdev) +{ + struct device *dev = &vdev->dev; + struct scmi_vio_channel *channels; + bool have_vq_rx; + int vq_cnt; + int i; + int ret; + struct virtqueue *vqs[VIRTIO_SCMI_VQ_MAX_CNT]; + + /* Only one SCMI VirtiO device allowed */ + if (scmi_vdev) + return -EINVAL; + + have_vq_rx = scmi_vio_have_vq_rx(vdev); + vq_cnt = have_vq_rx ? VIRTIO_SCMI_VQ_MAX_CNT : 1; + + channels = devm_kcalloc(dev, vq_cnt, sizeof(*channels), GFP_KERNEL); + if (!channels) + return -ENOMEM; + + if (have_vq_rx) + channels[VIRTIO_SCMI_VQ_RX].is_rx = true; + + ret = virtio_find_vqs(vdev, vq_cnt, vqs, scmi_vio_complete_callbacks, + scmi_vio_vqueue_names, NULL); + if (ret) { + dev_err(dev, "Failed to get %d virtqueue(s)\n", vq_cnt); + return ret; + } + + for (i = 0; i < vq_cnt; i++) { + unsigned int sz; + + spin_lock_init(&channels[i].lock); + spin_lock_init(&channels[i].ready_lock); + INIT_LIST_HEAD(&channels[i].free_list); + channels[i].vqueue = vqs[i]; + + sz = virtqueue_get_vring_size(channels[i].vqueue); + /* Tx messages need multiple descriptors. */ + if (!channels[i].is_rx) + sz /= DESCRIPTORS_PER_TX_MSG; + + if (sz > MSG_TOKEN_MAX) { + dev_info_once(dev, + "%s virtqueue could hold %d messages. Only %ld allowed to be pending.\n", + channels[i].is_rx ? "rx" : "tx", + sz, MSG_TOKEN_MAX); + sz = MSG_TOKEN_MAX; + } + channels[i].max_msg = sz; + } + + vdev->priv = channels; + scmi_vdev = vdev; + + return 0; +} + +static void scmi_vio_remove(struct virtio_device *vdev) +{ + vdev->config->reset(vdev); + vdev->config->del_vqs(vdev); + scmi_vdev = NULL; +} + +static int scmi_vio_validate(struct virtio_device *vdev) +{ + if (!virtio_has_feature(vdev, VIRTIO_F_VERSION_1)) { + dev_err(&vdev->dev, + "device does not comply with spec version 1.x\n"); + return -EINVAL; + } + + return 0; +} + +static unsigned int features[] = { + VIRTIO_SCMI_F_P2A_CHANNELS, +}; + +static const struct virtio_device_id id_table[] = { + { VIRTIO_ID_SCMI, VIRTIO_DEV_ANY_ID }, + { 0 } +}; + +static struct virtio_driver virtio_scmi_driver = { + .driver.name = "scmi-virtio", + .driver.owner = THIS_MODULE, + .feature_table = features, + .feature_table_size = ARRAY_SIZE(features), + .id_table = id_table, + .probe = scmi_vio_probe, + .remove = scmi_vio_remove, + .validate = scmi_vio_validate, +}; + +static int __init virtio_scmi_init(void) +{ + return register_virtio_driver(&virtio_scmi_driver); +} + +static void __exit virtio_scmi_exit(void) +{ + unregister_virtio_driver(&virtio_scmi_driver); +} + +const struct scmi_desc scmi_virtio_desc = { + .transport_init = virtio_scmi_init, + .transport_exit = virtio_scmi_exit, + .ops = &scmi_virtio_ops, + .max_rx_timeout_ms = 60000, /* for non-realtime virtio devices */ + .max_msg = 0, /* overridden by virtio_get_max_msg() */ + .max_msg_size = VIRTIO_SCMI_MAX_MSG_SIZE, +}; diff --git a/include/uapi/linux/virtio_ids.h b/include/uapi/linux/virtio_ids.h index 70a8057ad4bb..f74155f6882d 100644 --- a/include/uapi/linux/virtio_ids.h +++ b/include/uapi/linux/virtio_ids.h @@ -55,6 +55,7 @@ #define VIRTIO_ID_FS 26 /* virtio filesystem */ #define VIRTIO_ID_PMEM 27 /* virtio pmem */ #define VIRTIO_ID_MAC80211_HWSIM 29 /* virtio mac80211-hwsim */ +#define VIRTIO_ID_SCMI 32 /* virtio SCMI */ #define VIRTIO_ID_BT 40 /* virtio bluetooth */ /* diff --git a/include/uapi/linux/virtio_scmi.h b/include/uapi/linux/virtio_scmi.h new file mode 100644 index 000000000000..f8ddd04a3ace --- /dev/null +++ b/include/uapi/linux/virtio_scmi.h @@ -0,0 +1,24 @@ +/* SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR BSD-3-Clause) */ +/* + * Copyright (C) 2020-2021 OpenSynergy GmbH + * Copyright (C) 2021 ARM Ltd. + */ + +#ifndef _UAPI_LINUX_VIRTIO_SCMI_H +#define _UAPI_LINUX_VIRTIO_SCMI_H + +#include + +/* Device implements some SCMI notifications, or delayed responses. */ +#define VIRTIO_SCMI_F_P2A_CHANNELS 0 + +/* Device implements any SCMI statistics shared memory region */ +#define VIRTIO_SCMI_F_SHARED_MEMORY 1 + +/* Virtqueues */ + +#define VIRTIO_SCMI_VQ_TX 0 /* cmdq */ +#define VIRTIO_SCMI_VQ_RX 1 /* eventq */ +#define VIRTIO_SCMI_VQ_MAX_CNT 2 + +#endif /* _UAPI_LINUX_VIRTIO_SCMI_H */ From 1e7cbfaa66d39e78bd24df0c78b55df68176b59e Mon Sep 17 00:00:00 2001 From: Rishabh Bhatnagar Date: Wed, 4 Aug 2021 14:19:59 -0700 Subject: [PATCH 16/18] firmware: arm_scmi: Free mailbox channels if probe fails Mailbox channels for the base protocol are setup during probe. There can be a scenario where probe fails to acquire the base protocol due to a timeout leading to cleaning up of all device managed memory including the scmi_mailbox structure setup during mailbox_chan_setup function. | arm-scmi soc:qcom,scmi: timed out in resp(caller: version_get+0x84/0x140) | arm-scmi soc:qcom,scmi: unable to communicate with SCMI | arm-scmi: probe of soc:qcom,scmi failed with error -110 Now when a message arrives at cpu slightly after the timeout, the mailbox controller will try to call the rx_callback of the client and might end up accessing freed memory. | rx_callback+0x24/0x160 | mbox_chan_received_data+0x44/0x94 | __handle_irq_event_percpu+0xd4/0x240 This patch frees the mailbox channels setup during probe and adds some more error handling in case the probe fails. Link: https://lore.kernel.org/r/1628111999-21595-1-git-send-email-rishabhb@codeaurora.org Tested-by: Cristian Marussi Reviewed-by: Cristian Marussi Signed-off-by: Rishabh Bhatnagar Signed-off-by: Sudeep Holla --- drivers/firmware/arm_scmi/driver.c | 35 ++++++++++++++++++++---------- 1 file changed, 24 insertions(+), 11 deletions(-) diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c index 00fcacd06562..b28111ea7c8b 100644 --- a/drivers/firmware/arm_scmi/driver.c +++ b/drivers/firmware/arm_scmi/driver.c @@ -1787,6 +1787,21 @@ void scmi_protocol_device_unrequest(const struct scmi_device_id *id_table) mutex_unlock(&scmi_requested_devices_mtx); } +static int scmi_cleanup_txrx_channels(struct scmi_info *info) +{ + int ret; + struct idr *idr = &info->tx_idr; + + ret = idr_for_each(idr, info->desc->ops->chan_free, idr); + idr_destroy(&info->tx_idr); + + idr = &info->rx_idr; + ret = idr_for_each(idr, info->desc->ops->chan_free, idr); + idr_destroy(&info->rx_idr); + + return ret; +} + static int scmi_probe(struct platform_device *pdev) { int ret; @@ -1833,7 +1848,7 @@ static int scmi_probe(struct platform_device *pdev) ret = scmi_xfer_info_init(info); if (ret) - return ret; + goto clear_txrx_setup; if (scmi_notification_init(handle)) dev_err(dev, "SCMI Notifications NOT available.\n"); @@ -1846,7 +1861,7 @@ static int scmi_probe(struct platform_device *pdev) ret = scmi_protocol_acquire(handle, SCMI_PROTOCOL_BASE); if (ret) { dev_err(dev, "unable to communicate with SCMI\n"); - return ret; + goto notification_exit; } mutex_lock(&scmi_list_mutex); @@ -1885,6 +1900,12 @@ static int scmi_probe(struct platform_device *pdev) } return 0; + +notification_exit: + scmi_notification_exit(&info->handle); +clear_txrx_setup: + scmi_cleanup_txrx_channels(info); + return ret; } void scmi_free_channel(struct scmi_chan_info *cinfo, struct idr *idr, int id) @@ -1896,7 +1917,6 @@ static int scmi_remove(struct platform_device *pdev) { int ret = 0, id; struct scmi_info *info = platform_get_drvdata(pdev); - struct idr *idr = &info->tx_idr; struct device_node *child; mutex_lock(&scmi_list_mutex); @@ -1920,14 +1940,7 @@ static int scmi_remove(struct platform_device *pdev) idr_destroy(&info->active_protocols); /* Safe to free channels since no more users */ - ret = idr_for_each(idr, info->desc->ops->chan_free, idr); - idr_destroy(&info->tx_idr); - - idr = &info->rx_idr; - ret = idr_for_each(idr, info->desc->ops->chan_free, idr); - idr_destroy(&info->rx_idr); - - return ret; + return scmi_cleanup_txrx_channels(info); } static ssize_t protocol_version_show(struct device *dev, From d4fda7ec1d2a34ec50fc672896ba79d5ac8b7882 Mon Sep 17 00:00:00 2001 From: kernel test robot Date: Sun, 8 Aug 2021 01:31:27 +0800 Subject: [PATCH 17/18] firmware: arm_scmi: Fix boolconv.cocci warnings drivers/firmware/arm_scmi/virtio.c:225:40-45: WARNING: conversion to bool not needed here Remove unneeded conversion to bool Semantic patch information: Relational and logical operators evaluate to bool, explicit conversion is overly verbose and unneeded. Generated by: scripts/coccinelle/misc/boolconv.cocci Link: https://lore.kernel.org/r/20210807173127.GA43248@a24dbc127934 CC: Igor Skalkin Reported-by: kernel test robot Signed-off-by: kernel test robot Signed-off-by: Sudeep Holla --- drivers/firmware/arm_scmi/virtio.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/firmware/arm_scmi/virtio.c b/drivers/firmware/arm_scmi/virtio.c index 3dacf794b177..224577f86928 100644 --- a/drivers/firmware/arm_scmi/virtio.c +++ b/drivers/firmware/arm_scmi/virtio.c @@ -222,7 +222,7 @@ static bool virtio_chan_available(struct device *dev, int idx) return false; } - return vioch && !vioch->cinfo ? true : false; + return vioch && !vioch->cinfo; } static int virtio_chan_setup(struct scmi_chan_info *cinfo, struct device *dev, From c0397c85b53d0bc6b081ff22d0d07e8eae149bba Mon Sep 17 00:00:00 2001 From: Cristian Marussi Date: Mon, 9 Aug 2021 10:22:45 +0100 Subject: [PATCH 18/18] firmware: arm_scmi: Use WARN_ON() to check configured transports Use a WARN_ON() when SCMI stack is loaded to check the consistency of configured SCMI transports instead of the current compile-time check BUILD_BUG_ON() to avoid breaking bot-builds on random bad configs. Bail-out early and noisy during SCMI stack initialization if no transport was enabled in configuration since SCMI cannot work without at least one enabled transport and such constraint cannot be enforced in Kconfig due to circular dependency issues. Link: https://lore.kernel.org/r/20210809092245.8730-1-cristian.marussi@arm.com Reported-by: kernel test robot Signed-off-by: Cristian Marussi Signed-off-by: Sudeep Holla --- drivers/firmware/arm_scmi/driver.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c index b28111ea7c8b..b406b3f78f46 100644 --- a/drivers/firmware/arm_scmi/driver.c +++ b/drivers/firmware/arm_scmi/driver.c @@ -2067,9 +2067,11 @@ static int __init scmi_driver_init(void) { int ret; - scmi_bus_init(); + /* Bail out if no SCMI transport was configured */ + if (WARN_ON(!IS_ENABLED(CONFIG_ARM_SCMI_HAVE_TRANSPORT))) + return -EINVAL; - BUILD_BUG_ON(!IS_ENABLED(CONFIG_ARM_SCMI_HAVE_TRANSPORT)); + scmi_bus_init(); /* Initialize any compiled-in transport which provided an init/exit */ ret = scmi_transports_init();