[RFC PATCH 0/4] Rework SCMI transport drivers probing sequence

[RFC PATCH 0/4] Rework SCMI transport drivers probing sequence

[Cristian Marussi]

Hi,

when the SCMI transports were split out into standalone drivers [1] the
probe sequence was laid out in such a way that:

 - the transport drivers would have probed first, triggered by the firmware
   driven discovery process (DT/ACPI)

 - afterwards the control would have been passed to the core SCMI stack
   driver via the creation of a dedicated device that would have inherited
   the original firmware descriptor (since that same DT/ACPI node would
   have still needed by the SCMI core driver to be parsed)

The tricky part came around with some transport driver like virtio and
optee since they are, in turn, upfront dependent on an external distinct
kernel subsystem; IOW these have first to undergo their own subsystem
specific probe/initialization to become fully operational as transports:
this kind of initialization sequencing of course must deal with the
possibility of probe deferrals BUT at that time we avoid this by using the
trick in virtio/optee transports to register the next stage transport
drivers ONLY at the end of the subsystem specific probe routine, from
within the probe itself.

This behaviour has 2 issues:

 - it is frowned upon and can lead to hangs in the driver core whenever
   some core locking is changed as exposed in [2]
 - it limits these transport drivers to a single instance probing since of
   course you cannot register the same driver more than once

All of this was tolerable because optee/virtio are generally only employed
in a single SCMI instance scenario AND because no hang where triggered in
the core driver subsystem... (not saying that was perfectly fine :P)

This RFC series aims to solve both of the above issue by:

 - moving the problematic platform driver registation away from the probe
   into its own module_init/exit

 - introducing an optional mechanism for the transport drivers to provide
   to the SCMI core some sort of transport specific handle so that multiple
   instances can be supported and multiple probed transports instances can
   be registered and identified by the core. (who is who)
   Note that at the same time this optional handle-mechanism is used to
   synchronize the drivers probing sequences and possibly trigger, when
   needed, the probe deferrals, if the transport driver has still to
   undergo its own subsystem initialization and as such is still NOT fully
   operational.

Most of the above mechanism is provided in 1/4 and 2/4 while patches 3 and
4 take care to port virtio and optee to the new probe schema.

Note that optee required some additional way (an xarray) to match properly
the tee_client_device with the agent on the TEE removal phase, now that
multiple instances are an option: but I could have missed a simpler
solution given my TEE-ignorance.

Please be aware that all of this has been tested as a builtin or as modules
on an emulated setup BUT ONLY with the virtio transport as of now.
Optee is only built tested.

Based on v7.0-rc4

Being an RFC I am NOT completely sure of the final code layout (we may make
most of this transparent really), I am mostly interested of course in
gathering some feedback above the whole mechanism.

Thanks,
Cristian
----
[1]: https://lore.kernel.org/arm-scmi/20240812173340.3912830-1-cristian.marussi@arm.com/
[2]: https://lore.kernel.org/lkml/aaA6t-J2gRy3dE1_@pluto/

Cristian Marussi (4):
  firmware: arm_scmi: Add transport instance handle
  firmware: arm_scmi: Propagate transport instance handle
  firmware: arm_scmi: virtio: Rework transport probe sequence
  firmware: arm_scmi: optee: Rework transport probe sequence

 drivers/firmware/arm_scmi/common.h            |  46 +++++-
 drivers/firmware/arm_scmi/driver.c            |  15 +-
 .../firmware/arm_scmi/transports/mailbox.c    |   5 +-
 drivers/firmware/arm_scmi/transports/optee.c  | 143 ++++++++++--------
 drivers/firmware/arm_scmi/transports/smc.c    |   4 +-
 drivers/firmware/arm_scmi/transports/virtio.c |  99 +++++++-----
 6 files changed, 202 insertions(+), 110 deletions(-)

-- 
2.53.0

[RFC PATCH 1/4] firmware: arm_scmi: Add transport instance handle

[Cristian Marussi]

SCMI transport drivers are initialized first and then the control is
passed to the SCMI core stack: such driver can be instantiated and
probed multiple times and some of them are part also of some external
subsytem which must be initialized upfront before the transport driver
can be deemed to be ready for operation.

Transport drivers like virtio or optee need a way to defer the core SCMI
probing till they are operational and also a way to pass back and forth
some sort of transport handle that can be used to identify the transport
instance.

Add an optional mutexed list shared between the transports and the core
to use as such mechanism.

Note that this change also allows the removal of the frown-upon trick of
registering a platform driver at the end of the probe of the transport
driver as an alternative way of probe deferral.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/common.h | 42 +++++++++++++++++++++++++++++-
 drivers/firmware/arm_scmi/driver.c | 11 ++++++--
 2 files changed, 50 insertions(+), 3 deletions(-)

diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
index 7c35c95fddba..0a971cf28a44 100644
--- a/drivers/firmware/arm_scmi/common.h
+++ b/drivers/firmware/arm_scmi/common.h
@@ -17,6 +17,8 @@
 #include <linux/hashtable.h>
 #include <linux/list.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/property.h>
 #include <linux/refcount.h>
 #include <linux/scmi_protocol.h>
 #include <linux/spinlock.h>
@@ -458,6 +460,31 @@ struct scmi_transport_core_operations {
 	const struct scmi_message_operations *msg;
 };
 
