linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v7 00/15] Introduce SCMI transport based on VirtIO
@ 2021-08-03 13:10 Cristian Marussi
  2021-08-03 13:10 ` [PATCH v7 01/15] firmware: arm_scmi: Add support for type handling in common functions Cristian Marussi
                   ` (15 more replies)
  0 siblings, 16 replies; 19+ messages in thread
From: Cristian Marussi @ 2021-08-03 13:10 UTC (permalink / raw)
  To: linux-kernel, linux-arm-kernel, virtualization, virtio-dev
  Cc: sudeep.holla, james.quinlan, Jonathan.Cameron, f.fainelli,
	etienne.carriere, vincent.guittot, souvik.chakravarty,
	cristian.marussi, igor.skalkin, peter.hilber, alex.bennee,
	jean-philippe, mikhail.golubev, anton.yakovlev, Vasyl.Vavrychuk,
	Andriy.Tryshnivskyy

Hi all,

While reworking this series starting from the work done up to V3 by
OpenSynergy, I am keeping the original autorship and list distribution
unchanged.

The main aim of this rework, as said, is to simplify where possible the
SCMI VirtIO support added in V3 by adding at first some new general
mechanisms in the SCMI Transport layer.

Indeed, after some initial small fixes, patches 03/04/05 add such new
additional mechanisms to the SCMI core to ease implementation of more
complex transports like virtio, while also addressing a few general issues
already potentially affecting existing transports.

In terms of rework I dropped original V3 patches 05/06/07/08/12 as no more
needed, and modified where needed the remaining original patches to take
advantage of the above mentioned new SCMI transport features.

DT bindings patch has been ported on top of freshly YAML converted arm,scmi
bindings.

Moreover, since V5 I dropped support for polling mode from the virtio-scmi
transport, since it is an optional general mechanism provided by the core
to allow transports lacking a completion IRQ to work and it seemed a
needless addition/complication in the context of virtio transport.

Additionally, since V5 I simplified a bit the virtio-scmi transport probing
sequence observing that, as of now, only one single SCMI VirtIO device can
be possibly used, since the SCMI VirtIO devices are not identifiable from
the VirtIO layer and neither they are currently identifiable from the DT
config; as a consequence only one single SCMI transport channel is
currently supported when using virtio-scmi transport.

The series has been tested using an emulated fake SCMI device and also a
proper SCP-fw SCMI stack running through QEMU vhost-users, with the SCMI
stack compiled, in both cases, as builtin and as a loadable module, running
tests against mocked SCMI Sensors using HWMON and IIO interfaces to check
the functionality of notifications and sync/async commands.

Virtio-scmi support has been exercised in the following testing scenario
on a JUNO board:

 - normal sync/async command transfers
 - notifications
 - concurrent delivery of correlated response and delayed responses
 - out-of-order delivery of delayed responses before related responses
 - unexpected delayed response delivery for sync commands
 - late delivery of timed-out responses and delayed responses

Some basic regression testing against mailbox transport has been performed
for commands and notifications too.

No sensible overhead in total handling time of commands and notifications
has been observed, even though this series do indeed add a considerable
amount of code to execute on TX path.

This series is based on sudeep/for-next/scmi [1] on top of

