Re: [PATCH v2 4/6] tee: optee: Register TEE devices on the platform bus

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential
issues to consider:
- [High] firmware: qcom: duplicate module_init() and module_exit() in
  qcom_ice_tee.c
- [High] tee: missing EXPORT_SYMBOL_GPL() for
  tee_platform_device_register()
- [High] soc: qcom: Kconfig dependency and module cycle selecting
  QCOM_ICE_TEE
- [High] tee: optee: platform device name collision with optee_driver
- [High] soc: qcom: ice: qcom_ice_tz_is_available() requires both SCM
  and OP-TEE
- [Medium] tee: unchecked devm_add_action_or_reset() in
  tee_platform_device_register()
- [Medium] firmware: qcom: missing remove callback in
  qcom_ice_plat_driver
--

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

tee: optee: Register TEE devices on the platform bus

This commit adds tee_platform_device_register() to register OP-TEE devices on
the platform bus and updates the Qualcomm ICE driver and its TEE backend to
check whether OP-TEE has probed.

> diff --git a/drivers/firmware/qcom/qcom_ice_tee.c b/drivers/firmware/qcom/qcom_ice_tee.c
> index 694aa05847f0e..39f7094439d99 100644
> --- a/drivers/firmware/qcom/qcom_ice_tee.c
> +++ b/drivers/firmware/qcom/qcom_ice_tee.c

[ ... ]

