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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <s26gqvmc6wsifibgepaesaveyztnqp2s4ra4cbfmpv5s4sx674@nxxko33urctv>
Date: Mon, 27 Oct 2025 13:26:42 -0500
From: Bjorn Andersson <andersson@...nel.org>
To: Debraj Mukhopadhyay <quic_dmukhopa@...cinc.com>
Cc: quic_neersoni@...cinc.com, konradybcio@...nel.org, 
	linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] qcom: ice: Prevent client probe failures on unsupported
 ICE

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@...cinc.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
>