commit bdb8742dc6f7 ("firmware: arm_scmi: Fix range check for the maximum
		     number of pending messages")

Any feedback/testing is welcome :D

Thanks,
Cristian

[1]: https://git.kernel.org/pub/scm/linux/kernel/git/sudeep.holla/linux.git/log/?h=for-next/scmi
---
v6 --> v7
 - rebased on sudeep/for-next/scmi (v5.14-rc1)
 - added Cc: for maintainers regarding Virtio SCMI device number addition
   in include/uapi/linux/virtio_ids.h (previously only list was Cc'ed)
 - V6 patches 01 and 02 have been removed from V7 since already queued on
   sudeep/for-next/scmi
 - renamed scmi_desc .init/.exit to .transport_init/.exit
 - moved "firmware: arm_scmi: Add priv parameter to scmi_rx_callback" later
   in the series
 - simplified new SCMI Kconfig layout
 - refactored/simplified  support for non-polling transports
 - moved introduction of xfer refcounting from "firmware: arm_scmi:
   Introduce monotonically increasing tokens" to "firmware: arm_scmi:
   Handle concurrent and out-of-order messages"
 - renamed scmi_xfer_is_free to scmi_xfer_acquired
 - moved scmi_xfer_state_update() call inside scmi_xfer_command_acquire
 - added missing barrier in do_xfer()
 - added proper comment to justify pending_xfers sizing and relocated such
   #define directive
 - removed last_token atomic counter, now generating monotonic seqnums
   based on transfer_id

V5 --> V6:
 - removed delegated xfers and its usage
 - add and use *priv optional parameter in scmi_rx_callback()
 - made .poll_done and .clear_channel ops optional

V4 --> V5:
 - removed msg raw_payload helpers
 - reworked msg helpers to use xfer->priv reference
 - simplified SCMI device probe sequence (one static device)
 - added new SCMI Kconfig layout
 - removed SCMI virtio polling support

V3 --> V4:
 - using new delegated xfers support and monotonically increasing tokens
   in virtio transport
 - ported SCMI virtio transport DT bindings to YAML format
 - added virtio-scmi polling support
 - added delegated xfers support


Cristian Marussi (9):
  firmware: arm_scmi: Add support for type handling in common functions
  firmware: arm_scmi: Remove scmi_dump_header_dbg() helper
  firmware: arm_scmi: Add optional transport_init/exit support
  firmware: arm_scmi: Introduce monotonically increasing tokens
  firmware: arm_scmi: Handle concurrent and out-of-order messages
  firmware: arm_scmi: Make .clear_channel optional
  firmware: arm_scmi: Make polling mode optional
  firmware: arm_scmi: Make SCMI transports configurable
  firmware: arm_scmi: Add priv parameter to scmi_rx_callback

Igor Skalkin (4):
  firmware: arm_scmi: Make shmem support optional for transports
  firmware: arm_scmi: Add method to override max message number
  dt-bindings: arm: Add virtio transport for SCMI
  firmware: arm_scmi: Add virtio transport

Peter Hilber (2):
  firmware: arm_scmi: Add message passing abstractions for transports
  firmware: arm_scmi: Add optional link_supplier() transport op

 .../bindings/firmware/arm,scmi.yaml           |   8 +-
 MAINTAINERS                                   |   1 +
 drivers/firmware/Kconfig                      |  34 +-
 drivers/firmware/arm_scmi/Kconfig             |  95 +++
 drivers/firmware/arm_scmi/Makefile            |   8 +-
 drivers/firmware/arm_scmi/common.h            | 113 ++-
 drivers/firmware/arm_scmi/driver.c            | 649 +++++++++++++++---
 drivers/firmware/arm_scmi/mailbox.c           |   2 +-
 drivers/firmware/arm_scmi/msg.c               | 111 +++
 drivers/firmware/arm_scmi/smc.c               |   3 +-
 drivers/firmware/arm_scmi/virtio.c            | 491 +++++++++++++
 include/uapi/linux/virtio_ids.h               |   1 +
 include/uapi/linux/virtio_scmi.h              |  24 +
 13 files changed, 1403 insertions(+), 137 deletions(-)
 create mode 100644 drivers/firmware/arm_scmi/Kconfig
 create mode 100644 drivers/firmware/arm_scmi/msg.c
 create mode 100644 drivers/firmware/arm_scmi/virtio.c
 create mode 100644 include/uapi/linux/virtio_scmi.h

-- 
2.17.1


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v7 01/15] firmware: arm_scmi: Add support for type handling in common functions
  2021-08-03 13:10 [PATCH v7 00/15] Introduce SCMI transport based on VirtIO Cristian Marussi
@ 2021-08-03 13:10 ` Cristian Marussi
  2021-08-03 13:10 ` [PATCH v7 02/15] firmware: arm_scmi: Remove scmi_dump_header_dbg() helper Cristian Marussi
                   ` (14 subsequent siblings)
  15 siblings, 0 replies; 19+ messages in thread
From: Cristian Marussi @ 2021-08-03 13:10 UTC (permalink / raw)
  To: linux-kernel, linux-arm-kernel, virtualization, virtio-dev
  Cc: sudeep.holla, james.quinlan, Jonathan.Cameron, f.fainelli,
	etienne.carriere, vincent.guittot, souvik.chakravarty,
	cristian.marussi, igor.skalkin, peter.hilber, alex.bennee,
	jean-philippe, mikhail.golubev, anton.yakovlev, Vasyl.Vavrychuk,
	Andriy.Tryshnivskyy

Add SCMI type handling to pack/unpack_scmi_header common helper functions.
Initialize hdr.type properly when initializing a command xfer.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
Needed later in the series to support serialization
---
 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;
 
-- 
2.17.1


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v7 02/15] firmware: arm_scmi: Remove scmi_dump_header_dbg() helper
  2021-08-03 13:10 [PATCH v7 00/15] Introduce SCMI transport based on VirtIO Cristian Marussi
  2021-08-03 13:10 ` [PATCH v7 01/15] firmware: arm_scmi: Add support for type handling in common functions Cristian Marussi
@ 2021-08-03 13:10 ` Cristian Marussi
  2021-08-03 13:10 ` [PATCH v7 03/15] firmware: arm_scmi: Add optional transport_init/exit support Cristian Marussi
                   ` (13 subsequent siblings)
  15 siblings, 0 replies; 19+ messages in thread
From: Cristian Marussi @ 2021-08-03 13:10 UTC (permalink / raw)
  To: linux-kernel, linux-arm-kernel, virtualization, virtio-dev
  Cc: sudeep.holla, james.quinlan, Jonathan.Cameron, f.fainelli,
	etienne.carriere, vincent.guittot, souvik.chakravarty,
	cristian.marussi, igor.skalkin, peter.hilber, alex.bennee,
	jean-philippe, mikhail.golubev, anton.yakovlev, Vasyl.Vavrychuk,
	Andriy.Tryshnivskyy

Being a while that we have SCMI trace events in the SCMI stack, remove
this debug helper and its call sites.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 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,
-- 
2.17.1


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v7 03/15] firmware: arm_scmi: Add optional transport_init/exit support
  2021-08-03 13:10 [PATCH v7 00/15] Introduce SCMI transport based on VirtIO Cristian Marussi
  2021-08-03 13:10 ` [PATCH v7 01/15] firmware: arm_scmi: Add support for type handling in common functions Cristian Marussi
  2021-08-03 13:10 ` [PATCH v7 02/15] firmware: arm_scmi: Remove scmi_dump_header_dbg() helper Cristian Marussi
@ 2021-08-03 13:10 ` Cristian Marussi
  2021-08-03 13:10 ` [PATCH v7 04/15] firmware: arm_scmi: Introduce monotonically increasing tokens Cristian Marussi
                   ` (12 subsequent siblings)
  15 siblings, 0 replies; 19+ messages in thread
From: Cristian Marussi @ 2021-08-03 13:10 UTC (permalink / raw)
  To: linux-kernel, linux-arm-kernel, virtualization, virtio-dev
  Cc: sudeep.holla, james.quinlan, Jonathan.Cameron, f.fainelli,
	etienne.carriere, vincent.guittot, souvik.chakravarty,
	cristian.marussi, igor.skalkin, peter.hilber, alex.bennee,
	jean-philippe, mikhail.golubev, anton.yakovlev, Vasyl.Vavrychuk,
	Andriy.Tryshnivskyy

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. ]
Signed-off-by: Peter Hilber <peter.hilber@opensynergy.com>
[ Cristian: Fixed scmi_transports_exit point of invocation ]
Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
v6 --> v7
- renamed to transport_init/exit
v4 --> V5
- removed useless pr_debug
- moved scmi_transport_exit() invocation
---
 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);
-- 
2.17.1


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v7 04/15] firmware: arm_scmi: Introduce monotonically increasing tokens
  2021-08-03 13:10 [PATCH v7 00/15] Introduce SCMI transport based on VirtIO Cristian Marussi
                   ` (2 preceding siblings ...)
  2021-08-03 13:10 ` [PATCH v7 03/15] firmware: arm_scmi: Add optional transport_init/exit support Cristian Marussi
