[PATCH] rtc: optee: add RTC driver for OP-TEE RTC PTA

[PATCH] rtc: optee: add RTC driver for OP-TEE RTC PTA

[Clément Léger]

This drivers allows to communicate with a RTC PTA handled by OP-TEE [1].
This PTA allows to query RTC information, set/get time and set/get
offset depending on the supported features.

[1] https://github.com/OP-TEE/optee_os/pull/5179

Signed-off-by: Clément Léger <clement.leger@bootlin.com>
---
 MAINTAINERS             |   6 +
 drivers/rtc/Kconfig     |  10 +
 drivers/rtc/Makefile    |   1 +
 drivers/rtc/rtc-optee.c | 395 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 412 insertions(+)
 create mode 100644 drivers/rtc/rtc-optee.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 4383949ff654..4449d5ca2e11 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14381,6 +14381,12 @@ L:	op-tee@lists.trustedfirmware.org
 S:	Maintained
 F:	drivers/char/hw_random/optee-rng.c
 
+OP-TEE RTC DRIVER
+M:	Clément Léger <clement.leger@bootlin.com>
+L:	linux-rtc@vger.kernel.org
+S:	Maintained
+F:	drivers/rtc/rtc-optee.c
+
 OPA-VNIC DRIVER
 M:	Dennis Dalessandro <dennis.dalessandro@cornelisnetworks.com>
 M:	Mike Marciniszyn <mike.marciniszyn@cornelisnetworks.com>
diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index d85a3c31347c..b10d88db8e9c 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -1293,6 +1293,16 @@ config RTC_DRV_OPAL
 	  This driver can also be built as a module. If so, the module
 	  will be called rtc-opal.
 
+config RTC_DRV_OPTEE
+	tristate "OP-TEE based RTC driver"
+	depends on OPTEE
+	help
+	  Select this to get support for OP-TEE based RTC control on ARM SoCs
+	  where RTC are not accessible to the normal world (Linux).
+
+	  This driver can also be built as a module. If so, the module
+	  will be called rtc-optee.
+
 config RTC_DRV_ZYNQMP
 	tristate "Xilinx Zynq Ultrascale+ MPSoC RTC"
 	depends on OF && HAS_IOMEM
diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
index e92f3e943245..2d827d8261d5 100644
--- a/drivers/rtc/Makefile
+++ b/drivers/rtc/Makefile
@@ -115,6 +115,7 @@ obj-$(CONFIG_RTC_DRV_GAMECUBE)	+= rtc-gamecube.o
 obj-$(CONFIG_RTC_DRV_NTXEC)	+= rtc-ntxec.o
 obj-$(CONFIG_RTC_DRV_OMAP)	+= rtc-omap.o
 obj-$(CONFIG_RTC_DRV_OPAL)	+= rtc-opal.o
+obj-$(CONFIG_RTC_DRV_OPTEE)	+= rtc-optee.o
 obj-$(CONFIG_RTC_DRV_PALMAS)	+= rtc-palmas.o
 obj-$(CONFIG_RTC_DRV_PCAP)	+= rtc-pcap.o
 obj-$(CONFIG_RTC_DRV_PCF2123)	+= rtc-pcf2123.o
