Re: [PATCH v5 2/2] firmware: arm_scmi: Add optee transport
From: Etienne Carriere
Date: Tue Oct 26 2021 - 03:38:25 EST
Hi Cristian,
On Mon, 25 Oct 2021 at 17:45, Cristian Marussi <cristian.marussi@xxxxxxx> wrote:
>
> On Thu, Oct 21, 2021 at 05:07:17PM +0200, Etienne Carriere wrote:
> > Add a new transport channel to the SCMI firmware interface driver for
> > SCMI message exchange based on optee transport channel. The optee
> > transport is realized by connecting and invoking OP-TEE SCMI service
> > interface PTA.
> >
>
> Hi Etienne,
>
> I buld-tested and verified previuos remarks and it looks good to me.
>
> A few more new small remarks down below, though, that I missed previously.
>
> > Optee transport support (CONFIG_ARM_SCMI_TRANSPORT_OPTEE) is default
> > enabled when optee driver (CFG_OPTEE) is enabled. Effective optee
> nit: CONFIG_OPTEE ?
True.
Thanks you also to Jérôme who pointed that to me but I forgot to fix :(
>
> > transport is setup upon OP-TEE SCMI service discovery at optee
> > device initialization. For this SCMI UUID is registered to the optee
> > bus for probing. This is done from the link_supplier operator of the
> > SCMI optee transport.
> >
> > The optee transport can use a statically defined shared memory in
> > which case SCMI device tree node defines it using an "arm,scmi-shmem"
> > compatible phandle through property shmem. Alternatively, optee transport
> > allocates the shared memory buffer from the optee driver when no shmem
> > property is defined.
> >
> > The protocol used to exchange SCMI message over that shared memory is
> > negotiated between optee transport driver and the OP-TEE service through
> > capabilities exchange.
> >
> > OP-TEE SCMI service is integrated in OP-TEE since its release tag 3.13.0.
> > The service interface is published in [1].
> >
> > Link: [1] https://github.com/OP-TEE/optee_os/blob/3.13.0/lib/libutee/include/pta_scmi_client.h
> > Cc: Cristian Marussi <cristian.marussi@xxxxxxx>
> > Cc: Sudeep Holla <sudeep.holla@xxxxxxx>
> > Signed-off-by: Etienne Carriere <etienne.carriere@xxxxxxxxxx>
> > ---
> > Changes since v4:
> > - Fix commit log that was not updated to v4 changes.
> > - Operator scmi_optee_chan_setup() don't need the defer probe
> > operation, it's already done from scmi_optee_link_supplier().
> >
> > Changes since v3:
> > - Fix use of configuration switches when CONFIG_OPTEE and
> > CONFIG_ARM_SCMI_PROTOCOL are enabled/modules/disabled.
> > Mimics scmi virtio integration.
> > - Implement link_supplier operator for the scmi_optee transport
> > to possibly defer probing when optee bus has not yet enumerated
> > the SCMI OP-TEE service. The function ensures scmi_optee registers
> > to optee bus enumeration when probe is deferred.
> > - Add memory barriers to protect global optee service reference
> > when it's updated at transport initialization and removal.
> > - Replace enum pta_scmi_caps with macro definitions as enumerated
> > types do not really match bit flags definitions. The capabilities
> > data is now of type u32.
> > - Use scmi_optee_ prefix for scmi transport operator handles
> > and few other resources.
> > - Fix typo: s/optee_smci_pta_cmd/optee_scmi_pta_cmd/
> > - Remove useless DRIVER_NAME.
> > - Minor reordering in struct optee_channel.
> > - Removed some useless empty lines.
> >
> > Changes since v2:
> > - Rebase on for-next/scmi, based on Linux v5.15-rc1.
> > - Implement support for dynamic and static shared memory.
> > - Factorize some functions and simplify transport exit sequence.
> > - Rename driver source file from optee_service.c to optee.c.
> >
> > No change since v1
> > ---
> > drivers/firmware/arm_scmi/Kconfig | 12 +
> > 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.c | 572 +++++++++++++++++++++++++++++
> > 5 files changed, 591 insertions(+)
> > create mode 100644 drivers/firmware/arm_scmi/optee.c
> >
> > diff --git a/drivers/firmware/arm_scmi/Kconfig b/drivers/firmware/arm_scmi/Kconfig
> > index 3d7081e84853..30746350349c 100644
> > --- a/drivers/firmware/arm_scmi/Kconfig
> > +++ b/drivers/firmware/arm_scmi/Kconfig
> > @@ -77,6 +77,18 @@ config ARM_SCMI_TRANSPORT_VIRTIO
> > If you want the ARM SCMI PROTOCOL stack to include support for a
> > transport based on VirtIO, answer Y.
> >
> > +config ARM_SCMI_TRANSPORT_OPTEE
> > + bool "SCMI transport based on OP-TEE service"
> > + depends on OPTEE=y || OPTEE=ARM_SCMI_PROTOCOL
> > + select ARM_SCMI_HAVE_TRANSPORT
> > + select ARM_SCMI_HAVE_SHMEM
> > + default y
> > + help
> > + This enables the OP-TEE service based transport for SCMI.
> > +
> > + If you want the ARM SCMI PROTOCOL stack to include support for a
> > + transport based on OP-TEE SCMI service, answer Y.
> > +
> > endif #ARM_SCMI_PROTOCOL
> >
> > config ARM_SCMI_POWER_DOMAIN
> > diff --git a/drivers/firmware/arm_scmi/Makefile b/drivers/firmware/arm_scmi/Makefile
> > index 1dcf123d64ab..ef66ec8ca917 100644
> > --- a/drivers/firmware/arm_scmi/Makefile
> > +++ b/drivers/firmware/arm_scmi/Makefile
> > @@ -6,6 +6,7 @@ scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_MAILBOX) += mailbox.o
> > scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_SMC) += smc.o
> > scmi-transport-$(CONFIG_ARM_SCMI_HAVE_MSG) += msg.o
> > scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_VIRTIO) += virtio.o
> > +scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_OPTEE) += optee.o
> > scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o voltage.o
> > scmi-module-objs := $(scmi-bus-y) $(scmi-driver-y) $(scmi-protocols-y) \
> > $(scmi-transport-y)
> > diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
> > index dea1bfbe1052..6438b5248c24 100644
> > --- a/drivers/firmware/arm_scmi/common.h
> > +++ b/drivers/firmware/arm_scmi/common.h
> > @@ -421,6 +421,9 @@ extern const struct scmi_desc scmi_smc_desc;
> > #ifdef CONFIG_ARM_SCMI_TRANSPORT_VIRTIO
> > extern const struct scmi_desc scmi_virtio_desc;
> > #endif
> > +#ifdef CONFIG_ARM_SCMI_TRANSPORT_OPTEE
> > +extern const struct scmi_desc scmi_optee_desc;
> > +#endif
> >
> > void scmi_rx_callback(struct scmi_chan_info *cinfo, u32 msg_hdr, void *priv);
> > void scmi_free_channel(struct scmi_chan_info *cinfo, struct idr *idr, int id);
> > diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> > index b406b3f78f46..e3f87e0c4936 100644
> > --- a/drivers/firmware/arm_scmi/driver.c
> > +++ b/drivers/firmware/arm_scmi/driver.c
> > @@ -1999,6 +1999,9 @@ static const struct of_device_id scmi_of_match[] = {
> > #endif
> > #ifdef CONFIG_ARM_SCMI_TRANSPORT_VIRTIO
> > { .compatible = "arm,scmi-virtio", .data = &scmi_virtio_desc},
> > +#endif
> > +#ifdef CONFIG_ARM_SCMI_TRANSPORT_OPTEE
> > + { .compatible = "linaro,scmi-optee", .data = &scmi_optee_desc },
> > #endif
> > { /* Sentinel */ },
> > };
> > diff --git a/drivers/firmware/arm_scmi/optee.c b/drivers/firmware/arm_scmi/optee.c
> > new file mode 100644
> > index 000000000000..2b666c632800
> > --- /dev/null
> > +++ b/drivers/firmware/arm_scmi/optee.c
> > @@ -0,0 +1,572 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2019-2021 Linaro Ltd.
> > + */
> > +
> > +#include <linux/io.h>
> > +#include <linux/of.h>
> > +#include <linux/of_address.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/slab.h>
> > +#include <linux/tee_drv.h>
> > +#include <linux/uuid.h>
> > +#include <uapi/linux/tee.h>
> > +
> > +#include "common.h"
> > +
> > +#define SCMI_OPTEE_MAX_MSG_SIZE 128
> > +
> > +enum scmi_optee_pta_cmd {
> > + /*
> > + * PTA_SCMI_CMD_CAPABILITIES - Get channel capabilities
> > + *
> > + * [out] value[0].a: Capability bit mask (enum pta_scmi_caps)
> > + * [out] value[0].b: Extended capabilities or 0
> > + */
> > + 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).
> > + */
> > + 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).
> > + */
> > + PTA_SCMI_CMD_PROCESS_SMT_CHANNEL_MESSAGE = 2,
> > +
> > + /*
> > + * PTA_SCMI_CMD_GET_CHANNEL - Get channel handle
> > + *
> > + * SCMI shm information are 0 if agent expects to use OP-TEE regular SHM
> > + *
> > + * [in] value[0].a: Channel identifier
> > + * [out] value[0].a: Returned channel handle
> > + * [in] value[0].b: Requested capabilities mask (enum pta_scmi_caps)
> > + */
> > + PTA_SCMI_CMD_GET_CHANNEL = 3,
> > +};
> > +
> > +/*
> > + * OP-TEE SCMI service capabilities bit flags (32bit)
> > + *
> > + * PTA_SCMI_CAPS_SMT_HEADER
> > + * When set, OP-TEE supports command using SMT header protocol (SCMI shmem) in
> > + * shared memory buffers to carry SCMI protocol synchronisation information.
> > + */
> > +#define PTA_SCMI_CAPS_NONE 0
> > +#define PTA_SCMI_CAPS_SMT_HEADER BIT(0)
> > +
> > +/**
> > + * struct scmi_optee_channel - Description of an OP-TEE SCMI channel
> > + *
> > + * @channel_id: OP-TEE channel ID used for this transport
> > + * @tee_session: TEE session identifier
> > + * @caps: OP-TEE SCMI channel capabilities
> > + * @mu: Mutex protection on channel access
> > + * @cinfo: SCMI channel information
> > + * @shmem: Virtual base address of the shared memory
> > + * @tee_shm: Reference to TEE shared memory or NULL if using static shmem
> > + * @link: Reference in agent's channel list
> > + */
> > +struct scmi_optee_channel {
> > + u32 channel_id;
> > + u32 tee_session;
> > + u32 caps;
> > + struct mutex mu;
> > + struct scmi_chan_info *cinfo;
> > + struct scmi_shared_mem __iomem *shmem;
> > + 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;
> > +};
> > +
> > +/* There can be only 1 SCMI service in OP-TEE we connect to */
> > +static struct scmi_optee_agent *scmi_optee_private;
> > +
> > +/* Forward reference to scmi_optee transport initialization */
> > +static int scmi_optee_init(void);
> > +
> > +/* Open a session toward SCMI OP-TEE service with REE_KERNEL identity */
> > +static int open_session(struct scmi_optee_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 arg = { };
> > + int ret;
> > +
> > + memcpy(arg.uuid, scmi_pta->id.uuid.b, TEE_IOCTL_UUID_LEN);
> > + arg.clnt_login = TEE_IOCTL_LOGIN_REE_KERNEL;
> > +
> > + ret = tee_client_open_session(agent->tee_ctx, &arg, NULL);
> > + if (ret < 0 || arg.ret) {
> > + dev_err(dev, "Can't open tee session: %d / %#x\n", ret, arg.ret);
> > + return -EOPNOTSUPP;
> > + }
> > +
> > + *tee_session = arg.session;
> > +
> > + return 0;
> > +}
> > +
> > +static void close_session(struct scmi_optee_agent *agent, u32 tee_session)
> > +{
> > + tee_client_close_session(agent->tee_ctx, tee_session);
> > +}
> > +
> > +static int get_capabilities(struct scmi_optee_agent *agent)
> > +{
> > + struct tee_ioctl_invoke_arg arg = { };
> > + struct tee_param param[1] = { };
> > + u32 caps;
> > + u32 tee_session;
> > + int ret;
> > +
> > + ret = open_session(agent, &tee_session);
> > + if (ret)
> > + return ret;
> > +
> > + arg.func = PTA_SCMI_CMD_CAPABILITIES;
> > + arg.session = tee_session;
> > + arg.num_params = 1;
> > +
> > + param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT;
> > +
> > + ret = tee_client_invoke_func(agent->tee_ctx, &arg, param);
> > +
> > + close_session(agent, tee_session);
> > +
> > + if (ret < 0 || arg.ret) {
> > + dev_err(agent->dev, "Can't get capabilities: %d / %#x\n", ret, arg.ret);
> > + return -EOPNOTSUPP;
> > + }
> > +
> > + caps = param[0].u.value.a;
> > +
> > + if (!(caps & PTA_SCMI_CAPS_SMT_HEADER)) {
> > + dev_err(agent->dev, "OP-TEE SCMI PTA doesn't support SMT\n");
> > + return -EOPNOTSUPP;
> > + }
> > +
> > + agent->caps = caps;
> > +
> > + return 0;
> > +}
> > +
> > +static int get_channel(struct scmi_optee_channel *channel)
> > +{
> > + struct device *dev = scmi_optee_private->dev;
> > + struct tee_ioctl_invoke_arg arg = { };
> > + struct tee_param param[1] = { };
> > + unsigned int caps = PTA_SCMI_CAPS_SMT_HEADER;
> > + int ret;
> > +
> > + arg.func = PTA_SCMI_CMD_GET_CHANNEL;
> > + arg.session = channel->tee_session;
> > + arg.num_params = 1;
> > +
> > + param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INOUT;
> > + 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);
> > +
> > + if (ret || arg.ret) {
> > + dev_err(dev, "Can't get channel with caps %#x: %d / %#x\n", caps, ret, arg.ret);
> > + return -EOPNOTSUPP;
> > + }
> > +
> > + /* From now on use channel identifer provided by OP-TEE SCMI service */
> > + channel->channel_id = param[0].u.value.a;
> > + channel->caps = caps;
> > +
> > + return 0;
> > +}
> > +
> > +static int invoke_process_smt_channel(struct scmi_optee_channel *channel)
> > +{
> > + struct tee_ioctl_invoke_arg arg = { };
> > + struct tee_param param[2] = { };
> > + int ret;
> > +
> > + arg.session = channel->tee_session;
> > + param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT;
> > + param[0].u.value.a = channel->channel_id;
> > +
> > + if (channel->tee_shm) {
> > + param[1].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT;
> > + param[1].u.memref.shm = channel->tee_shm;
> > + param[1].u.memref.size = SCMI_OPTEE_MAX_MSG_SIZE;
> > + arg.num_params = 2;
> > + arg.func = PTA_SCMI_CMD_PROCESS_SMT_CHANNEL_MESSAGE;
> > + } else {
> > + arg.num_params = 1;
> > + arg.func = PTA_SCMI_CMD_PROCESS_SMT_CHANNEL;
> > + }
> > +
> > + ret = tee_client_invoke_func(scmi_optee_private->tee_ctx, &arg, param);
> > + if (ret < 0 || arg.ret) {
> > + dev_err(scmi_optee_private->dev, "Can't invoke channel %u: %d / %#x\n",
> > + channel->channel_id, ret, arg.ret);
> > + return -EIO;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int scmi_optee_link_supplier(struct device *dev)
> > +{
> > + if (!scmi_optee_private || !scmi_optee_private->tee_ctx) {
>
> Is not this !tee_ctx redundant since scmi_optee_private->tee_ctx is already
> assured not NULL when scmi_optee_private is found set, given the order of ops in
> the probe() and the barrier you added ?
you're right, !scmi_optee_private is enough.
>
> > + if (scmi_optee_init())
> > + dev_dbg(dev, "Optee bus not yet ready\n");
> > +
> > + /* Wait for optee bus */
> > + return -EPROBE_DEFER;
> > + }
> > +
> > + if (!device_link_add(dev, scmi_optee_private->dev, DL_FLAG_AUTOREMOVE_CONSUMER)) {
> > + dev_err(dev, "Adding link to supplier optee device failed\n");
> > + return -ECANCELED;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static bool scmi_optee_chan_available(struct device *dev, int idx)
> > +{
> > + u32 channel_id;
> > +
> > + return !of_property_read_u32_index(dev->of_node, "linaro,optee-channel-id",
> > + idx, &channel_id);
> > +}
> > +
> > +static void scmi_optee_clear_channel(struct scmi_chan_info *cinfo)
> > +{
> > + struct scmi_optee_channel *channel = cinfo->transport_info;
> > +
> > + shmem_clear_channel(channel->shmem);
> > +}
> > +
> > +static int setup_dynamic_shmem(struct device *dev, struct scmi_optee_channel *channel)
> > +{
> > + const size_t msg_size = SCMI_OPTEE_MAX_MSG_SIZE;
> > +
> > + channel->tee_shm = tee_shm_alloc_kernel_buf(scmi_optee_private->tee_ctx, msg_size);
> > + if (IS_ERR(channel->tee_shm)) {
> > + dev_err(channel->cinfo->dev, "shmem allocation failed\n");
> > + return -ENOMEM;
> > + }
> > +
> > + channel->shmem = (void *)tee_shm_get_va(channel->tee_shm, 0);
> > + memset(channel->shmem, 0, msg_size);
> > + shmem_clear_channel(channel->shmem);
> > +
> > + return 0;
> > +}
> > +
> > +static int setup_static_shmem(struct device *dev, struct scmi_chan_info *cinfo,
> > + struct scmi_optee_channel *channel)
> > +{
> > + struct device_node *np;
> > + resource_size_t size;
> > + struct resource res;
> > + int ret;
> > +
> > + np = of_parse_phandle(cinfo->dev->of_node, "shmem", 0);
> > + if (!of_device_is_compatible(np, "arm,scmi-shmem")) {
> > + ret = -ENXIO;
> > + goto out;
> > + }
> > +
> > + ret = of_address_to_resource(np, 0, &res);
> > + if (ret) {
> > + dev_err(dev, "Failed to get SCMI Tx shared memory\n");
> > + goto out;
> > + }
> > +
> > + size = resource_size(&res);
> > +
> > + channel->shmem = devm_ioremap(dev, res.start, size);
> > + if (!channel->shmem) {
> > + dev_err(dev, "Failed to ioremap SCMI Tx shared memory\n");
> > + ret = -EADDRNOTAVAIL;
> > + goto out;
> > + }
> > +
> > + ret = 0;
> > +
> > +out:
> > + of_node_put(np);
> > +
> > + return ret;
> > +}
> > +
> > +static int setup_shmem(struct device *dev, struct scmi_chan_info *cinfo,
> > + struct scmi_optee_channel *channel)
> > +{
> > + if (of_find_property(cinfo->dev->of_node, "shmem", NULL))
> > + return setup_static_shmem(dev, cinfo, channel);
> > + else
> > + return setup_dynamic_shmem(dev, channel);
> > +}
> > +
> > +static int scmi_optee_chan_setup(struct scmi_chan_info *cinfo, struct device *dev, bool tx)
> > +{
> > + struct scmi_optee_channel *channel;
> > + uint32_t channel_id;
> > + int ret;
> > +
> > + if (!tx)
> > + return -ENODEV;
> > +
> > + channel = devm_kzalloc(dev, sizeof(*channel), GFP_KERNEL);
> > + if (!channel)
> > + return -ENOMEM;
> > +
> > + ret = of_property_read_u32_index(cinfo->dev->of_node, "linaro,optee-channel-id",
> > + 0, &channel_id);
> > + if (ret)
> > + return ret;
> > +
> > + cinfo->transport_info = channel;
> > + channel->cinfo = cinfo;
> > + channel->channel_id = channel_id;
> > + mutex_init(&channel->mu);
> > +
> > + ret = setup_shmem(dev, cinfo, channel);
> > + if (ret)
> > + return ret;
> > +
> > + ret = open_session(scmi_optee_private, &channel->tee_session);
> > + if (ret)
> > + return ret;
> > +
> > + ret = get_channel(channel);
> > + if (ret) {
> > + close_session(scmi_optee_private, channel->tee_session);
> > + return ret;
> > + }
>
> Shouldn't dynamic shmem (if used) be freed explicitly on the err path if
> open_session or get_channel fails (like you do in scmi_optee_chan_free()) ?
>
> if (channel->tee_shm) {
> tee_shm_free(channel->tee_shm);
> ...
>
> Since if .chan_setup() fails here the SCMI core won't register this
> channel in the related IDR map and .chan_free won't be called ever.
> (or initialize shmem later once you have opened the session and got the
> channel and you know you cannot fail anymore...if it is not needed by
> open_session and get_channel as it seems but only later by the SCMI msg
> flow)
Thanks for pointing that. I missed it.
Yes, should release allocated shm upon failure.
>
> > +
> > + mutex_lock(&scmi_optee_private->mu);
> > + list_add(&channel->link, &scmi_optee_private->channel_list);
> > + mutex_unlock(&scmi_optee_private->mu);
> > +
> > + return 0;
> > +}
> > +
> > +static int scmi_optee_chan_free(int id, void *p, void *data)
> > +{
> > + struct scmi_chan_info *cinfo = p;
> > + struct scmi_optee_channel *channel = cinfo->transport_info;
> > +
> > + mutex_lock(&scmi_optee_private->mu);
> > + list_del(&channel->link);
> > + mutex_unlock(&scmi_optee_private->mu);
> > +
>
> I may be wrong (not familiar wih optee) but, somewhere around here, should
> you not also close the session left open on .chan_setup() success (which you
> used actively for SCMI msg exchanges via send_message()/
> invoke_process_smt_channel() ?
You are fully right. I failed most resource release path.
Will fix. Thanks again.
>
> > + if (channel->tee_shm) {
> > + tee_shm_free(channel->tee_shm);
> > + channel->tee_shm = NULL;
> > + }
> > +
> > + cinfo->transport_info = NULL;
> > + channel->cinfo = NULL;
> > +
> > + scmi_free_channel(cinfo, data, id);
> > +
> > + return 0;
> > +}
> > +
> > +static struct scmi_shared_mem *get_channel_shm(struct scmi_optee_channel *chan,
> > + struct scmi_xfer *xfer)
> > +{
> > + if (!chan)
> > + return NULL;
> > +
> > + return chan->shmem;
> > +}
> > +
> > +
> > +static int scmi_optee_send_message(struct scmi_chan_info *cinfo,
> > + struct scmi_xfer *xfer)
> > +{
> > + struct scmi_optee_channel *channel = cinfo->transport_info;
> > + struct scmi_shared_mem *shmem = get_channel_shm(channel, xfer);
> > + int ret;
> > +
> > + mutex_lock(&channel->mu);
> > + shmem_tx_prepare(shmem, xfer);
> > +
> > + ret = invoke_process_smt_channel(channel);
> > +
> > + scmi_rx_callback(cinfo, shmem_read_header(shmem), NULL);
> > + mutex_unlock(&channel->mu);
> > +
> > + return ret;
> > +}
> > +
> > +static void scmi_optee_fetch_response(struct scmi_chan_info *cinfo,
> > + struct scmi_xfer *xfer)
> > +{
> > + struct scmi_optee_channel *channel = cinfo->transport_info;
> > + struct scmi_shared_mem *shmem = get_channel_shm(channel, xfer);
> > +
> > + shmem_fetch_response(shmem, xfer);
> > +}
> > +
> > +static bool scmi_optee_poll_done(struct scmi_chan_info *cinfo,
> > + struct scmi_xfer *xfer)
> > +{
> > + struct scmi_optee_channel *channel = cinfo->transport_info;
> > + struct scmi_shared_mem *shmem = get_channel_shm(channel, xfer);
> > +
> > + return shmem_poll_done(shmem, xfer);
> > +}
>
> Will polling ever be a thing on this transport given that
> invoke_process_smt_channel() is supposed to return atomically ?
>
> (i.e. should we just drop this and .poll_done in the ops below,
> given with v5.15 you can safely omit polling support and the SCMI core
> will refuse to use it even if requested ?)
>From the v5.15-rc7 implementation I understand that poll_done operator
is required when a caller explicitly asks for a polling mode. I can
find only SCMI perf as a caller upon a cpufreq fast switch request. So
It seems to me the SCMI transport should provide a handler for that,
in case it's needed, unless what do_xfer() would warn.
Am I wrong?
>
> > +
> > +static struct scmi_transport_ops scmi_optee_ops = {
> > + .link_supplier = scmi_optee_link_supplier,
> > + .chan_available = scmi_optee_chan_available,
> > + .chan_setup = scmi_optee_chan_setup,
> > + .chan_free = scmi_optee_chan_free,
> > + .send_message = scmi_optee_send_message,
> > + .fetch_response = scmi_optee_fetch_response,
> > + .clear_channel = scmi_optee_clear_channel,
> > + .poll_done = scmi_optee_poll_done,
> > +};
> > +
> > +static int scmi_optee_ctx_match(struct tee_ioctl_version_data *ver, const void *data)
> > +{
> > + return ver->impl_id == TEE_IMPL_ID_OPTEE;
> > +}
> > +
> > +static int scmi_optee_service_probe(struct device *dev)
> > +{
> > + struct scmi_optee_agent *agent;
> > + 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;
> > +
> > + agent = devm_kzalloc(dev, sizeof(*agent), GFP_KERNEL);
> > + if (!agent) {
> > + ret = -ENOMEM;
> > + goto err;
> > + }
> > +
> > + agent->dev = dev;
> > + agent->tee_ctx = tee_ctx;
> > + INIT_LIST_HEAD(&agent->channel_list);
> > +
> > + ret = get_capabilities(agent);
> > + if (ret)
> > + goto err;
> > +
> > + /* Ensure agent resources are all visible before scmi_optee_private is */
> > + smp_mb();
> > + scmi_optee_private = agent;
> > +
> > + return 0;
> > +
> > +err:
> > + tee_client_close_context(tee_ctx);
> > +
> > + return ret;
> > +}
> > +
> > +static int scmi_optee_service_remove(struct device *dev)
> > +{
> > + struct scmi_optee_agent *agent = scmi_optee_private;
> > +
> > + if (!scmi_optee_private)
> > + return -EINVAL;
> > +
> > + if (!list_empty(&scmi_optee_private->channel_list))
> > + return -EBUSY;
> > +
> > + /* Ensure cleared reference is visible before resources are released */
> > + smp_store_mb(scmi_optee_private, NULL);
> > +
> > + tee_client_close_context(agent->tee_ctx);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct tee_client_device_id scmi_optee_service_id[] = {
> > + {
> > + UUID_INIT(0xa8cfe406, 0xd4f5, 0x4a2e,
> > + 0x9f, 0x8d, 0xa2, 0x5d, 0xc7, 0x54, 0xc0, 0x99)
> > + },
> > + { }
> > +};
> > +
> > +MODULE_DEVICE_TABLE(tee, scmi_optee_service_id);
> > +
> > +static struct tee_client_driver scmi_optee_driver = {
> > + .id_table = scmi_optee_service_id,
> > + .driver = {
> > + .name = "scmi-optee",
> > + .bus = &tee_bus_type,
> > + .probe = scmi_optee_service_probe,
> > + .remove = scmi_optee_service_remove,
> > + },
> > +};
> > +
> > +static int scmi_optee_init(void)
> > +{
> > + return driver_register(&scmi_optee_driver.driver);
> > +}
> > +
> > +static void scmi_optee_exit(void)
> > +{
> > + driver_unregister(&scmi_optee_driver.driver);
> > +}
> > +
> > +const struct scmi_desc scmi_optee_desc = {
> > + .transport_exit = scmi_optee_exit,
> > + .ops = &scmi_optee_ops,
> > + .max_rx_timeout_ms = 30,
> > + .max_msg = 20,
> > + .max_msg_size = 128,
>
> Shouldn't we use SCMI_OPTEE_MAX_MSG_SIZE now here too that we have it ?
indeed!
Thanks for all the findings.
Regards,
Etienne
>
> Thanks,
> Cristian