qcom: ice: Prevent client probe failures on unsupported ICE

[PATCH] qcom: ice: Prevent client probe failures on unsupported ICE

Posted by Debraj Mukhopadhyay 3 months, 2 weeks ago

Storage clients (ex. UFS and MMC) invoke of_qcom_ice_get() to obtain the
handle from ICE (Inline Crypto Engine) driver. Currently if ICE is
unsupported, the return code from probe could prevent the client
initialization which is a bug. To fix this a new flag
qcom_ice_create_error is introduced which caches the error encountered
during ICE probe.

The qcom_ice_create() and of_qcom_ice_get() functions are updated to
return -EOPNOTSUPP when ICE is unsupported, allowing clients to proceed
without ICE.

For other failures, such as ICE not yet initialized, the existing
behavior (e.g., -EPROBE_DEFER) is retained to ensure proper error
handling.

This improves error signaling and ensures that client initialization is
not blocked unnecessarily.

Signed-off-by: Debraj Mukhopadhyay <quic_dmukhopa@quicinc.com>
---
 drivers/soc/qcom/ice.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/soc/qcom/ice.c b/drivers/soc/qcom/ice.c
index c467b55b4174..5460436d9158 100644
--- a/drivers/soc/qcom/ice.c
+++ b/drivers/soc/qcom/ice.c
@@ -85,6 +85,8 @@ union crypto_cfg {
 #define qcom_ice_readl(engine, reg)	\
 	readl((engine)->base + (reg))
 
+static bool qcom_ice_create_error;
+
 static bool qcom_ice_use_wrapped_keys;
 module_param_named(use_wrapped_keys, qcom_ice_use_wrapped_keys, bool, 0660);
 MODULE_PARM_DESC(use_wrapped_keys,
@@ -524,7 +526,7 @@ static struct qcom_ice *qcom_ice_create(struct device *dev,
 
 	if (!qcom_scm_ice_available()) {
 		dev_warn(dev, "ICE SCM interface not found\n");
-		return NULL;
+		return ERR_PTR(-EOPNOTSUPP);
 	}
 
 	engine = devm_kzalloc(dev, sizeof(*engine), GFP_KERNEL);
@@ -604,7 +606,7 @@ static struct qcom_ice *of_qcom_ice_get(struct device *dev)
 	struct device_node *node __free(device_node) = of_parse_phandle(dev->of_node,
 									"qcom,ice", 0);
 	if (!node)
-		return NULL;
+		return ERR_PTR(-EOPNOTSUPP);
 
 	pdev = of_find_device_by_node(node);
 	if (!pdev) {
@@ -617,7 +619,10 @@ static struct qcom_ice *of_qcom_ice_get(struct device *dev)
 		dev_err(dev, "Cannot get ice instance from %s\n",
 			dev_name(&pdev->dev));
 		platform_device_put(pdev);
-		return ERR_PTR(-EPROBE_DEFER);
+		if (qcom_ice_create_error)
+			return ERR_PTR(-EOPNOTSUPP);
+		else
+			return ERR_PTR(-EPROBE_DEFER);
 	}
 
 	link = device_link_add(dev, &pdev->dev, DL_FLAG_AUTOREMOVE_SUPPLIER);
@@ -691,8 +696,10 @@ static int qcom_ice_probe(struct platform_device *pdev)
 	}
 
 	engine = qcom_ice_create(&pdev->dev, base);
-	if (IS_ERR(engine))
+	if (IS_ERR(engine)) {
+		qcom_ice_create_error = true;
 		return PTR_ERR(engine);
+	}
 
 	platform_set_drvdata(pdev, engine);
 
-- 
2.34.1

Re: [PATCH] qcom: ice: Prevent client probe failures on unsupported ICE

Posted by Bjorn Andersson 3 months, 2 weeks ago

On Tue, Oct 21, 2025 at 09:33:37AM +0530, Debraj Mukhopadhyay wrote:
> Storage clients (ex. UFS and MMC) invoke of_qcom_ice_get() to obtain the
> handle from ICE (Inline Crypto Engine) driver. Currently if ICE is
> unsupported, the return code from probe could prevent the client

In light of Konrad's questions about this, I think you should rework
this sentence to make it clear that you're referring to a platform where
TZ doesn't implement ICE. I don't think "which is a bug" accurately
reflect the outcome, it's not a bug, it just sounds like you changed the
rules - and need to update the logic as such.

> initialization which is a bug. To fix this a new flag
> qcom_ice_create_error is introduced which caches the error encountered
> during ICE probe.
> 
> The qcom_ice_create() and of_qcom_ice_get() functions are updated to
> return -EOPNOTSUPP when ICE is unsupported, allowing clients to proceed
> without ICE.
> 
> For other failures, such as ICE not yet initialized, the existing
> behavior (e.g., -EPROBE_DEFER) is retained to ensure proper error
> handling.
> 
> This improves error signaling and ensures that client initialization is
> not blocked unnecessarily.
> 
> Signed-off-by: Debraj Mukhopadhyay <quic_dmukhopa@quicinc.com>

Please use oss.qualcomm.com

> ---
>  drivers/soc/qcom/ice.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/soc/qcom/ice.c b/drivers/soc/qcom/ice.c
> index c467b55b4174..5460436d9158 100644
> --- a/drivers/soc/qcom/ice.c
> +++ b/drivers/soc/qcom/ice.c
> @@ -85,6 +85,8 @@ union crypto_cfg {
>  #define qcom_ice_readl(engine, reg)	\
>  	readl((engine)->base + (reg))
>  
> +static bool qcom_ice_create_error;

What happens the day the HW guys put two of these in a chip?

> +
>  static bool qcom_ice_use_wrapped_keys;
>  module_param_named(use_wrapped_keys, qcom_ice_use_wrapped_keys, bool, 0660);
>  MODULE_PARM_DESC(use_wrapped_keys,
> @@ -524,7 +526,7 @@ static struct qcom_ice *qcom_ice_create(struct device *dev,
>  
>  	if (!qcom_scm_ice_available()) {
>  		dev_warn(dev, "ICE SCM interface not found\n");
> -		return NULL;
> +		return ERR_PTR(-EOPNOTSUPP);

I think this is supported by your commit message - i.e. tz doesn't
provide ICE, but it's documented in the DT, so we should return
EOPNOTSUPP and get that warning from the UFS code.

>  	}
>  
>  	engine = devm_kzalloc(dev, sizeof(*engine), GFP_KERNEL);
> @@ -604,7 +606,7 @@ static struct qcom_ice *of_qcom_ice_get(struct device *dev)
>  	struct device_node *node __free(device_node) = of_parse_phandle(dev->of_node,
>  									"qcom,ice", 0);
>  	if (!node)
> -		return NULL;
> +		return ERR_PTR(-EOPNOTSUPP);

But this falls in the category "the DeviceTree doesn't claim we have ICE
support", so here I think the NULL makes sense. At least, there
shouldn't be a warning during boot, because the hardware description
says there's no ICE, so how can we disable it...

>  
>  	pdev = of_find_device_by_node(node);
>  	if (!pdev) {
> @@ -617,7 +619,10 @@ static struct qcom_ice *of_qcom_ice_get(struct device *dev)
>  		dev_err(dev, "Cannot get ice instance from %s\n",
>  			dev_name(&pdev->dev));
>  		platform_device_put(pdev);
> -		return ERR_PTR(-EPROBE_DEFER);
> +		if (qcom_ice_create_error)
> +			return ERR_PTR(-EOPNOTSUPP);
> +		else
> +			return ERR_PTR(-EPROBE_DEFER);

There's the theoretical chance that qcom_ice_create() will return
ERR_PTR(EPROBE_DEFER) in which case you will set qcom_ice_create_error
and then return EOPNOTSUPP here.



PS. The definition of "Return:" doesn't adequately capture what the
client should expect from this function; but that would probably be
better served by a patch of its own.

Regards,
Bjorn

>  	}
>  
>  	link = device_link_add(dev, &pdev->dev, DL_FLAG_AUTOREMOVE_SUPPLIER);
> @@ -691,8 +696,10 @@ static int qcom_ice_probe(struct platform_device *pdev)
>  	}
>  
>  	engine = qcom_ice_create(&pdev->dev, base);
> -	if (IS_ERR(engine))
> +	if (IS_ERR(engine)) {
> +		qcom_ice_create_error = true;
>  		return PTR_ERR(engine);
> +	}
>  
>  	platform_set_drvdata(pdev, engine);
>  
> -- 
> 2.34.1
>

Re: [PATCH] qcom: ice: Prevent client probe failures on unsupported ICE

Posted by Konrad Dybcio 3 months, 2 weeks ago

On 10/21/25 6:03 AM, Debraj Mukhopadhyay wrote:
> Storage clients (ex. UFS and MMC) invoke of_qcom_ice_get() to obtain the
> handle from ICE (Inline Crypto Engine) driver. Currently if ICE is
> unsupported, the return code from probe could prevent the client
> initialization which is a bug. To fix this a new flag
> qcom_ice_create_error is introduced which caches the error encountered
> during ICE probe.

Probe currently only happens if the ICE node is present in the DT and
referred to from the storage controller. What does this patch solve?

Konrad

> 
> The qcom_ice_create() and of_qcom_ice_get() functions are updated to
> return -EOPNOTSUPP when ICE is unsupported, allowing clients to proceed
> without ICE.
> 
> For other failures, such as ICE not yet initialized, the existing
> behavior (e.g., -EPROBE_DEFER) is retained to ensure proper error
> handling.
> 
> This improves error signaling and ensures that client initialization is
> not blocked unnecessarily.
> 
> Signed-off-by: Debraj Mukhopadhyay <quic_dmukhopa@quicinc.com>
> ---

Re: [PATCH] qcom: ice: Prevent client probe failures on unsupported ICE

Posted by Debraj Mukhopadhyay 3 months, 2 weeks ago

Hi Konrad,

Thanks for your comment. Please find my response inline below.

On 10/21/2025 2:57 PM, Konrad Dybcio wrote:
> On 10/21/25 6:03 AM, Debraj Mukhopadhyay wrote:
>> Storage clients (ex. UFS and MMC) invoke of_qcom_ice_get() to obtain the
>> handle from ICE (Inline Crypto Engine) driver. Currently if ICE is
>> unsupported, the return code from probe could prevent the client
>> initialization which is a bug. To fix this a new flag
>> qcom_ice_create_error is introduced which caches the error encountered
>> during ICE probe.
> Probe currently only happens if the ICE node is present in the DT and
> referred to from the storage controller. What does this patch solve?
>
> Konrad

Even if the DT node is present it is possible that The SCM support for 
ICE is unavailable in the underlying TZ framework. With the existing 
logic, qcom_scm_ice_available() would have failed in such cases, 
returning NULL to storage clients where the clients like storage may 
keep retrying which potentially can cause boot up issues. This patch 
corrects that behavior by explicitly returning -EOPNOTSUPP to the 
clients. I will update the commit message accordingly.

Thanks,

Debraj

>
>> The qcom_ice_create() and of_qcom_ice_get() functions are updated to
>> return -EOPNOTSUPP when ICE is unsupported, allowing clients to proceed
>> without ICE.
>>
>> For other failures, such as ICE not yet initialized, the existing
>> behavior (e.g., -EPROBE_DEFER) is retained to ensure proper error
>> handling.
>>
>> This improves error signaling and ensures that client initialization is
>> not blocked unnecessarily.
>>
>> Signed-off-by: Debraj Mukhopadhyay <quic_dmukhopa@quicinc.com>
>> ---

Re: [PATCH] qcom: ice: Prevent client probe failures on unsupported ICE

Posted by Konrad Dybcio 3 months, 2 weeks ago

On 10/21/25 2:34 PM, Debraj Mukhopadhyay wrote:
> Hi Konrad,
> 
> Thanks for your comment. Please find my response inline below.

https://lore.kernel.org/linux-arm-msm/2023042722-humble-unthread-9597@gregkh/
> On 10/21/2025 2:57 PM, Konrad Dybcio wrote:
>> On 10/21/25 6:03 AM, Debraj Mukhopadhyay wrote:
>>> Storage clients (ex. UFS and MMC) invoke of_qcom_ice_get() to obtain the
>>> handle from ICE (Inline Crypto Engine) driver. Currently if ICE is
>>> unsupported, the return code from probe could prevent the client
>>> initialization which is a bug. To fix this a new flag
>>> qcom_ice_create_error is introduced which caches the error encountered
>>> during ICE probe.
>> Probe currently only happens if the ICE node is present in the DT and
>> referred to from the storage controller. What does this patch solve?
>>
>> Konrad
> 
> Even if the DT node is present it is possible that The SCM support for ICE is unavailable in the underlying TZ framework. With the existing logic, qcom_scm_ice_available() would have failed in such cases, returning NULL to storage clients where the clients like storage may keep retrying which potentially can cause boot up issues. This patch corrects that behavior by explicitly returning -EOPNOTSUPP to the clients. I will update the commit message accordingly.

Wouldn't that mean that we have a broken TZ?

And wouldn't this be better solved by simply moving the SCM checks
to of_qcom_ice_get()?

Konrad