@ 2021-08-03 13:10 ` Cristian Marussi
  2021-08-03 13:10 ` [PATCH v7 05/15] firmware: arm_scmi: Handle concurrent and out-of-order messages Cristian Marussi
                   ` (11 subsequent siblings)
  15 siblings, 0 replies; 19+ messages in thread
From: Cristian Marussi @ 2021-08-03 13:10 UTC (permalink / raw)
  To: linux-kernel, linux-arm-kernel, virtualization, virtio-dev
  Cc: sudeep.holla, james.quinlan, Jonathan.Cameron, f.fainelli,
	etienne.carriere, vincent.guittot, souvik.chakravarty,
	cristian.marussi, igor.skalkin, peter.hilber, alex.bennee,
	jean-philippe, mikhail.golubev, anton.yakovlev, Vasyl.Vavrychuk,
	Andriy.Tryshnivskyy

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.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
v6 --> v7
- avoid 'ghost' naming in commit message
- removed xfer->users refcounting (it will be added later in the series
  where it is a better fit
- removed redundant check in scmi_xfer_lookup_unlocked
- added proper comment to justify pending_xfers sizing and moved
  such define away into common.h
- removed last_token atomic counters, now generating monotonic seqnums
  based on transfer_id
v4 --> V5
- removed empirical profiling info from commit msg
- do NOT use monotonic tokens and pending HT for notifications (not needed)
- release xfer_lock later in scmi_xfer_get
---
 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 <linux/device.h>
 #include <linux/errno.h>
 #include <linux/kernel.h>
+#include <linux/hashtable.h>
+#include <linux/list.h>
 #include <linux/module.h>
 #include <linux/scmi_protocol.h>
 #include <linux/types.h>
@@ -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 <linux/io.h>
 #include <linux/kernel.h>
 #include <linux/ktime.h>
+#include <linux/hashtable.h>
 #include <linux/list.h>
 #include <linux/module.h>
 #include <linux/of_address.h>
@@ -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);
-- 
2.17.1


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v7 05/15] firmware: arm_scmi: Handle concurrent and out-of-order messages
  2021-08-03 13:10 [PATCH v7 00/15] Introduce SCMI transport based on VirtIO Cristian Marussi
                   ` (3 preceding siblings ...)
  2021-08-03 13:10 ` [PATCH v7 04/15] firmware: arm_scmi: Introduce monotonically increasing tokens Cristian Marussi
@ 2021-08-03 13:10 ` Cristian Marussi
  2021-08-03 13:10 ` [PATCH v7 06/15] firmware: arm_scmi: Make .clear_channel optional Cristian Marussi
                   ` (10 subsequent siblings)
  15 siblings, 0 replies; 19+ messages in thread
From: Cristian Marussi @ 2021-08-03 13:10 UTC (permalink / raw)
  To: linux-kernel, linux-arm-kernel, virtualization, virtio-dev
  Cc: sudeep.holla, james.quinlan, Jonathan.Cameron, f.fainelli,
	etienne.carriere, vincent.guittot, souvik.chakravarty,
	cristian.marussi, igor.skalkin, peter.hilber, alex.bennee,
	jean-philippe, mikhail.golubev, anton.yakovlev, Vasyl.Vavrychuk,
	Andriy.Tryshnivskyy

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.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
v6 --> v7
- re-added xfer refcounting moved out of previous 'monotonic' patch
- removed un-needed 2-way switch in scmi_xfer_state_update()
- renamed scmi_xfer_is_free to scmi_xfer_acquired
- moved scmi_xfer_state_update() call inside scmi_xfer_command_acquire
  to avoid races between msg validation and xfer state update
- added smp_mb() after xfer->state init before .send_message
v5 --> v6
- added spinlock comment
---
 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 <linux/hashtable.h>
 #include <linux/list.h>
 #include <linux/module.h>
+#include <linux/refcount.h>
 #include <linux/scmi_protocol.h>
+#include <linux/spinlock.h>
 #include <linux/types.h>
 
 #include <asm/unaligned.h>
@@ -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);
-- 
2.17.1


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v7 06/15] firmware: arm_scmi: Make .clear_channel optional
  2021-08-03 13:10 [PATCH v7 00/15] Introduce SCMI transport based on VirtIO Cristian Marussi
                   ` (4 preceding siblings ...)
  2021-08-03 13:10 ` [PATCH v7 05/15] firmware: arm_scmi: Handle concurrent and out-of-order messages Cristian Marussi
@ 2021-08-03 13:10 ` Cristian Marussi
  2021-08-03 13:10 ` [PATCH v7 07/15] firmware: arm_scmi: Make polling mode optional Cristian Marussi
                   ` (9 subsequent siblings)
  15 siblings, 0 replies; 19+ messages in thread
From: Cristian Marussi @ 2021-08-03 13:10 UTC (permalink / raw)
  To: linux-kernel, linux-arm-kernel, virtualization, virtio-dev
  Cc: sudeep.holla, james.quinlan, Jonathan.Cameron, f.fainelli,
	etienne.carriere, vincent.guittot, souvik.chakravarty,
	cristian.marussi, igor.skalkin, peter.hilber, alex.bennee,
	jean-philippe, mikhail.golubev, anton.yakovlev, Vasyl.Vavrychuk,
	Andriy.Tryshnivskyy

Make transport operation .clear_channel optional since some transports
do not need it and so avoid to have them implement dummy callbacks.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 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);
-- 
2.17.1


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v7 07/15] firmware: arm_scmi: Make polling mode optional
  2021-08-03 13:10 [PATCH v7 00/15] Introduce SCMI transport based on VirtIO Cristian Marussi
                   ` (5 preceding siblings ...)
  2021-08-03 13:10 ` [PATCH v7 06/15] firmware: arm_scmi: Make .clear_channel optional Cristian Marussi
@ 2021-08-03 13:10 ` Cristian Marussi
  2021-08-03 13:10 ` [PATCH v7 08/15] firmware: arm_scmi: Make SCMI transports configurable Cristian Marussi
                   ` (8 subsequent siblings)
  15 siblings, 0 replies; 19+ messages in thread
From: Cristian Marussi @ 2021-08-03 13:10 UTC (permalink / raw)
  To: linux-kernel, linux-arm-kernel, virtualization, virtio-dev
  Cc: sudeep.holla, james.quinlan, Jonathan.Cameron, f.fainelli,
	etienne.carriere, vincent.guittot, souvik.chakravarty,
	cristian.marussi, igor.skalkin, peter.hilber, alex.bennee,
	jean-philippe, mikhail.golubev, anton.yakovlev, Vasyl.Vavrychuk,
	Andriy.Tryshnivskyy

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.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
v6 --> v7
- s/EINVAL/-EINVAL/
- refactored to bail out early when polling mode not supported
---
 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;
 
-- 
2.17.1


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v7 08/15] firmware: arm_scmi: Make SCMI transports configurable
  2021-08-03 13:10 [PATCH v7 00/15] Introduce SCMI transport based on VirtIO Cristian Marussi
                   ` (6 preceding siblings ...)
  2021-08-03 13:10 ` [PATCH v7 07/15] firmware: arm_scmi: Make polling mode optional Cristian Marussi
