Re: [PATCH v2 2/6] firmware: qcom_scm: Transition to ICE TZ agnostic operations

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] firmware: qcom_scm: broken qcom_ice_tz_ops_register() linkage
  when ICE=m
- [Medium] firmware: qcom_scm: missing static on
  __qcom_scm_ice_available()
- [Low] firmware: qcom_scm: kernel-doc mismatch on static
  __qcom_scm_ice_*()
--

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

firmware: qcom_scm: Transition to ICE TZ agnostic operations

This commit transitions the ICE-related SCM calls to static helpers
registered as a qcom_ice_tz_ops backend while keeping the existing
exported wrappers in place.

> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> index 43e250faea75a..6b66bcedc84fd 100644
> --- a/drivers/firmware/qcom/qcom_scm.c
> +++ b/drivers/firmware/qcom/qcom_scm.c
[ ... ]
> @@ -1397,6 +1399,14 @@ int qcom_scm_ocmem_unlock(enum qcom_scm_ocmem_client id, u32 offset, u32 size)
>  }
>  EXPORT_SYMBOL_GPL(qcom_scm_ocmem_unlock);
>  
> +bool __qcom_scm_ice_available(struct device *dev)
> +{
> +	return __qcom_scm_is_call_available(dev, QCOM_SCM_SVC_ES,
> +					    QCOM_SCM_ES_INVALIDATE_ICE_KEY) &&
> +		__qcom_scm_is_call_available(dev, QCOM_SCM_SVC_ES,
> +					     QCOM_SCM_ES_CONFIG_SET_ICE_KEY);
> +}

[Severity: Medium]
Should __qcom_scm_ice_available() be marked static?

It is only used within drivers/firmware/qcom/qcom_scm.c and has no prior
prototype declaration, which triggers a -Wmissing-prototypes warning and
breaks CONFIG_WERROR=y builds at this commit until commit "firmware: qcom:
scm: Drop exported ICE wrapper functions" removes the helper later in the
series.

> @@ -1424,7 +1431,7 @@ EXPORT_SYMBOL_GPL(qcom_scm_ice_available);
>   *
>   * Return: 0 on success; -errno on failure.
>   */
> -int qcom_scm_ice_invalidate_key(u32 index)
> +static int __qcom_scm_ice_invalidate_key(struct device *dev, u32 index)
>  {
>  	struct qcom_scm_desc desc = {
>  		.svc = QCOM_SCM_SVC_ES,
> @@ -1434,7 +1441,12 @@ int qcom_scm_ice_invalidate_key(u32 index)
>  		.owner = ARM_SMCCC_OWNER_SIP,
>  	};
>  
> -	return qcom_scm_call(__scm->dev, &desc, NULL);
> +	return qcom_scm_call(dev, &desc, NULL);
> +}
> +
> +int qcom_scm_ice_invalidate_key(u32 index)
> +{
> +	return __qcom_scm_ice_invalidate_key(__scm->dev, index);
>  }
>  EXPORT_SYMBOL_GPL(qcom_scm_ice_invalidate_key);

[Severity: Low]
Does leaving the kernel-doc comments above __qcom_scm_ice_invalidate_key()
and the five other __qcom_scm_*() helpers cause scripts/kernel-doc warnings?

Each comment still documents the exported qcom_scm_*() symbol name and
omits the new @dev parameter on __qcom_scm_ice_invalidate_key(),
__qcom_scm_ice_set_key(), __qcom_scm_derive_sw_secret(),
__qcom_scm_generate_ice_key(), __qcom_scm_prepare_ice_key(), and
__qcom_scm_import_ice_key().  Later in the series, commit "firmware: qcom:
scm: Drop exported ICE wrapper functions" renames these static helpers
back to qcom_scm_*(), but @dev remains undocumented in all six kernel-doc
blocks.

[ ... ]

> @@ -2820,6 +2888,18 @@ static int qcom_scm_probe(struct platform_device *pdev)
>  					     "Failed to request qcom-scm irq\n");
>  	}
>  
> +	/*
> +	 * If the ICE SCM interface is available, the operations MUST be
> +	 * registered before the SCM API is published below, otherwise
> +	 * ICE driver could attempt to call the ICE SCM pointers before
> +	 * they are registered and assume that they aren't available.
> +	 */
> +	if (__qcom_scm_ice_available(scm->dev)) {
> +		qcom_ice_scm_ops.dev = scm->dev;
> +		ret = qcom_ice_tz_ops_register(&qcom_ice_scm_ops);
> +		WARN(ret < 0, "failed to register ice SCM ops: %d\n", ret);
> +	}
> +
>  	/*
>  	 * Paired with smp_load_acquire() in qcom_scm_is_available().
>  	 *
> @@ -2876,6 +2956,7 @@ static void qcom_scm_shutdown(struct platform_device *pdev)
>  	/* Clean shutdown, disable download mode to allow normal restart */
>  	qcom_scm_set_download_mode(__scm, QCOM_DLOAD_NODUMP);
>  	qcom_pas_ops_unregister();
> +	qcom_ice_tz_ops_unregister(&qcom_ice_scm_ops);
>  }

[Severity: High]
Does qcom_scm_probe() silently call the no-op stub for
qcom_ice_tz_ops_register() when CONFIG_QCOM_INLINE_CRYPTO_ENGINE=m?

In include/soc/qcom/ice.h, the declarations 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 (and defaults to =m
in arm64 defconfig via CONFIG_SCSI_UFS_QCOM=m while CONFIG_QCOM_SCM=y),
qcom_scm_probe() and qcom_scm_shutdown() compile against the static inline
stubs, so qcom_ice_scm_ops is never registered and inline encryption fails
once the ICE driver checks qcom_ice_tz_ops_registered().

Even with IS_ENABLED(CONFIG_QCOM_INLINE_CRYPTO_ENGINE), CONFIG_QCOM_SCM=y
with CONFIG_QCOM_INLINE_CRYPTO_ENGINE=m fails to link vmlinux against
qcom_ice_tz_ops_register() and qcom_ice_tz_ops_unregister() in qcom_ice.ko,
and building both as modules creates a circular dependency between
qcom-scm.ko and qcom_ice.ko while qcom_ice still calls qcom_scm_is_available().

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