> @@ -398,5 +414,30 @@ static struct tee_client_driver qcom_ice_tee_driver = {
>  
>  module_tee_client_driver(qcom_ice_tee_driver);
>  
> +static int qcom_ice_plat_probe(struct platform_device *pdev)
> +{
> +	/* Paired with smp_load_acquire() in qcom_ice_tee_is_available() */
> +	smp_store_release(&__teedev, &pdev->dev);
> +	return 0;
> +}
> +
> +static const struct platform_device_id qcom_ice_plat_id_table[] = {
> +	{
> +		.name = "optee",
> +	},
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(platform, qcom_ice_plat_id_table);
> +
> +static struct platform_driver qcom_ice_plat_driver = {
> +	.probe = qcom_ice_plat_probe,
> +	.id_table = qcom_ice_plat_id_table,
> +	.driver = {
> +		.name = "qcom-ice-plat",
> +	},
> +};

[Severity: Medium]
Should qcom_ice_plat_driver also provide a .remove callback to clear
__teedev?  When the parent tee_device is unregistered or qcom-ice-plat is
unbound, plat_tee_free() unregisters and frees pdev, leaving __teedev as a
dangling pointer and causing qcom_ice_tee_is_available() to keep returning
true.

> +
> +module_platform_driver(qcom_ice_plat_driver);

[Severity: High]
Does adding module_platform_driver(qcom_ice_plat_driver) in the same file as
module_tee_client_driver(qcom_ice_tee_driver) break modular builds?  Both
macros expand to module_driver(), which defines __inittest(), init_module(),
__exittest(), and cleanup_module(), causing duplicate symbol definitions when
built as a module.

A single module_init() and module_exit() pair is needed to register
qcom_ice_tee_driver before qcom_ice_plat_driver, unwind on registration
failure, and unregister both in reverse order.

> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> index 535c8619197b3..8dc75c252f52b 100644
> --- a/drivers/soc/qcom/Kconfig
> +++ b/drivers/soc/qcom/Kconfig
> @@ -296,6 +296,7 @@ endif
>  config QCOM_INLINE_CRYPTO_ENGINE
>  	tristate
>  	select QCOM_SCM
> +	select QCOM_ICE_TEE

[Severity: High]
Can QCOM_INLINE_CRYPTO_ENGINE unconditionally select QCOM_ICE_TEE here?

QCOM_ICE_TEE depends on TEE and !CPU_BIG_ENDIAN in
drivers/firmware/qcom/Kconfig, whereas QCOM_INLINE_CRYPTO_ENGINE is selected
by MMC_SDHCI_MSM and SCSI_UFS_QCOM without those dependencies.  Selecting
QCOM_ICE_TEE when TEE is disabled (or when QCOM_INLINE_CRYPTO_ENGINE is
built-in and TEE is modular) causes unmet direct dependency warnings and link
failures for tee_client_* symbols, and include/linux/firmware/qcom/qcom_ice_tee.h
has no fallback stub for qcom_ice_tee_is_available() (nor an include of
linux/types.h for bool).

In addition, qcom_ice.c now calls qcom_ice_tee_is_available() from
qcom_ice_tee.c while qcom_ice_tee.c calls qcom_ice_tz_ops_register() and
qcom_ice_tz_ops_unregister() from qcom_ice.c, creating a circular dependency
between the two modules.

> diff --git a/drivers/soc/qcom/ice.c b/drivers/soc/qcom/ice.c
> index 452896b1858f1..4844621b74970 100644
> --- a/drivers/soc/qcom/ice.c
> +++ b/drivers/soc/qcom/ice.c
> @@ -18,6 +18,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/xarray.h>
>  
> +#include <linux/firmware/qcom/qcom_ice_tee.h>
>  #include <linux/firmware/qcom/qcom_scm.h>
>  
>  #include <soc/qcom/ice.h>
> @@ -132,7 +133,13 @@ static bool qcom_ice_tz_ops_registered(void)
>  
>  static bool qcom_ice_tz_is_available(void)
>  {
> -	return qcom_scm_is_available();
> +	/*
> +	 * Both back-ends should be available before we check for
> +	 * qcom_ice_tz_ops_registered(), otherwise we could pre-maturely
> +	 * assume that TZ does not provide any ICE secure interface.
> +	 */
> +	return (qcom_scm_is_available() & qcom_ice_tee_is_available());
> +
>  }

[Severity: High]
Does requiring both qcom_scm_is_available() and qcom_ice_tee_is_available()
break all QTEE-based platforms where OP-TEE is not present?

__teedev is only set when OP-TEE probes and registers its platform device.
On platforms using QTEE via SCM (or when OP-TEE is not enabled),
qcom_ice_tee_is_available() remains false forever, so once qcom_ice_create()
checks qcom_ice_tz_is_available() in the next patch it will defer probe
indefinitely and prevent UFS/SDHC storage from coming online.

Also, this uses bitwise & on booleans instead of &&, and at this commit
qcom_ice_tz_is_available() is an unused static function, triggering a
-Wunused-function warning during bisection.

> diff --git a/drivers/tee/optee/smc_abi.c b/drivers/tee/optee/smc_abi.c
> index 51624443359bf..85ee761739826 100644
> --- a/drivers/tee/optee/smc_abi.c
> +++ b/drivers/tee/optee/smc_abi.c
> @@ -1920,6 +1920,14 @@ static int optee_probe(struct platform_device *pdev)
>  	if (rc)
>  		goto err_disable_shm_cache;
>  
> +	/*
> +	 * Device should only be visible on the platform bus once the
> +	 * child devices have already been enumerated on the TEE bus.
> +	 */
> +	rc = tee_platform_device_register(optee->teedev, "optee");
> +	if (rc)
> +		goto err_disable_shm_cache;

[Severity: High]
Does registering a platform device named "optee" here (and in
optee_ffa_probe()) collide with optee_driver on the platform bus?

drivers/tee/optee/smc_abi.c defines optee_driver with:

drivers/tee/optee/smc_abi.c:
	static struct platform_driver optee_driver = {
		.probe  = optee_probe,
		.remove = optee_smc_remove,
		.shutdown = optee_shutdown,
		.driver = {
			.name = "optee",
			.of_match_table = optee_dt_match,
		},
	};

Because optee_driver has .driver.name = "optee" and no .id_table,
platform_match() falls back to strcmp(pdev->name, drv->name) == 0 and matches
this new child platform device against optee_driver, recursively calling
optee_probe() (which logs errors and fails with -ENXIO).  On non-DT systems
that already have a platform device named "optee" with PLATFORM_DEVID_NONE,
registering a second one also fails with -EEXIST, and only a single platform
driver can bind to the device at a time.

> diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c
> index 1aac50c7c1de7..0d3c266b29c7b 100644
> --- a/drivers/tee/tee_core.c
> +++ b/drivers/tee/tee_core.c

[ ... ]

> @@ -1225,6 +1226,26 @@ int tee_device_register(struct tee_device *teedev)
>  }
>  EXPORT_SYMBOL_GPL(tee_device_register);
>  
> +static void plat_tee_free(void *data)
> +{
> +	struct platform_device *plat_dev = data;
> +
> +	platform_device_unregister(plat_dev);
> +}
> +
> +int tee_platform_device_register(struct tee_device *teedev, const char *teename)
> +{
> +	struct platform_device *plat_dev;
> +
> +	plat_dev = platform_device_register_data(&teedev->dev, teename,
> +						 PLATFORM_DEVID_NONE, NULL, 0);
> +	if (IS_ERR(plat_dev))
> +		return PTR_ERR(plat_dev);
> +
> +	devm_add_action_or_reset(&teedev->dev, plat_tee_free, plat_dev);
> +	return 0;

[Severity: Medium]
Should tee_platform_device_register() return the result of
devm_add_action_or_reset()?  __devm_add_action_or_reset() is marked
__must_check and on allocation failure it runs plat_tee_free() to unregister
plat_dev, while this function still returns 0 to the caller.

> +}

[Severity: High]
Does tee_platform_device_register() need EXPORT_SYMBOL_GPL()?  It is defined
in tee.ko and called from optee_probe() and optee_ffa_probe() in optee.ko, so
building OP-TEE as a module fails at modpost with an undefined symbol.

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