From: Sumit Garg <sumit.garg@oss.qualcomm.com>

Add support for Peripheral Authentication Service (PAS) driver based
on TEE bus with OP-TEE providing the backend PAS service implementation.

The TEE PAS service ABI is designed to be extensible with additional API
as PTA_QCOM_PAS_CAPABILITIES. This allows to accommodate any future
extensions of the PAS service needed while still maintaining backwards
compatibility.

Signed-off-by: Sumit Garg <sumit.garg@oss.qualcomm.com>
---
 drivers/firmware/qcom/Kconfig        |  10 +
 drivers/firmware/qcom/Makefile       |   1 +
 drivers/firmware/qcom/qcom_pas_tee.c | 478 +++++++++++++++++++++++++++
 3 files changed, 489 insertions(+)
 create mode 100644 drivers/firmware/qcom/qcom_pas_tee.c

diff --git a/drivers/firmware/qcom/Kconfig b/drivers/firmware/qcom/Kconfig
index 9a12ae2b639d..300b3a1bd178 100644
--- a/drivers/firmware/qcom/Kconfig
+++ b/drivers/firmware/qcom/Kconfig
@@ -14,6 +14,16 @@ config QCOM_PAS
 	  backends plugged in whether it's an SCM implementation or a proper
 	  TEE bus based PAS service implementation.
 
+config QCOM_PAS_TEE
+	tristate
+	select QCOM_PAS
+	depends on TEE
+	depends on !CPU_BIG_ENDIAN
+	default m if ARCH_QCOM
+	help
+	  Enable the generic Peripheral Authentication Service (PAS) provided
+	  by the firmware TEE implementation as the backend.
+
 config QCOM_SCM
 	select QCOM_PAS
 	select QCOM_TZMEM
diff --git a/drivers/firmware/qcom/Makefile b/drivers/firmware/qcom/Makefile
index dc5ab45f906a..48801d18f37b 100644
--- a/drivers/firmware/qcom/Makefile
+++ b/drivers/firmware/qcom/Makefile
@@ -9,3 +9,4 @@ obj-$(CONFIG_QCOM_TZMEM)	+= qcom_tzmem.o
 obj-$(CONFIG_QCOM_QSEECOM)	+= qcom_qseecom.o
 obj-$(CONFIG_QCOM_QSEECOM_UEFISECAPP) += qcom_qseecom_uefisecapp.o
 obj-$(CONFIG_QCOM_PAS)		+= qcom_pas.o