@ 2021-08-03 13:10 ` Cristian Marussi
  2021-08-03 13:10 ` [PATCH v7 09/15] firmware: arm_scmi: Make shmem support optional for transports Cristian Marussi
                   ` (7 subsequent siblings)
  15 siblings, 0 replies; 19+ messages in thread
From: Cristian Marussi @ 2021-08-03 13:10 UTC (permalink / raw)
  To: linux-kernel, linux-arm-kernel, virtualization, virtio-dev
  Cc: sudeep.holla, james.quinlan, Jonathan.Cameron, f.fainelli,
	etienne.carriere, vincent.guittot, souvik.chakravarty,
	cristian.marussi, igor.skalkin, peter.hilber, alex.bennee,
	jean-philippe, mikhail.golubev, anton.yakovlev, Vasyl.Vavrychuk,
	Andriy.Tryshnivskyy

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.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
Used a BUILD_BUG_ON() to avoid the scenario where SCMI is configured
without any transport. Coul dnot do in any other way in Kconfig due to
circular dependencies.

This will be neeed later on to add new Virtio based transport and
optionally exclude other transports.

v6 --> v7
- using if ARM_SCMI_PROTOCOL to simplify depends
---
 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)
-- 
2.17.1


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v7 09/15] firmware: arm_scmi: Make shmem support optional for transports
  2021-08-03 13:10 [PATCH v7 00/15] Introduce SCMI transport based on VirtIO Cristian Marussi
                   ` (7 preceding siblings ...)
  2021-08-03 13:10 ` [PATCH v7 08/15] firmware: arm_scmi: Make SCMI transports configurable Cristian Marussi
@ 2021-08-03 13:10 ` Cristian Marussi
  2021-08-03 13:10 ` [PATCH v7 10/15] firmware: arm_scmi: Add method to override max message number Cristian Marussi
                   ` (6 subsequent siblings)
  15 siblings, 0 replies; 19+ messages in thread
From: Cristian Marussi @ 2021-08-03 13:10 UTC (permalink / raw)
  To: linux-kernel, linux-arm-kernel, virtualization, virtio-dev
  Cc: sudeep.holla, james.quinlan, Jonathan.Cameron, f.fainelli,
	etienne.carriere, vincent.guittot, souvik.chakravarty,
	cristian.marussi, igor.skalkin, peter.hilber, alex.bennee,
	jean-philippe, mikhail.golubev, anton.yakovlev, Vasyl.Vavrychuk,
	Andriy.Tryshnivskyy

From: Igor Skalkin <igor.skalkin@opensynergy.com>

Upcoming new SCMI transports won't need any kind of shared memory support.
Compile shmem.c only if a shmem based transport is selected.

Signed-off-by: Igor Skalkin <igor.skalkin@opensynergy.com>
[ Peter: Adapted patch for submission to upstream. ]
Co-developed-by: Peter Hilber <peter.hilber@opensynergy.com>
Signed-off-by: Peter Hilber <peter.hilber@opensynergy.com>
[ Cristian: Adapted patch/commit_msg to new SCMI Kconfig layout ]
Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
v4 --> v5
- Adapted to new SCMI Kconfig layout
---
 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
-- 
2.17.1


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v7 10/15] firmware: arm_scmi: Add method to override max message number
  2021-08-03 13:10 [PATCH v7 00/15] Introduce SCMI transport based on VirtIO Cristian Marussi
                   ` (8 preceding siblings ...)
  2021-08-03 13:10 ` [PATCH v7 09/15] firmware: arm_scmi: Make shmem support optional for transports Cristian Marussi
@ 2021-08-03 13:10 ` Cristian Marussi
  2021-08-03 13:10 ` [PATCH v7 11/15] firmware: arm_scmi: Add message passing abstractions for transports Cristian Marussi
                   ` (5 subsequent siblings)
  15 siblings, 0 replies; 19+ messages in thread
From: Cristian Marussi @ 2021-08-03 13:10 UTC (permalink / raw)
  To: linux-kernel, linux-arm-kernel, virtualization, virtio-dev
  Cc: sudeep.holla, james.quinlan, Jonathan.Cameron, f.fainelli,
	etienne.carriere, vincent.guittot, souvik.chakravarty,
	cristian.marussi, igor.skalkin, peter.hilber, alex.bennee,
	jean-philippe, mikhail.golubev, anton.yakovlev, Vasyl.Vavrychuk,
	Andriy.Tryshnivskyy

From: Igor Skalkin <igor.skalkin@opensynergy.com>

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.

Signed-off-by: Igor Skalkin <igor.skalkin@opensynergy.com>
[ Peter: Adapted patch for submission to upstream. ]
Co-developed-by: Peter Hilber <peter.hilber@opensynergy.com>
Signed-off-by: Peter Hilber <peter.hilber@opensynergy.com>
[ Cristian: refactored how get_max_msg() is used to minimize core changes ]
Co-developed-by: Cristian Marussi <cristian.marussi@arm.com>
Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
v4 --> v5
- refactored usage of get_max_msg()
---
 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);
 
-- 
2.17.1


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v7 11/15] firmware: arm_scmi: Add message passing abstractions for transports
  2021-08-03 13:10 [PATCH v7 00/15] Introduce SCMI transport based on VirtIO Cristian Marussi
                   ` (9 preceding siblings ...)
  2021-08-03 13:10 ` [PATCH v7 10/15] firmware: arm_scmi: Add method to override max message number Cristian Marussi
@ 2021-08-03 13:10 ` Cristian Marussi
  2021-08-03 13:10 ` [PATCH v7 12/15] firmware: arm_scmi: Add optional link_supplier() transport op Cristian Marussi
                   ` (4 subsequent siblings)
  15 siblings, 0 replies; 19+ messages in thread
From: Cristian Marussi @ 2021-08-03 13:10 UTC (permalink / raw)
  To: linux-kernel, linux-arm-kernel, virtualization, virtio-dev
  Cc: sudeep.holla, james.quinlan, Jonathan.Cameron, f.fainelli,
	etienne.carriere, vincent.guittot, souvik.chakravarty,
	cristian.marussi, igor.skalkin, peter.hilber, alex.bennee,
	jean-philippe, mikhail.golubev, anton.yakovlev, Vasyl.Vavrychuk,
	Andriy.Tryshnivskyy

From: Peter Hilber <peter.hilber@opensynergy.com>

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.

Signed-off-by: Peter Hilber <peter.hilber@opensynergy.com>
[ Cristian: Adapted to new SCMI Kconfig layout, updated Copyrights ]
Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
v6 --> v7
- removed uneeded includes
v4 --> v5
- adapted to new SCMI Kconfig
- removed raw_payload msg helpers
v3 --> v4
- added raw_payload msg helpers
---
 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 <linux/types.h>
+
+#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);
+}
-- 
2.17.1


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v7 12/15] firmware: arm_scmi: Add optional link_supplier() transport op
  2021-08-03 13:10 [PATCH v7 00/15] Introduce SCMI transport based on VirtIO Cristian Marussi
                   ` (10 preceding siblings ...)
  2021-08-03 13:10 ` [PATCH v7 11/15] firmware: arm_scmi: Add message passing abstractions for transports Cristian Marussi
