Hi,
under some configurations the SCMI core stack, which is now initialized
as a whole at the subsys_initcall level, can be dependent on some other
Kernel subsystems (like TEE) when some SCMI transport backend like optee
is used.
This has been reported to lead to some awkward probe loop which, even
though successful at the end, leaves a track of errors in the logs coming
directly from the core Linux driver model facilities.
In order to solve this issue and cleaning up a bit the SCMI stack startup
sequence, this small series reviews and reworks the SCMI core stack
initialization and probe logic.
Basically the SCMI Bus is split into its own module (scmi-core.ko) which is
initialized at subsys_initcall, while the SCMI core stack, including its
various transport backends (like optee, mailbox, virtio, smc), is kept into
a distinct module (scmi-module.ko) which get initialized at module_init.
The SCMI driver users initlevel, instead, remains unchanged at module_init.
No change is made to the Kconfig: the main ARM_SCMI_PROTOCOL option will
now cause both modules to be built.
This allows the other possibly needed subsystems to be up and running
well before the core SCMI stack and its dependent transport backends, so
solving the reported issue.
Tested with SCMI transports mailbox/virtio and, in a previous draft, optee,
in a number of different load/unload/bind/unbind combinations both as
builtin and as LKMs.
Applies on v6.1.
Any feedback, testing welcome.
Thanks,
Cristian
Cristian Marussi (9):
firmware: arm_scmi: Simplify chan_available transport operation
firmware: arm_scmi: Use dedicated devices to initialize channels
firmware: arm_scmi: Move protocol registration helpers
firmware: arm_scmi: Add common notifier helpers
firmware: arm_scmi: Refactor protocol device creation
firmware: arm_scmi: Move handle get/set helpers
firmware: arm_scmi: Refactor device create/destroy helpers
firmware: arm_scmi: Introduce a new lifecycle for protocol devices
firmware: arm_scmi: Split bus and driver into distinct modules
drivers/firmware/arm_scmi/Makefile | 8 +-
drivers/firmware/arm_scmi/bus.c | 388 ++++++++++++-----
drivers/firmware/arm_scmi/common.h | 25 +-
drivers/firmware/arm_scmi/driver.c | 623 ++++++++++++++--------------
drivers/firmware/arm_scmi/mailbox.c | 6 +-
drivers/firmware/arm_scmi/optee.c | 6 +-
drivers/firmware/arm_scmi/smc.c | 6 +-
drivers/firmware/arm_scmi/virtio.c | 4 +-
include/linux/scmi_protocol.h | 5 -
9 files changed, 622 insertions(+), 449 deletions(-)
--
2.34.1
SCMI transport operation .chan_available determines in a transport-specific
way if an SCMI channel is still available and to be configured: such
information is derived analyzing bits of DT in a transport specific way;
all it needs is a DT node to operate upon, not necessarily a full blown
device.
Simplify the helper to receive in input a reference to a device_node
instead of a device carrying a pointer to such device_node.
Signed-off-by: Cristian Marussi <[email protected]>
---
drivers/firmware/arm_scmi/common.h | 2 +-
drivers/firmware/arm_scmi/driver.c | 2 +-
drivers/firmware/arm_scmi/mailbox.c | 4 ++--
drivers/firmware/arm_scmi/optee.c | 4 ++--
drivers/firmware/arm_scmi/smc.c | 4 ++--
drivers/firmware/arm_scmi/virtio.c | 2 +-
6 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
index a1c0154c31c6..16db1e177123 100644
--- a/drivers/firmware/arm_scmi/common.h
+++ b/drivers/firmware/arm_scmi/common.h
@@ -153,7 +153,7 @@ struct scmi_chan_info {
*/
struct scmi_transport_ops {
int (*link_supplier)(struct device *dev);
- bool (*chan_available)(struct device *dev, int idx);
+ bool (*chan_available)(struct device_node *of_node, int idx);
int (*chan_setup)(struct scmi_chan_info *cinfo, struct device *dev,
bool tx);
int (*chan_free)(int id, void *p, void *data);
diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index ffdad59ec81f..f1ebadffdfe4 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -2003,7 +2003,7 @@ static int scmi_chan_setup(struct scmi_info *info, struct device *dev,
if (cinfo)
return 0;
- if (!info->desc->ops->chan_available(dev, idx)) {
+ if (!info->desc->ops->chan_available(dev->of_node, idx)) {
cinfo = idr_find(idr, SCMI_PROTOCOL_BASE);
if (unlikely(!cinfo)) /* Possible only if platform has no Rx */
return -EINVAL;
diff --git a/drivers/firmware/arm_scmi/mailbox.c b/drivers/firmware/arm_scmi/mailbox.c
index 1e40cb035044..c33dcadafea8 100644
--- a/drivers/firmware/arm_scmi/mailbox.c
+++ b/drivers/firmware/arm_scmi/mailbox.c
@@ -46,9 +46,9 @@ static void rx_callback(struct mbox_client *cl, void *m)
scmi_rx_callback(smbox->cinfo, shmem_read_header(smbox->shmem), NULL);
}
-static bool mailbox_chan_available(struct device *dev, int idx)
+static bool mailbox_chan_available(struct device_node *of_node, int idx)
{
- return !of_parse_phandle_with_args(dev->of_node, "mboxes",
+ return !of_parse_phandle_with_args(of_node, "mboxes",
"#mbox-cells", idx, NULL);
}
diff --git a/drivers/firmware/arm_scmi/optee.c b/drivers/firmware/arm_scmi/optee.c
index 2a7aeab40e54..5a11091c72da 100644
--- a/drivers/firmware/arm_scmi/optee.c
+++ b/drivers/firmware/arm_scmi/optee.c
@@ -328,11 +328,11 @@ static int scmi_optee_link_supplier(struct device *dev)
return 0;
}
-static bool scmi_optee_chan_available(struct device *dev, int idx)
+static bool scmi_optee_chan_available(struct device_node *of_node, int idx)
{
u32 channel_id;
- return !of_property_read_u32_index(dev->of_node, "linaro,optee-channel-id",
+ return !of_property_read_u32_index(of_node, "linaro,optee-channel-id",
idx, &channel_id);
}
diff --git a/drivers/firmware/arm_scmi/smc.c b/drivers/firmware/arm_scmi/smc.c
index 87a7b13cf868..122128d56d2f 100644
--- a/drivers/firmware/arm_scmi/smc.c
+++ b/drivers/firmware/arm_scmi/smc.c
@@ -52,9 +52,9 @@ static irqreturn_t smc_msg_done_isr(int irq, void *data)
return IRQ_HANDLED;
}
-static bool smc_chan_available(struct device *dev, int idx)
+static bool smc_chan_available(struct device_node *of_node, int idx)
{
- struct device_node *np = of_parse_phandle(dev->of_node, "shmem", 0);
+ struct device_node *np = of_parse_phandle(of_node, "shmem", 0);
if (!np)
return false;
diff --git a/drivers/firmware/arm_scmi/virtio.c b/drivers/firmware/arm_scmi/virtio.c
index 1db975c08896..a917eca7d902 100644
--- a/drivers/firmware/arm_scmi/virtio.c
+++ b/drivers/firmware/arm_scmi/virtio.c
@@ -385,7 +385,7 @@ static int virtio_link_supplier(struct device *dev)
return 0;
}
-static bool virtio_chan_available(struct device *dev, int idx)
+static bool virtio_chan_available(struct device_node *of_node, int idx)
{
struct scmi_vio_channel *channels, *vioch = NULL;
--
2.34.1
Add a pair of notifier chains and generic empty notifier callbacks: still
currently unused but they will be used to act properly on device request
and creation events.
Signed-off-by: Cristian Marussi <[email protected]>
---
drivers/firmware/arm_scmi/bus.c | 6 ++-
drivers/firmware/arm_scmi/common.h | 5 ++
drivers/firmware/arm_scmi/driver.c | 83 +++++++++++++++++++++++++++++-
3 files changed, 92 insertions(+), 2 deletions(-)
diff --git a/drivers/firmware/arm_scmi/bus.c b/drivers/firmware/arm_scmi/bus.c
index 089957f5fb9b..bed566f58029 100644
--- a/drivers/firmware/arm_scmi/bus.c
+++ b/drivers/firmware/arm_scmi/bus.c
@@ -15,6 +15,9 @@
#include "common.h"
+BLOCKING_NOTIFIER_HEAD(scmi_requested_devices_nh);
+EXPORT_SYMBOL_GPL(scmi_requested_devices_nh);
+
static DEFINE_IDA(scmi_bus_id);
static const struct scmi_device_id *
@@ -94,12 +97,13 @@ static void scmi_dev_remove(struct device *dev)
scmi_drv->remove(scmi_dev);
}
-static struct bus_type scmi_bus_type = {
+struct bus_type scmi_bus_type = {
.name = "scmi_protocol",
.match = scmi_dev_match,
.probe = scmi_dev_probe,
.remove = scmi_dev_remove,
};
+EXPORT_SYMBOL_GPL(scmi_bus_type);
int scmi_driver_register(struct scmi_driver *driver, struct module *owner,
const char *mod_name)
diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
index 6a38244494fd..7ddae90eb945 100644
--- a/drivers/firmware/arm_scmi/common.h
+++ b/drivers/firmware/arm_scmi/common.h
@@ -105,6 +105,11 @@ void scmi_setup_protocol_implemented(const struct scmi_protocol_handle *ph,
int __init scmi_bus_init(void);
void __exit scmi_bus_exit(void);
+extern struct bus_type scmi_bus_type;
+
+#define SCMI_BUS_NOTIFY_DEVICE_REQUEST 0
+#define SCMI_BUS_NOTIFY_DEVICE_UNREQUEST 1
+extern struct blocking_notifier_head scmi_requested_devices_nh;
int scmi_protocol_acquire(const struct scmi_handle *handle, u8 protocol_id);
void scmi_protocol_release(const struct scmi_handle *handle, u8 protocol_id);
diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index d1e32ea6d90a..62760bed1645 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -150,6 +150,9 @@ struct scmi_protocol_instance {
* @notify_priv: Pointer to private data structure specific to notifications.
* @node: List head
* @users: Number of users of this instance
+ * @bus_nb: A notifier to listen for device bind/unbind on the scmi bus
+ * @dev_req_nb: A notifier to listen for device request/unrequest on the scmi
+ * bus
*/
struct scmi_info {
struct device *dev;
@@ -169,9 +172,13 @@ struct scmi_info {
void *notify_priv;
struct list_head node;
int users;
+ struct notifier_block bus_nb;
+ struct notifier_block dev_req_nb;
};
#define handle_to_scmi_info(h) container_of(h, struct scmi_info, handle)
+#define bus_nb_to_scmi_info(nb) container_of(nb, struct scmi_info, bus_nb)
+#define req_nb_to_scmi_info(nb) container_of(nb, struct scmi_info, dev_req_nb)
static const int scmi_linux_errmap[] = {
/* better than switch case as long as return value is continuous */
@@ -2509,6 +2516,60 @@ static void scmi_cleanup_txrx_channels(struct scmi_info *info)
scmi_cleanup_channels(info, &info->rx_idr);
}
+static int scmi_bus_notifier(struct notifier_block *nb,
+ unsigned long action, void *data)
+{
+ struct scmi_info *info = bus_nb_to_scmi_info(nb);
+ struct scmi_device *sdev = to_scmi_dev(data);
+
+ /* Skip transport devices and devices of different SCMI instances */
+ if (!strncmp(sdev->name, "__scmi_transport_device", 23) ||
+ sdev->dev.parent != info->dev)
+ return NOTIFY_DONE;
+
+ switch (action) {
+ case BUS_NOTIFY_BIND_DRIVER:
+ break;
+ case BUS_NOTIFY_UNBOUND_DRIVER:
+ break;
+ default:
+ return NOTIFY_DONE;
+ }
+
+ dev_dbg(info->dev, "Device %s (%s) is now %s\n", dev_name(&sdev->dev),
+ sdev->name, action == BUS_NOTIFY_BIND_DRIVER ?
+ "about to be BOUND." : "UNBOUND.");
+
+ return NOTIFY_OK;
+}
+
+static int scmi_device_request_notifier(struct notifier_block *nb,
+ unsigned long action, void *data)
+{
+ struct device_node *np;
+ struct scmi_device_id *id_table = data;
+ struct scmi_info *info = req_nb_to_scmi_info(nb);
+
+ np = idr_find(&info->active_protocols, id_table->protocol_id);
+ if (!np)
+ return NOTIFY_DONE;
+
+ dev_dbg(info->dev, "%sRequested device (%s) for protocol 0x%x\n",
+ action == SCMI_BUS_NOTIFY_DEVICE_REQUEST ? "" : "UN-",
+ id_table->name, id_table->protocol_id);
+
+ switch (action) {
+ case SCMI_BUS_NOTIFY_DEVICE_REQUEST:
+ break;
+ case SCMI_BUS_NOTIFY_DEVICE_UNREQUEST:
+ break;
+ default:
+ return NOTIFY_DONE;
+ }
+
+ return NOTIFY_OK;
+}
+
static int scmi_probe(struct platform_device *pdev)
{
int ret;
@@ -2528,6 +2589,8 @@ static int scmi_probe(struct platform_device *pdev)
info->dev = dev;
info->desc = desc;
+ info->bus_nb.notifier_call = scmi_bus_notifier;
+ info->dev_req_nb.notifier_call = scmi_device_request_notifier;
INIT_LIST_HEAD(&info->node);
idr_init(&info->protocols);
mutex_init(&info->protocols_mtx);
@@ -2563,10 +2626,19 @@ static int scmi_probe(struct platform_device *pdev)
if (ret)
return ret;
- ret = scmi_xfer_info_init(info);
+ ret = bus_register_notifier(&scmi_bus_type, &info->bus_nb);
if (ret)
goto clear_txrx_setup;
+ ret = blocking_notifier_chain_register(&scmi_requested_devices_nh,
+ &info->dev_req_nb);
+ if (ret)
+ goto clear_bus_notifier;
+
+ ret = scmi_xfer_info_init(info);
+ if (ret)
+ goto clear_dev_req_notifier;
+
if (scmi_notification_init(handle))
dev_err(dev, "SCMI Notifications NOT available.\n");
@@ -2624,6 +2696,11 @@ static int scmi_probe(struct platform_device *pdev)
notification_exit:
scmi_notification_exit(&info->handle);
+clear_dev_req_notifier:
+ blocking_notifier_chain_unregister(&scmi_requested_devices_nh,
+ &info->dev_req_nb);
+clear_bus_notifier:
+ bus_unregister_notifier(&scmi_bus_type, &info->bus_nb);
clear_txrx_setup:
scmi_cleanup_txrx_channels(info);
return ret;
@@ -2652,6 +2729,10 @@ static int scmi_remove(struct platform_device *pdev)
of_node_put(child);
idr_destroy(&info->active_protocols);
+ blocking_notifier_chain_unregister(&scmi_requested_devices_nh,
+ &info->dev_req_nb);
+ bus_unregister_notifier(&scmi_bus_type, &info->bus_nb);
+
/* Safe to free channels since no more users */
scmi_cleanup_txrx_channels(info);
--
2.34.1
Move handle get/set helpers definitions into driver.c and invoke them
through the bus notifier helper.
Signed-off-by: Cristian Marussi <[email protected]>
---
drivers/firmware/arm_scmi/bus.c | 17 ---------------
drivers/firmware/arm_scmi/common.h | 4 ----
drivers/firmware/arm_scmi/driver.c | 35 +++++++++++++++++++++---------
3 files changed, 25 insertions(+), 31 deletions(-)
diff --git a/drivers/firmware/arm_scmi/bus.c b/drivers/firmware/arm_scmi/bus.c
index e63cc1194d43..61113def5a9a 100644
--- a/drivers/firmware/arm_scmi/bus.c
+++ b/drivers/firmware/arm_scmi/bus.c
@@ -344,27 +344,10 @@ scmi_device_create(struct device_node *np, struct device *parent, int protocol,
void scmi_device_destroy(struct scmi_device *scmi_dev)
{
kfree_const(scmi_dev->name);
- scmi_handle_put(scmi_dev->handle);
ida_free(&scmi_bus_id, scmi_dev->id);
device_unregister(&scmi_dev->dev);
}
-void scmi_device_link_add(struct device *consumer, struct device *supplier)
-{
- struct device_link *link;
-
- link = device_link_add(consumer, supplier, DL_FLAG_AUTOREMOVE_CONSUMER);
-
- WARN_ON(!link);
-}
-
-void scmi_set_handle(struct scmi_device *scmi_dev)
-{
- scmi_dev->handle = scmi_handle_get(&scmi_dev->dev);
- if (scmi_dev->handle)
- scmi_device_link_add(&scmi_dev->dev, scmi_dev->handle->dev);
-}
-
static int __scmi_devices_unregister(struct device *dev, void *data)
{
struct scmi_device *scmi_dev = to_scmi_dev(dev);
diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
index 0f411679df7e..07d34954c060 100644
--- a/drivers/firmware/arm_scmi/common.h
+++ b/drivers/firmware/arm_scmi/common.h
@@ -96,10 +96,6 @@ static inline void unpack_scmi_header(u32 msg_hdr, struct scmi_msg_hdr *hdr)
struct scmi_revision_info *
scmi_revision_area_get(const struct scmi_protocol_handle *ph);
-int scmi_handle_put(const struct scmi_handle *handle);
-void scmi_device_link_add(struct device *consumer, struct device *supplier);
-struct scmi_handle *scmi_handle_get(struct device *dev);
-void scmi_set_handle(struct scmi_device *scmi_dev);
void scmi_setup_protocol_implemented(const struct scmi_protocol_handle *ph,
u8 *prot_imp);
diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index 73f640e9448f..8591b2c740c6 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -1890,13 +1890,6 @@ static bool scmi_is_transport_atomic(const struct scmi_handle *handle,
return ret;
}
-static inline
-struct scmi_handle *scmi_handle_get_from_info_unlocked(struct scmi_info *info)
-{
- info->users++;
- return &info->handle;
-}
-
/**
* scmi_handle_get() - Get the SCMI handle for a device
*
@@ -1908,7 +1901,7 @@ struct scmi_handle *scmi_handle_get_from_info_unlocked(struct scmi_info *info)
*
* Return: pointer to handle if successful, NULL on error
*/
-struct scmi_handle *scmi_handle_get(struct device *dev)
+static struct scmi_handle *scmi_handle_get(struct device *dev)
{
struct list_head *p;
struct scmi_info *info;
@@ -1918,7 +1911,8 @@ struct scmi_handle *scmi_handle_get(struct device *dev)
list_for_each(p, &scmi_list) {
info = list_entry(p, struct scmi_info, node);
if (dev->parent == info->dev) {
- handle = scmi_handle_get_from_info_unlocked(info);
+ info->users++;
+ handle = &info->handle;
break;
}
}
@@ -1939,7 +1933,7 @@ struct scmi_handle *scmi_handle_get(struct device *dev)
* Return: 0 is successfully released
* if null was passed, it returns -EINVAL;
*/
-int scmi_handle_put(const struct scmi_handle *handle)
+static int scmi_handle_put(const struct scmi_handle *handle)
{
struct scmi_info *info;
@@ -1955,6 +1949,23 @@ int scmi_handle_put(const struct scmi_handle *handle)
return 0;
}
+static void scmi_device_link_add(struct device *consumer,
+ struct device *supplier)
+{
+ struct device_link *link;
+
+ link = device_link_add(consumer, supplier, DL_FLAG_AUTOREMOVE_CONSUMER);
+
+ WARN_ON(!link);
+}
+
+static void scmi_set_handle(struct scmi_device *scmi_dev)
+{
+ scmi_dev->handle = scmi_handle_get(&scmi_dev->dev);
+ if (scmi_dev->handle)
+ scmi_device_link_add(&scmi_dev->dev, scmi_dev->handle->dev);
+}
+
static int __scmi_xfer_info_init(struct scmi_info *sinfo,
struct scmi_xfers_info *info)
{
@@ -2234,8 +2245,12 @@ static int scmi_bus_notifier(struct notifier_block *nb,
switch (action) {
case BUS_NOTIFY_BIND_DRIVER:
+ /* setup handle now as the transport is ready */
+ scmi_set_handle(sdev);
break;
case BUS_NOTIFY_UNBOUND_DRIVER:
+ scmi_handle_put(sdev->handle);
+ sdev->handle = NULL;
break;
default:
return NOTIFY_DONE;
--
2.34.1
On Fri, 23 Dec 2022 at 00:22, Cristian Marussi <[email protected]> wrote:
>
> Hi,
>
> under some configurations the SCMI core stack, which is now initialized
> as a whole at the subsys_initcall level, can be dependent on some other
> Kernel subsystems (like TEE) when some SCMI transport backend like optee
> is used.
Thanks Cristian for the rework, but this doesn't seem to address
reluctance to carry forward the DT legacy (see [1]).
TLDR, it has led to misrepresentation of OP-TEE transport as follows:
First represented as a platform device via DT (compatible =
"linaro,scmi-optee";) and then
Migrated to being a TEE bus device (UUID: 0xa8cfe406, 0xd4f5,
0x4a2e, 0x9f, 0x8d, 0xa2, 0x5d, 0xc7, 0x54, 0xc0, 0x99)
Do we really need to have a platform device for every SCMI transport?
[1] https://lore.kernel.org/lkml/CAFA6WYPwku8d7EiJ8rF5pVh568oy+jXMXLdxSr6r476e0SD2nw@mail.gmail.com/
-Sumit
>
> This has been reported to lead to some awkward probe loop which, even
> though successful at the end, leaves a track of errors in the logs coming
> directly from the core Linux driver model facilities.
>
> In order to solve this issue and cleaning up a bit the SCMI stack startup
> sequence, this small series reviews and reworks the SCMI core stack
> initialization and probe logic.
>
> Basically the SCMI Bus is split into its own module (scmi-core.ko) which is
> initialized at subsys_initcall, while the SCMI core stack, including its
> various transport backends (like optee, mailbox, virtio, smc), is kept into
> a distinct module (scmi-module.ko) which get initialized at module_init.
>
> The SCMI driver users initlevel, instead, remains unchanged at module_init.
>
> No change is made to the Kconfig: the main ARM_SCMI_PROTOCOL option will
> now cause both modules to be built.
>
> This allows the other possibly needed subsystems to be up and running
> well before the core SCMI stack and its dependent transport backends, so
> solving the reported issue.
>
> Tested with SCMI transports mailbox/virtio and, in a previous draft, optee,
> in a number of different load/unload/bind/unbind combinations both as
> builtin and as LKMs.
>
> Applies on v6.1.
>
> Any feedback, testing welcome.
>
> Thanks,
> Cristian
>
> Cristian Marussi (9):
> firmware: arm_scmi: Simplify chan_available transport operation
> firmware: arm_scmi: Use dedicated devices to initialize channels
> firmware: arm_scmi: Move protocol registration helpers
> firmware: arm_scmi: Add common notifier helpers
> firmware: arm_scmi: Refactor protocol device creation
> firmware: arm_scmi: Move handle get/set helpers
> firmware: arm_scmi: Refactor device create/destroy helpers
> firmware: arm_scmi: Introduce a new lifecycle for protocol devices
> firmware: arm_scmi: Split bus and driver into distinct modules
>
> drivers/firmware/arm_scmi/Makefile | 8 +-
> drivers/firmware/arm_scmi/bus.c | 388 ++++++++++++-----
> drivers/firmware/arm_scmi/common.h | 25 +-
> drivers/firmware/arm_scmi/driver.c | 623 ++++++++++++++--------------
> drivers/firmware/arm_scmi/mailbox.c | 6 +-
> drivers/firmware/arm_scmi/optee.c | 6 +-
> drivers/firmware/arm_scmi/smc.c | 6 +-
> drivers/firmware/arm_scmi/virtio.c | 4 +-
> include/linux/scmi_protocol.h | 5 -
> 9 files changed, 622 insertions(+), 449 deletions(-)
>
> --
> 2.34.1
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Fri, Dec 23, 2022 at 11:06:29AM +0530, Sumit Garg wrote:
> On Fri, 23 Dec 2022 at 00:22, Cristian Marussi <[email protected]> wrote:
> >
> > Hi,
> >
> > under some configurations the SCMI core stack, which is now initialized
> > as a whole at the subsys_initcall level, can be dependent on some other
> > Kernel subsystems (like TEE) when some SCMI transport backend like optee
> > is used.
>
> Thanks Cristian for the rework, but this doesn't seem to address
> reluctance to carry forward the DT legacy (see [1]).
>
> TLDR, it has led to misrepresentation of OP-TEE transport as follows:
>
> First represented as a platform device via DT (compatible =
> "linaro,scmi-optee";) and then
> Migrated to being a TEE bus device (UUID: 0xa8cfe406, 0xd4f5,
> 0x4a2e, 0x9f, 0x8d, 0xa2, 0x5d, 0xc7, 0x54, 0xc0, 0x99)
>
> Do we really need to have a platform device for every SCMI transport?
>
> [1] https://lore.kernel.org/lkml/CAFA6WYPwku8d7EiJ8rF5pVh568oy+jXMXLdxSr6r476e0SD2nw@mail.gmail.com/
>
Hi Sumit,
thanks for the feedback first of all.
This series represents really a long standing point on my todo-list and it
is meant to start addressing/reviewing the whole SCMI stack init/probe
sequencing and transports setup while taking the chance/opportunity to
fix the issue reported by Ludvig.
The natural next step in my (and Sudeep) view would be to split out the SCMI
transports too into proper full fledged drivers, that can be probed by their
own susbsys eventually (when possible) and that will then register with the
SCMI core as available transports; so that we can avoid some of the cruft
when multiple backend subsystems are involved...
...it is just that I have NOT dug deep into this further evolution and I did
NOT want to do it in this series, but just starting laying out some basic rework
toward this direction while fixing Ludvig issue. (... also because there are a
lot of bit and pieces to get right in SCMI around protocols/modules and DT
parsing and I was trying not to break too many things at a time :P...)
Anyway, even in the perspective of the above possible evolution into full
fledged drivers, I doubt that we can get rid completely of the DT based
per-transport platform devices since their DT nodes can carry a bit of
transport related information (even for auto-discoverable transport I think)
...it will just be that such devices, bound to the compatibles, will be used
probably in a different way (also for backward compatibility with DT
bindings...)...indeed...such platform devices now DO carry some information
about the underlying transport to use BUT most of all they represent also
an SCMI platform instance, so that will not definitely go away completely,
it will just loose most of the transport related functionalities
..but... as said...I have not dived too much into this further evolution so
I maybe wrong here on the details... anyway the plan going further, as spoken
also with Sudeep offline, could/should be that depicted above.
Not sure if this answers all of your questions but I'll keep you posted
on this series and next evolutions...
Thanks,
Cristian
On Fri, 23 Dec 2022 at 17:07, Cristian Marussi <[email protected]> wrote:
>
> On Fri, Dec 23, 2022 at 11:06:29AM +0530, Sumit Garg wrote:
> > On Fri, 23 Dec 2022 at 00:22, Cristian Marussi <[email protected]> wrote:
> > >
> > > Hi,
> > >
> > > under some configurations the SCMI core stack, which is now initialized
> > > as a whole at the subsys_initcall level, can be dependent on some other
> > > Kernel subsystems (like TEE) when some SCMI transport backend like optee
> > > is used.
> >
> > Thanks Cristian for the rework, but this doesn't seem to address
> > reluctance to carry forward the DT legacy (see [1]).
> >
> > TLDR, it has led to misrepresentation of OP-TEE transport as follows:
> >
> > First represented as a platform device via DT (compatible =
> > "linaro,scmi-optee";) and then
> > Migrated to being a TEE bus device (UUID: 0xa8cfe406, 0xd4f5,
> > 0x4a2e, 0x9f, 0x8d, 0xa2, 0x5d, 0xc7, 0x54, 0xc0, 0x99)
> >
> > Do we really need to have a platform device for every SCMI transport?
> >
> > [1] https://lore.kernel.org/lkml/CAFA6WYPwku8d7EiJ8rF5pVh568oy+jXMXLdxSr6r476e0SD2nw@mail.gmail.com/
> >
>
> Hi Sumit,
>
> thanks for the feedback first of all.
>
> This series represents really a long standing point on my todo-list and it
> is meant to start addressing/reviewing the whole SCMI stack init/probe
> sequencing and transports setup while taking the chance/opportunity to
> fix the issue reported by Ludvig.
>
> The natural next step in my (and Sudeep) view would be to split out the SCMI
> transports too into proper full fledged drivers, that can be probed by their
> own susbsys eventually (when possible) and that will then register with the
> SCMI core as available transports; so that we can avoid some of the cruft
> when multiple backend subsystems are involved...
>
> ...it is just that I have NOT dug deep into this further evolution and I did
> NOT want to do it in this series, but just starting laying out some basic rework
> toward this direction while fixing Ludvig issue. (... also because there are a
> lot of bit and pieces to get right in SCMI around protocols/modules and DT
> parsing and I was trying not to break too many things at a time :P...)
>
> Anyway, even in the perspective of the above possible evolution into full
> fledged drivers, I doubt that we can get rid completely of the DT based
> per-transport platform devices since their DT nodes can carry a bit of
> transport related information (even for auto-discoverable transport I think)
>
> ...it will just be that such devices, bound to the compatibles, will be used
> probably in a different way (also for backward compatibility with DT
> bindings...)...indeed...such platform devices now DO carry some information
> about the underlying transport to use BUT most of all they represent also
> an SCMI platform instance, so that will not definitely go away completely,
> it will just loose most of the transport related functionalities
>
> ..but... as said...I have not dived too much into this further evolution so
> I maybe wrong here on the details... anyway the plan going further, as spoken
> also with Sudeep offline, could/should be that depicted above.
>
> Not sure if this answers all of your questions but I'll keep you posted
> on this series and next evolutions...
Thanks for the detailed clarification. I don't have the deep insights
regarding how SCMI subsystem works but generally dealing with a device
being probed on multiple buses is prone to system integration problems
such as:
- Is the device present on the platform bus (in DT)? Is the device
present on a discoverable bus (eg. TEE bus)?
- Do both buses represent synchronised device views? IOW, version skew problems.
I hope we should be able to address those with the evolution you are planning.
-Sumit
>
> Thanks,
> Cristian
On Thu, 22 Dec 2022 18:50:40 +0000, Cristian Marussi wrote:
> under some configurations the SCMI core stack, which is now initialized
> as a whole at the subsys_initcall level, can be dependent on some other
> Kernel subsystems (like TEE) when some SCMI transport backend like optee
> is used.
>
> This has been reported to lead to some awkward probe loop which, even
> though successful at the end, leaves a track of errors in the logs coming
> directly from the core Linux driver model facilities.
>
> [...]
Applied to sudeep.holla/linux (for-next/scmi), thanks!
[1/9] firmware: arm_scmi: Simplify chan_available transport operation
https://git.kernel.org/sudeep.holla/c/7a75b7afd8ff
[2/9] firmware: arm_scmi: Use dedicated devices to initialize channels
https://git.kernel.org/sudeep.holla/c/05a2801d8b90
[3/9] firmware: arm_scmi: Move protocol registration helpers
https://git.kernel.org/sudeep.holla/c/9115c20ac1ee
[4/9] firmware: arm_scmi: Add common notifier helpers
https://git.kernel.org/sudeep.holla/c/53b8c25df708
[5/9] firmware: arm_scmi: Refactor protocol device creation
https://git.kernel.org/sudeep.holla/c/d3cd7c525fd2
[6/9] firmware: arm_scmi: Move handle get/set helpers
https://git.kernel.org/sudeep.holla/c/971fc0665f13
[7/9] firmware: arm_scmi: Refactor device create/destroy helpers
https://git.kernel.org/sudeep.holla/c/2c3e674465e7
[8/9] firmware: arm_scmi: Introduce a new lifecycle for protocol devices
https://git.kernel.org/sudeep.holla/c/ee5dcedaf72d
[9/9] firmware: arm_scmi: Split bus and driver into distinct modules
https://git.kernel.org/sudeep.holla/c/37b5be828032
--
Regards,
Sudeep