diff --git a/drivers/rtc/rtc-optee.c b/drivers/rtc/rtc-optee.c
new file mode 100644
index 000000000000..f265056bf19e
--- /dev/null
+++ b/drivers/rtc/rtc-optee.c
@@ -0,0 +1,395 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2022 Microchip.
+ */
+
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/rtc.h>
+#include <linux/tee_drv.h>
+
+#define RTC_INFO_VERSION	0x1
+
+#define TA_CMD_RTC_GET_INFO		0x0
+#define TA_CMD_RTC_GET_TIME		0x1
+#define TA_CMD_RTC_SET_TIME		0x2
+#define TA_CMD_RTC_GET_OFFSET		0x3
+#define TA_CMD_RTC_SET_OFFSET		0x4
+
+#define TA_RTC_FEATURE_CORRECTION	BIT(0)
+
+struct optee_rtc_time {
+	u32 tm_sec;
+	u32 tm_min;
+	u32 tm_hour;
+	u32 tm_mday;
+	u32 tm_mon;
+	u32 tm_year;
+	u32 tm_wday;
+};
+
+struct optee_rtc_info {
+	u64 version;
+	u64 features;
+	struct optee_rtc_time range_min;
+	struct optee_rtc_time range_max;
+};
+
+/**
+ * struct optee_rtc - OP-TEE RTC private data
+ * @dev:		OP-TEE based RTC device.
+ * @ctx:		OP-TEE context handler.
+ * @session_id:		RTC TA session identifier.
+ * @entropy_shm_pool:	Memory pool shared with RTC device.
+ * @features:		Bitfield of RTC features
+ */
+struct optee_rtc {
+	struct device *dev;
+	struct tee_context *ctx;
+	u32 session_id;
+	struct tee_shm *entropy_shm_pool;
+	u64 features;
+};
+
+static int optee_rtc_readtime(struct device *dev, struct rtc_time *tm)
+{
+	struct optee_rtc *priv = dev_get_drvdata(dev);
+	struct tee_ioctl_invoke_arg inv_arg = {0};
+	struct optee_rtc_time *optee_tm;
+	struct tee_param param[4] = {0};
+	int ret = 0;
+
+	inv_arg.func = TA_CMD_RTC_GET_TIME;
+	inv_arg.session = priv->session_id;
+	inv_arg.num_params = 4;
+
+	/* Fill invoke cmd params */
+	param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT;
+	param[0].u.memref.shm = priv->entropy_shm_pool;
+	param[0].u.memref.size = sizeof(struct optee_rtc_time);
+	param[0].u.memref.shm_offs = 0;
+
+	ret = tee_client_invoke_func(priv->ctx, &inv_arg, param);
+	if ((ret < 0) || (inv_arg.ret != 0)) {
+		dev_err(dev, "TA_CMD_RTC_GET_TIME invoke err: %x\n",
+			inv_arg.ret);
+		return ret;
+	}
+
+	optee_tm = tee_shm_get_va(priv->entropy_shm_pool, 0);
+	if (IS_ERR(optee_tm)) {
+		dev_err(dev, "tee_shm_get_va failed\n");
+		return PTR_ERR(optee_tm);
+	}
+
+	if (param[0].u.memref.size != sizeof(*optee_tm)) {
+		dev_err(dev, "Invalid read size from OPTEE\n");
+		return -EINVAL;
+	}
+
+	tm->tm_sec = optee_tm->tm_sec;
+	tm->tm_min = optee_tm->tm_min;
+	tm->tm_hour = optee_tm->tm_hour;
+	tm->tm_mday = optee_tm->tm_mday;
+	tm->tm_mon = optee_tm->tm_mon;
+	tm->tm_year = optee_tm->tm_year - 1900;
+	tm->tm_wday = optee_tm->tm_wday;
+	tm->tm_yday = rtc_year_days(tm->tm_mday, tm->tm_mon, tm->tm_year);
+
+	return 0;
+}
+
+static int optee_rtc_settime(struct device *dev, struct rtc_time *tm)
+{
+	struct optee_rtc *priv = dev_get_drvdata(dev);
+	struct tee_ioctl_invoke_arg inv_arg = {0};
+	struct optee_rtc_time optee_tm;
+	struct tee_param param[4] = {0};
+	void *rtc_data;
+	int ret = 0;
+
+	optee_tm.tm_sec = tm->tm_sec;
+	optee_tm.tm_min = tm->tm_min;
+	optee_tm.tm_hour = tm->tm_hour;
+	optee_tm.tm_mday = tm->tm_mday;
+	optee_tm.tm_mon = tm->tm_mon;
+	optee_tm.tm_year = tm->tm_year + 1900;
+	optee_tm.tm_wday = tm->tm_wday;
+
+	inv_arg.func = TA_CMD_RTC_SET_TIME;
+	inv_arg.session = priv->session_id;
+	inv_arg.num_params = 4;
+
+	param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT;
+	param[0].u.memref.shm = priv->entropy_shm_pool;
+	param[0].u.memref.size = sizeof(struct optee_rtc_time);
+	param[0].u.memref.shm_offs = 0;
+
+	rtc_data = tee_shm_get_va(priv->entropy_shm_pool, 0);
+	if (IS_ERR(rtc_data)) {
+		dev_err(dev, "tee_shm_get_va failed\n");
+		return PTR_ERR(rtc_data);
+	}
+
+	memcpy(rtc_data, &optee_tm, sizeof(struct optee_rtc_time));
+
+	ret = tee_client_invoke_func(priv->ctx, &inv_arg, param);
+	if ((ret < 0) || (inv_arg.ret != 0)) {
+		dev_err(dev, "TA_CMD_RTC_GET_TIME invoke err: %x\n",
+			inv_arg.ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int optee_rtc_readoffset(struct device *dev, long *offset)
+{
+	struct optee_rtc *priv = dev_get_drvdata(dev);
+	struct tee_ioctl_invoke_arg inv_arg = {0};
+	struct tee_param param[4] = {0};
+	int ret = 0;
+
+	if (!(priv->features & TA_RTC_FEATURE_CORRECTION))
+		return -EOPNOTSUPP;
+
+	inv_arg.func = TA_CMD_RTC_GET_OFFSET;
+	inv_arg.session = priv->session_id;
+	inv_arg.num_params = 4;
+
+	param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT;
+
+	ret = tee_client_invoke_func(priv->ctx, &inv_arg, param);
+	if ((ret < 0) || (inv_arg.ret != 0)) {
+		dev_err(dev, "TA_CMD_RTC_GET_RNG_INFO invoke err: %x\n",
+			inv_arg.ret);
+		return -EINVAL;
+	}
+
+	*offset = param[0].u.value.a;
+
+	return 0;
+}
+
+static int optee_rtc_setoffset(struct device *dev, long offset)
+{
+	struct optee_rtc *priv = dev_get_drvdata(dev);
+	struct tee_ioctl_invoke_arg inv_arg = {0};
+	struct tee_param param[4] = {0};
+	int ret = 0;
+
+	if (!(priv->features & TA_RTC_FEATURE_CORRECTION))
+		return -EOPNOTSUPP;
+
+	inv_arg.func = TA_CMD_RTC_SET_OFFSET;
+	inv_arg.session = priv->session_id;
+	inv_arg.num_params = 4;
+
+	param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT;
+	param[0].u.value.a = offset;
+
+	ret = tee_client_invoke_func(priv->ctx, &inv_arg, param);
+	if ((ret < 0) || (inv_arg.ret != 0)) {
+		dev_err(dev, "TA_CMD_RTC_GET_RNG_INFO invoke err: %x\n",
+			inv_arg.ret);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+static const struct rtc_class_ops optee_rtc_ops = {
+	.read_time	= optee_rtc_readtime,
+	.set_time	= optee_rtc_settime,
+	.set_offset	= optee_rtc_setoffset,
+	.read_offset	= optee_rtc_readoffset,
+};
+
+static int optee_rtc_read_info(struct device *dev, struct rtc_device *rtc,
+			       u64 *features)
+{
+	struct optee_rtc *priv = dev_get_drvdata(dev);
+	struct tee_ioctl_invoke_arg inv_arg = {0};
+	struct tee_param param[4] = {0};
+	struct optee_rtc_info *info;
+	struct optee_rtc_time *tm;
+	int ret = 0;
+
+	inv_arg.func = TA_CMD_RTC_GET_INFO;
+	inv_arg.session = priv->session_id;
+	inv_arg.num_params = 4;
+
+	param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT;
+	param[0].u.memref.shm = priv->entropy_shm_pool;
+	param[0].u.memref.size = sizeof(*info);
+	param[0].u.memref.shm_offs = 0;
+
+	ret = tee_client_invoke_func(priv->ctx, &inv_arg, param);
+	if ((ret < 0) || (inv_arg.ret != 0)) {
+		dev_err(dev, "TA_CMD_RTC_GET_RNG_INFO invoke err: %x\n",
+			inv_arg.ret);
+		return -EINVAL;
+	}
+
+	info = tee_shm_get_va(priv->entropy_shm_pool, 0);
+	if (IS_ERR(info)) {
+		dev_err(dev, "tee_shm_get_va failed\n");
+		return PTR_ERR(info);
+	}
+
+	if (param[0].u.memref.size < sizeof(*info)) {
+		dev_err(dev, "Invalid read size from OPTEE\n");
+		return -EINVAL;
+	}
+
+	if (info->version != RTC_INFO_VERSION) {
+		dev_err(dev, "Unsupported information version %llu\n",
+			      info->version);
+		return -EINVAL;
+	}
+
+	*features = info->features;
+
+	tm = &info->range_min;
+	rtc->range_min = mktime64(tm->tm_year, tm->tm_mon, tm->tm_mday,
+				  tm->tm_hour, tm->tm_min, tm->tm_sec);
+	tm = &info->range_max;
+	rtc->range_max = mktime64(tm->tm_year, tm->tm_mon, tm->tm_mday,
+				  tm->tm_hour, tm->tm_min, tm->tm_sec);
+
+	return 0;
+}
+
+static int optee_ctx_match(struct tee_ioctl_version_data *ver, const void *data)
+{
+	if (ver->impl_id == TEE_IMPL_ID_OPTEE)
+		return 1;
+	else
+		return 0;
+}
+
+static int optee_rtc_probe(struct device *dev)
+{
+	struct tee_client_device *rtc_device = to_tee_client_device(dev);
+	struct tee_ioctl_open_session_arg sess_arg;
+	struct tee_shm *entropy_shm_pool = NULL;
+	int ret = 0, err = -ENODEV;
+	struct optee_rtc *priv;
+	struct rtc_device *rtc;
+
+	memset(&sess_arg, 0, sizeof(sess_arg));
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	rtc = devm_rtc_allocate_device(dev);
+	if (IS_ERR(rtc))
+		return PTR_ERR(rtc);
+
+	/* Open context with TEE driver */
+	priv->ctx = tee_client_open_context(NULL, optee_ctx_match, NULL,
+					       NULL);
+	if (IS_ERR(priv->ctx))
+		return -ENODEV;
+
+	/* Open session with rtc Trusted App */
+	memcpy(sess_arg.uuid, rtc_device->id.uuid.b, TEE_IOCTL_UUID_LEN);
+	sess_arg.clnt_login = TEE_IOCTL_LOGIN_REE_KERNEL;
+	sess_arg.num_params = 0;
+
+	ret = tee_client_open_session(priv->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);
+		err = -EINVAL;
+		goto out_ctx;
+	}
+	priv->session_id = sess_arg.session;
+
+	entropy_shm_pool = tee_shm_alloc(priv->ctx,
+					 sizeof(struct optee_rtc_info),
+					 TEE_SHM_MAPPED | TEE_SHM_DMA_BUF);
+	if (IS_ERR(entropy_shm_pool)) {
+		dev_err(priv->dev, "tee_shm_alloc failed\n");
+		err = PTR_ERR(entropy_shm_pool);
+		goto out_sess;
+	}
+
+	priv->entropy_shm_pool = entropy_shm_pool;
+	priv->dev = dev;
+	dev_set_drvdata(dev, priv);
+
+	rtc->ops = &optee_rtc_ops;
+
+	err = optee_rtc_read_info(dev, rtc, &priv->features);
+	if (err) {
+		dev_err(dev, "Failed to get RTC features from OP-TEE\n");
+		goto out_sess;
+	}
+
+	err = devm_rtc_register_device(rtc);
+	if (err)
+		goto out_sess;
+
+	/*
+	 * We must clear this bit after registering because rtc_register_device
+	 * will set it if it sees that .set_offset is provided.
+	 */
+	if (!(priv->features & TA_RTC_FEATURE_CORRECTION))
+		clear_bit(RTC_FEATURE_CORRECTION, rtc->features);
+
+	return 0;
+
+out_sess:
+	tee_client_close_session(priv->ctx, priv->session_id);
+out_ctx:
+	tee_client_close_context(priv->ctx);
+
+	return err;
+}
+
+static int optee_rtc_remove(struct device *dev)
+{
+	struct optee_rtc *priv = dev_get_drvdata(dev);
+
+	tee_client_close_session(priv->ctx, priv->session_id);
+	tee_client_close_context(priv->ctx);
+
+	return 0;
+}
+
+static const struct tee_client_device_id optee_rtc_id_table[] = {
+	{UUID_INIT(0xf389f8c8, 0x845f, 0x496c,
+		   0x8b, 0xbe, 0xd6, 0x4b, 0xd2, 0x4c, 0x92, 0xfd)},
+	{}
+};
+
+MODULE_DEVICE_TABLE(tee, optee_rtc_id_table);
+
+static struct tee_client_driver optee_rtc_driver = {
+	.id_table	= optee_rtc_id_table,
+	.driver		= {
+		.name		= "optee_rtc",
+		.bus		= &tee_bus_type,
+		.probe		= optee_rtc_probe,
+		.remove		= optee_rtc_remove,
+	},
+};
+
+static int __init optee_rtc_mod_init(void)
+{
+	return driver_register(&optee_rtc_driver.driver);
+}
+
+static void __exit optee_rtc_mod_exit(void)
+{
+	driver_unregister(&optee_rtc_driver.driver);
+}
+
+module_init(optee_rtc_mod_init);
+module_exit(optee_rtc_mod_exit);
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Clément Léger <clement.leger@bootlin.com>");
+MODULE_DESCRIPTION("OP-TEE based RTC driver");
-- 
2.34.1

Re: [PATCH] rtc: optee: add RTC driver for OP-TEE RTC PTA

[Clément Léger]

Le Thu, 3 Mar 2022 08:35:17 +0100,
Jens Wiklander <jens.wiklander@linaro.org> a écrit :

Hi Jens, looks like you forgot to "Reply All".

> > +
> > +       /* Fill invoke cmd params */
> > +       param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT;
> > +       param[0].u.memref.shm = priv->entropy_shm_pool;
> > +       param[0].u.memref.size = sizeof(struct optee_rtc_time);
> > +       param[0].u.memref.shm_offs = 0;
> > +
> > +       ret = tee_client_invoke_func(priv->ctx, &inv_arg, param);
> > +       if ((ret < 0) || (inv_arg.ret != 0)) {  
> 
> if (ret < 0 || inv_arg.ret) looks easier to me. Please check the patch
> with checkpatch --strict do catch all these and other trivial checks.

Acked, the rng-optee driver might not be a good starting point :/ Will
check that.

> > +static int optee_rtc_read_info(struct device *dev, struct rtc_device *rtc,
> > +                              u64 *features)
> > +{
> > +       struct optee_rtc *priv = dev_get_drvdata(dev);
> > +       struct tee_ioctl_invoke_arg inv_arg = {0};
> > +       struct tee_param param[4] = {0};
> > +       struct optee_rtc_info *info;
> > +       struct optee_rtc_time *tm;
> > +       int ret = 0;
> > +
> > +       inv_arg.func = TA_CMD_RTC_GET_INFO;
> > +       inv_arg.session = priv->session_id;
> > +       inv_arg.num_params = 4;
> > +
> > +       param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT;
> > +       param[0].u.memref.shm = priv->entropy_shm_pool;
> > +       param[0].u.memref.size = sizeof(*info);
> > +       param[0].u.memref.shm_offs = 0;
> > +
> > +       ret = tee_client_invoke_func(priv->ctx, &inv_arg, param);
> > +       if ((ret < 0) || (inv_arg.ret != 0)) {
> > +               dev_err(dev, "TA_CMD_RTC_GET_RNG_INFO invoke err: %x\n",
> > +                       inv_arg.ret);
> > +               return -EINVAL;
> > +       }
> > +
> > +       info = tee_shm_get_va(priv->entropy_shm_pool, 0);
> > +       if (IS_ERR(info)) {
> > +               dev_err(dev, "tee_shm_get_va failed\n");
> > +               return PTR_ERR(info);
> > +       }
> > +
> > +       if (param[0].u.memref.size < sizeof(*info)) {
> > +               dev_err(dev, "Invalid read size from OPTEE\n");
> > +               return -EINVAL;
> > +       }
> > +
> > +       if (info->version != RTC_INFO_VERSION) {
> > +               dev_err(dev, "Unsupported information version %llu\n",
> > +                             info->version);
> > +               return -EINVAL;
> > +       }
> > +
> > +       *features = info->features;
> > +
> > +       tm = &info->range_min;
> > +       rtc->range_min = mktime64(tm->tm_year, tm->tm_mon, tm->tm_mday,
> > +                                 tm->tm_hour, tm->tm_min, tm->tm_sec);
> > +       tm = &info->range_max;
> > +       rtc->range_max = mktime64(tm->tm_year, tm->tm_mon, tm->tm_mday,
> > +                                 tm->tm_hour, tm->tm_min, tm->tm_sec);  
> 
> If this is the only thing we're going to do with the returned min and
> max, how come this wasn't done on the OP-TEE side already? Without
> these large structs we could have defined an ABI without the need for
> shared memory.

mktime64 converts the time range to the number of seconds elapsed since
1970. I thought it would be better not to assess anything about the way
the time is stored on OP-TEE side and thus return the full time struct.
If you think it's acceptable to suppose that, then, we can still switch
to using timestamps as the ABI with OP-TEE RTC PTA. The same could be
done to get/set time if needed in this case.

> > +
> > +static int optee_rtc_probe(struct device *dev)
> > +{
> > +       struct tee_client_device *rtc_device = to_tee_client_device(dev);
> > +       struct tee_ioctl_open_session_arg sess_arg;
> > +       struct tee_shm *entropy_shm_pool = NULL;
> > +       int ret = 0, err = -ENODEV;  
> 
> There is generally no need to initialize these here.
> 
> > +       struct optee_rtc *priv;
> > +       struct rtc_device *rtc;
> > +
> > +       memset(&sess_arg, 0, sizeof(sess_arg));
> > +
> > +       priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > +       if (!priv)
> > +               return -ENOMEM;
> > +
> > +       rtc = devm_rtc_allocate_device(dev);
> > +       if (IS_ERR(rtc))
> > +               return PTR_ERR(rtc);
> > +
> > +       /* Open context with TEE driver */
> > +       priv->ctx = tee_client_open_context(NULL, optee_ctx_match, NULL,
> > +                                              NULL);
> > +       if (IS_ERR(priv->ctx))
> > +               return -ENODEV;
> > +
> > +       /* Open session with rtc Trusted App */
> > +       memcpy(sess_arg.uuid, rtc_device->id.uuid.b, TEE_IOCTL_UUID_LEN);  
> 
> Perhaps uuid_copy() would be better.

Ok, I will use export_uuid() then.

> 
> > +       sess_arg.clnt_login = TEE_IOCTL_LOGIN_REE_KERNEL;
> > +       sess_arg.num_params = 0;
> > +
> > +       ret = tee_client_open_session(priv->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);
> > +               err = -EINVAL;
> > +               goto out_ctx;
> > +       }
> > +       priv->session_id = sess_arg.session;
> > +
> > +       entropy_shm_pool = tee_shm_alloc(priv->ctx,
> > +                                        sizeof(struct optee_rtc_info),
> > +                                        TEE_SHM_MAPPED | TEE_SHM_DMA_BUF);  
> 
> tee_shm_alloc() is about to be removed. Please rebase on
> https://git.linaro.org/people/jens.wiklander/linux-tee.git/tag/?h=tee-shm-for-v5.18

Acked.

-- 
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com

Re: [PATCH] rtc: optee: add RTC driver for OP-TEE RTC PTA

[Jens Wiklander]

On Thu, Mar 3, 2022 at 9:17 AM Clément Léger <clement.leger@bootlin.com> wrote:
>
> Le Thu, 3 Mar 2022 08:35:17 +0100,
> Jens Wiklander <jens.wiklander@linaro.org> a écrit :
>
> Hi Jens, looks like you forgot to "Reply All".

You're right.

>
> > > +
> > > +       /* Fill invoke cmd params */
> > > +       param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT;
> > > +       param[0].u.memref.shm = priv->entropy_shm_pool;
> > > +       param[0].u.memref.size = sizeof(struct optee_rtc_time);
> > > +       param[0].u.memref.shm_offs = 0;
> > > +
> > > +       ret = tee_client_invoke_func(priv->ctx, &inv_arg, param);
> > > +       if ((ret < 0) || (inv_arg.ret != 0)) {
> >
> > if (ret < 0 || inv_arg.ret) looks easier to me. Please check the patch
> > with checkpatch --strict do catch all these and other trivial checks.
>
> Acked, the rng-optee driver might not be a good starting point :/ Will
> check that.
>
> > > +static int optee_rtc_read_info(struct device *dev, struct rtc_device *rtc,
> > > +                              u64 *features)
> > > +{
> > > +       struct optee_rtc *priv = dev_get_drvdata(dev);
> > > +       struct tee_ioctl_invoke_arg inv_arg = {0};
> > > +       struct tee_param param[4] = {0};
> > > +       struct optee_rtc_info *info;
> > > +       struct optee_rtc_time *tm;
> > > +       int ret = 0;
> > > +
> > > +       inv_arg.func = TA_CMD_RTC_GET_INFO;
> > > +       inv_arg.session = priv->session_id;
> > > +       inv_arg.num_params = 4;
> > > +
> > > +       param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT;
> > > +       param[0].u.memref.shm = priv->entropy_shm_pool;
> > > +       param[0].u.memref.size = sizeof(*info);
> > > +       param[0].u.memref.shm_offs = 0;
> > > +
> > > +       ret = tee_client_invoke_func(priv->ctx, &inv_arg, param);
> > > +       if ((ret < 0) || (inv_arg.ret != 0)) {
> > > +               dev_err(dev, "TA_CMD_RTC_GET_RNG_INFO invoke err: %x\n",
> > > +                       inv_arg.ret);
> > > +               return -EINVAL;
> > > +       }
> > > +
> > > +       info = tee_shm_get_va(priv->entropy_shm_pool, 0);
> > > +       if (IS_ERR(info)) {
> > > +               dev_err(dev, "tee_shm_get_va failed\n");
> > > +               return PTR_ERR(info);
> > > +       }
> > > +
> > > +       if (param[0].u.memref.size < sizeof(*info)) {
> > > +               dev_err(dev, "Invalid read size from OPTEE\n");
> > > +               return -EINVAL;
> > > +       }
> > > +
> > > +       if (info->version != RTC_INFO_VERSION) {
> > > +               dev_err(dev, "Unsupported information version %llu\n",
> > > +                             info->version);
> > > +               return -EINVAL;
> > > +       }
> > > +
> > > +       *features = info->features;
> > > +
> > > +       tm = &info->range_min;
> > > +       rtc->range_min = mktime64(tm->tm_year, tm->tm_mon, tm->tm_mday,
> > > +                                 tm->tm_hour, tm->tm_min, tm->tm_sec);
> > > +       tm = &info->range_max;
> > > +       rtc->range_max = mktime64(tm->tm_year, tm->tm_mon, tm->tm_mday,
> > > +                                 tm->tm_hour, tm->tm_min, tm->tm_sec);
> >
> > If this is the only thing we're going to do with the returned min and
> > max, how come this wasn't done on the OP-TEE side already? Without
> > these large structs we could have defined an ABI without the need for
> > shared memory.
>
> mktime64 converts the time range to the number of seconds elapsed since
> 1970. I thought it would be better not to assess anything about the way
> the time is stored on OP-TEE side and thus return the full time struct.
> If you think it's acceptable to suppose that, then, we can still switch
> to using timestamps as the ABI with OP-TEE RTC PTA. The same could be
> done to get/set time if needed in this case.

This is a bit more complicated than I first thought. The proposed ABI
is fine from my point of view.

Cheers,
Jens

>
> > > +
> > > +static int optee_rtc_probe(struct device *dev)
> > > +{
> > > +       struct tee_client_device *rtc_device = to_tee_client_device(dev);
> > > +       struct tee_ioctl_open_session_arg sess_arg;
> > > +       struct tee_shm *entropy_shm_pool = NULL;
> > > +       int ret = 0, err = -ENODEV;
> >
> > There is generally no need to initialize these here.
> >
> > > +       struct optee_rtc *priv;
> > > +       struct rtc_device *rtc;
> > > +
> > > +       memset(&sess_arg, 0, sizeof(sess_arg));
> > > +
> > > +       priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > > +       if (!priv)
> > > +               return -ENOMEM;
> > > +
> > > +       rtc = devm_rtc_allocate_device(dev);
> > > +       if (IS_ERR(rtc))
> > > +               return PTR_ERR(rtc);
> > > +
> > > +       /* Open context with TEE driver */
> > > +       priv->ctx = tee_client_open_context(NULL, optee_ctx_match, NULL,
> > > +                                              NULL);
> > > +       if (IS_ERR(priv->ctx))
> > > +               return -ENODEV;
> > > +
> > > +       /* Open session with rtc Trusted App */
> > > +       memcpy(sess_arg.uuid, rtc_device->id.uuid.b, TEE_IOCTL_UUID_LEN);
> >
> > Perhaps uuid_copy() would be better.
>
> Ok, I will use export_uuid() then.
>
> >
> > > +       sess_arg.clnt_login = TEE_IOCTL_LOGIN_REE_KERNEL;
> > > +       sess_arg.num_params = 0;
> > > +
> > > +       ret = tee_client_open_session(priv->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);
> > > +               err = -EINVAL;
> > > +               goto out_ctx;
> > > +       }
> > > +       priv->session_id = sess_arg.session;
> > > +
> > > +       entropy_shm_pool = tee_shm_alloc(priv->ctx,
> > > +                                        sizeof(struct optee_rtc_info),
> > > +                                        TEE_SHM_MAPPED | TEE_SHM_DMA_BUF);
> >
> > tee_shm_alloc() is about to be removed. Please rebase on
> > https://git.linaro.org/people/jens.wiklander/linux-tee.git/tag/?h=tee-shm-for-v5.18
>
> Acked.
>
> --
> Clément Léger,
> Embedded Linux and Kernel engineer at Bootlin
> https://bootlin.com

Re: [PATCH] rtc: optee: add RTC driver for OP-TEE RTC PTA

[Etienne Carriere]

Hello Clement,

In the code, rtc-optee driver opens a session with optee and allocates
shared memory from probe to remove time.
This uses resources in the TEE but RTC is not accessed often by Linux I guess.
Would it be interesting if a session is opened and shared memory
allocated only when a RPC command is to be invoked?

Minor comments below.

Br,
etienne



On Wed, 2 Mar 2022 at 16:08, Clément Léger <clement.leger@bootlin.com> wrote:
>
> This drivers allows to communicate with a RTC PTA handled by OP-TEE [1].
> This PTA allows to query RTC information, set/get time and set/get
> offset depending on the supported features.
>
> [1] https://github.com/OP-TEE/optee_os/pull/5179
>
> Signed-off-by: Clément Léger <clement.leger@bootlin.com>
> ---
>  MAINTAINERS             |   6 +
>  drivers/rtc/Kconfig     |  10 +
>  drivers/rtc/Makefile    |   1 +
>  drivers/rtc/rtc-optee.c | 395 ++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 412 insertions(+)
>  create mode 100644 drivers/rtc/rtc-optee.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 4383949ff654..4449d5ca2e11 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -14381,6 +14381,12 @@ L:     op-tee@lists.trustedfirmware.org
>  S:     Maintained
>  F:     drivers/char/hw_random/optee-rng.c
>
> +OP-TEE RTC DRIVER
> +M:     Clément Léger <clement.leger@bootlin.com>
> +L:     linux-rtc@vger.kernel.org
> +S:     Maintained
> +F:     drivers/rtc/rtc-optee.c
> +
>  OPA-VNIC DRIVER
>  M:     Dennis Dalessandro <dennis.dalessandro@cornelisnetworks.com>
>  M:     Mike Marciniszyn <mike.marciniszyn@cornelisnetworks.com>
> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> index d85a3c31347c..b10d88db8e9c 100644
> --- a/drivers/rtc/Kconfig
> +++ b/drivers/rtc/Kconfig
> @@ -1293,6 +1293,16 @@ config RTC_DRV_OPAL
>           This driver can also be built as a module. If so, the module
>           will be called rtc-opal.
>
> +config RTC_DRV_OPTEE
> +       tristate "OP-TEE based RTC driver"
> +       depends on OPTEE
> +       help
> +         Select this to get support for OP-TEE based RTC control on ARM SoCs

I would remove the Arm SoCs reference. OP-TEE may be available on
other architectures in the future.

> +         where RTC are not accessible to the normal world (Linux).
> +
> +         This driver can also be built as a module. If so, the module
> +         will be called rtc-optee.
> +
>  config RTC_DRV_ZYNQMP
>         tristate "Xilinx Zynq Ultrascale+ MPSoC RTC"
>         depends on OF && HAS_IOMEM
> diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
> index e92f3e943245..2d827d8261d5 100644
> --- a/drivers/rtc/Makefile
> +++ b/drivers/rtc/Makefile
> @@ -115,6 +115,7 @@ obj-$(CONFIG_RTC_DRV_GAMECUBE)      += rtc-gamecube.o
>  obj-$(CONFIG_RTC_DRV_NTXEC)    += rtc-ntxec.o
>  obj-$(CONFIG_RTC_DRV_OMAP)     += rtc-omap.o
>  obj-$(CONFIG_RTC_DRV_OPAL)     += rtc-opal.o
> +obj-$(CONFIG_RTC_DRV_OPTEE)    += rtc-optee.o
>  obj-$(CONFIG_RTC_DRV_PALMAS)   += rtc-palmas.o
>  obj-$(CONFIG_RTC_DRV_PCAP)     += rtc-pcap.o
>  obj-$(CONFIG_RTC_DRV_PCF2123)  += rtc-pcf2123.o
> diff --git a/drivers/rtc/rtc-optee.c b/drivers/rtc/rtc-optee.c
> new file mode 100644
> index 000000000000..f265056bf19e
> --- /dev/null
> +++ b/drivers/rtc/rtc-optee.c
> @@ -0,0 +1,395 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2022 Microchip.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/rtc.h>
> +#include <linux/tee_drv.h>
> +
> +#define RTC_INFO_VERSION       0x1
> +
> +#define TA_CMD_RTC_GET_INFO            0x0
> +#define TA_CMD_RTC_GET_TIME            0x1
> +#define TA_CMD_RTC_SET_TIME            0x2
> +#define TA_CMD_RTC_GET_OFFSET          0x3
> +#define TA_CMD_RTC_SET_OFFSET          0x4
> +
> +#define TA_RTC_FEATURE_CORRECTION      BIT(0)
> +
> +struct optee_rtc_time {
> +       u32 tm_sec;
> +       u32 tm_min;
> +       u32 tm_hour;
> +       u32 tm_mday;
> +       u32 tm_mon;
> +       u32 tm_year;
> +       u32 tm_wday;
> +};
> +
> +struct optee_rtc_info {
> +       u64 version;
> +       u64 features;
> +       struct optee_rtc_time range_min;
> +       struct optee_rtc_time range_max;
> +};
> +
> +/**
> + * struct optee_rtc - OP-TEE RTC private data
> + * @dev:               OP-TEE based RTC device.
> + * @ctx:               OP-TEE context handler.
> + * @session_id:                RTC TA session identifier.
> + * @entropy_shm_pool:  Memory pool shared with RTC device.
> + * @features:          Bitfield of RTC features
> + */
> +struct optee_rtc {
> +       struct device *dev;
> +       struct tee_context *ctx;
> +       u32 session_id;
> +       struct tee_shm *entropy_shm_pool;
> +       u64 features;
> +};
> +
> +static int optee_rtc_readtime(struct device *dev, struct rtc_time *tm)
> +{
> +       struct optee_rtc *priv = dev_get_drvdata(dev);
> +       struct tee_ioctl_invoke_arg inv_arg = {0};
> +       struct optee_rtc_time *optee_tm;
> +       struct tee_param param[4] = {0};
> +       int ret = 0;
> +
> +       inv_arg.func = TA_CMD_RTC_GET_TIME;
> +       inv_arg.session = priv->session_id;
> +       inv_arg.num_params = 4;
> +
> +       /* Fill invoke cmd params */
> +       param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT;
> +       param[0].u.memref.shm = priv->entropy_shm_pool;
> +       param[0].u.memref.size = sizeof(struct optee_rtc_time);
> +       param[0].u.memref.shm_offs = 0;

can remove this line

> +
> +       ret = tee_client_invoke_func(priv->ctx, &inv_arg, param);
> +       if ((ret < 0) || (inv_arg.ret != 0)) {
> +               dev_err(dev, "TA_CMD_RTC_GET_TIME invoke err: %x\n",
> +                       inv_arg.ret);

print also 'ret' in case error is reported by its value?

> +               return ret;

Be careful here to return an errno when ret==0 and inv_arg.ret!=0.
suggestion: return ret ? ret : -EPROTO;

> +       }
> +
> +       optee_tm = tee_shm_get_va(priv->entropy_shm_pool, 0);
> +       if (IS_ERR(optee_tm)) {
> +               dev_err(dev, "tee_shm_get_va failed\n");
> +               return PTR_ERR(optee_tm);
> +       }
> +
> +       if (param[0].u.memref.size != sizeof(*optee_tm)) {
> +               dev_err(dev, "Invalid read size from OPTEE\n");
> +               return -EINVAL;

-EINVAL or -EPROTO.


> +       }
> +
> +       tm->tm_sec = optee_tm->tm_sec;
> +       tm->tm_min = optee_tm->tm_min;
> +       tm->tm_hour = optee_tm->tm_hour;
> +       tm->tm_mday = optee_tm->tm_mday;
> +       tm->tm_mon = optee_tm->tm_mon;
> +       tm->tm_year = optee_tm->tm_year - 1900;
> +       tm->tm_wday = optee_tm->tm_wday;
> +       tm->tm_yday = rtc_year_days(tm->tm_mday, tm->tm_mon, tm->tm_year);
> +
> +       return 0;
> +}
> +
> +static int optee_rtc_settime(struct device *dev, struct rtc_time *tm)
> +{
> +       struct optee_rtc *priv = dev_get_drvdata(dev);
> +       struct tee_ioctl_invoke_arg inv_arg = {0};
> +       struct optee_rtc_time optee_tm;
> +       struct tee_param param[4] = {0};
> +       void *rtc_data;
> +       int ret = 0;
> +
> +       optee_tm.tm_sec = tm->tm_sec;
> +       optee_tm.tm_min = tm->tm_min;
> +       optee_tm.tm_hour = tm->tm_hour;
> +       optee_tm.tm_mday = tm->tm_mday;
> +       optee_tm.tm_mon = tm->tm_mon;
> +       optee_tm.tm_year = tm->tm_year + 1900;
> +       optee_tm.tm_wday = tm->tm_wday;
> +
> +       inv_arg.func = TA_CMD_RTC_SET_TIME;
> +       inv_arg.session = priv->session_id;
> +       inv_arg.num_params = 4;
> +
> +       param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT;
> +       param[0].u.memref.shm = priv->entropy_shm_pool;
> +       param[0].u.memref.size = sizeof(struct optee_rtc_time);
> +       param[0].u.memref.shm_offs = 0;
> +
> +       rtc_data = tee_shm_get_va(priv->entropy_shm_pool, 0);
> +       if (IS_ERR(rtc_data)) {
> +               dev_err(dev, "tee_shm_get_va failed\n");
> +               return PTR_ERR(rtc_data);
> +       }
> +
> +       memcpy(rtc_data, &optee_tm, sizeof(struct optee_rtc_time));
> +
> +       ret = tee_client_invoke_func(priv->ctx, &inv_arg, param);
> +       if ((ret < 0) || (inv_arg.ret != 0)) {
> +               dev_err(dev, "TA_CMD_RTC_GET_TIME invoke err: %x\n",
> +                       inv_arg.ret);
> +               return ret;

Ditto: 'return ret ? ret : -EPROTO;'

> +       }
> +
> +       return 0;
> +}
> +
> +static int optee_rtc_readoffset(struct device *dev, long *offset)
> +{
> +       struct optee_rtc *priv = dev_get_drvdata(dev);
> +       struct tee_ioctl_invoke_arg inv_arg = {0};
> +       struct tee_param param[4] = {0};
> +       int ret = 0;
> +
> +       if (!(priv->features & TA_RTC_FEATURE_CORRECTION))
> +               return -EOPNOTSUPP;
> +
> +       inv_arg.func = TA_CMD_RTC_GET_OFFSET;
> +       inv_arg.session = priv->session_id;
> +       inv_arg.num_params = 4;
> +
> +       param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT;
> +
> +       ret = tee_client_invoke_func(priv->ctx, &inv_arg, param);
> +       if ((ret < 0) || (inv_arg.ret != 0)) {
> +               dev_err(dev, "TA_CMD_RTC_GET_RNG_INFO invoke err: %x\n",
> +                       inv_arg.ret);
> +               return -EINVAL;
> +       }
> +
> +       *offset = param[0].u.value.a;
> +
> +       return 0;
> +}
> +
> +static int optee_rtc_setoffset(struct device *dev, long offset)
> +{
> +       struct optee_rtc *priv = dev_get_drvdata(dev);
> +       struct tee_ioctl_invoke_arg inv_arg = {0};
> +       struct tee_param param[4] = {0};
> +       int ret = 0;
> +
> +       if (!(priv->features & TA_RTC_FEATURE_CORRECTION))
> +               return -EOPNOTSUPP;
> +
> +       inv_arg.func = TA_CMD_RTC_SET_OFFSET;
> +       inv_arg.session = priv->session_id;
> +       inv_arg.num_params = 4;
> +
> +       param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT;
> +       param[0].u.value.a = offset;
> +
> +       ret = tee_client_invoke_func(priv->ctx, &inv_arg, param);
> +       if ((ret < 0) || (inv_arg.ret != 0)) {
> +               dev_err(dev, "TA_CMD_RTC_GET_RNG_INFO invoke err: %x\n",
> +                       inv_arg.ret);
> +               return -EINVAL;
> +       }
> +
> +       return 0;
> +}
> +static const struct rtc_class_ops optee_rtc_ops = {
> +       .read_time      = optee_rtc_readtime,
> +       .set_time       = optee_rtc_settime,
> +       .set_offset     = optee_rtc_setoffset,
> +       .read_offset    = optee_rtc_readoffset,
> +};
> +
> +static int optee_rtc_read_info(struct device *dev, struct rtc_device *rtc,
> +                              u64 *features)
> +{
> +       struct optee_rtc *priv = dev_get_drvdata(dev);
> +       struct tee_ioctl_invoke_arg inv_arg = {0};
> +       struct tee_param param[4] = {0};
> +       struct optee_rtc_info *info;
> +       struct optee_rtc_time *tm;
> +       int ret = 0;
> +
> +       inv_arg.func = TA_CMD_RTC_GET_INFO;
> +       inv_arg.session = priv->session_id;
> +       inv_arg.num_params = 4;
> +
> +       param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT;
> +       param[0].u.memref.shm = priv->entropy_shm_pool;
> +       param[0].u.memref.size = sizeof(*info);
> +       param[0].u.memref.shm_offs = 0;
> +
> +       ret = tee_client_invoke_func(priv->ctx, &inv_arg, param);
> +       if ((ret < 0) || (inv_arg.ret != 0)) {
> +               dev_err(dev, "TA_CMD_RTC_GET_RNG_INFO invoke err: %x\n",
> +                       inv_arg.ret);
> +               return -EINVAL;
> +       }
> +
> +       info = tee_shm_get_va(priv->entropy_shm_pool, 0);
> +       if (IS_ERR(info)) {
> +               dev_err(dev, "tee_shm_get_va failed\n");
> +               return PTR_ERR(info);
> +       }
> +
> +       if (param[0].u.memref.size < sizeof(*info)) {
> +               dev_err(dev, "Invalid read size from OPTEE\n");
> +               return -EINVAL;

This is also a protocol error. Prefer -EPROTO.

> +       }
> +
> +       if (info->version != RTC_INFO_VERSION) {
> +               dev_err(dev, "Unsupported information version %llu\n",
> +                             info->version);
> +               return -EINVAL;
> +       }
> +
> +       *features = info->features;
> +
> +       tm = &info->range_min;
> +       rtc->range_min = mktime64(tm->tm_year, tm->tm_mon, tm->tm_mday,
> +                                 tm->tm_hour, tm->tm_min, tm->tm_sec);
> +       tm = &info->range_max;
> +       rtc->range_max = mktime64(tm->tm_year, tm->tm_mon, tm->tm_mday,
> +                                 tm->tm_hour, tm->tm_min, tm->tm_sec);
> +
> +       return 0;
> +}
> +
> +static int optee_ctx_match(struct tee_ioctl_version_data *ver, const void *data)
> +{
> +       if (ver->impl_id == TEE_IMPL_ID_OPTEE)
> +               return 1;
> +       else
> +               return 0;
> +}
> +
> +static int optee_rtc_probe(struct device *dev)
> +{
> +       struct tee_client_device *rtc_device = to_tee_client_device(dev);
> +       struct tee_ioctl_open_session_arg sess_arg;
> +       struct tee_shm *entropy_shm_pool = NULL;
> +       int ret = 0, err = -ENODEV;
> +       struct optee_rtc *priv;
> +       struct rtc_device *rtc;
> +
> +       memset(&sess_arg, 0, sizeof(sess_arg));
> +
> +       priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +       if (!priv)
> +               return -ENOMEM;
> +
> +       rtc = devm_rtc_allocate_device(dev);
> +       if (IS_ERR(rtc))
> +               return PTR_ERR(rtc);
> +
> +       /* Open context with TEE driver */
> +       priv->ctx = tee_client_open_context(NULL, optee_ctx_match, NULL,
> +                                              NULL);

Fits in 100 chars. You could get rid of few line breaks in this change.

> +       if (IS_ERR(priv->ctx))
> +               return -ENODEV;
> +
> +       /* Open session with rtc Trusted App */
> +       memcpy(sess_arg.uuid, rtc_device->id.uuid.b, TEE_IOCTL_UUID_LEN);
> +       sess_arg.clnt_login = TEE_IOCTL_LOGIN_REE_KERNEL;
> +       sess_arg.num_params = 0;

not needed.

> +
> +       ret = tee_client_open_session(priv->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);
> +               err = -EINVAL;
> +               goto out_ctx;
> +       }
> +       priv->session_id = sess_arg.session;
> +
> +       entropy_shm_pool = tee_shm_alloc(priv->ctx,
> +                                        sizeof(struct optee_rtc_info),
> +                                        TEE_SHM_MAPPED | TEE_SHM_DMA_BUF);
> +       if (IS_ERR(entropy_shm_pool)) {
> +               dev_err(priv->dev, "tee_shm_alloc failed\n");
> +               err = PTR_ERR(entropy_shm_pool);
> +               goto out_sess;
> +       }
> +
> +       priv->entropy_shm_pool = entropy_shm_pool;
> +       priv->dev = dev;
> +       dev_set_drvdata(dev, priv);
> +
> +       rtc->ops = &optee_rtc_ops;
> +
> +       err = optee_rtc_read_info(dev, rtc, &priv->features);
> +       if (err) {
> +               dev_err(dev, "Failed to get RTC features from OP-TEE\n");
> +               goto out_sess;

Should release the allocated shared memory on exit.

> +       }
> +
> +       err = devm_rtc_register_device(rtc);
> +       if (err)
> +               goto out_sess;

Ditto.

> +
> +       /*
> +        * We must clear this bit after registering because rtc_register_device
> +        * will set it if it sees that .set_offset is provided.
> +        */
> +       if (!(priv->features & TA_RTC_FEATURE_CORRECTION))
> +               clear_bit(RTC_FEATURE_CORRECTION, rtc->features);
> +
> +       return 0;
> +
> +out_sess:
> +       tee_client_close_session(priv->ctx, priv->session_id);
> +out_ctx:
> +       tee_client_close_context(priv->ctx);
> +
> +       return err;
> +}
> +
> +static int optee_rtc_remove(struct device *dev)
> +{
> +       struct optee_rtc *priv = dev_get_drvdata(dev);
> +
> +       tee_client_close_session(priv->ctx, priv->session_id);
> +       tee_client_close_context(priv->ctx);
> +
> +       return 0;
> +}
> +
> +static const struct tee_client_device_id optee_rtc_id_table[] = {
> +       {UUID_INIT(0xf389f8c8, 0x845f, 0x496c,
> +                  0x8b, 0xbe, 0xd6, 0x4b, 0xd2, 0x4c, 0x92, 0xfd)},
> +       {}
> +};
> +
> +MODULE_DEVICE_TABLE(tee, optee_rtc_id_table);
> +
> +static struct tee_client_driver optee_rtc_driver = {
> +       .id_table       = optee_rtc_id_table,
> +       .driver         = {
> +               .name           = "optee_rtc",
> +               .bus            = &tee_bus_type,
> +               .probe          = optee_rtc_probe,
> +               .remove         = optee_rtc_remove,
> +       },
> +};
> +
> +static int __init optee_rtc_mod_init(void)
> +{
> +       return driver_register(&optee_rtc_driver.driver);
> +}
> +
> +static void __exit optee_rtc_mod_exit(void)
> +{
> +       driver_unregister(&optee_rtc_driver.driver);
> +}
> +
> +module_init(optee_rtc_mod_init);
> +module_exit(optee_rtc_mod_exit);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR("Clément Léger <clement.leger@bootlin.com>");
> +MODULE_DESCRIPTION("OP-TEE based RTC driver");
> --
> 2.34.1
>

Re: [PATCH] rtc: optee: add RTC driver for OP-TEE RTC PTA

[Clément Léger]

Le Sun, 6 Mar 2022 22:08:48 +0100,
Etienne Carriere <etienne.carriere@linaro.org> a écrit :

> Hello Clement,
> 
> In the code, rtc-optee driver opens a session with optee and allocates
> shared memory from probe to remove time.
> This uses resources in the TEE but RTC is not accessed often by Linux I guess.
> Would it be interesting if a session is opened and shared memory
> allocated only when a RPC command is to be invoked?

Hi Etienne, that is an idea that could be added later I guess ?

> 
> Minor comments below.

Thanks for the review.

-- 
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com