+obj-$(CONFIG_QCOM_PAS_TEE)	+= qcom_pas_tee.o
diff --git a/drivers/firmware/qcom/qcom_pas_tee.c b/drivers/firmware/qcom/qcom_pas_tee.c
new file mode 100644
index 000000000000..d63122c34f04
--- /dev/null
+++ b/drivers/firmware/qcom/qcom_pas_tee.c
@@ -0,0 +1,478 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
+ */
+
+#include <linux/delay.h>
+#include <linux/of.h>
+#include <linux/firmware/qcom/qcom_pas.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/tee_drv.h>
+#include <linux/uuid.h>
+
+#include "qcom_pas.h"
+
+/*
+ * Peripheral Authentication Service (PAS) supported.
+ *
+ * [in]  params[0].value.a:	Unique 32bit remote processor identifier
+ */
+#define TA_QCOM_PAS_IS_SUPPORTED		1
+
+/*
+ * PAS capabilities.
+ *
+ * [in]  params[0].value.a:	Unique 32bit remote processor identifier
+ * [out] params[1].value.a:	PAS capability flags
+ */
+#define TA_QCOM_PAS_CAPABILITIES		2
+
+/*
+ * PAS image initialization.
+ *
+ * [in]  params[0].value.a:	Unique 32bit remote processor identifier
+ * [in]  params[1].memref:	Loadable firmware metadata
+ */
+#define TA_QCOM_PAS_INIT_IMAGE			3
+
+/*
+ * PAS memory setup.
+ *
+ * [in]  params[0].value.a:	Unique 32bit remote processor identifier
+ * [in]  params[0].value.b:	Relocatable firmware size
+ * [in]  params[1].value.a:	32bit LSB relocatable firmware memory address
+ * [in]  params[1].value.b:	32bit MSB relocatable firmware memory address
+ */
+#define TA_QCOM_PAS_MEM_SETUP			4
+
+/*
+ * PAS get resource table.
+ *
+ * [in]     params[0].value.a:	Unique 32bit remote processor identifier
+ * [inout]  params[1].memref:	Resource table config
+ */
+#define TA_QCOM_PAS_GET_RESOURCE_TABLE		5
+
+/*
+ * PAS image authentication and co-processor reset.
+ *
+ * [in]  params[0].value.a:	Unique 32bit remote processor identifier
+ * [in]  params[0].value.b:	Firmware size
+ * [in]  params[1].value.a:	32bit LSB firmware memory address
+ * [in]  params[1].value.b:	32bit MSB firmware memory address
+ * [in]  params[2].memref:	Optional fw memory space shared/lent
+ */
+#define TA_QCOM_PAS_AUTH_AND_RESET		6
+
+/*
+ * PAS co-processor set suspend/resume state.
+ *
+ * [in]  params[0].value.a:	Unique 32bit remote processor identifier
+ * [in]  params[0].value.b:	Co-processor state identifier
+ */
+#define TA_QCOM_PAS_SET_REMOTE_STATE		7
+
+/*
+ * PAS co-processor shutdown.
+ *
+ * [in]  params[0].value.a:	Unique 32bit remote processor identifier
+ */
+#define TA_QCOM_PAS_SHUTDOWN			8
+
+#define TEE_NUM_PARAMS				4
+
+/**
+ * struct qcom_pas_tee_private - PAS service private data
+ * @dev:		PAS service device.
+ * @ctx:		TEE context handler.
+ * @session_id:		PAS TA session identifier.
+ */
+struct qcom_pas_tee_private {
+	struct device *dev;
+	struct tee_context *ctx;
+	u32 session_id;
+};
+
+static bool qcom_pas_tee_supported(struct device *dev, u32 pas_id)
+{
+	struct qcom_pas_tee_private *data = dev_get_drvdata(dev);
+	struct tee_ioctl_invoke_arg inv_arg = {
+		.func = TA_QCOM_PAS_IS_SUPPORTED,
+		.session = data->session_id,
+		.num_params = TEE_NUM_PARAMS
+	};
+	struct tee_param param[4] = {
+		[0] = {
+			.attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT,
+			.u.value.a = pas_id
+		}
+	};
+	int ret;
+
+	ret = tee_client_invoke_func(data->ctx, &inv_arg, param);
+	if (ret < 0 || inv_arg.ret != 0) {
+		dev_err(dev, "PAS not supported, pas_id: %d, err: %x\n",
+			pas_id, inv_arg.ret);
+		return false;
+	}
+
+	return true;
+}
+
+static int qcom_pas_tee_init_image(struct device *dev, u32 pas_id,
+				   const void *metadata, size_t size,
+				   struct qcom_pas_context *ctx)
+{
+	struct qcom_pas_tee_private *data = dev_get_drvdata(dev);
+	struct tee_ioctl_invoke_arg inv_arg = {
+		.func = TA_QCOM_PAS_INIT_IMAGE,
+		.session = data->session_id,
+		.num_params = TEE_NUM_PARAMS
+	};
+	struct tee_param param[4] = {
+		[0] = {
+			.attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT,
+			.u.value.a = pas_id
+		},
+		[1] = {
+			.attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT,
+		}
+	};
+	struct tee_shm *mdata_shm;
+	u8 *mdata_buf = NULL;
+	int ret;
+
+	if (!ctx)
+		return -EINVAL;
+
+	mdata_shm = tee_shm_alloc_kernel_buf(data->ctx, size);
+	if (IS_ERR(mdata_shm)) {
+		dev_err(dev, "mdata_shm allocation failed\n");
+		return PTR_ERR(mdata_shm);
+	}
+
+	mdata_buf = tee_shm_get_va(mdata_shm, 0);
+	if (IS_ERR(mdata_buf)) {
+		dev_err(dev, "mdata_buf get VA failed\n");
+		tee_shm_free(mdata_shm);
+		return PTR_ERR(mdata_buf);
+	}
+	memcpy(mdata_buf, metadata, size);
+
+	param[1].u.memref.shm = mdata_shm;
+	param[1].u.memref.size = size;
+
+	ret = tee_client_invoke_func(data->ctx, &inv_arg, param);
+	if (ret < 0 || inv_arg.ret != 0) {
+		dev_err(dev, "PAS init image failed, pas_id: %d, err: %x\n",
+			pas_id, inv_arg.ret);
+		tee_shm_free(mdata_shm);
+		return -EINVAL;
+	}
+	ctx->ptr = (void *)mdata_shm;
+
+	return 0;
+}
+
+static int qcom_pas_tee_mem_setup(struct device *dev, u32 pas_id,
+				  phys_addr_t addr, phys_addr_t size)
+{
+	struct qcom_pas_tee_private *data = dev_get_drvdata(dev);
+	struct tee_ioctl_invoke_arg inv_arg = {
+		.func = TA_QCOM_PAS_MEM_SETUP,
+		.session = data->session_id,
+		.num_params = TEE_NUM_PARAMS
+	};
+	struct tee_param param[4] = {
+		[0] = {
+			.attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT,
+			.u.value.a = pas_id,
+			.u.value.b = size,
+		},
+		[1] = {
+			.attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT,
+			.u.value.a = lower_32_bits(addr),
+			.u.value.b = upper_32_bits(addr),
+		}
+	};
+	int ret;
+
+	ret = tee_client_invoke_func(data->ctx, &inv_arg, param);
+	if (ret < 0 || inv_arg.ret != 0) {
+		dev_err(dev, "PAS mem setup failed, pas_id: %d, err: %x\n",
+			pas_id, inv_arg.ret);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+DEFINE_FREE(shm_free, struct tee_shm *, tee_shm_free(_T))
+
+static void *qcom_pas_tee_get_rsc_table(struct device *dev,
+					struct qcom_pas_context *ctx,
+					void *input_rt, size_t input_rt_size,
+					size_t *output_rt_size)
+{
+	struct qcom_pas_tee_private *data = dev_get_drvdata(dev);
+	struct tee_ioctl_invoke_arg inv_arg = {
+		.func = TA_QCOM_PAS_GET_RESOURCE_TABLE,
+		.session = data->session_id,
+		.num_params = TEE_NUM_PARAMS
+	};
+	struct tee_param param[4] = {
+		[0] = {
+			.attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT,
+			.u.value.a = ctx->pas_id,
+		},
+		[1] = {
+			.attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT,
+			.u.memref.size = input_rt_size,
+		}
+	};
+	void *rt_buf = NULL;
+	int ret;
+
+	ret = tee_client_invoke_func(data->ctx, &inv_arg, param);
+	if (ret < 0 || inv_arg.ret != 0) {
+		dev_err(dev, "PAS get RT failed, pas_id: %d, err: %x\n",
+			ctx->pas_id, inv_arg.ret);
+		return ERR_PTR(-EINVAL);
+	}
+
+	if (param[1].u.memref.size) {
+		struct tee_shm *rt_shm __free(shm_free) =
+			tee_shm_alloc_kernel_buf(data->ctx,
+						 param[1].u.memref.size);
+		void *rt_shm_va;
+
+		if (IS_ERR(rt_shm)) {
+			dev_err(dev, "rt_shm allocation failed\n");
+			return rt_shm;
+		}
+
+		rt_shm_va = tee_shm_get_va(rt_shm, 0);
+		if (IS_ERR_OR_NULL(rt_shm_va)) {
+			dev_err(dev, "rt_shm get VA failed\n");
+			return ERR_PTR(-EINVAL);
+		}
+		memcpy(rt_shm_va, input_rt, input_rt_size);
+
+		param[1].u.memref.shm = rt_shm;
+		ret = tee_client_invoke_func(data->ctx, &inv_arg, param);
+		if (ret < 0 || inv_arg.ret != 0) {
+			dev_err(dev, "PAS get RT failed, pas_id: %d, err: %x\n",
+				ctx->pas_id, inv_arg.ret);
+			return ERR_PTR(-EINVAL);
+		}
+
+		if (param[1].u.memref.size) {
+			*output_rt_size = param[1].u.memref.size;
+			rt_buf = kmemdup(rt_shm_va, *output_rt_size, GFP_KERNEL);
+			if (!rt_buf)
+				return ERR_PTR(-ENOMEM);
+		}
+	}
+
+	return rt_buf;
+}
+
+static int __qcom_pas_tee_auth_and_reset(struct device *dev, u32 pas_id,
+					 phys_addr_t mem_phys, size_t mem_size)
+{
+	struct qcom_pas_tee_private *data = dev_get_drvdata(dev);
+	struct tee_ioctl_invoke_arg inv_arg = {
+		.func = TA_QCOM_PAS_AUTH_AND_RESET,
+		.session = data->session_id,
+		.num_params = TEE_NUM_PARAMS
+	};
+	struct tee_param param[4] = {
+		[0] = {
+			.attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT,
+			.u.value.a = pas_id,
+			.u.value.b = mem_size,
+		},
+		[1] = {
+			.attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT,
+			.u.value.a = lower_32_bits(mem_phys),
+			.u.value.b = upper_32_bits(mem_phys),
+		},
+		/* Reserved for fw memory space to be shared or lent */
+		[2] = {
+			.attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT,
+		}
+	};
+	int ret;
+
+	ret = tee_client_invoke_func(data->ctx, &inv_arg, param);
+	if (ret < 0 || inv_arg.ret != 0) {
+		dev_err(dev, "PAS auth reset failed, pas_id: %d, err: %x\n",
+			pas_id, inv_arg.ret);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int qcom_pas_tee_auth_and_reset(struct device *dev, u32 pas_id)
+{
+	return __qcom_pas_tee_auth_and_reset(dev, pas_id, 0, 0);
+}
+
+static int qcom_pas_tee_prepare_and_auth_reset(struct device *dev,
+					       struct qcom_pas_context *ctx)
+{
+	return __qcom_pas_tee_auth_and_reset(dev, ctx->pas_id, ctx->mem_phys,
+					     ctx->mem_size);
+}
+
+static int qcom_pas_tee_set_remote_state(struct device *dev, u32 state,
+					 u32 pas_id)
+{
+	struct qcom_pas_tee_private *data = dev_get_drvdata(dev);
+	struct tee_ioctl_invoke_arg inv_arg = {
+		.func = TA_QCOM_PAS_SET_REMOTE_STATE,
+		.session = data->session_id,
+		.num_params = TEE_NUM_PARAMS
+	};
+	struct tee_param param[4] = {
+		[0] = {
+			.attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT,
+			.u.value.a = pas_id,
+			.u.value.b = state,
+		}
+	};
+	int ret;
+
+	ret = tee_client_invoke_func(data->ctx, &inv_arg, param);
+	if (ret < 0 || inv_arg.ret != 0) {
+		dev_err(dev, "PAS shutdown failed, pas_id: %d, err: %x\n",
+			pas_id, inv_arg.ret);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int qcom_pas_tee_shutdown(struct device *dev, u32 pas_id)
+{
+	struct qcom_pas_tee_private *data = dev_get_drvdata(dev);
+	struct tee_ioctl_invoke_arg inv_arg = {
+		.func = TA_QCOM_PAS_SHUTDOWN,
+		.session = data->session_id,
+		.num_params = TEE_NUM_PARAMS
+	};
+	struct tee_param param[4] = {
+		[0] = {
+			.attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT,
+			.u.value.a = pas_id
+		}
+	};
+	int ret = 0;
+
+	ret = tee_client_invoke_func(data->ctx, &inv_arg, param);
+	if (ret < 0 || inv_arg.ret != 0) {
+		dev_err(dev, "PAS shutdown failed, pas_id: %d, err: %x\n",
+			pas_id, inv_arg.ret);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static void qcom_pas_tee_metadata_release(struct device *dev,
+					  struct qcom_pas_context *ctx)
+{
+	struct tee_shm *mdata_shm = ctx->ptr;
+
+	tee_shm_free(mdata_shm);
+}
+
+static struct qcom_pas_ops qcom_pas_ops_tee = {
+	.drv_name		= "qcom-pas-tee",
+	.supported		= qcom_pas_tee_supported,
+	.init_image		= qcom_pas_tee_init_image,
+	.mem_setup		= qcom_pas_tee_mem_setup,
+	.get_rsc_table		= qcom_pas_tee_get_rsc_table,
+	.auth_and_reset		= qcom_pas_tee_auth_and_reset,
+	.prepare_and_auth_reset	= qcom_pas_tee_prepare_and_auth_reset,
+	.set_remote_state	= qcom_pas_tee_set_remote_state,
+	.shutdown		= qcom_pas_tee_shutdown,
+	.metadata_release	= qcom_pas_tee_metadata_release,
+};
+
+static int optee_ctx_match(struct tee_ioctl_version_data *ver, const void *data)
+{
+	return ver->impl_id == TEE_IMPL_ID_OPTEE;
+}
+
+static int qcom_pas_tee_probe(struct tee_client_device *pas_dev)
+{
+	struct device *dev = &pas_dev->dev;
+	struct qcom_pas_tee_private *data;
+	struct tee_ioctl_open_session_arg sess_arg = {
+		.clnt_login = TEE_IOCTL_LOGIN_REE_KERNEL
+	};
+	int ret, err = -ENODEV;
+
+	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->ctx = tee_client_open_context(NULL, optee_ctx_match, NULL, NULL);
+	if (IS_ERR(data->ctx))
+		return -ENODEV;
+
+	export_uuid(sess_arg.uuid, &pas_dev->id.uuid);
+	ret = tee_client_open_session(data->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;
+	}
+
+	data->session_id = sess_arg.session;
+	dev_set_drvdata(dev, data);
+	qcom_pas_ops_tee.dev = dev;
+	qcom_pas_ops_register(&qcom_pas_ops_tee);
+
+	return 0;
+out_ctx:
+	tee_client_close_context(data->ctx);
+
+	return err;
+}
+
+static void qcom_pas_tee_remove(struct tee_client_device *pas_dev)
+{
+	struct device *dev = &pas_dev->dev;
+	struct qcom_pas_tee_private *data = dev_get_drvdata(dev);
+
+	qcom_pas_ops_unregister();
+	tee_client_close_session(data->ctx, data->session_id);
+	tee_client_close_context(data->ctx);
+}
+
+static const struct tee_client_device_id qcom_pas_tee_id_table[] = {
+	{UUID_INIT(0xcff7d191, 0x7ca0, 0x4784,
+		   0xaf, 0x13, 0x48, 0x22, 0x3b, 0x9a, 0x4f, 0xbe)},
+	{}
+};
+MODULE_DEVICE_TABLE(tee, qcom_pas_tee_id_table);
+
+static struct tee_client_driver optee_pas_tee_driver = {
+	.probe		= qcom_pas_tee_probe,
+	.remove		= qcom_pas_tee_remove,
+	.id_table	= qcom_pas_tee_id_table,
+	.driver		= {
+		.name		= "qcom-pas-tee",
+	},
+};
+
+module_tee_client_driver(optee_pas_tee_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("TEE bus based Qualcomm PAS driver");
-- 
2.51.0

On 3/27/2026 6:40 PM, Sumit Garg wrote:
> From: Sumit Garg <sumit.garg@oss.qualcomm.com>
> 
> Add support for Peripheral Authentication Service (PAS) driver based
> on TEE bus with OP-TEE providing the backend PAS service implementation.
> 
> The TEE PAS service ABI is designed to be extensible with additional API
> as PTA_QCOM_PAS_CAPABILITIES. This allows to accommodate any future
> extensions of the PAS service needed while still maintaining backwards
> compatibility.
> 
> Signed-off-by: Sumit Garg <sumit.garg@oss.qualcomm.com>
> ---
>  drivers/firmware/qcom/Kconfig        |  10 +
>  drivers/firmware/qcom/Makefile       |   1 +
>  drivers/firmware/qcom/qcom_pas_tee.c | 478 +++++++++++++++++++++++++++
>  3 files changed, 489 insertions(+)
>  create mode 100644 drivers/firmware/qcom/qcom_pas_tee.c
>
[...]

> diff --git a/drivers/firmware/qcom/qcom_pas_tee.c b/drivers/firmware/qcom/qcom_pas_tee.c
> new file mode 100644
> index 000000000000..d63122c34f04
> --- /dev/null
> +++ b/drivers/firmware/qcom/qcom_pas_tee.c
> @@ -0,0 +1,478 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/of.h>
> +#include <linux/firmware/qcom/qcom_pas.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/tee_drv.h>
> +#include <linux/uuid.h>
> +
> +#include "qcom_pas.h"
> +
> +/*
> + * Peripheral Authentication Service (PAS) supported.
> + *
> + * [in]  params[0].value.a:	Unique 32bit remote processor identifier
> + */
> +#define TA_QCOM_PAS_IS_SUPPORTED		1
> +
> +/*
> + * PAS capabilities.
> + *
> + * [in]  params[0].value.a:	Unique 32bit remote processor identifier
> + * [out] params[1].value.a:	PAS capability flags
> + */
> +#define TA_QCOM_PAS_CAPABILITIES		2
> +
> +/*
> + * PAS image initialization.
> + *
> + * [in]  params[0].value.a:	Unique 32bit remote processor identifier
> + * [in]  params[1].memref:	Loadable firmware metadata
> + */
> +#define TA_QCOM_PAS_INIT_IMAGE			3
> +
> +/*
> + * PAS memory setup.
> + *
> + * [in]  params[0].value.a:	Unique 32bit remote processor identifier
> + * [in]  params[0].value.b:	Relocatable firmware size
> + * [in]  params[1].value.a:	32bit LSB relocatable firmware memory address
> + * [in]  params[1].value.b:	32bit MSB relocatable firmware memory address
> + */
> +#define TA_QCOM_PAS_MEM_SETUP			4
> +
> +/*
> + * PAS get resource table.
> + *
> + * [in]     params[0].value.a:	Unique 32bit remote processor identifier
> + * [inout]  params[1].memref:	Resource table config
> + */
> +#define TA_QCOM_PAS_GET_RESOURCE_TABLE		5
> +
> +/*
> + * PAS image authentication and co-processor reset.
> + *
> + * [in]  params[0].value.a:	Unique 32bit remote processor identifier
> + * [in]  params[0].value.b:	Firmware size
> + * [in]  params[1].value.a:	32bit LSB firmware memory address
> + * [in]  params[1].value.b:	32bit MSB firmware memory address
> + * [in]  params[2].memref:	Optional fw memory space shared/lent
> + */
> +#define TA_QCOM_PAS_AUTH_AND_RESET		6
> +
> +/*
> + * PAS co-processor set suspend/resume state.
> + *
> + * [in]  params[0].value.a:	Unique 32bit remote processor identifier
> + * [in]  params[0].value.b:	Co-processor state identifier
> + */
> +#define TA_QCOM_PAS_SET_REMOTE_STATE		7
> +
> +/*
> + * PAS co-processor shutdown.
> + *
> + * [in]  params[0].value.a:	Unique 32bit remote processor identifier
> + */
> +#define TA_QCOM_PAS_SHUTDOWN			8
> +
> +#define TEE_NUM_PARAMS				4
> +
> +/**
> + * struct qcom_pas_tee_private - PAS service private data
> + * @dev:		PAS service device.
> + * @ctx:		TEE context handler.
> + * @session_id:		PAS TA session identifier.

Nit: Column alignment of comment descriptions.

> + */
> +struct qcom_pas_tee_private {
> +	struct device *dev;
> +	struct tee_context *ctx;
> +	u32 session_id;
> +};
> +
> +static bool qcom_pas_tee_supported(struct device *dev, u32 pas_id)
> +{
> +	struct qcom_pas_tee_private *data = dev_get_drvdata(dev);
> +	struct tee_ioctl_invoke_arg inv_arg = {
> +		.func = TA_QCOM_PAS_IS_SUPPORTED,
> +		.session = data->session_id,
> +		.num_params = TEE_NUM_PARAMS
> +	};
> +	struct tee_param param[4] = {
> +		[0] = {
> +			.attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT,
> +			.u.value.a = pas_id
> +		}
> +	};
> +	int ret;
> +
> +	ret = tee_client_invoke_func(data->ctx, &inv_arg, param);
> +	if (ret < 0 || inv_arg.ret != 0) {
> +		dev_err(dev, "PAS not supported, pas_id: %d, err: %x\n",
> +			pas_id, inv_arg.ret);

I can see that similar error handling pattern for tee_client_invoke_func() is
being done by other clients. But it seems that this error log doesn't really convey
whether we failed before or after entering OPTEE (or some other TEE) for invoking
the service.

Could be better if we print 'ret' when ret < 0. And print 'inv_arg.ret' when
inv_arg.ret != 0. So for example, if the param marshalling fails, or some other
issue is encountered when processing the params in a different back-end TEE
driver, we could know from the logs.

> +		return false;
> +	}
> +
> +	return true;
> +}
> +
> +static int qcom_pas_tee_init_image(struct device *dev, u32 pas_id,
> +				   const void *metadata, size_t size,
> +				   struct qcom_pas_context *ctx)
> +{
> +	struct qcom_pas_tee_private *data = dev_get_drvdata(dev);
> +	struct tee_ioctl_invoke_arg inv_arg = {
> +		.func = TA_QCOM_PAS_INIT_IMAGE,
> +		.session = data->session_id,
> +		.num_params = TEE_NUM_PARAMS
> +	};
> +	struct tee_param param[4] = {
> +		[0] = {
> +			.attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT,
> +			.u.value.a = pas_id
> +		},
> +		[1] = {
> +			.attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT,
> +		}
> +	};
> +	struct tee_shm *mdata_shm;
> +	u8 *mdata_buf = NULL;
> +	int ret;
> +
> +	if (!ctx)
> +		return -EINVAL;
> +
> +	mdata_shm = tee_shm_alloc_kernel_buf(data->ctx, size);

Why not move the DEFINE_FREE() above this function so we can use scoped free
here as well for mdata_shm?

struct tee_shm *mdata_shm __free(shm_free) = ...

> +	if (IS_ERR(mdata_shm)) {
> +		dev_err(dev, "mdata_shm allocation failed\n");
> +		return PTR_ERR(mdata_shm);
> +	}
> +
> +	mdata_buf = tee_shm_get_va(mdata_shm, 0);
> +	if (IS_ERR(mdata_buf)) {
> +		dev_err(dev, "mdata_buf get VA failed\n");
> +		tee_shm_free(mdata_shm);
> +		return PTR_ERR(mdata_buf);
> +	}
> +	memcpy(mdata_buf, metadata, size);
> +
> +	param[1].u.memref.shm = mdata_shm;
> +	param[1].u.memref.size = size;
> +
> +	ret = tee_client_invoke_func(data->ctx, &inv_arg, param);
> +	if (ret < 0 || inv_arg.ret != 0) {
> +		dev_err(dev, "PAS init image failed, pas_id: %d, err: %x\n",

Nit: We can prefix %x with 0x. "err: 0x%x\n."

> +			pas_id, inv_arg.ret);
> +		tee_shm_free(mdata_shm);
> +		return -EINVAL;
> +	}
> +	ctx->ptr = (void *)mdata_shm;
> +
> +	return 0;
> +}
> +
> +static int qcom_pas_tee_mem_setup(struct device *dev, u32 pas_id,
> +				  phys_addr_t addr, phys_addr_t size)
> +{
> +	struct qcom_pas_tee_private *data = dev_get_drvdata(dev);
> +	struct tee_ioctl_invoke_arg inv_arg = {
> +		.func = TA_QCOM_PAS_MEM_SETUP,
> +		.session = data->session_id,
> +		.num_params = TEE_NUM_PARAMS
> +	};
> +	struct tee_param param[4] = {
> +		[0] = {
> +			.attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT,
> +			.u.value.a = pas_id,
> +			.u.value.b = size,
> +		},
> +		[1] = {
> +			.attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT,
> +			.u.value.a = lower_32_bits(addr),
> +			.u.value.b = upper_32_bits(addr),
> +		}
> +	};
> +	int ret;
> +
> +	ret = tee_client_invoke_func(data->ctx, &inv_arg, param);
> +	if (ret < 0 || inv_arg.ret != 0) {
> +		dev_err(dev, "PAS mem setup failed, pas_id: %d, err: %x\n",
> +			pas_id, inv_arg.ret);
> +		return -EINVAL;

I can see that originally in-case of error, qcom_scm_pas_mem_setup() bubbles
up the return value from qcom_scm_call(). Perhaps we should do the same as well,
return ret here instead of hard-coded '-EINVAL' which overrides any useful
return values returned from the back-end TEE driver.
 
> +	}
> +
> +	return 0;
> +}
> +
> +DEFINE_FREE(shm_free, struct tee_shm *, tee_shm_free(_T))
> +
> +static void *qcom_pas_tee_get_rsc_table(struct device *dev,
> +					struct qcom_pas_context *ctx,
> +					void *input_rt, size_t input_rt_size,
> +					size_t *output_rt_size)
> +{
> +	struct qcom_pas_tee_private *data = dev_get_drvdata(dev);
> +	struct tee_ioctl_invoke_arg inv_arg = {
> +		.func = TA_QCOM_PAS_GET_RESOURCE_TABLE,
> +		.session = data->session_id,
> +		.num_params = TEE_NUM_PARAMS
> +	};
> +	struct tee_param param[4] = {
> +		[0] = {
> +			.attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT,
> +			.u.value.a = ctx->pas_id,
> +		},
> +		[1] = {
> +			.attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT,
> +			.u.memref.size = input_rt_size,
> +		}
> +	};
> +	void *rt_buf = NULL;
> +	int ret;
> +
> +	ret = tee_client_invoke_func(data->ctx, &inv_arg, param);
> +	if (ret < 0 || inv_arg.ret != 0) {
> +		dev_err(dev, "PAS get RT failed, pas_id: %d, err: %x\n",
> +			ctx->pas_id, inv_arg.ret);
> +		return ERR_PTR(-EINVAL);

Same comment here, we could return ERR_PTR(ret) instead of overriding to
'-EINVAL'.

> +	}
> +
> +	if (param[1].u.memref.size) {
> +		struct tee_shm *rt_shm __free(shm_free) =
> +			tee_shm_alloc_kernel_buf(data->ctx,
> +						 param[1].u.memref.size);
> +		void *rt_shm_va;
> +
> +		if (IS_ERR(rt_shm)) {
> +			dev_err(dev, "rt_shm allocation failed\n");
> +			return rt_shm;
> +		}
> +
> +		rt_shm_va = tee_shm_get_va(rt_shm, 0);
> +		if (IS_ERR_OR_NULL(rt_shm_va)) {
> +			dev_err(dev, "rt_shm get VA failed\n");
> +			return ERR_PTR(-EINVAL);

Why not just return rt_shm_va? It already encodes the error in the pointer.

> +		}
> +		memcpy(rt_shm_va, input_rt, input_rt_size);
> +
> +		param[1].u.memref.shm = rt_shm;
> +		ret = tee_client_invoke_func(data->ctx, &inv_arg, param);
> +		if (ret < 0 || inv_arg.ret != 0) {
> +			dev_err(dev, "PAS get RT failed, pas_id: %d, err: %x\n",
> +				ctx->pas_id, inv_arg.ret);
> +			return ERR_PTR(-EINVAL);

Same comment.

> +		}
> +
> +		if (param[1].u.memref.size) {

Is it possible for param[1].u.memref.size to be 0 after a successful tee_client_invoke_func()?

> +			*output_rt_size = param[1].u.memref.size;
> +			rt_buf = kmemdup(rt_shm_va, *output_rt_size, GFP_KERNEL);
> +			if (!rt_buf)
> +				return ERR_PTR(-ENOMEM);
> +		}
> +	}
> +
> +	return rt_buf;
> +}
> +
> +static int __qcom_pas_tee_auth_and_reset(struct device *dev, u32 pas_id,
> +					 phys_addr_t mem_phys, size_t mem_size)
> +{
> +	struct qcom_pas_tee_private *data = dev_get_drvdata(dev);
> +	struct tee_ioctl_invoke_arg inv_arg = {
> +		.func = TA_QCOM_PAS_AUTH_AND_RESET,
> +		.session = data->session_id,
> +		.num_params = TEE_NUM_PARAMS
> +	};
> +	struct tee_param param[4] = {
> +		[0] = {
> +			.attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT,
> +			.u.value.a = pas_id,
> +			.u.value.b = mem_size,
> +		},
> +		[1] = {
> +			.attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT,
> +			.u.value.a = lower_32_bits(mem_phys),
> +			.u.value.b = upper_32_bits(mem_phys),
> +		},
> +		/* Reserved for fw memory space to be shared or lent */

Can you explain this comment a bit more? Plan is to allow passing a MEM_REF here
which could be lent/shared to TEE via FFA ABI?

> +		[2] = {
> +			.attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT,
> +		}
> +	};
> +	int ret;
> +
> +	ret = tee_client_invoke_func(data->ctx, &inv_arg, param);
> +	if (ret < 0 || inv_arg.ret != 0) {
> +		dev_err(dev, "PAS auth reset failed, pas_id: %d, err: %x\n",
> +			pas_id, inv_arg.ret);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int qcom_pas_tee_auth_and_reset(struct device *dev, u32 pas_id)

I'm guessing once all clients have migrated to PAS, plan is to drop this wrapper?

> +{
> +	return __qcom_pas_tee_auth_and_reset(dev, pas_id, 0, 0);
> +}
> +
> +static int qcom_pas_tee_prepare_and_auth_reset(struct device *dev,
> +					       struct qcom_pas_context *ctx)
> +{
> +	return __qcom_pas_tee_auth_and_reset(dev, ctx->pas_id, ctx->mem_phys,
> +					     ctx->mem_size);
> +}
> +
> +static int qcom_pas_tee_set_remote_state(struct device *dev, u32 state,
> +					 u32 pas_id)
> +{
> +	struct qcom_pas_tee_private *data = dev_get_drvdata(dev);
> +	struct tee_ioctl_invoke_arg inv_arg = {
> +		.func = TA_QCOM_PAS_SET_REMOTE_STATE,
> +		.session = data->session_id,
> +		.num_params = TEE_NUM_PARAMS
> +	};
> +	struct tee_param param[4] = {
> +		[0] = {
> +			.attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT,
> +			.u.value.a = pas_id,
> +			.u.value.b = state,
> +		}
> +	};
> +	int ret;

Nit: Why not ret = 0 initialize and always use ret?

> +
> +	ret = tee_client_invoke_func(data->ctx, &inv_arg, param);
> +	if (ret < 0 || inv_arg.ret != 0) {
> +		dev_err(dev, "PAS shutdown failed, pas_id: %d, err: %x\n",
> +			pas_id, inv_arg.ret);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int qcom_pas_tee_shutdown(struct device *dev, u32 pas_id)
> +{
> +	struct qcom_pas_tee_private *data = dev_get_drvdata(dev);
> +	struct tee_ioctl_invoke_arg inv_arg = {
> +		.func = TA_QCOM_PAS_SHUTDOWN,
> +		.session = data->session_id,
> +		.num_params = TEE_NUM_PARAMS
> +	};
> +	struct tee_param param[4] = {
> +		[0] = {
> +			.attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT,
> +			.u.value.a = pas_id
> +		}
> +	};
> +	int ret = 0;
> +
> +	ret = tee_client_invoke_func(data->ctx, &inv_arg, param);
> +	if (ret < 0 || inv_arg.ret != 0) {
> +		dev_err(dev, "PAS shutdown failed, pas_id: %d, err: %x\n",
> +			pas_id, inv_arg.ret);
> +		return -EINVAL;
> +	}
> +
> +	return 0;

You could just return 'ret' here. :)

> +}
> +
> +static void qcom_pas_tee_metadata_release(struct device *dev,
> +					  struct qcom_pas_context *ctx)
> +{
> +	struct tee_shm *mdata_shm = ctx->ptr;
> +

Nit: Unnecessary extra line.

> +	tee_shm_free(mdata_shm);
> +}
> +

[...]

> +
> +module_tee_client_driver(optee_pas_tee_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("TEE bus based Qualcomm PAS driver");

Since this is a tee_client driver, isn't it self-explanatary that it's TEE bus based?

On Mon, Mar 30, 2026 at 04:06:47PM +0530, Harshal Dev wrote:
> 
> 
> On 3/27/2026 6:40 PM, Sumit Garg wrote:
> > From: Sumit Garg <sumit.garg@oss.qualcomm.com>
> > 
> > Add support for Peripheral Authentication Service (PAS) driver based
> > on TEE bus with OP-TEE providing the backend PAS service implementation.
> > 
> > The TEE PAS service ABI is designed to be extensible with additional API
> > as PTA_QCOM_PAS_CAPABILITIES. This allows to accommodate any future
> > extensions of the PAS service needed while still maintaining backwards
> > compatibility.
> > 
> > Signed-off-by: Sumit Garg <sumit.garg@oss.qualcomm.com>
> > ---
> >  drivers/firmware/qcom/Kconfig        |  10 +
> >  drivers/firmware/qcom/Makefile       |   1 +
> >  drivers/firmware/qcom/qcom_pas_tee.c | 478 +++++++++++++++++++++++++++
> >  3 files changed, 489 insertions(+)
> >  create mode 100644 drivers/firmware/qcom/qcom_pas_tee.c
> >
> [...]
> 
> > diff --git a/drivers/firmware/qcom/qcom_pas_tee.c b/drivers/firmware/qcom/qcom_pas_tee.c
> > new file mode 100644
> > index 000000000000..d63122c34f04
> > --- /dev/null
> > +++ b/drivers/firmware/qcom/qcom_pas_tee.c
> > @@ -0,0 +1,478 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
> > + */
> > +
> > +#include <linux/delay.h>
> > +#include <linux/of.h>
> > +#include <linux/firmware/qcom/qcom_pas.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/slab.h>
> > +#include <linux/tee_drv.h>
> > +#include <linux/uuid.h>
> > +
> > +#include "qcom_pas.h"
> > +
> > +/*
> > + * Peripheral Authentication Service (PAS) supported.
> > + *
> > + * [in]  params[0].value.a:	Unique 32bit remote processor identifier
> > + */
> > +#define TA_QCOM_PAS_IS_SUPPORTED		1
> > +
> > +/*
> > + * PAS capabilities.
> > + *
> > + * [in]  params[0].value.a:	Unique 32bit remote processor identifier
> > + * [out] params[1].value.a:	PAS capability flags
> > + */
> > +#define TA_QCOM_PAS_CAPABILITIES		2
> > +
> > +/*
> > + * PAS image initialization.
> > + *
> > + * [in]  params[0].value.a:	Unique 32bit remote processor identifier
> > + * [in]  params[1].memref:	Loadable firmware metadata
> > + */
> > +#define TA_QCOM_PAS_INIT_IMAGE			3
> > +
> > +/*
> > + * PAS memory setup.
> > + *
> > + * [in]  params[0].value.a:	Unique 32bit remote processor identifier
> > + * [in]  params[0].value.b:	Relocatable firmware size
> > + * [in]  params[1].value.a:	32bit LSB relocatable firmware memory address
> > + * [in]  params[1].value.b:	32bit MSB relocatable firmware memory address
> > + */
> > +#define TA_QCOM_PAS_MEM_SETUP			4
> > +
> > +/*
> > + * PAS get resource table.
> > + *
> > + * [in]     params[0].value.a:	Unique 32bit remote processor identifier
> > + * [inout]  params[1].memref:	Resource table config
> > + */
> > +#define TA_QCOM_PAS_GET_RESOURCE_TABLE		5
> > +
> > +/*
> > + * PAS image authentication and co-processor reset.
> > + *
> > + * [in]  params[0].value.a:	Unique 32bit remote processor identifier
> > + * [in]  params[0].value.b:	Firmware size
> > + * [in]  params[1].value.a:	32bit LSB firmware memory address
> > + * [in]  params[1].value.b:	32bit MSB firmware memory address
> > + * [in]  params[2].memref:	Optional fw memory space shared/lent
> > + */
> > +#define TA_QCOM_PAS_AUTH_AND_RESET		6
> > +
> > +/*
> > + * PAS co-processor set suspend/resume state.
> > + *
> > + * [in]  params[0].value.a:	Unique 32bit remote processor identifier
> > + * [in]  params[0].value.b:	Co-processor state identifier
> > + */
> > +#define TA_QCOM_PAS_SET_REMOTE_STATE		7
> > +
> > +/*
> > + * PAS co-processor shutdown.
> > + *
> > + * [in]  params[0].value.a:	Unique 32bit remote processor identifier
> > + */
> > +#define TA_QCOM_PAS_SHUTDOWN			8
> > +
> > +#define TEE_NUM_PARAMS				4
> > +
> > +/**
> > + * struct qcom_pas_tee_private - PAS service private data
> > + * @dev:		PAS service device.
> > + * @ctx:		TEE context handler.
> > + * @session_id:		PAS TA session identifier.
> 
> Nit: Column alignment of comment descriptions.

These are aligned if you look at them after applying the patch-set.
Plain email text isn't good at showing the alignment here.

> 
> > + */
> > +struct qcom_pas_tee_private {
> > +	struct device *dev;
> > +	struct tee_context *ctx;
> > +	u32 session_id;
> > +};
> > +
> > +static bool qcom_pas_tee_supported(struct device *dev, u32 pas_id)
> > +{
> > +	struct qcom_pas_tee_private *data = dev_get_drvdata(dev);
> > +	struct tee_ioctl_invoke_arg inv_arg = {
> > +		.func = TA_QCOM_PAS_IS_SUPPORTED,
> > +		.session = data->session_id,
> > +		.num_params = TEE_NUM_PARAMS
> > +	};
> > +	struct tee_param param[4] = {
> > +		[0] = {
> > +			.attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT,
> > +			.u.value.a = pas_id
> > +		}
> > +	};
> > +	int ret;
> > +
> > +	ret = tee_client_invoke_func(data->ctx, &inv_arg, param);
> > +	if (ret < 0 || inv_arg.ret != 0) {
> > +		dev_err(dev, "PAS not supported, pas_id: %d, err: %x\n",
> > +			pas_id, inv_arg.ret);
> 
> I can see that similar error handling pattern for tee_client_invoke_func() is
> being done by other clients. But it seems that this error log doesn't really convey
> whether we failed before or after entering OPTEE (or some other TEE) for invoking
> the service.
> 
> Could be better if we print 'ret' when ret < 0. And print 'inv_arg.ret' when
> inv_arg.ret != 0. So for example, if the param marshalling fails, or some other
> issue is encountered when processing the params in a different back-end TEE
> driver, we could know from the logs.

Okay, I will add print for "ret" as well.

> 
> > +		return false;
> > +	}
> > +
> > +	return true;
> > +}
> > +
> > +static int qcom_pas_tee_init_image(struct device *dev, u32 pas_id,
> > +				   const void *metadata, size_t size,
> > +				   struct qcom_pas_context *ctx)
> > +{
> > +	struct qcom_pas_tee_private *data = dev_get_drvdata(dev);
> > +	struct tee_ioctl_invoke_arg inv_arg = {
> > +		.func = TA_QCOM_PAS_INIT_IMAGE,
> > +		.session = data->session_id,
> > +		.num_params = TEE_NUM_PARAMS
> > +	};
> > +	struct tee_param param[4] = {
> > +		[0] = {
> > +			.attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT,
> > +			.u.value.a = pas_id
> > +		},
> > +		[1] = {
> > +			.attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT,
> > +		}
> > +	};
> > +	struct tee_shm *mdata_shm;
> > +	u8 *mdata_buf = NULL;
> > +	int ret;
> > +
> > +	if (!ctx)
> > +		return -EINVAL;
> > +
> > +	mdata_shm = tee_shm_alloc_kernel_buf(data->ctx, size);
> 
> Why not move the DEFINE_FREE() above this function so we can use scoped free
> here as well for mdata_shm?
> 
> struct tee_shm *mdata_shm __free(shm_free) = ...

mdata_shm gets freed as part of qcom_pas_tee_metadata_release(), so no
automatic free here.

> 
> > +	if (IS_ERR(mdata_shm)) {
> > +		dev_err(dev, "mdata_shm allocation failed\n");
> > +		return PTR_ERR(mdata_shm);
> > +	}
> > +
> > +	mdata_buf = tee_shm_get_va(mdata_shm, 0);
> > +	if (IS_ERR(mdata_buf)) {
> > +		dev_err(dev, "mdata_buf get VA failed\n");
> > +		tee_shm_free(mdata_shm);
> > +		return PTR_ERR(mdata_buf);
> > +	}
> > +	memcpy(mdata_buf, metadata, size);
> > +
> > +	param[1].u.memref.shm = mdata_shm;
> > +	param[1].u.memref.size = size;
> > +
> > +	ret = tee_client_invoke_func(data->ctx, &inv_arg, param);
> > +	if (ret < 0 || inv_arg.ret != 0) {
> > +		dev_err(dev, "PAS init image failed, pas_id: %d, err: %x\n",
> 
> Nit: We can prefix %x with 0x. "err: 0x%x\n."

Ack

> 
> > +			pas_id, inv_arg.ret);
> > +		tee_shm_free(mdata_shm);
> > +		return -EINVAL;
> > +	}
> > +	ctx->ptr = (void *)mdata_shm;
> > +
> > +	return 0;
> > +}
> > +
> > +static int qcom_pas_tee_mem_setup(struct device *dev, u32 pas_id,
> > +				  phys_addr_t addr, phys_addr_t size)
> > +{
> > +	struct qcom_pas_tee_private *data = dev_get_drvdata(dev);
> > +	struct tee_ioctl_invoke_arg inv_arg = {
> > +		.func = TA_QCOM_PAS_MEM_SETUP,
> > +		.session = data->session_id,
> > +		.num_params = TEE_NUM_PARAMS
> > +	};
> > +	struct tee_param param[4] = {
> > +		[0] = {
> > +			.attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT,
> > +			.u.value.a = pas_id,
> > +			.u.value.b = size,
> > +		},
> > +		[1] = {
> > +			.attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT,
> > +			.u.value.a = lower_32_bits(addr),
> > +			.u.value.b = upper_32_bits(addr),
> > +		}
> > +	};
> > +	int ret;
> > +
> > +	ret = tee_client_invoke_func(data->ctx, &inv_arg, param);
> > +	if (ret < 0 || inv_arg.ret != 0) {
> > +		dev_err(dev, "PAS mem setup failed, pas_id: %d, err: %x\n",
> > +			pas_id, inv_arg.ret);
> > +		return -EINVAL;
> 
> I can see that originally in-case of error, qcom_scm_pas_mem_setup() bubbles
> up the return value from qcom_scm_call(). Perhaps we should do the same as well,
> return ret here instead of hard-coded '-EINVAL' which overrides any useful
> return values returned from the back-end TEE driver.

ret can even be 0 while still error from TEE. Let's rather do following:

	return ret ? ret : -EINVAL;

>  
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +DEFINE_FREE(shm_free, struct tee_shm *, tee_shm_free(_T))
> > +
> > +static void *qcom_pas_tee_get_rsc_table(struct device *dev,
> > +					struct qcom_pas_context *ctx,
> > +					void *input_rt, size_t input_rt_size,
> > +					size_t *output_rt_size)
> > +{
> > +	struct qcom_pas_tee_private *data = dev_get_drvdata(dev);
> > +	struct tee_ioctl_invoke_arg inv_arg = {
> > +		.func = TA_QCOM_PAS_GET_RESOURCE_TABLE,
> > +		.session = data->session_id,
> > +		.num_params = TEE_NUM_PARAMS
> > +	};
> > +	struct tee_param param[4] = {
> > +		[0] = {
> > +			.attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT,
> > +			.u.value.a = ctx->pas_id,
> > +		},
> > +		[1] = {
> > +			.attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT,
> > +			.u.memref.size = input_rt_size,
> > +		}
> > +	};
> > +	void *rt_buf = NULL;
> > +	int ret;
> > +
> > +	ret = tee_client_invoke_func(data->ctx, &inv_arg, param);
> > +	if (ret < 0 || inv_arg.ret != 0) {
> > +		dev_err(dev, "PAS get RT failed, pas_id: %d, err: %x\n",
> > +			ctx->pas_id, inv_arg.ret);
> > +		return ERR_PTR(-EINVAL);
> 
> Same comment here, we could return ERR_PTR(ret) instead of overriding to
> '-EINVAL'.
> 
> > +	}
> > +
> > +	if (param[1].u.memref.size) {
> > +		struct tee_shm *rt_shm __free(shm_free) =
> > +			tee_shm_alloc_kernel_buf(data->ctx,
> > +						 param[1].u.memref.size);
> > +		void *rt_shm_va;
> > +
> > +		if (IS_ERR(rt_shm)) {
> > +			dev_err(dev, "rt_shm allocation failed\n");
> > +			return rt_shm;
> > +		}
> > +
> > +		rt_shm_va = tee_shm_get_va(rt_shm, 0);
> > +		if (IS_ERR_OR_NULL(rt_shm_va)) {
> > +			dev_err(dev, "rt_shm get VA failed\n");
> > +			return ERR_PTR(-EINVAL);
> 
> Why not just return rt_shm_va? It already encodes the error in the pointer.

Again let me add a null check while returning rt_shm_va.

> 
> > +		}
> > +		memcpy(rt_shm_va, input_rt, input_rt_size);
> > +
> > +		param[1].u.memref.shm = rt_shm;
> > +		ret = tee_client_invoke_func(data->ctx, &inv_arg, param);
> > +		if (ret < 0 || inv_arg.ret != 0) {
> > +			dev_err(dev, "PAS get RT failed, pas_id: %d, err: %x\n",
> > +				ctx->pas_id, inv_arg.ret);
> > +			return ERR_PTR(-EINVAL);
> 
> Same comment.
> 
> > +		}
> > +
> > +		if (param[1].u.memref.size) {
> 
> Is it possible for param[1].u.memref.size to be 0 after a successful tee_client_invoke_func()?

Yeah it's possible if there is no resource table entry needed for a
particular PAS id.

> 
> > +			*output_rt_size = param[1].u.memref.size;
> > +			rt_buf = kmemdup(rt_shm_va, *output_rt_size, GFP_KERNEL);
> > +			if (!rt_buf)
> > +				return ERR_PTR(-ENOMEM);
> > +		}
> > +	}
> > +
> > +	return rt_buf;
> > +}
> > +
> > +static int __qcom_pas_tee_auth_and_reset(struct device *dev, u32 pas_id,
> > +					 phys_addr_t mem_phys, size_t mem_size)
> > +{
> > +	struct qcom_pas_tee_private *data = dev_get_drvdata(dev);
> > +	struct tee_ioctl_invoke_arg inv_arg = {
> > +		.func = TA_QCOM_PAS_AUTH_AND_RESET,
> > +		.session = data->session_id,
> > +		.num_params = TEE_NUM_PARAMS
> > +	};
> > +	struct tee_param param[4] = {
> > +		[0] = {
> > +			.attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT,
> > +			.u.value.a = pas_id,
> > +			.u.value.b = mem_size,
> > +		},
> > +		[1] = {
> > +			.attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT,
> > +			.u.value.a = lower_32_bits(mem_phys),
> > +			.u.value.b = upper_32_bits(mem_phys),
> > +		},
> > +		/* Reserved for fw memory space to be shared or lent */
> 
> Can you explain this comment a bit more? Plan is to allow passing a MEM_REF here
> which could be lent/shared to TEE via FFA ABI?

This param is added for future compatibility where PAS firmware is
loaded in arbitrarity allocated memory region by the kernel. Then that
can be shared or lend with TZ for authentication and reset sequence.

Right now the kernel is already loading the PAS firmware is fixed
reserved memory regions, for which this param is unused.

> 
> > +		[2] = {
> > +			.attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT,
> > +		}
> > +	};
> > +	int ret;
> > +
> > +	ret = tee_client_invoke_func(data->ctx, &inv_arg, param);
> > +	if (ret < 0 || inv_arg.ret != 0) {
> > +		dev_err(dev, "PAS auth reset failed, pas_id: %d, err: %x\n",
> > +			pas_id, inv_arg.ret);
> > +		return -EINVAL;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int qcom_pas_tee_auth_and_reset(struct device *dev, u32 pas_id)
> 
> I'm guessing once all clients have migrated to PAS, plan is to drop this wrapper?

Yeah, this patch-set doesn't aim to change the kernel client PAS API
contract apart from just renaming APIs. Surely future work can change
the PAS API contract as needed.

> 
> > +{
> > +	return __qcom_pas_tee_auth_and_reset(dev, pas_id, 0, 0);
> > +}
> > +
> > +static int qcom_pas_tee_prepare_and_auth_reset(struct device *dev,
> > +					       struct qcom_pas_context *ctx)
> > +{
> > +	return __qcom_pas_tee_auth_and_reset(dev, ctx->pas_id, ctx->mem_phys,
> > +					     ctx->mem_size);
> > +}
> > +
> > +static int qcom_pas_tee_set_remote_state(struct device *dev, u32 state,
> > +					 u32 pas_id)
> > +{
> > +	struct qcom_pas_tee_private *data = dev_get_drvdata(dev);
> > +	struct tee_ioctl_invoke_arg inv_arg = {
> > +		.func = TA_QCOM_PAS_SET_REMOTE_STATE,
> > +		.session = data->session_id,
> > +		.num_params = TEE_NUM_PARAMS
> > +	};
> > +	struct tee_param param[4] = {
> > +		[0] = {
> > +			.attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT,
> > +			.u.value.a = pas_id,
> > +			.u.value.b = state,
> > +		}
> > +	};
> > +	int ret;
> 
> Nit: Why not ret = 0 initialize and always use ret?

zero init will still be redundant..

> 
> > +
> > +	ret = tee_client_invoke_func(data->ctx, &inv_arg, param);
> > +	if (ret < 0 || inv_arg.ret != 0) {
> > +		dev_err(dev, "PAS shutdown failed, pas_id: %d, err: %x\n",
> > +			pas_id, inv_arg.ret);
> > +		return -EINVAL;
> > +	}
> > +
> > +	return 0;

.. will rather change this to return ret.

> > +}
> > +
> > +static int qcom_pas_tee_shutdown(struct device *dev, u32 pas_id)
> > +{
> > +	struct qcom_pas_tee_private *data = dev_get_drvdata(dev);
> > +	struct tee_ioctl_invoke_arg inv_arg = {
> > +		.func = TA_QCOM_PAS_SHUTDOWN,
> > +		.session = data->session_id,
> > +		.num_params = TEE_NUM_PARAMS
> > +	};
> > +	struct tee_param param[4] = {
> > +		[0] = {
> > +			.attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT,
> > +			.u.value.a = pas_id
> > +		}
> > +	};
> > +	int ret = 0;
> > +
> > +	ret = tee_client_invoke_func(data->ctx, &inv_arg, param);
> > +	if (ret < 0 || inv_arg.ret != 0) {
> > +		dev_err(dev, "PAS shutdown failed, pas_id: %d, err: %x\n",
> > +			pas_id, inv_arg.ret);
> > +		return -EINVAL;
> > +	}
> > +
> > +	return 0;
> 
> You could just return 'ret' here. :)

Ack.

> 
> > +}
> > +
> > +static void qcom_pas_tee_metadata_release(struct device *dev,
> > +					  struct qcom_pas_context *ctx)
> > +{
> > +	struct tee_shm *mdata_shm = ctx->ptr;
> > +
> 
> Nit: Unnecessary extra line.

Extra line is recommended after variables declaration.

> 
> > +	tee_shm_free(mdata_shm);
> > +}
> > +
> 
> [...]
> 
> > +
> > +module_tee_client_driver(optee_pas_tee_driver);
> > +
> > +MODULE_LICENSE("GPL");
> > +MODULE_DESCRIPTION("TEE bus based Qualcomm PAS driver");
> 
> Since this is a tee_client driver, isn't it self-explanatary that it's TEE bus based?
> 

I can rather say:

MODULE_DESCRIPTION("Qualcomm PAS TEE driver");

-Sumit