@ 2021-08-03 13:10 ` Cristian Marussi
  2021-08-03 13:10 ` [PATCH v7 13/15] dt-bindings: arm: Add virtio transport for SCMI Cristian Marussi
                   ` (3 subsequent siblings)
  15 siblings, 0 replies; 19+ messages in thread
From: Cristian Marussi @ 2021-08-03 13:10 UTC (permalink / raw)
  To: linux-kernel, linux-arm-kernel, virtualization, virtio-dev
  Cc: sudeep.holla, james.quinlan, Jonathan.Cameron, f.fainelli,
	etienne.carriere, vincent.guittot, souvik.chakravarty,
	cristian.marussi, igor.skalkin, peter.hilber, alex.bennee,
	jean-philippe, mikhail.golubev, anton.yakovlev, Vasyl.Vavrychuk,
	Andriy.Tryshnivskyy

From: Peter Hilber <peter.hilber@opensynergy.com>

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.

Signed-off-by: Peter Hilber <peter.hilber@opensynergy.com>
[ Cristian: reworded commit message ]
Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
v6 --> v7
- fixed commit message
v5 --> v6
- reworded commit message
---
 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;
-- 
2.17.1


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v7 13/15] dt-bindings: arm: Add virtio transport for SCMI
  2021-08-03 13:10 [PATCH v7 00/15] Introduce SCMI transport based on VirtIO Cristian Marussi
                   ` (11 preceding siblings ...)
  2021-08-03 13:10 ` [PATCH v7 12/15] firmware: arm_scmi: Add optional link_supplier() transport op Cristian Marussi
@ 2021-08-03 13:10 ` Cristian Marussi
  2021-08-03 13:10 ` [PATCH v7 14/15] firmware: arm_scmi: Add priv parameter to scmi_rx_callback Cristian Marussi
                   ` (2 subsequent siblings)
  15 siblings, 0 replies; 19+ messages in thread
From: Cristian Marussi @ 2021-08-03 13:10 UTC (permalink / raw)
  To: linux-kernel, linux-arm-kernel, virtualization, virtio-dev
  Cc: sudeep.holla, james.quinlan, Jonathan.Cameron, f.fainelli,
	etienne.carriere, vincent.guittot, souvik.chakravarty,
	cristian.marussi, igor.skalkin, peter.hilber, alex.bennee,
	jean-philippe, mikhail.golubev, anton.yakovlev, Vasyl.Vavrychuk,
	Andriy.Tryshnivskyy

From: Igor Skalkin <igor.skalkin@opensynergy.com>

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

Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Igor Skalkin <igor.skalkin@opensynergy.com>
[ Peter: Adapted patch for submission to upstream. ]
Co-developed-by: Peter Hilber <peter.hilber@opensynergy.com>
Signed-off-by: Peter Hilber <peter.hilber@opensynergy.com>
[ Cristian: converted to yaml format, moved shmen required property. ]
Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
v3 --> V4
- convertd to YAML
- make shmem required only for pre-existing mailbox and smc transport
- updated VirtIO specification patch message reference
- dropped virtio-mmio SCMI device example since really not pertinent to
  virtio-scmi dt bindings transport: it is not even referenced in SCMI
  virtio DT node since they are enumerated by VirtIO subsystem and there
  could be PCI based SCMI devices anyway.
---
 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:
   - |
-- 
2.17.1


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v7 14/15] firmware: arm_scmi: Add priv parameter to scmi_rx_callback
  2021-08-03 13:10 [PATCH v7 00/15] Introduce SCMI transport based on VirtIO Cristian Marussi
                   ` (12 preceding siblings ...)
  2021-08-03 13:10 ` [PATCH v7 13/15] dt-bindings: arm: Add virtio transport for SCMI Cristian Marussi
@ 2021-08-03 13:10 ` Cristian Marussi
  2021-08-03 13:10 ` [PATCH v7 15/15] firmware: arm_scmi: Add virtio transport Cristian Marussi
  2021-08-09  4:57 ` [PATCH v7 00/15] Introduce SCMI transport based on VirtIO Sudeep Holla
  15 siblings, 0 replies; 19+ messages in thread
From: Cristian Marussi @ 2021-08-03 13:10 UTC (permalink / raw)
  To: linux-kernel, linux-arm-kernel, virtualization, virtio-dev
  Cc: sudeep.holla, james.quinlan, Jonathan.Cameron, f.fainelli,
	etienne.carriere, vincent.guittot, souvik.chakravarty,
	cristian.marussi, igor.skalkin, peter.hilber, alex.bennee,
	jean-philippe, mikhail.golubev, anton.yakovlev, Vasyl.Vavrychuk,
	Andriy.Tryshnivskyy

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.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
v6 --> v7
- moved this patch later in the series right before virtio support
  that directly needs it.
---
 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);
 
-- 
2.17.1


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v7 15/15] firmware: arm_scmi: Add virtio transport
  2021-08-03 13:10 [PATCH v7 00/15] Introduce SCMI transport based on VirtIO Cristian Marussi
                   ` (13 preceding siblings ...)
  2021-08-03 13:10 ` [PATCH v7 14/15] firmware: arm_scmi: Add priv parameter to scmi_rx_callback Cristian Marussi
@ 2021-08-03 13:10 ` Cristian Marussi
  2021-09-04 13:03   ` [virtio-dev] " Michael S. Tsirkin
  2021-08-09  4:57 ` [PATCH v7 00/15] Introduce SCMI transport based on VirtIO Sudeep Holla
  15 siblings, 1 reply; 19+ messages in thread
From: Cristian Marussi @ 2021-08-03 13:10 UTC (permalink / raw)
  To: linux-kernel, linux-arm-kernel, virtualization, virtio-dev
  Cc: sudeep.holla, james.quinlan, Jonathan.Cameron, f.fainelli,
	etienne.carriere, vincent.guittot, souvik.chakravarty,
	cristian.marussi, igor.skalkin, peter.hilber, alex.bennee,
	jean-philippe, mikhail.golubev, anton.yakovlev, Vasyl.Vavrychuk,
	Andriy.Tryshnivskyy, Michael S. Tsirkin, Jason Wang

From: Igor Skalkin <igor.skalkin@opensynergy.com>

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

Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Jason Wang <jasowang@redhat.com>
Signed-off-by: Igor Skalkin <igor.skalkin@opensynergy.com>
[ Peter: Adapted patch for submission to upstream. ]
Co-developed-by: Peter Hilber <peter.hilber@opensynergy.com>
Signed-off-by: Peter Hilber <peter.hilber@opensynergy.com>
[ Cristian: simplified driver logic, changed link_supplier and channel
	    available/setup logic, removed dummy callbacks ]
