Re: [PATCH v2 3/6] firmware: qcom: Add a TEE bus based ICE back-end

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] firmware: qcom: missing qcom_ice_tee.h breaks build in
  qcom_ice_tee.c
- [High] firmware: qcom: broken stub guard and Kconfig deps for
  qcom_ice_tee_probe()
--

commit dca5daf641df3494a0c5504854d7604efeca0f27
Author: Harshal Dev <harshal.dev@oss.qualcomm.com>

firmware: qcom: Add a TEE bus based ICE back-end

This commit adds an OP-TEE client driver that registers TrustZone operations
for the Qualcomm Inline Crypto Engine (ICE) driver.

> diff --git a/drivers/firmware/qcom/Kconfig b/drivers/firmware/qcom/Kconfig
> index 3ad22f8fc3e5f..189ada7fdcd67 100644
> --- a/drivers/firmware/qcom/Kconfig
> +++ b/drivers/firmware/qcom/Kconfig
> @@ -25,6 +25,15 @@ config QCOM_PAS_TEE
>  	  Enable the generic Peripheral Authentication Service (PAS) provided
>  	  by the firmware TEE implementation as the backend.
>  
> +config QCOM_ICE_TEE
> +	tristate
> +	depends on TEE
> +	depends on !CPU_BIG_ENDIAN
> +	default m if ARCH_QCOM
> +	help
> +	  Enable the generic Inline Crypto Engine (ICE) firmware service TEE
> +	  implementation as a backend.
> +
>  config QCOM_SCM
>  	tristate "Qualcomm PAS SCM interface driver" if ARCH_QCOM || COMPILE_TEST
>  	select QCOM_PAS
> diff --git a/drivers/firmware/qcom/Makefile b/drivers/firmware/qcom/Makefile
> index 88ce74d74c3e2..5ef64f17e6e59 100644
> --- a/drivers/firmware/qcom/Makefile
> +++ b/drivers/firmware/qcom/Makefile
> @@ -11,3 +11,4 @@ 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
> +obj-$(CONFIG_QCOM_ICE_TEE)     += qcom_ice_tee.o
> diff --git a/drivers/firmware/qcom/qcom_ice_tee.c b/drivers/firmware/qcom/qcom_ice_tee.c
> new file mode 100644
> index 0000000000000..694aa05847f0e
> --- /dev/null
> +++ b/drivers/firmware/qcom/qcom_ice_tee.c
> @@ -0,0 +1,402 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/tee_drv.h>
> +#include <linux/uuid.h>
> +#include <linux/firmware/qcom/qcom_ice_tee.h>

[Severity: High]
Does including <linux/firmware/qcom/qcom_ice_tee.h> here break compilation at
this commit?

include/linux/firmware/qcom/qcom_ice_tee.h is not created until a later patch
in the series, commit "tee: optee: Register TEE devices on the platform bus",
and nothing in qcom_ice_tee.c uses it.  Moving this #include to that commit
would keep this patch compilable and bisectable.

[ ... ]

> +	qcom_ice_tee_ops.dev = dev;
> +	ret = qcom_ice_tz_ops_register(&qcom_ice_tee_ops);
> +	if (ret)
> +		goto err_ops;
> +
> +	return 0;
> +err_ops:
> +	tee_client_close_session(data->ctx, data->session_id);
> +err_sess:
> +	tee_client_close_context(data->ctx);
> +	return ret;
> +}
> +
> +static void qcom_ice_tee_remove(struct tee_client_device *ice_dev)
> +{
> +	struct device *dev = &ice_dev->dev;
> +	struct qcom_ice_tee_private *data = dev_get_drvdata(dev);
> +
> +	qcom_ice_tz_ops_unregister(&qcom_ice_tee_ops);

[Severity: High]
When the ICE driver is built as a module, do qcom_ice_tz_ops_register() and
qcom_ice_tz_ops_unregister() here resolve to the no-op stubs in
include/soc/qcom/ice.h?

In commit "soc: qcom: ice: Add TZ agnostic re-direction functions", the
declarations in include/soc/qcom/ice.h are guarded with #ifdef rather than
IS_ENABLED():

include/soc/qcom/ice.h:
    #ifdef CONFIG_QCOM_INLINE_CRYPTO_ENGINE
    int qcom_ice_tz_ops_register(struct qcom_ice_tz_ops *ops);
    void qcom_ice_tz_ops_unregister(struct qcom_ice_tz_ops *ops);
    #else
    static inline int qcom_ice_tz_ops_register(struct qcom_ice_tz_ops *__unused)
    {
    	return 0;
    }

Because qcom_inline_crypto_engine is a tristate symbol (defaulting to m in
arm64 defconfig), modular builds define CONFIG_QCOM_INLINE_CRYPTO_ENGINE_MODULE
instead of CONFIG_QCOM_INLINE_CRYPTO_ENGINE, so qcom_ice_tee_probe() silently
calls the stub and never registers qcom_ice_tee_ops with the ICE driver.

Also, in drivers/firmware/qcom/Kconfig, the new qcom_ice_tee entry has no
dependency on qcom_inline_crypto_engine (allowing qcom_ice_tee to be built-in
while qcom_inline_crypto_engine is a module, or built when it is disabled) and
defines help text on a promptless tristate symbol.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111-ice-migrate-tee-svc-v2-0-29d19f6c92ab@oss.qualcomm.com?part=3