+/**
+ * struct scmi_transport_instance_queue  - Transport instance queue descriptor
+ * @mtx: A mutex to protect the list
+ * @head: A list head to propagate per-instance and transport specific item
+ *	  descriptors to the SCMI core in an common way
+ */
+struct scmi_transport_instance_queue {
+	/* Protect the list */
+	struct mutex mtx;
+	struct list_head head;
+};
+
+#define DEFINE_SCMI_TRANSPORT_INSTANCE_QUEUE(_instaq)			\
+struct scmi_transport_instance_queue _instaq = {			\
+	__MUTEX_INITIALIZER(_instaq.mtx),				\
+	LIST_HEAD_INIT(_instaq.head),					\
+}
+
+#define SCMI_TRANSPORT_INSTANCE_REGISTER(_item, _instaq)	\
+do {								\
+	mutex_lock(&_instaq.mtx);				\
+	list_add_tail(&(_item), &_instaq.head);			\
+	mutex_unlock(&_instaq.mtx);				\
+} while (0)
+
 /**
  * struct scmi_transport  - A structure representing a configured transport
  *
@@ -466,11 +493,13 @@ struct scmi_transport_core_operations {
  * @desc: Transport descriptor
  * @core_ops: A pointer to a pointer used by the core SCMI stack to make the
  *	      core transport operations accessible to the transports.
+ * @hndl: An optional handle to the initialized transport instance.
  */
 struct scmi_transport {
 	struct device *supplier;
 	struct scmi_desc desc;
 	struct scmi_transport_core_operations **core_ops;
+	void *hndl;
 };
 
 #define DEFINE_SCMI_TRANSPORT_DRIVER(__tag, __drv, __desc, __match, __core_ops)\