Co-developed-by: Cristian Marussi <cristian.marussi@arm.com>
Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
NOTE THAT VIRTIO TRANSPORT IS ADDED AS default=n

V6 --> V7
- renamed desc init/exit to transport_init/exit
- reviewed Kconfig option to fit V7 ARM_SCMI Kconfig

V5 --> V6
- removed usage of delegated xfers
- using new scmi_rx_callback with priv argument
- removed .dummy clear_channel/.poll_done callbacks
- added missing spinlock comments
- updated Copyrights

V4 --> V5
- adapted Virtio transport config to new SCMI Kconfig layout
- removed support for polling
- added validate virtio method support
- removed usage of raw_payload helpers
- removed dynamic search of matching devices
- added one single statically configured device

V3 --> V4
- using delegated xfers
- using raw_payload msg helpers
---
 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 <sre@kernel.org>
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 <linux/errno.h>
+#include <linux/slab.h>
+#include <linux/virtio.h>
+#include <linux/virtio_config.h>
+
+#include <uapi/linux/virtio_ids.h>
+#include <uapi/linux/virtio_scmi.h>
+
+#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 <linux/virtio_types.h>
+
+/* 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 */
-- 
2.17.1


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v7 00/15] Introduce SCMI transport based on VirtIO
  2021-08-03 13:10 [PATCH v7 00/15] Introduce SCMI transport based on VirtIO Cristian Marussi
                   ` (14 preceding siblings ...)
  2021-08-03 13:10 ` [PATCH v7 15/15] firmware: arm_scmi: Add virtio transport Cristian Marussi
@ 2021-08-09  4:57 ` Sudeep Holla
  15 siblings, 0 replies; 19+ messages in thread
From: Sudeep Holla @ 2021-08-09  4:57 UTC (permalink / raw)
  To: virtualization, virtio-dev, Cristian Marussi, linux-arm-kernel,
	linux-kernel
  Cc: Sudeep Holla, anton.yakovlev, Andriy.Tryshnivskyy,
	souvik.chakravarty, Jonathan.Cameron, peter.hilber,
	james.quinlan, vincent.guittot, etienne.carriere, jean-philippe,
	Vasyl.Vavrychuk, igor.skalkin, mikhail.golubev, f.fainelli,
	alex.bennee

On Tue, 3 Aug 2021 14:10:09 +0100, Cristian Marussi wrote:
> While reworking this series starting from the work done up to V3 by
> OpenSynergy, I am keeping the original autorship and list distribution
> unchanged.
> 
> The main aim of this rework, as said, is to simplify where possible the
> SCMI VirtIO support added in V3 by adding at first some new general
> mechanisms in the SCMI Transport layer.
> 
> [...]

Applied to sudeep.holla/linux (for-next/scmi), thanks!

[01/15] firmware: arm_scmi: Add support for type handling in common functions
        https://git.kernel.org/sudeep.holla/c/63b282f172
[02/15] firmware: arm_scmi: Remove scmi_dump_header_dbg() helper
        https://git.kernel.org/sudeep.holla/c/3669032514
[03/15] firmware: arm_scmi: Add optional transport_init/exit support
        https://git.kernel.org/sudeep.holla/c/ceac257db0
[04/15] firmware: arm_scmi: Introduce monotonically increasing tokens
        https://git.kernel.org/sudeep.holla/c/9ca5a1838e
[05/15] firmware: arm_scmi: Handle concurrent and out-of-order messages
        https://git.kernel.org/sudeep.holla/c/ed7c04c1fe
[06/15] firmware: arm_scmi: Make .clear_channel optional
        https://git.kernel.org/sudeep.holla/c/e9b21c9618
[07/15] firmware: arm_scmi: Make polling mode optional
        https://git.kernel.org/sudeep.holla/c/2930abcffd
[08/15] firmware: arm_scmi: Make SCMI transports configurable
        https://git.kernel.org/sudeep.holla/c/e8419c24ba
[09/15] firmware: arm_scmi: Make shmem support optional for transports
        https://git.kernel.org/sudeep.holla/c/a7b1138b92
[10/15] firmware: arm_scmi: Add method to override max message number
        https://git.kernel.org/sudeep.holla/c/c92c3e382e
[11/15] firmware: arm_scmi: Add message passing abstractions for transports
        https://git.kernel.org/sudeep.holla/c/f301bba0ca
[12/15] firmware: arm_scmi: Add optional link_supplier() transport op
        https://git.kernel.org/sudeep.holla/c/7885281260
[13/15] dt-bindings: arm: Add virtio transport for SCMI
        https://git.kernel.org/sudeep.holla/c/60625667c4
[14/15] firmware: arm_scmi: Add priv parameter to scmi_rx_callback
        https://git.kernel.org/sudeep.holla/c/13fba878cc
[15/15] firmware: arm_scmi: Add virtio transport
        https://git.kernel.org/sudeep.holla/c/46abe13b5e

--

Regards,
Sudeep


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [virtio-dev] [PATCH v7 15/15] firmware: arm_scmi: Add virtio transport
  2021-08-03 13:10 ` [PATCH v7 15/15] firmware: arm_scmi: Add virtio transport Cristian Marussi
@ 2021-09-04 13:03   ` Michael S. Tsirkin
  2021-09-05  7:14     ` Cristian Marussi
  0 siblings, 1 reply; 19+ messages in thread
From: Michael S. Tsirkin @ 2021-09-04 13:03 UTC (permalink / raw)
  To: Cristian Marussi
  Cc: linux-kernel, linux-arm-kernel, virtualization, virtio-dev,
	sudeep.holla, james.quinlan, Jonathan.Cameron, f.fainelli,
	etienne.carriere, vincent.guittot, souvik.chakravarty,
	igor.skalkin, peter.hilber, alex.bennee, jean-philippe,
	mikhail.golubev, anton.yakovlev, Vasyl.Vavrychuk,
	Andriy.Tryshnivskyy, Jason Wang

