Re: [PATCH v2 4/4] firmware: arm_scmi: add optee transport

[Etienne Carriere]

Hello Christian,

On Thu, 27 May 2021 at 17:11, Cristian Marussi <cristian.marussi@xxxxxxx> wrote:
>
> Hi Etienne,
>
> some remarks below.
>
> On Fri, May 21, 2021 at 03:40:54PM +0200, Etienne Carriere wrote:
> > Add a new transport channel to the SCMI firmware interface driver for
> > SCMI message exchange based on OP-TEE transport channel that leverage
> > OP-TEE secure threaded context for processing of SCMI messages.
> >
> > The current proposal uses a statically defined physical memory area
> > to be used as shared memory SCMI endpoints. The location is extracted
> > from the FDT upon property "shmem".
> >
> > Entry in OP-TEE threaded context is realized by invoking a service
> > PTA (Pseudo TA) in OP-TEE OS. Each invocation carries a agent
> > numerical identifier for which a message is pending in the shared
> > memory. The OP-TEE service provides means to get the agent identifier
> > value for a given shared memory location.
> >
> > OPTEE transport driver depends on CONFIG_OPTEE and probes from SCMI
> > compatible identifier "linaro,scmi-optee".
> >
> > Signed-off-by: Etienne Carriere <etienne.carriere@xxxxxxxxxx>
> > ---
> > No change since v1
> > ---
> >  drivers/firmware/Kconfig                  |   2 +-
> >  drivers/firmware/arm_scmi/Makefile        |   1 +
> >  drivers/firmware/arm_scmi/common.h        |   3 +
> >  drivers/firmware/arm_scmi/driver.c        |   3 +
> >  drivers/firmware/arm_scmi/optee_service.c | 529 ++++++++++++++++++++++
> >  5 files changed, 537 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/firmware/arm_scmi/optee_service.c
> >
> > diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> > index a9c613c32282..7413e89e5628 100644
> > --- a/drivers/firmware/Kconfig
> > +++ b/drivers/firmware/Kconfig
> > @@ -9,7 +9,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
> > +     depends on MAILBOX || HAVE_ARM_SMCCC_DISCOVERY || OPTEE
> >       help
> >         ARM System Control and Management Interface (SCMI) protocol is a
> >         set of operating system-independent software interfaces that are
> > diff --git a/drivers/firmware/arm_scmi/Makefile b/drivers/firmware/arm_scmi/Makefile
> > index 6a2ef63306d6..0e09d302a82e 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-y = shmem.o
> >  scmi-transport-$(CONFIG_MAILBOX) += mailbox.o
> >  scmi-transport-$(CONFIG_HAVE_ARM_SMCCC_DISCOVERY) += smc.o
> > +scmi-transport-$(CONFIG_OPTEE) += optee_service.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)
>
> I would say, now that we have multiple transport to make these
> configurable on their own (CONFIG_OPTEE_SCMI ?), so that you can avoid
> having SCMI optee transport built into the SCMI module even though you
> don't need it but you have optee support enabled for other reasons.
>
> Same goes for other transports, but for this series it's probably
> irrelevant, so we could leave it this way but I will make all transports
> fully configurable in the future while reworking virtio-scmi since that is
> already that way in the original Peter patch and it seemed sensible.
>
> > diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
> > index 8685619d38f9..a4db1a1a98e2 100644
> > --- a/drivers/firmware/arm_scmi/common.h
> > +++ b/drivers/firmware/arm_scmi/common.h
> > @@ -334,6 +334,9 @@ extern const struct scmi_desc scmi_mailbox_desc;
> >  #ifdef CONFIG_HAVE_ARM_SMCCC_DISCOVERY
> >  extern const struct scmi_desc scmi_smc_desc;
> >  #endif
> > +#ifdef CONFIG_OPTEE
> > +extern const struct scmi_desc scmi_optee_desc;
> > +#endif
> >
> >  void scmi_rx_callback(struct scmi_chan_info *cinfo, u32 msg_hdr);
> >  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 66eb3f0e5daf..501a9ec28791 100644
> > --- a/drivers/firmware/arm_scmi/driver.c
> > +++ b/drivers/firmware/arm_scmi/driver.c
> > @@ -1570,6 +1570,9 @@ static const struct of_device_id scmi_of_match[] = {
> >       { .compatible = "arm,scmi", .data = &scmi_mailbox_desc },
> >  #ifdef CONFIG_HAVE_ARM_SMCCC_DISCOVERY
> >       { .compatible = "arm,scmi-smc", .data = &scmi_smc_desc},
> > +#endif
> > +#ifdef CONFIG_OPTEE
> > +     { .compatible = "linaro,scmi-optee", .data = &scmi_optee_desc },
> >  #endif
> >       { /* Sentinel */ },
> >  };
> > diff --git a/drivers/firmware/arm_scmi/optee_service.c b/drivers/firmware/arm_scmi/optee_service.c
> > new file mode 100644
> > index 000000000000..de3432a4c179
> > --- /dev/null
> > +++ b/drivers/firmware/arm_scmi/optee_service.c
> > @@ -0,0 +1,529 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2019-2021 Linaro Limited
> > + */
> > +
> > +#include <uapi/linux/tee.h>
> > +#include <linux/freezer.h>
> > +#include <linux/io.h>
> > +#include <linux/ioport.h>
> > +#include <linux/kernel.h>
> > +#include <linux/kthread.h>
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/of.h>
> > +#include <linux/of_address.h>
> > +#include <linux/slab.h>
> > +#include <linux/tee_drv.h>
> > +#include <linux/uuid.h>
> > +
> > +#include "common.h"
> > +
> > +#define DRIVER_NAME "optee-scmi-agent"
> > +
> > +/*
> > + * PTA_SCMI_CMD_CAPABILITIES - Get channel capabilities
> > + *
> > + * [out]    value[0].a: Capability bit mask (PTA_SCMI_CAPS_*)
> > + * [out]    value[0].b: Extended capabilities or 0
> > + */
> > +#define PTA_SCMI_CMD_CAPABILITIES    0
> > +
> > +/*
> > + * PTA_SCMI_CMD_PROCESS_SMT_CHANNEL - Process SCMI message in SMT buffer
> > + *
> > + * [in]     value[0].a: Channel handle
> > + *
> > + * Shared memory used for SCMI message/response exhange is expected
> > + * already identified and bound to channel handle in both SCMI agent
> > + * and SCMI server (OP-TEE) parts.
> > + * The memory uses SMT header to carry SCMI meta-data (protocol ID and
> > + * protocol message ID).
> > + */
> > +#define PTA_SCMI_CMD_PROCESS_SMT_CHANNEL     1
> > +
> > +/*
> > + * PTA_SCMI_CMD_PROCESS_SMT_CHANNEL_MESSAGE - Process SMT/SCMI message
> > + *
> > + * [in]     value[0].a: Channel handle
> > + * [in/out] memref[1]: Message/response buffer (SMT and SCMI payload)
> > + *
> > + * Shared memory used for SCMI message/response is a SMT buffer
> > + * referenced by param[1]. It shall be 128 bytes large to fit response
> > + * payload whatever message playload size.
> > + * The memory uses SMT header to carry SCMI meta-data (protocol ID and
> > + * protocol message ID).
> > + */
> > +#define PTA_SCMI_CMD_PROCESS_SMT_CHANNEL_MESSAGE     2
> > +
> > +/*
> > + * PTA_SCMI_CMD_GET_CHANNEL_HANDLE - Get handle for an SCMI channel
> > + *
> > + * Get a handle for the SCMI channel. This handle value is to be passed
> > + * as argument to some commands as PTA_SCMI_CMD_PROCESS_*.
> > + *
> > + * [in]     value[0].a: Channel identifier or 0 if no assigned ID
> > + * [in]     value[0].b: Requested capabilities mask (PTA_SCMI_CAPS_*)
> > + * [out]    value[0].a: Returned channel handle
> > + */
> > +#define PTA_SCMI_CMD_GET_CHANNEL_HANDLE              3
> > +
> > +/*
> > + * Channel capabilities
> > + */
> > +
> > +/* Channel supports shared memory using the SMT header protocol */
> > +#define PTA_SCMI_CAPS_SMT_HEADER                     BIT(0)
> > +
> > +struct optee_scmi_channel {
> > +     /* Channel handle retrieved from OP-TEE */
> > +     u32 channel_hdl;
> > +     /* Channel entry protection */
> > +     struct mutex mu;
> > +     /* Channel private data */
> > +     u32 tee_session;
> > +     struct optee_scmi_agent *agent;
> > +     struct scmi_chan_info *cinfo;
> > +     struct scmi_shared_mem __iomem *shmem;
> > +     /* Channel capabilities */
> > +     u32 caps;
> > +     /* Reference in agent's channel list */
> > +     struct list_head link;
> > +};
> > +
> > +/**
> > + * struct optee_scmi_agent - OP-TEE transport private data
> > + */
> > +struct optee_scmi_agent {
> > +     /* TEE context the agent operates with */
> > +     struct device *dev;
> > +     struct tee_context *tee_ctx;
> > +     /* Supported channel capabilities (PTA_SCMI_CAPS_*) */
> > +     u32 caps;
> > +     /* List all created channels */
> > +     struct list_head channel_list;
> > +};
> > +
> > +/* There is only 1 SCMI PTA to connect to */
> > +static struct optee_scmi_agent *agent_private;
> > +
> > +static struct list_head optee_agent_list = LIST_HEAD_INIT(optee_agent_list);
> > +static DEFINE_MUTEX(list_mutex);
> > +
> > +static int open_session(struct optee_scmi_agent *agent, u32 *tee_session)
> > +{
> > +     struct device *dev = agent->dev;
> > +     struct tee_client_device *scmi_pta = to_tee_client_device(dev);
> > +     struct tee_ioctl_open_session_arg sess_arg;
> > +     int ret;
> > +
> > +     memset(&sess_arg, 0, sizeof(sess_arg));
> > +
> > +     /* Open a session toward SCMI PTA with REE_KERNEL identity */
> > +     memcpy(sess_arg.uuid, scmi_pta->id.uuid.b, TEE_IOCTL_UUID_LEN);
> > +     sess_arg.clnt_login = TEE_IOCTL_LOGIN_REE_KERNEL;
> > +
> > +     ret = tee_client_open_session(agent->tee_ctx, &sess_arg, NULL);
> > +     if ((ret < 0) || (sess_arg.ret != 0)) {
> > +             dev_err(dev, "tee_client_open_session failed, err: %x\n",
> > +                     sess_arg.ret);
> > +             return -EINVAL;
> > +     }
> > +
> > +     *tee_session = sess_arg.session;
> > +
> > +     return 0;
> > +}
> > +
> > +static void close_session(struct optee_scmi_agent *agent, u32 tee_session)
> > +{
> > +     tee_client_close_session(agent->tee_ctx, tee_session);
> > +}
> > +
> > +static int get_capabilities(struct optee_scmi_agent *agent)
> > +{
> > +     int ret;
> > +     struct tee_ioctl_invoke_arg inv_arg;
> > +     struct tee_param param[1];
> > +     u32 tee_session;
> > +
> > +     ret = open_session(agent, &tee_session);
> > +     if (ret)
> > +             return ret;
> > +
> > +     memset(&inv_arg, 0, sizeof(inv_arg));
> > +     memset(&param, 0, sizeof(param));
> > +
> > +     inv_arg.func = PTA_SCMI_CMD_CAPABILITIES;
> > +     inv_arg.session = tee_session;
> > +     inv_arg.num_params = 1;
> > +
> > +     param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT;
> > +
> > +     ret = tee_client_invoke_func(agent->tee_ctx, &inv_arg, param);
> > +
> > +     close_session(agent, tee_session);
> > +
> > +     if ((ret < 0) || (inv_arg.ret != 0)) {
> > +             dev_err(agent->dev, "Can't get capabilities: %d / %#x\n",
> > +                     ret, inv_arg.ret);
> > +
> > +             return -EOPNOTSUPP;
> > +     }
> > +
> > +     agent->caps = param[0].u.value.a;
> > +
> > +     if (!(agent->caps & PTA_SCMI_CAPS_SMT_HEADER)) {
> > +             dev_err(agent->dev, "OP-TEE SCMI PTA doesn't support SMT\n");
> > +
> > +             return -ENODEV;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static int get_channel(struct optee_scmi_channel *channel)
> > +{
> > +     struct device *dev = channel->agent->dev;
> > +     int ret;
> > +     struct tee_ioctl_invoke_arg inv_arg;
> > +     struct tee_param param[1];
> > +     unsigned int caps;
> > +
> > +     caps = PTA_SCMI_CAPS_SMT_HEADER;
> > +
> > +     memset(&inv_arg, 0, sizeof(inv_arg));
> > +     memset(&param, 0, sizeof(param));
> > +
> > +     inv_arg.func = PTA_SCMI_CMD_GET_CHANNEL_HANDLE;
> > +     inv_arg.session = channel->tee_session;
> > +     inv_arg.num_params = 1;
> > +
> > +     param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INOUT;
> > +     param[0].u.value.a = 0;
> > +     param[0].u.value.b = caps;
> > +
> > +     ret = tee_client_invoke_func(channel->agent->tee_ctx, &inv_arg, param);
> > +
> > +     if (ret || inv_arg.ret) {
> > +             dev_err(dev, "Can't get channel with caps %#x: ret=%d, tee-res=%#x\n",
> > +                     caps, ret, inv_arg.ret);
> > +
> > +             return -EOPNOTSUPP;
> > +     }
> > +
> > +     /* Only channel handler is used, can discard old channel ID value */
> > +     channel->channel_hdl = param[0].u.value.a;
> > +     channel->caps = caps;
> > +
> > +     return 0;
> > +}
> > +
> > +/* Invocation of the PTA through a regular command invoke */
> > +static int invoke_process_smt_channel(struct optee_scmi_channel *channel)
> > +{
> > +     int ret;
> > +     struct tee_ioctl_invoke_arg inv_arg;
> > +     struct tee_param param[1];
> > +
> > +     if (!(channel->caps & PTA_SCMI_CAPS_SMT_HEADER))
> > +             return -EINVAL;
> > +
> > +     memset(&inv_arg, 0, sizeof(inv_arg));
> > +     memset(&param, 0, sizeof(param));
> > +
> > +     inv_arg.func = PTA_SCMI_CMD_PROCESS_SMT_CHANNEL;
> > +     inv_arg.session = channel->tee_session;
> > +     inv_arg.num_params = 1;
> > +
> > +     param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT;
> > +     param[0].u.value.a = channel->channel_hdl;
> > +
> > +     ret = tee_client_invoke_func(channel->agent->tee_ctx, &inv_arg, param);
> > +     if (ret < 0 || inv_arg.ret) {
> > +             dev_err(channel->agent->dev, "Failed on channel handle %u: 0x%x\n",
> > +                     channel->channel_hdl, inv_arg.ret);
> > +             return -EIO;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static bool optee_chan_available(struct device *dev, int idx)
> > +{
> > +     struct device_node *np = of_parse_phandle(dev->of_node, "shmem", 0);
> > +
> > +     if (!np)
> > +             return false;
> > +
> > +     of_node_put(np);
> > +
> > +     return true;
> > +}
> > +
> > +static int optee_chan_setup_shmem(struct scmi_chan_info *cinfo, bool tx,
> > +                               struct optee_scmi_channel *channel)
> > +{
> > +     struct device *cdev = cinfo->dev;
> > +     struct device_node *np;
> > +     resource_size_t size;
> > +     struct resource res;
> > +     int ret;
> > +
> > +     np = of_parse_phandle(cdev->of_node, "shmem", 0);
> > +     ret = of_address_to_resource(np, 0, &res);
> > +     of_node_put(np);
> > +     if (ret) {
> > +             dev_err(cdev, "failed to get SCMI Tx shared memory\n");
> > +             return ret;
> > +     }
> > +
> > +     size = resource_size(&res);
> > +
> > +     channel->shmem = devm_ioremap(cdev, res.start, size);
> > +     if (!channel->shmem) {
> > +             dev_err(cdev, "failed to ioremap SCMI Tx shared memory\n");
> > +             return -EADDRNOTAVAIL;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static void optee_clear_channel(struct scmi_chan_info *cinfo)
> > +{
> > +     struct optee_scmi_channel *channel = cinfo->transport_info;
> > +
> > +     shmem_clear_channel(channel->shmem);
> > +}
> > +
> > +static int optee_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
> > +                         bool tx)
> > +{
> > +     struct device *cdev = cinfo->dev;
>
> cdev is unused
>
> > +     struct optee_scmi_channel *channel;
> > +     int ret, idx = tx ? 0 : 1;
>
> idx is unused
>

Right, i'll remove both.

> > +
> > +     /* Shall wait for OP-TEE driver to be up and ready */
> > +     if (!agent_private || !agent_private->tee_ctx)
> > +             return -EPROBE_DEFER;
> > +
> > +     channel = devm_kzalloc(dev, sizeof(*channel), GFP_KERNEL);
> > +     if (!channel)
> > +             return -ENOMEM;
> > +
> > +     ret = optee_chan_setup_shmem(cinfo, tx, channel);
> > +     if (ret)
> > +             return ret;
> > +
> > +     cinfo->transport_info = channel;
> > +     channel->cinfo = cinfo;
> > +     channel->agent = agent_private;
> > +
> > +     ret = open_session(channel->agent, &channel->tee_session);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = get_channel(channel);
> > +     if (ret)
> > +             goto err;
> > +
> > +     mutex_init(&channel->mu);
> > +
> > +     mutex_lock(&list_mutex);
> > +     list_add(&channel->link, &channel->agent->channel_list);
> > +     mutex_unlock(&list_mutex);
> > +
> > +     return 0;
> > +
> > +err:
> > +     close_session(channel->agent, channel->tee_session);
> > +     channel->tee_session = 0;
> > +
> > +     return ret;
> > +}
> > +
> > +static int optee_chan_free(int id, void *p, void *data)
> > +{
> > +     struct scmi_chan_info *cinfo = p;
> > +     struct optee_scmi_channel *channel = cinfo->transport_info;
> > +
> > +     mutex_lock(&list_mutex);
> > +     list_del(&channel->link);
> > +     mutex_unlock(&list_mutex);
> > +
> > +     cinfo->transport_info = NULL;
> > +     channel->cinfo = NULL;
> > +
> > +     scmi_free_channel(cinfo, data, id);
> > +
> > +     return 0;
> > +}
> > +
> > +static struct scmi_shared_mem *get_channel_shm(struct optee_scmi_channel *chan,
> > +                                            struct scmi_xfer *xfer)
> > +{
> > +     if (!chan)
> > +             return NULL;
> > +
> > +     return chan->shmem;
> > +}
> > +
> > +static int optee_send_message(struct scmi_chan_info *cinfo,
> > +                           struct scmi_xfer *xfer)
> > +{
> > +     struct optee_scmi_channel *channel = cinfo->transport_info;
> > +     struct scmi_shared_mem *shmem;
> > +     int ret = 0;
> > +
> > +     if (!channel && !channel->agent && !channel->agent->tee_ctx)
> > +             return -ENODEV;
> > +
> > +     shmem = get_channel_shm(channel, xfer);
> > +
> > +     mutex_lock(&channel->mu);
> > +     shmem_tx_prepare(shmem, xfer);
> > +
> > +     ret = invoke_process_smt_channel(channel);
>
> Shouldn't we check ret before calling scmi_rx_callback ?
>
> > +
> > +     scmi_rx_callback(cinfo, shmem_read_header(shmem));
> > +     mutex_unlock(&channel->mu);
> > +
> > +     return ret;
> > +}
> > +
> > +static void optee_fetch_response(struct scmi_chan_info *cinfo,
> > +                              struct scmi_xfer *xfer)
> > +{
> > +     struct optee_scmi_channel *channel = cinfo->transport_info;
> > +     struct scmi_shared_mem *shmem = get_channel_shm(channel, xfer);
> > +
> > +     shmem_fetch_response(shmem, xfer);
> > +}
> > +
> > +static bool optee_poll_done(struct scmi_chan_info *cinfo,
> > +                         struct scmi_xfer *xfer)
> > +{
> > +     struct optee_scmi_channel *channel = cinfo->transport_info;
> > +     struct scmi_shared_mem *shmem = get_channel_shm(channel, xfer);
> > +
> > +     return shmem_poll_done(shmem, xfer);
> > +}
> > +
> > +static struct scmi_transport_ops scmi_optee_ops = {
> > +     .chan_available = optee_chan_available,
> > +     .chan_setup = optee_chan_setup,
> > +     .chan_free = optee_chan_free,
> > +     .send_message = optee_send_message,
> > +     .fetch_response = optee_fetch_response,
> > +     .clear_channel = optee_clear_channel,
> > +     .poll_done = optee_poll_done,
> > +};
> > +
> > +const struct scmi_desc scmi_optee_desc = {
> > +     .ops = &scmi_optee_ops,
> > +     .max_rx_timeout_ms = 30, /* We may increase this if required */
> > +     .max_msg = 8,
> > +     .max_msg_size = 128,
> > +};
> > +
> > +static int optee_ctx_match(struct tee_ioctl_version_data *ver, const void *data)
> > +{
> > +     return ver->impl_id == TEE_IMPL_ID_OPTEE;
> > +}
> > +
> > +static int optee_scmi_probe(struct device *dev)
> > +{
> > +     struct optee_scmi_agent *agent;
> > +     struct tee_context *tee_ctx;
> > +     int ret;
> > +
> > +     tee_ctx = tee_client_open_context(NULL, optee_ctx_match, NULL, NULL);
> > +     if (IS_ERR(tee_ctx))
> > +             return -ENODEV;
> > +
> > +     agent = devm_kzalloc(dev, sizeof(*agent), GFP_KERNEL);
> > +     if (!agent) {
> > +             ret = -ENOMEM;
> > +             goto err;
> > +     }
> > +
> > +     agent->dev = dev;
> > +     agent->tee_ctx = tee_ctx;
> > +
> > +     ret = get_capabilities(agent);
> > +     if (ret)
> > +             goto err;
> > +
> > +     /* We currently support only 1 OP-TEE device */
> > +     if (WARN_ON(agent_private)) {
> > +             ret = -EINVAL;
> > +             goto err;
> > +     }
> > +     agent_private = agent;
> > +
> > +     INIT_LIST_HEAD(&agent->channel_list);
> > +
> > +     dev_dbg(dev, "OP-TEE SCMI channel probed\n");
> > +
> > +     return 0;
> > +
> > +err:
> > +     tee_client_close_context(tee_ctx);
> > +     return ret;
> > +}
> > +
> > +static int optee_scmi_remove(struct device *dev)
> > +{
> > +     struct optee_scmi_channel *channel;
> > +     struct list_head *elt, *n;
> > +
> > +     mutex_lock(&list_mutex);
> > +     list_for_each_safe(elt, n, &agent_private->channel_list) {
> > +             channel = list_entry(elt, struct optee_scmi_channel, link);
> > +             list_del(&channel->link);
> > +     }
> > +     mutex_unlock(&list_mutex);
> > +
> > +     tee_client_close_context(agent_private->tee_ctx);
> > +
> > +     agent_private = NULL;
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct tee_client_device_id optee_scmi_id_table[] = {
> > +     {
> > +             UUID_INIT(0xa8cfe406, 0xd4f5, 0x4a2e,
> > +                       0x9f, 0x8d, 0xa2, 0x5d, 0xc7, 0x54, 0xc0, 0x99)
> > +     },
> > +     { }
> > +};
> > +
> > +MODULE_DEVICE_TABLE(tee, optee_scmi_id_table);
> > +
> > +static struct tee_client_driver optee_scmi_driver = {
> > +     .id_table       = optee_scmi_id_table,
> > +     .driver         = {
> > +             .name = DRIVER_NAME,
> > +             .bus = &tee_bus_type,
> > +             .probe = optee_scmi_probe,
> > +             .remove = optee_scmi_remove,
> > +     },
> > +};
> > +
> > +static int __init optee_scmi_init(void)
> > +{
> > +     return driver_register(&optee_scmi_driver.driver);
> > +}
> > +
> > +static void __exit optee_scmi_exit(void)
> > +{
> > +     driver_unregister(&optee_scmi_driver.driver);
> > +}
> > +
> > +module_init(optee_scmi_init);
> > +module_exit(optee_scmi_exit);
> > +
>
> This cannot compile is the full SCMI statck ARM_SCMI_PROTOCOL is configured as =m
>
> /opt/toolchains/gcc-arm-8.3-2019.03-x86_64-aarch64-linux-gnu/bin/aarch64-linux-gnu-ld:
> drivers/firmware/arm_scmi/optee_service.o: in function
> `optee_scmi_init':
> /home/crimar01/ARM/dev/src/pdsw/linux/drivers/firmware/arm_scmi/optee_service.c:515:
> multiple definition of `init_module';
> drivers/firmware/arm_scmi/driver.o:/home/crimar01/ARM/dev/src/pdsw/linux/drivers/firmware/arm_scmi/driver.c:1593:
> first defined here
> /opt/toolchains/gcc-arm-8.3-2019.03-x86_64-aarch64-linux-gnu/bin/aarch64-linux-gnu-ld:
> drivers/firmware/arm_scmi/optee_service.o: in function
> `optee_scmi_exit':
> /home/crimar01/ARM/dev/src/pdsw/linux/drivers/firmware/arm_scmi/optee_service.c:520:
> multiple definition of `cleanup_module';
> drivers/firmware/arm_scmi/driver.o:/home/crimar01/ARM/dev/src/pdsw/linux/drivers/firmware/arm_scmi/driver.c:1611:
> first defined here
> /home/crimar01/ARM/dev/src/pdsw/linux/scripts/Makefile.build:449: recipe
> for target 'drivers/firmware/arm_scmi/scmi-module.o' failed
> make[4]: *** [drivers/firmware/arm_scmi/scmi-module.o] Error 1
>
>
> Indeed it was the same issue we faced in the virtio-scmi series, and it
> derives from the fact that SCMI transports 'driver' are really NOT
> standalone drivers but only extension of the main SCMI module, so you
> cannot have them initialize their stuff using usual kernel module_ machinery.
>
> In order to address this, and avoid a hell of ifdeffery probably,
> in the context of virtio-scmi, I added a couple of transport's
> optionalily provided init/deinit functions so that a transport can provide
> some specific init code and be assured they are called at SCMI stack init,
> so definitely even before the SCMI stack is probed and the selected
> transport used.
>
> This is the patch from the virtio-scmi series:
>
> https://lore.kernel.org/linux-arm-kernel/20210511002040.802226-3-peter.hilber@xxxxxxxxxxxxxxx/
>
> which in that context is used like:
>
> +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 = {
> +       .init = virtio_scmi_init,
> +       .exit = virtio_scmi_exit,
> +       .ops = &scmi_virtio_ops,
>
> I'll cleanup further that init/deinit patch and move it out into that bunch
> of SCMI core changes that I'm making (in a separate series) to aid in virtio-scmi
> devel.
> In the meantime for your testing the above lore patch should work fine.

Thanks for the details, i'll upgrade to the series.

Cheers,
Etienne

>
> Thanks,
> Cristian
>
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_AUTHOR("Etienne Carriere <etienne.carriere@xxxxxxxxxx>");
> > +MODULE_DESCRIPTION("OP-TEE SCMI transport driver");
> > --
> > 2.17.1
> >