@@ -483,11 +512,22 @@ static void __tag##_dev_free(void *data)				       \
 									       \
 static int __tag##_probe(struct platform_device *pdev)			       \
 {									       \
+	struct scmi_transport_instance_queue *tq;			       \
+	struct scmi_transport strans = {};				       \
 	struct device *dev = &pdev->dev;				       \
 	struct platform_device *spdev;					       \
-	struct scmi_transport strans;					       \
 	int ret;							       \
 									       \
+	tq = (void *)device_get_match_data(dev);			       \
+	if (tq) {							       \
+		scoped_guard(mutex, &tq->mtx) {				       \
+			if (list_empty(&tq->head))			       \
+				return -EPROBE_DEFER;			       \
+			strans.hndl = tq->head.next;			       \
+			list_del_init(tq->head.next);			       \
+		}							       \
+	}								       \
+									       \
 	spdev = platform_device_alloc("arm-scmi", PLATFORM_DEVID_AUTO);	       \
 	if (!spdev)							       \
 		return -ENOMEM;						       \
diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index 3e76a3204ba4..69361385be61 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -121,6 +121,7 @@ struct scmi_protocol_instance {
  * @id: A sequence number starting from zero identifying this instance
  * @dev: Device pointer
  * @desc: SoC description for this instance
+ * @thndl: Optional transport instance handle
  * @version: SCMI revision information containing protocol version,
  *	implementation version and (sub-)vendor identification.
  * @handle: Instance of SCMI handle to send to clients
@@ -151,6 +152,7 @@ struct scmi_info {
 	int id;
 	struct device *dev;
 	const struct scmi_desc *desc;
+	void *thndl;
 	struct scmi_revision_info version;
 	struct scmi_handle handle;
 	struct scmi_xfers_info tx_minfo;
@@ -3115,7 +3117,8 @@ static int scmi_debugfs_raw_mode_setup(struct scmi_info *info)
 	return ret;
 }
 
-static const struct scmi_desc *scmi_transport_setup(struct device *dev)
+static const struct scmi_desc *
+scmi_transport_setup(struct device *dev, void **thndl)
 {
 	struct scmi_transport *trans;
 	int ret;
@@ -3162,6 +3165,8 @@ static const struct scmi_desc *scmi_transport_setup(struct device *dev)
 			 "SCMI System wide atomic threshold set to %u us\n",
 			 trans->desc.atomic_threshold);
 
+	*thndl = trans->hndl;
+
 	return &trans->desc;
 }
 
@@ -3180,6 +3185,7 @@ static void scmi_enable_matching_quirks(struct scmi_info *info)
 static int scmi_probe(struct platform_device *pdev)
 {
 	int ret;
+	void *thndl;
 	char *err_str = "probe failure\n";
 	struct scmi_handle *handle;
 	const struct scmi_desc *desc;
@@ -3188,7 +3194,7 @@ static int scmi_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct device_node *child, *np = dev->of_node;
 
-	desc = scmi_transport_setup(dev);
+	desc = scmi_transport_setup(dev, &thndl);
 	if (!desc) {
 		err_str = "transport invalid\n";
 		ret = -EINVAL;
@@ -3205,6 +3211,7 @@ static int scmi_probe(struct platform_device *pdev)
 
 	info->dev = dev;
 	info->desc = desc;
+	info->thndl = thndl;
 	info->bus_nb.notifier_call = scmi_bus_notifier;
 	info->dev_req_nb.notifier_call = scmi_device_request_notifier;
 	INIT_LIST_HEAD(&info->node);
-- 
2.53.0

[RFC PATCH 2/4] firmware: arm_scmi: Propagate transport instance handle

[Cristian Marussi]

Propagate down from the SCMI core to the transport channel operations
the per-instance handle received by the transport drivers upon the end of
their probe.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/common.h             | 4 ++--
 drivers/firmware/arm_scmi/driver.c             | 4 ++--
 drivers/firmware/arm_scmi/transports/mailbox.c | 5 +++--
 drivers/firmware/arm_scmi/transports/optee.c   | 6 ++++--
 drivers/firmware/arm_scmi/transports/smc.c     | 4 ++--
 drivers/firmware/arm_scmi/transports/virtio.c  | 6 +++---
 6 files changed, 16 insertions(+), 13 deletions(-)

diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
index 0a971cf28a44..2e0116ff8522 100644
--- a/drivers/firmware/arm_scmi/common.h
+++ b/drivers/firmware/arm_scmi/common.h
@@ -205,9 +205,9 @@ struct scmi_chan_info {
  * @poll_done: Callback to poll transfer status
  */
 struct scmi_transport_ops {
-	bool (*chan_available)(struct device_node *of_node, int idx);
+	bool (*chan_available)(struct device_node *of_node, int idx, void *hndl);
 	int (*chan_setup)(struct scmi_chan_info *cinfo, struct device *dev,
-			  bool tx);
+			  bool tx, void *hndl);
 	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,
diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index 69361385be61..b6d46fc08795 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -2723,7 +2723,7 @@ static int scmi_chan_setup(struct scmi_info *info, struct device_node *of_node,
 	idx = tx ? 0 : 1;
 	idr = tx ? &info->tx_idr : &info->rx_idr;
 
-	if (!info->desc->ops->chan_available(of_node, idx)) {
+	if (!info->desc->ops->chan_available(of_node, idx, info->thndl)) {
 		cinfo = idr_find(idr, SCMI_PROTOCOL_BASE);
 		if (unlikely(!cinfo)) /* Possible only if platform has no Rx */
 			return -EINVAL;
@@ -2753,7 +2753,7 @@ static int scmi_chan_setup(struct scmi_info *info, struct device_node *of_node,
 
 	cinfo->id = prot_id;
 	cinfo->dev = &tdev->dev;
-	ret = info->desc->ops->chan_setup(cinfo, info->dev, tx);
+	ret = info->desc->ops->chan_setup(cinfo, info->dev, tx, info->thndl);
 	if (ret) {
 		of_node_put(of_node);
 		scmi_device_destroy(info->dev, prot_id, name);
diff --git a/drivers/firmware/arm_scmi/transports/mailbox.c b/drivers/firmware/arm_scmi/transports/mailbox.c
index ae0f67e6cc45..cf996724971d 100644
--- a/drivers/firmware/arm_scmi/transports/mailbox.c
+++ b/drivers/firmware/arm_scmi/transports/mailbox.c
@@ -77,7 +77,8 @@ static void rx_callback(struct mbox_client *cl, void *m)
 		      core->shmem->read_header(smbox->shmem), NULL);
 }
 
-static bool mailbox_chan_available(struct device_node *of_node, int idx)
+static bool
+mailbox_chan_available(struct device_node *of_node, int idx, void *hndl)
 {
 	int num_mb;
 
@@ -180,7 +181,7 @@ static int mailbox_chan_validate(struct device *cdev, int *a2p_rx_chan,
 }
 
 static int mailbox_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
-			      bool tx)
+			      bool tx, void *hndl)
 {
 	const char *desc = tx ? "Tx" : "Rx";
 	struct device *cdev = cinfo->dev;
diff --git a/drivers/firmware/arm_scmi/transports/optee.c b/drivers/firmware/arm_scmi/transports/optee.c
index 07ae18d5279d..ab578152e046 100644
--- a/drivers/firmware/arm_scmi/transports/optee.c
+++ b/drivers/firmware/arm_scmi/transports/optee.c
@@ -312,7 +312,8 @@ static int invoke_process_msg_channel(struct scmi_optee_channel *channel, size_t
 	return 0;
 }
 
-static bool scmi_optee_chan_available(struct device_node *of_node, int idx)
+static bool
+scmi_optee_chan_available(struct device_node *of_node, int idx, void *hndl)
 {
 	u32 channel_id;
 
@@ -367,7 +368,8 @@ static int setup_shmem(struct device *dev, struct scmi_chan_info *cinfo,
 		return setup_dynamic_shmem(dev, channel);
 }
 
-static int scmi_optee_chan_setup(struct scmi_chan_info *cinfo, struct device *dev, bool tx)
+static int scmi_optee_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
+				 bool tx, void *hndl)
 {
 	struct scmi_optee_channel *channel;
 	uint32_t channel_id;
diff --git a/drivers/firmware/arm_scmi/transports/smc.c b/drivers/firmware/arm_scmi/transports/smc.c
index 21abb571e4f2..cb0d9a8f93ca 100644
--- a/drivers/firmware/arm_scmi/transports/smc.c
+++ b/drivers/firmware/arm_scmi/transports/smc.c
@@ -84,7 +84,7 @@ static irqreturn_t smc_msg_done_isr(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
-static bool smc_chan_available(struct device_node *of_node, int idx)
+static bool smc_chan_available(struct device_node *of_node, int idx, void *hndl)
 {
 	struct device_node *np __free(device_node) =
 					of_parse_phandle(of_node, "shmem", 0);
@@ -130,7 +130,7 @@ static inline void smc_channel_lock_release(struct scmi_smc *scmi_info)
 }
 
 static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
-			  bool tx)
+			  bool tx, void *hndl)
 {
 	struct device *cdev = cinfo->dev;
 	unsigned long cap_id = ULONG_MAX;
diff --git a/drivers/firmware/arm_scmi/transports/virtio.c b/drivers/firmware/arm_scmi/transports/virtio.c
index 326c4a93e44b..6e1ce01507a6 100644
--- a/drivers/firmware/arm_scmi/transports/virtio.c
+++ b/drivers/firmware/arm_scmi/transports/virtio.c
@@ -373,7 +373,7 @@ static unsigned int virtio_get_max_msg(struct scmi_chan_info *base_cinfo)
 	return vioch->max_msg;
 }
 
-static bool virtio_chan_available(struct device_node *of_node, int idx)
+static bool virtio_chan_available(struct device_node *of_node, int idx, void *hndl)
 {
 	struct scmi_vio_channel *channels, *vioch = NULL;
 
@@ -402,8 +402,8 @@ static void scmi_destroy_tx_workqueue(void *deferred_tx_wq)
 	destroy_workqueue(deferred_tx_wq);
 }
 
-static int virtio_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
-			     bool tx)
+static int
+virtio_chan_setup(struct scmi_chan_info *cinfo, struct device *dev, bool tx, void *hndl)
 {
 	struct scmi_vio_channel *vioch;
 	int index = tx ? VIRTIO_SCMI_VQ_TX : VIRTIO_SCMI_VQ_RX;
-- 
2.53.0

[RFC PATCH 3/4] firmware: arm_scmi: virtio: Rework transport probe sequence

[Cristian Marussi]

Use the new per-instance transport handles list to synchronize and
optionally defer the core SCMI probe up until the transport driver has
completely been initialized and is fully operational.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/transports/virtio.c | 93 ++++++++++++-------
 1 file changed, 57 insertions(+), 36 deletions(-)

diff --git a/drivers/firmware/arm_scmi/transports/virtio.c b/drivers/firmware/arm_scmi/transports/virtio.c
index 6e1ce01507a6..ee7247081be1 100644
--- a/drivers/firmware/arm_scmi/transports/virtio.c
+++ b/drivers/firmware/arm_scmi/transports/virtio.c
@@ -4,7 +4,7 @@
  * (SCMI).
  *
  * Copyright (C) 2020-2022 OpenSynergy.
- * Copyright (C) 2021-2024 ARM Ltd.
+ * Copyright (C) 2021-2026 ARM Ltd.
  */
 
 /**
@@ -113,8 +113,12 @@ struct scmi_vio_msg {
 
 static struct scmi_transport_core_operations *core;
 
-/* Only one SCMI VirtIO device can possibly exist */
-static struct virtio_device *scmi_vdev;
+struct scmi_virtio_device {
+	struct virtio_device *vdev;
+	struct list_head node;
+};
+
+static DEFINE_SCMI_TRANSPORT_INSTANCE_QUEUE(scmi_available_vdevs);
 
 static void scmi_vio_channel_ready(struct scmi_vio_channel *vioch,
 				   struct scmi_chan_info *cinfo)
@@ -376,24 +380,28 @@ static unsigned int virtio_get_max_msg(struct scmi_chan_info *base_cinfo)
 static bool virtio_chan_available(struct device_node *of_node, int idx, void *hndl)
 {
 	struct scmi_vio_channel *channels, *vioch = NULL;
+	struct scmi_virtio_device *sdev;
 
-	if (WARN_ON_ONCE(!scmi_vdev))
-		return false;
+	sdev = list_entry(hndl, struct scmi_virtio_device, node);
 
-	channels = (struct scmi_vio_channel *)scmi_vdev->priv;
+	channels = (struct scmi_vio_channel *)sdev->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))
+		if (scmi_vio_have_vq_rx(sdev->vdev))
 			vioch = &channels[VIRTIO_SCMI_VQ_RX];
 		break;
 	default:
 		return false;
 	}
 
+	dev_dbg(&sdev->vdev->dev, "%s Channel %sAVAILABLE on SCMI Virtio device.\n",
+		idx == VIRTIO_SCMI_VQ_TX ? "TX" : "RX",
+		(vioch && !vioch->cinfo) ? "" : "NOT ");
+
 	return vioch && !vioch->cinfo;
 }
 
@@ -405,21 +413,20 @@ static void scmi_destroy_tx_workqueue(void *deferred_tx_wq)
 static int
 virtio_chan_setup(struct scmi_chan_info *cinfo, struct device *dev, bool tx, void *hndl)
 {
+	struct scmi_virtio_device *sdev;
 	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];
+	sdev = list_entry(hndl, struct scmi_virtio_device, node);
+	vioch = &((struct scmi_vio_channel *)sdev->vdev->priv)[index];
 
 	/* Setup a deferred worker for polling. */
 	if (tx && !vioch->deferred_tx_wq) {
 		int ret;
 
 		vioch->deferred_tx_wq =
-			alloc_workqueue(dev_name(&scmi_vdev->dev),
+			alloc_workqueue(dev_name(&sdev->vdev->dev),
 					WQ_UNBOUND | WQ_FREEZABLE | WQ_SYSFS,
 					0);
 		if (!vioch->deferred_tx_wq)
@@ -460,6 +467,9 @@ virtio_chan_setup(struct scmi_chan_info *cinfo, struct device *dev, bool tx, voi
 
 	scmi_vio_channel_ready(vioch, cinfo);
 
+	dev_dbg(&sdev->vdev->dev, "%s Channel SETUP on SCMI Virtio device.\n",
+		tx ? "TX" : "RX");
+
 	return 0;
 }
 
@@ -801,7 +811,7 @@ static struct scmi_desc scmi_virtio_desc = {
 };
 
 static const struct of_device_id scmi_of_match[] = {
-	{ .compatible = "arm,scmi-virtio" },
+	{ .compatible = "arm,scmi-virtio", .data = &scmi_available_vdevs},
 	{ /* Sentinel */ },
 };
 
@@ -812,22 +822,20 @@ static int scmi_vio_probe(struct virtio_device *vdev)
 {
 	struct device *dev = &vdev->dev;
 	struct scmi_vio_channel *channels;
+	struct scmi_virtio_device *sdev;
 	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) {
-		dev_err(dev,
-			"One SCMI Virtio device was already initialized: only one allowed.\n");
-		return -EBUSY;
-	}
-
 	have_vq_rx = scmi_vio_have_vq_rx(vdev);
 	vq_cnt = have_vq_rx ? VIRTIO_SCMI_VQ_MAX_CNT : 1;
 
+	sdev = devm_kzalloc(dev, sizeof(*sdev), GFP_KERNEL);
+	if (!sdev)
+		return -ENOMEM;
+
 	channels = devm_kcalloc(dev, vq_cnt, sizeof(*channels), GFP_KERNEL);
 	if (!channels)
 		return -ENOMEM;
@@ -864,33 +872,25 @@ static int scmi_vio_probe(struct virtio_device *vdev)
 			sz = MSG_TOKEN_MAX;
 		}
 		channels[i].max_msg = sz;
+		dev_info(dev, "VQ%d initialized with max_msg: %d\n", i, sz);
 	}
 
 	vdev->priv = channels;
 
-	/* Ensure initialized scmi_vdev is visible */
-	smp_store_mb(scmi_vdev, vdev);
+	/* Ensure initialized vdev is visible */
+	smp_store_mb(sdev->vdev, vdev);
+	SCMI_TRANSPORT_INSTANCE_REGISTER(sdev->node, scmi_available_vdevs);
 
 	/* Set device ready */
 	virtio_device_ready(vdev);
 
-	ret = platform_driver_register(&scmi_virtio_driver);
-	if (ret) {
-		vdev->priv = NULL;
-		vdev->config->del_vqs(vdev);
-		/* Ensure NULLified scmi_vdev is visible */
-		smp_store_mb(scmi_vdev, NULL);
-
-		return ret;
-	}
+	dev_info(dev, "Probed and initialized SCMI Virtio device.\n");
 
 	return 0;
 }
 
 static void scmi_vio_remove(struct virtio_device *vdev)
 {
-	platform_driver_unregister(&scmi_virtio_driver);
-
 	/*
 	 * Once we get here, virtio_chan_free() will have already been called by
 	 * the SCMI core for any existing channel and, as a consequence, all the
@@ -900,8 +900,6 @@ static void scmi_vio_remove(struct virtio_device *vdev)
 	 */
 	virtio_reset_device(vdev);
 	vdev->config->del_vqs(vdev);
-	/* Ensure scmi_vdev is visible as NULL */
-	smp_store_mb(scmi_vdev, NULL);
 }
 
 static int scmi_vio_validate(struct virtio_device *vdev)
@@ -936,7 +934,30 @@ static struct virtio_driver virtio_scmi_driver = {
 	.validate = scmi_vio_validate,
 };
 
-module_virtio_driver(virtio_scmi_driver);
+static int __init scmi_transport_virtio_init(void)
+{
+	int ret;
+
+	ret = register_virtio_driver(&virtio_scmi_driver);
+	if (ret)
+		return ret;
+
+	ret = platform_driver_register(&scmi_virtio_driver);
+	if (ret) {
+		unregister_virtio_driver(&virtio_scmi_driver);
+		return ret;
+	}
+
+	return ret;
+}
+module_init(scmi_transport_virtio_init);
+
+static void __exit scmi_transport_virtio_exit(void)
+{
+	platform_driver_unregister(&scmi_virtio_driver);
+	unregister_virtio_driver(&virtio_scmi_driver);
+}
+module_exit(scmi_transport_virtio_exit);
 
 MODULE_AUTHOR("Igor Skalkin <igor.skalkin@opensynergy.com>");
 MODULE_AUTHOR("Peter Hilber <peter.hilber@opensynergy.com>");
-- 
2.53.0

[RFC PATCH 4/4] firmware: arm_scmi: optee: Rework transport probe sequence

[Cristian Marussi]

Use the new per-instance transport handles list to synchronize and
optionally defer the core SCMI probe up until the transport driver has
completely been initialized and is fully operational.

Add also a local xarray to keep track of the association between the
tee_client_device and the per-instance agent for the purpose of resource
cleanup.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/transports/optee.c | 137 +++++++++++--------
 1 file changed, 79 insertions(+), 58 deletions(-)

diff --git a/drivers/firmware/arm_scmi/transports/optee.c b/drivers/firmware/arm_scmi/transports/optee.c
index ab578152e046..08f7b2b9b9d6 100644
--- a/drivers/firmware/arm_scmi/transports/optee.c
+++ b/drivers/firmware/arm_scmi/transports/optee.c
@@ -13,6 +13,7 @@
 #include <linux/slab.h>
 #include <linux/tee_drv.h>
 #include <linux/uuid.h>
+#include <linux/xarray.h>
 #include <uapi/linux/tee.h>
 
 #include "../common.h"
@@ -99,6 +100,26 @@ enum scmi_optee_pta_cmd {
 #define PTA_SCMI_CAPS_MASK		(PTA_SCMI_CAPS_SMT_HEADER | \
 					 PTA_SCMI_CAPS_MSG_HEADER)
 
+/**
+ * struct scmi_optee_agent - OP-TEE transport private data
+ *
+ * @dev: Device used for communication with TEE
+ * @tee_ctx: TEE context used for communication
+ * @caps: Supported channel capabilities
+ * @mu: Mutex for protection of @channel_list
+ * @channel_list: List of all created channels for the agent
+ * @node: Used to enqueue this agent
+ */
+struct scmi_optee_agent {
+	struct device *dev;
+	struct tee_context *tee_ctx;
+	u32 caps;
+	/* Protect channel_list*/
+	struct mutex mu;
+	struct list_head channel_list;
+	struct list_head node;
+};
+
 /**
  * struct scmi_optee_channel - Description of an OP-TEE SCMI channel
  *
@@ -115,6 +136,7 @@ enum scmi_optee_pta_cmd {
  * @io_ops: Transport specific I/O operations
  * @tee_shm: TEE shared memory handle @req or NULL if using IOMEM shmem
  * @link: Reference in agent's channel list
+ * @agent: Reference to the agent owning this channel
  */
 struct scmi_optee_channel {
 	u32 channel_id;
@@ -130,29 +152,13 @@ struct scmi_optee_channel {
 	struct scmi_shmem_io_ops *io_ops;
 	struct tee_shm *tee_shm;
 	struct list_head link;
-};
-
-/**
- * struct scmi_optee_agent - OP-TEE transport private data
- *
- * @dev: Device used for communication with TEE
- * @tee_ctx: TEE context used for communication
- * @caps: Supported channel capabilities
- * @mu: Mutex for protection of @channel_list
- * @channel_list: List of all created channels for the agent
- */
-struct scmi_optee_agent {
-	struct device *dev;
-	struct tee_context *tee_ctx;
-	u32 caps;
-	struct mutex mu;
-	struct list_head channel_list;
+	struct scmi_optee_agent *agent;
 };
 
 static struct scmi_transport_core_operations *core;
 
-/* There can be only 1 SCMI service in OP-TEE we connect to */
-static struct scmi_optee_agent *scmi_optee_private;
+static DEFINE_XARRAY(scmi_active_agents);
+static DEFINE_SCMI_TRANSPORT_INSTANCE_QUEUE(scmi_available_agents);
 
 /* Open a session toward SCMI OP-TEE service with REE_KERNEL identity */
 static int open_session(struct scmi_optee_agent *agent, u32 *tee_session)
@@ -222,7 +228,7 @@ static int get_capabilities(struct scmi_optee_agent *agent)
 
 static int get_channel(struct scmi_optee_channel *channel)
 {
-	struct device *dev = scmi_optee_private->dev;
+	struct device *dev = channel->agent->dev;
 	struct tee_ioctl_invoke_arg arg = { };
 	struct tee_param param[1] = { };
 	unsigned int caps = 0;
@@ -241,7 +247,7 @@ static int get_channel(struct scmi_optee_channel *channel)
 	param[0].u.value.a = channel->channel_id;
 	param[0].u.value.b = caps;
 
-	ret = tee_client_invoke_func(scmi_optee_private->tee_ctx, &arg, param);
+	ret = tee_client_invoke_func(channel->agent->tee_ctx, &arg, param);
 
 	if (ret || arg.ret) {
 		dev_err(dev, "Can't get channel with caps %#x: %d / %#x\n", caps, ret, arg.ret);
@@ -268,9 +274,9 @@ static int invoke_process_smt_channel(struct scmi_optee_channel *channel)
 	param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT;
 	param[0].u.value.a = channel->channel_id;
 
-	ret = tee_client_invoke_func(scmi_optee_private->tee_ctx, &arg, param);
+	ret = tee_client_invoke_func(channel->agent->tee_ctx, &arg, param);
 	if (ret < 0 || arg.ret) {
-		dev_err(scmi_optee_private->dev, "Can't invoke channel %u: %d / %#x\n",
+		dev_err(channel->agent->dev, "Can't invoke channel %u: %d / %#x\n",
 			channel->channel_id, ret, arg.ret);
 		return -EIO;
 	}
@@ -299,9 +305,9 @@ static int invoke_process_msg_channel(struct scmi_optee_channel *channel, size_t
 	param[2].u.memref.shm = channel->tee_shm;
 	param[2].u.memref.size = SCMI_SHMEM_MAX_PAYLOAD_SIZE;
 
-	ret = tee_client_invoke_func(scmi_optee_private->tee_ctx, &arg, param);
+	ret = tee_client_invoke_func(channel->agent->tee_ctx, &arg, param);
 	if (ret < 0 || arg.ret) {
-		dev_err(scmi_optee_private->dev, "Can't invoke channel %u: %d / %#x\n",
+		dev_err(channel->agent->dev, "Can't invoke channel %u: %d / %#x\n",
 			channel->channel_id, ret, arg.ret);
 		return -EIO;
 	}
@@ -334,7 +340,8 @@ static int setup_dynamic_shmem(struct device *dev, struct scmi_optee_channel *ch
 	const size_t msg_size = SCMI_SHMEM_MAX_PAYLOAD_SIZE;
 	void *shbuf;
 
-	channel->tee_shm = tee_shm_alloc_kernel_buf(scmi_optee_private->tee_ctx, msg_size);
+	channel->tee_shm = tee_shm_alloc_kernel_buf(channel->agent->tee_ctx,
+						    msg_size);
 	if (IS_ERR(channel->tee_shm)) {
 		dev_err(channel->cinfo->dev, "shmem allocation failed\n");
 		return -ENOMEM;
@@ -390,17 +397,19 @@ static int scmi_optee_chan_setup(struct scmi_chan_info *cinfo, struct device *de
 	cinfo->transport_info = channel;
 	channel->cinfo = cinfo;
 	channel->channel_id = channel_id;
+	channel->agent = list_entry(hndl, struct scmi_optee_agent, node);
 	mutex_init(&channel->mu);
 
 	ret = setup_shmem(dev, cinfo, channel);
 	if (ret)
 		return ret;
 
-	ret = open_session(scmi_optee_private, &channel->tee_session);
+	ret = open_session(channel->agent, &channel->tee_session);
 	if (ret)
 		goto err_free_shm;
 
-	ret = tee_client_system_session(scmi_optee_private->tee_ctx, channel->tee_session);
+	ret = tee_client_system_session(channel->agent->tee_ctx,
+					channel->tee_session);
 	if (ret)
 		dev_warn(dev, "Could not switch to system session, do best effort\n");
 
@@ -411,14 +420,14 @@ static int scmi_optee_chan_setup(struct scmi_chan_info *cinfo, struct device *de
 	/* Enable polling */
 	cinfo->no_completion_irq = true;
 
-	mutex_lock(&scmi_optee_private->mu);
-	list_add(&channel->link, &scmi_optee_private->channel_list);
-	mutex_unlock(&scmi_optee_private->mu);
+	mutex_lock(&channel->agent->mu);
+	list_add(&channel->link, &channel->agent->channel_list);
+	mutex_unlock(&channel->agent->mu);
 
 	return 0;
 
 err_close_sess:
-	close_session(scmi_optee_private, channel->tee_session);
+	close_session(channel->agent, channel->tee_session);
 err_free_shm:
 	if (channel->tee_shm)
 		tee_shm_free(channel->tee_shm);
@@ -438,11 +447,11 @@ static int scmi_optee_chan_free(int id, void *p, void *data)
 	if (!channel)
 		return 0;
 
-	mutex_lock(&scmi_optee_private->mu);
+	mutex_lock(&channel->agent->mu);
 	list_del(&channel->link);
-	mutex_unlock(&scmi_optee_private->mu);
+	mutex_unlock(&channel->agent->mu);
 
-	close_session(scmi_optee_private, channel->tee_session);
+	close_session(channel->agent, channel->tee_session);
 
 	if (channel->tee_shm) {
 		tee_shm_free(channel->tee_shm);
@@ -524,7 +533,7 @@ static struct scmi_desc scmi_optee_desc = {
 };
 
 static const struct of_device_id scmi_of_match[] = {
-	{ .compatible = "linaro,scmi-optee" },
+	{ .compatible = "linaro,scmi-optee", .data = &scmi_available_agents},
 	{ /* Sentinel */ },
 };
 
@@ -538,12 +547,6 @@ static int scmi_optee_service_probe(struct tee_client_device *scmi_pta)
 	struct tee_context *tee_ctx;
 	int ret;
 
-	/* Only one SCMI OP-TEE device allowed */
-	if (scmi_optee_private) {
-		dev_err(dev, "An SCMI OP-TEE device was already initialized: only one allowed\n");
-		return -EBUSY;
-	}
-
 	tee_ctx = tee_client_open_context(NULL, scmi_optee_ctx_match, NULL, NULL);
 	if (IS_ERR(tee_ctx))
 		return -ENODEV;
@@ -563,15 +566,14 @@ static int scmi_optee_service_probe(struct tee_client_device *scmi_pta)
 	if (ret)
 		goto err;
 
-	/* Ensure agent resources are all visible before scmi_optee_private is */
+	/* Ensure agent resources are visible */
 	smp_mb();
-	scmi_optee_private = agent;
-
-	ret = platform_driver_register(&scmi_optee_driver);
-	if (ret) {
-		scmi_optee_private = NULL;
+	ret = xa_insert(&scmi_active_agents, (unsigned long)scmi_pta, agent,
+			GFP_KERNEL);
+	if (ret)
 		goto err;
-	}
+
+	SCMI_TRANSPORT_INSTANCE_REGISTER(agent->node, scmi_available_agents);
 
 	return 0;
 
@@ -583,19 +585,15 @@ static int scmi_optee_service_probe(struct tee_client_device *scmi_pta)
 
 static void scmi_optee_service_remove(struct tee_client_device *scmi_pta)
 {
-	struct scmi_optee_agent *agent = scmi_optee_private;
+	struct scmi_optee_agent *agent;
 
-	if (!scmi_optee_private)
+	agent = xa_erase(&scmi_active_agents, (unsigned long)scmi_pta);
+	if (!agent)
 		return;
 
-	platform_driver_unregister(&scmi_optee_driver);
-
-	if (!list_empty(&scmi_optee_private->channel_list))
+	if (!list_empty(&agent->channel_list))
 		return;
 
-	/* Ensure cleared reference is visible before resources are released */
-	smp_store_mb(scmi_optee_private, NULL);
-
 	tee_client_close_context(agent->tee_ctx);
 }
 
@@ -618,7 +616,30 @@ static struct tee_client_driver scmi_optee_service_driver = {
 	},
 };
 
-module_tee_client_driver(scmi_optee_service_driver);
+static int __init scmi_transport_optee_init(void)
+{
+	int ret;
+
+	ret = tee_client_driver_register(&scmi_optee_service_driver);
+	if (ret)
+		return ret;
+
+	ret = platform_driver_register(&scmi_optee_driver);
+	if (ret) {
+		tee_client_driver_unregister(&scmi_optee_service_driver);
+		return ret;
+	}
+
+	return ret;
+}
+module_init(scmi_transport_optee_init);
+
+static void __exit scmi_transport_optee_exit(void)
+{
+	platform_driver_unregister(&scmi_optee_driver);
+	tee_client_driver_unregister(&scmi_optee_service_driver);
+}
+module_exit(scmi_transport_optee_exit);
 
 MODULE_AUTHOR("Etienne Carriere <etienne.carriere@foss.st.com>");
 MODULE_DESCRIPTION("SCMI OPTEE Transport driver");
-- 
2.53.0

Re: [RFC PATCH 0/4] Rework SCMI transport drivers probing sequence

[Cristian Marussi]

On Tue, Mar 17, 2026 at 04:58:07PM +0000, Cristian Marussi wrote:
> Hi,
> 

Hi,

a clarification replying to myself down-below... O_o

> when the SCMI transports were split out into standalone drivers [1] the
> probe sequence was laid out in such a way that:
> 
>  - the transport drivers would have probed first, triggered by the firmware
>    driven discovery process (DT/ACPI)
> 
>  - afterwards the control would have been passed to the core SCMI stack
>    driver via the creation of a dedicated device that would have inherited
>    the original firmware descriptor (since that same DT/ACPI node would
>    have still needed by the SCMI core driver to be parsed)
> 
> The tricky part came around with some transport driver like virtio and
> optee since they are, in turn, upfront dependent on an external distinct
> kernel subsystem; IOW these have first to undergo their own subsystem
> specific probe/initialization to become fully operational as transports:
> this kind of initialization sequencing of course must deal with the
> possibility of probe deferrals BUT at that time we avoid this by using the
> trick in virtio/optee transports to register the next stage transport
> drivers ONLY at the end of the subsystem specific probe routine, from
> within the probe itself.
> 
> This behaviour has 2 issues:
> 
>  - it is frowned upon and can lead to hangs in the driver core whenever
>    some core locking is changed as exposed in [2]
>  - it limits these transport drivers to a single instance probing since of
>    course you cannot register the same driver more than once
> 
> All of this was tolerable because optee/virtio are generally only employed
> in a single SCMI instance scenario AND because no hang where triggered in
> the core driver subsystem... (not saying that was perfectly fine :P)
> 
> This RFC series aims to solve both of the above issue by:
> 
>  - moving the problematic platform driver registation away from the probe
>    into its own module_init/exit
> 
>  - introducing an optional mechanism for the transport drivers to provide
>    to the SCMI core some sort of transport specific handle so that multiple
>    instances can be supported and multiple probed transports instances can
>    be registered and identified by the core. (who is who)
>    Note that at the same time this optional handle-mechanism is used to
>    synchronize the drivers probing sequences and possibly trigger, when
>    needed, the probe deferrals, if the transport driver has still to
>    undergo its own subsystem initialization and as such is still NOT fully
>    operational.

Implementation issues aside, one thing that I failed to make clear about
this series is that, while this series addresses the platform registration
@probe-time issue above and enables the capability to technically support
multiple SCMI instances also for optee/virtio transports (like it is
already for mailbox and smc), the multi instance support for optee/virtio,
as of now, still suffers from some 'structural' limitations that make it
unfit for production and useful ONLY on a testing scenario (...like in my
virtio based setup :D)

The crux of the matter lays in the fact that there is still not a way
in such transports to be able to match a specific probed transport instance
(device) with the corresponding SCMI DT top node instance descriptor that
usually, in a multi instance scenario, describes a different set of
protocols, or the same set of protocols enumerating a different set of
resources...so, e.g., if you want to match and reference by phandle a
specific clock domain from a specific instance you cannot really be sure
that the instance reference that you have is the instance that you wanted
since depends on how that was associated during the probe.
(in mailbox and smc you have an explicit channel/shmem association ...)

Still a lot to reason about this as of now...any suggestion about this
is even more very much welcome :P

Thanks,
Cristian