On Tue, Aug 03, 2021 at 02:10:24PM +0100, Cristian Marussi wrote:
> From: Igor Skalkin <igor.skalkin@opensynergy.com>
> 
> 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
> 
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Igor Skalkin <igor.skalkin@opensynergy.com>
> [ Peter: Adapted patch for submission to upstream. ]
> Co-developed-by: Peter Hilber <peter.hilber@opensynergy.com>
> Signed-off-by: Peter Hilber <peter.hilber@opensynergy.com>
> [ Cristian: simplified driver logic, changed link_supplier and channel
> 	    available/setup logic, removed dummy callbacks ]
> Co-developed-by: Cristian Marussi <cristian.marussi@arm.com>
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> ---
> NOTE THAT VIRTIO TRANSPORT IS ADDED AS default=n
> 
> V6 --> V7
> - renamed desc init/exit to transport_init/exit
> - reviewed Kconfig option to fit V7 ARM_SCMI Kconfig
> 
> V5 --> V6
> - removed usage of delegated xfers
> - using new scmi_rx_callback with priv argument
> - removed .dummy clear_channel/.poll_done callbacks
> - added missing spinlock comments
> - updated Copyrights
> 
> V4 --> V5
> - adapted Virtio transport config to new SCMI Kconfig layout
> - removed support for polling
> - added validate virtio method support
> - removed usage of raw_payload helpers
> - removed dynamic search of matching devices
> - added one single statically configured device
> 
> V3 --> V4
> - using delegated xfers
> - using raw_payload msg helpers
> ---
>  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

For all virtio bits please include
L: virtualization@lists.linux-foundation.org
as well.

Thanks!



>  
>  SYSTEM RESET/SHUTDOWN DRIVERS
>  M:	Sebastian Reichel <sre@kernel.org>
> 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 <linux/errno.h>
> +#include <linux/slab.h>
> +#include <linux/virtio.h>
> +#include <linux/virtio_config.h>
> +
> +#include <uapi/linux/virtio_ids.h>
> +#include <uapi/linux/virtio_scmi.h>
> +
> +#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);

maybe take the lock out of the loop for speed?

> +
> +		if (!vioch->ready) {
> +			if (!cb_enabled)
> +				(void)virtqueue_enable_cb(vqueue);
> +			goto unlock_ready_out;
> +		}
> +
> +		spin_lock_irqsave(&vioch->lock, flags);


no reason to re-save interrupts here - saved already ...

> +		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,


why are all of these warnings once? There'll never be too many of these
and it can make debugging harder.

> +			     "%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;

Or EBUSY maybe? And print an error message pls so users
can figure out what is going on.

> +
> +	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);

And free allocated memory here?

> +		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;


What happens if sz is too small here? E.g. 0? Should we fail probe?

> +	}
> +
> +	vdev->priv = channels;
> +	scmi_vdev = vdev;

So this is read in parallel, unless there's a lock I think you need an
smp_wmb here and WRITE_ONCE. Or maybe add locking instead so we
do not worry about ordering ...

> +
> +	return 0;
> +}
> +
> +static void scmi_vio_remove(struct virtio_device *vdev)
> +{
> +	vdev->config->reset(vdev);
This will stop processing buffers immediately.
You generally need to do something about outstanding
buffers though.


> +	vdev->config->del_vqs(vdev);
> +	scmi_vdev = NULL;

Well doing this here does not prevent some users being outstanding.
I.e. I see  device_link_add above but we should do
device_link_del somewhere ...


> +}
> +
> +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 <linux/virtio_types.h>
> +
> +/* 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 */
> -- 
> 2.17.1
> 
> 
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [virtio-dev] [PATCH v7 15/15] firmware: arm_scmi: Add virtio transport
  2021-09-04 13:03   ` [virtio-dev] " Michael S. Tsirkin
@ 2021-09-05  7:14     ` Cristian Marussi
  0 siblings, 0 replies; 19+ messages in thread
From: Cristian Marussi @ 2021-09-05  7:14 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: linux-kernel, linux-arm-kernel, virtualization, virtio-dev,
	sudeep.holla, james.quinlan, Jonathan.Cameron, f.fainelli,
	etienne.carriere, vincent.guittot, souvik.chakravarty,
	igor.skalkin, peter.hilber, alex.bennee, jean-philippe,
	mikhail.golubev, anton.yakovlev, Vasyl.Vavrychuk,
	Andriy.Tryshnivskyy, Jason Wang

On Sat, Sep 04, 2021 at 09:03:53AM -0400, Michael S. Tsirkin wrote:
> On Tue, Aug 03, 2021 at 02:10:24PM +0100, Cristian Marussi wrote:
> > From: Igor Skalkin <igor.skalkin@opensynergy.com>
> > 
> > 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
> > 
> > Cc: "Michael S. Tsirkin" <mst@redhat.com>
> > Cc: Jason Wang <jasowang@redhat.com>
> > Signed-off-by: Igor Skalkin <igor.skalkin@opensynergy.com>
> > [ Peter: Adapted patch for submission to upstream. ]
> > Co-developed-by: Peter Hilber <peter.hilber@opensynergy.com>
> > Signed-off-by: Peter Hilber <peter.hilber@opensynergy.com>
> > [ Cristian: simplified driver logic, changed link_supplier and channel
> > 	    available/setup logic, removed dummy callbacks ]
> > Co-developed-by: Cristian Marussi <cristian.marussi@arm.com>
> > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> > ---
> > NOTE THAT VIRTIO TRANSPORT IS ADDED AS default=n
> > 
> > V6 --> V7
> > - renamed desc init/exit to transport_init/exit
> > - reviewed Kconfig option to fit V7 ARM_SCMI Kconfig
> > 
> > V5 --> V6
> > - removed usage of delegated xfers
> > - using new scmi_rx_callback with priv argument
> > - removed .dummy clear_channel/.poll_done callbacks
> > - added missing spinlock comments
> > - updated Copyrights
> > 
> > V4 --> V5
> > - adapted Virtio transport config to new SCMI Kconfig layout
> > - removed support for polling
> > - added validate virtio method support
> > - removed usage of raw_payload helpers
> > - removed dynamic search of matching devices
> > - added one single statically configured device
> > 
> > V3 --> V4
> > - using delegated xfers
> > - using raw_payload msg helpers
> > ---
> >  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
> 

Hi Michael,

thanks for your comments, replies inline.

> For all virtio bits please include
> L: virtualization@lists.linux-foundation.org
> as well.
> 
> Thanks!
> 

Apologies, I'm not sure what was the issue but this series was supposed
to have been posted also To: virtualization@lists.linux-foundation.org
even for the previous versions of the series: in this last v7 I also added
you in Cc.
Have you not received any of the previous v* on the virtualization list ?

> 
> 
> >  
> >  SYSTEM RESET/SHUTDOWN DRIVERS
> >  M:	Sebastian Reichel <sre@kernel.org>
> > 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 <linux/errno.h>
> > +#include <linux/slab.h>
> > +#include <linux/virtio.h>
> > +#include <linux/virtio_config.h>
> > +
> > +#include <uapi/linux/virtio_ids.h>
> > +#include <uapi/linux/virtio_scmi.h>
> > +
> > +#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);
> 
> maybe take the lock out of the loop for speed?
> 

