Messages in this thread | | | From | Etienne Carriere <> | Date | Tue, 26 Oct 2021 09:38:09 +0200 | Subject | Re: [PATCH v5 2/2] firmware: arm_scmi: Add optee transport |
| |
Hi Cristian,
On Mon, 25 Oct 2021 at 17:45, Cristian Marussi <cristian.marussi@arm.com> 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@arm.com> > > Cc: Sudeep Holla <sudeep.holla@arm.com> > > Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org> > > --- > > 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
| |