I'll review this, but it seems to me that moving the spinlock out of the
loop you won't be able to set the vioch NOT ready when shutting down,
until all message are processed, while acquiring/releasing the lock
inside the loop enables the possibility to check for ready flag between
each message processing. (more on this later down below...)

> > +
> > +		if (!vioch->ready) {
> > +			if (!cb_enabled)
> > +				(void)virtqueue_enable_cb(vqueue);
> > +			goto unlock_ready_out;
> > +		}
> > +
> > +		spin_lock_irqsave(&vioch->lock, flags);
> 
> 
> no reason to re-save interrupts here - saved already ...
> 

Sure, I'll do.

> > +		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,
> 
> 
> why are all of these warnings once? There'll never be too many of these
> and it can make debugging harder.
> 

Ok, I'll remove the once on the error paths.

> > +			     "%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;
> 
> Or EBUSY maybe? And print an error message pls so users
> can figure out what is going on.
> 

Yes of course, I'll fix.

> > +
> > +	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);
> 
> And free allocated memory here?

Sorry, which memory: the only allocated memory at this point is channels
and that is done using a devm_ call, so I suppose on failure here we
bail out of the probe and that memeory is automagically freed by devres,
or am I missing something ?

> > +		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;
> 
> 
> What happens if sz is too small here? E.g. 0? Should we fail probe?
> 

No, because I think it should be up to the SCMI core that uses this
SCMI virtio transport to decide WHICH size is too small and what to do
when it happens (.max_msg is exposed to the SCMI core via .get_max_msg)

On the other side I'll check for max size because that's builtin limit
of the protocol, since the SCMI msg header contains a sequence number
bit-field limited to MSG_TOKEN_MAX: the SCMI can and will indeed limit
probably the effective maximum number of pending inflight messages to
some lower limit depending on the specific platform, but anyway is not
possible to have more than MSG_TOKEN_MAX inflight messages because there
are not enough bit in the header to represent the seq numbers.

Btw in this moment, in the core, the logic for the check of min size
I mentioned above is still under discussion.

> > +	}
> > +
> > +	vdev->priv = channels;
> > +	scmi_vdev = vdev;
> 
> So this is read in parallel, unless there's a lock I think you need an
> smp_wmb here and WRITE_ONCE. Or maybe add locking instead so we
> do not worry about ordering ...
> 

Right, I'll use a barrier since I'd prefer to avoid introducing locking
on a global which is really written once at start and then only read
until the stack is shutdown and the virtio is removed.

> > +
> > +	return 0;
> > +}
> > +
> > +static void scmi_vio_remove(struct virtio_device *vdev)
> > +{
> > +	vdev->config->reset(vdev);
> This will stop processing buffers immediately.
> You generally need to do something about outstanding
> buffers though.
> 

Yes, but in fact the complete_cb processes messages only when
vioch->ready is true: such flag is set to true on chan_setup()
after a successfull probe and reset to false in .chan_free() which is
called by the SCMI core well before this _remove is called; moreover
such vioch->ready flag is spinlocked so an outstanding message
possibly actively processed by complete_cb when chan_free is called
will be orderly and fully processed while any subsequent oustanding
message will hit the !ready flag and ignored, till the vqs are deleted
in the next step.

It certainly will help adding a comment about this.

> 
> > +	vdev->config->del_vqs(vdev);
> > +	scmi_vdev = NULL;
> 
> Well doing this here does not prevent some users being outstanding.

If what I said above holds true, once we get here the channel is marked
NOT ready and any outstanding buffer would have been fully processed or
it will be totally ignored.

> I.e. I see  device_link_add above but we should do
> device_link_del somewhere ...
> 

So my understanding of this was that since the device_link_add() uses
DL_FLAG_AUTOREMOVE_CONSUMER the link deletion happens automatically once
the consumer (SCMI stack dev) unbinds on shutdown. (but I'm not so
familiar with these so I could be wrong, even though I have not seen any
anomaly on repeated load/unload and related re-inits of the whole stack)

If what I said above in general makes sense to You, I would post after
-rc1 some fixes for the issues you spotted (barrier / unneeded_irqsave /
ret_EBUSY / print _onces)

Thanks again for the review !

Cristian


^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2021-09-05  7:14 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-03 13:10 [PATCH v7 00/15] Introduce SCMI transport based on VirtIO Cristian Marussi
2021-08-03 13:10 ` [PATCH v7 01/15] firmware: arm_scmi: Add support for type handling in common functions Cristian Marussi
2021-08-03 13:10 ` [PATCH v7 02/15] firmware: arm_scmi: Remove scmi_dump_header_dbg() helper Cristian Marussi
2021-08-03 13:10 ` [PATCH v7 03/15] firmware: arm_scmi: Add optional transport_init/exit support Cristian Marussi
2021-08-03 13:10 ` [PATCH v7 04/15] firmware: arm_scmi: Introduce monotonically increasing tokens Cristian Marussi
2021-08-03 13:10 ` [PATCH v7 05/15] firmware: arm_scmi: Handle concurrent and out-of-order messages Cristian Marussi
2021-08-03 13:10 ` [PATCH v7 06/15] firmware: arm_scmi: Make .clear_channel optional Cristian Marussi
2021-08-03 13:10 ` [PATCH v7 07/15] firmware: arm_scmi: Make polling mode optional Cristian Marussi
2021-08-03 13:10 ` [PATCH v7 08/15] firmware: arm_scmi: Make SCMI transports configurable Cristian Marussi
2021-08-03 13:10 ` [PATCH v7 09/15] firmware: arm_scmi: Make shmem support optional for transports Cristian Marussi
2021-08-03 13:10 ` [PATCH v7 10/15] firmware: arm_scmi: Add method to override max message number Cristian Marussi
2021-08-03 13:10 ` [PATCH v7 11/15] firmware: arm_scmi: Add message passing abstractions for transports Cristian Marussi
2021-08-03 13:10 ` [PATCH v7 12/15] firmware: arm_scmi: Add optional link_supplier() transport op Cristian Marussi
2021-08-03 13:10 ` [PATCH v7 13/15] dt-bindings: arm: Add virtio transport for SCMI Cristian Marussi
2021-08-03 13:10 ` [PATCH v7 14/15] firmware: arm_scmi: Add priv parameter to scmi_rx_callback Cristian Marussi
2021-08-03 13:10 ` [PATCH v7 15/15] firmware: arm_scmi: Add virtio transport Cristian Marussi
2021-09-04 13:03   ` [virtio-dev] " Michael S. Tsirkin
2021-09-05  7:14     ` Cristian Marussi
2021-08-09  4:57 ` [PATCH v7 00/15] Introduce SCMI transport based on VirtIO Sudeep Holla

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).