Re: [PATCH] soc: qcom: ice: Remove platform_driver support and expose as a pure library

[Neeraj Soni]

On 2/18/2026 7:11 PM, Manivannan Sadhasivam wrote:
> On Tue, Feb 17, 2026 at 10:03:27AM +0530, Neeraj Soni wrote:
>>
>>
>> On 2/3/2026 1:40 PM, Manivannan Sadhasivam wrote:
>>> On Tue, Feb 03, 2026 at 01:37:12PM +0530, Manivannan Sadhasivam wrote:
>>>> The current platform driver design causes probe ordering races with clients
>>>> (UFS, eMMC) due to ICE's dependency on SCM firmware calls. If ICE probe
>>>> fails (missing ICE SCM or DT registers), devm_of_qcom_ice_get() loops with
>>>> -EPROBE_DEFER, leaving clients non-functional even when ICE should be
>>>> gracefully disabled. devm_of_qcom_ice_get() cannot know if the ICE driver
>>>> probe has failed due to above reasons or it is waiting for the SCM driver.
>>>>
>>>> Moreover, there is no devlink dependency between ICE and client drivers
>>>> as 'qcom,ice' is not considered as a DT 'supplier'. So the client drivers
>>>> have no idea of when the ICE driver is going to probe.
>>>>
>>>> To avoid all this hassle, remove the platform driver support altogether and
>>>> just expose the ICE driver as a pure library to client drivers. With this
>>>> design, when devm_of_qcom_ice_get() is called, it will check if the ICE
>>>> instance is available or not. If not, it will create one based on the ICE
>>>> DT node, increase the refcount and return the handle. When the next client
>>>> calls the API again, the ICE instance would be available. So this function
>>>> will just increment the refcount and return the instance.
>>>>
>>>> Finally, when the client devices get destroyed, refcount will be
>>>> decremented and finally the cleanup will happen once all clients are
>>>> destroyed.
>>>>
>>>> For the clients using the old DT binding of providing the separate 'ice'
>>>> register range in their node, this change has no impact.
>>>>
>>>> Cc: stable@xxxxxxxxxxxxxxx
>>>> Cc: Abel Vesa <abel.vesa@xxxxxxxxxxxxxxxx>
>>>> Reported-by: Sumit Garg <sumit.garg@xxxxxxxxxxxxxxxx>
>>>> Fixes: 2afbf43a4aec ("soc: qcom: Make the Qualcomm UFS/SDCC ICE a dedicated driver")
>>>> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxxxxxxxx>
>>>> ---
>>>>  drivers/soc/qcom/ice.c | 100 ++++++++++++++++-------------------------
>>>>  1 file changed, 39 insertions(+), 61 deletions(-)
>>>>
>>>> diff --git a/drivers/soc/qcom/ice.c b/drivers/soc/qcom/ice.c
>>>> index b203bc685cad..b5a9cf8de6e4 100644
>>>> --- a/drivers/soc/qcom/ice.c
>>>> +++ b/drivers/soc/qcom/ice.c
>>>> @@ -107,12 +107,16 @@ struct qcom_ice {
>>>>  	struct device *dev;
>>>>  	void __iomem *base;
>>>>  
>>>> +	struct kref refcount;
>>>>  	struct clk *core_clk;
>>>>  	bool use_hwkm;
>>>>  	bool hwkm_init_complete;
>>>>  	u8 hwkm_version;
>>>>  };
>>>>  
>>>> +static DEFINE_MUTEX(ice_mutex);
>>>> +struct qcom_ice *ice_handle;
>>>> +
>>>>  static bool qcom_ice_check_supported(struct qcom_ice *ice)
>>>>  {
>>>>  	u32 regval = qcom_ice_readl(ice, QCOM_ICE_REG_VERSION);
>>>> @@ -599,8 +603,8 @@ static struct qcom_ice *qcom_ice_create(struct device *dev,
>>>>   * This function will provide an ICE instance either by creating one for the
>>>>   * consumer device if its DT node provides the 'ice' reg range and the 'ice'
>>>>   * clock (for legacy DT style). On the other hand, if consumer provides a
>>>> - * phandle via 'qcom,ice' property to an ICE DT, the ICE instance will already
>>>> - * be created and so this function will return that instead.
>>>> + * phandle via 'qcom,ice' property to an ICE DT node, then the ICE instance will
>>>> + * be created if not already done and will be returned.
>>>>   *
>>>>   * Return: ICE pointer on success, NULL if there is no ICE data provided by the
>>>>   * consumer or ERR_PTR() on error.
>>>> @@ -611,11 +615,12 @@ static struct qcom_ice *of_qcom_ice_get(struct device *dev)
>>>>  	struct qcom_ice *ice;
>>>>  	struct resource *res;
>>>>  	void __iomem *base;
>>>> -	struct device_link *link;
>>>>  
>>>>  	if (!dev || !dev->of_node)
>>>>  		return ERR_PTR(-ENODEV);
>>>>  
>>>> +	guard(mutex)(&ice_mutex);
>>>> +
>>>>  	/*
>>>>  	 * In order to support legacy style devicetree bindings, we need
>>>>  	 * to create the ICE instance using the consumer device and the reg
>>>> @@ -631,6 +636,16 @@ static struct qcom_ice *of_qcom_ice_get(struct device *dev)
>>>>  		return qcom_ice_create(&pdev->dev, base);
>>>>  	}
>>>>  
>>>> +	/*
>>>> +	 * If the ICE node has been initialized already, just increase the
>>>> +	 * refcount and return the handle.
>>>> +	 */
>>>> +	if (ice_handle) {
>>>> +		kref_get(&ice_handle->refcount);
>>>> +
>>>> +		return ice_handle;
>>
>> How will this work for a device using both UFS and eMMC storage (one being primary storage
>> and other being secondary)? UFS and eMMC will have seperate ICE DT node so returning same
>> handle to both clients will not be correct.
>>
> 
> I'm not aware of any platforms using separate ICE nodes. All are using shared
> node only. Which platform are you referring to?

Example talos uses both UFS and eMMC:
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/arch/arm64/boot/dts/qcom/talos.dtsi#n699
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/arch/arm64/boot/dts/qcom/talos.dtsi#n1398

For few more targets where eMMC is used along with UFS, the patches to add ICE handle for eMMC is in flight with this patch:
https://lore.kernel.org/all/20260217052526.2335759-1-neeraj.soni@xxxxxxxxxxxxxxxx/

and more are planned to be added.

> 
> - Mani
> 
>>>> +	}
>>>> +
>>>>  	/*
>>>>  	 * If the consumer node does not provider an 'ice' reg range
>>>>  	 * (legacy DT binding), then it must at least provide a phandle
>>>> @@ -643,41 +658,43 @@ static struct qcom_ice *of_qcom_ice_get(struct device *dev)
>>>>  
>>>>  	pdev = of_find_device_by_node(node);
>>>>  	if (!pdev) {
>>>> -		dev_err(dev, "Cannot find device node %s\n", node->name);
>>>> +		dev_err(dev, "Cannot find ICE platform device\n");
>>>> +		platform_device_put(pdev);
>>>
>>> This somehow slipped in...
>>>
>>> - Mani
>>>
>>>>  		return ERR_PTR(-EPROBE_DEFER);
>>>>  	}
>>>>  
>>>> -	ice = platform_get_drvdata(pdev);
>>>> -	if (!ice) {
>>>> -		dev_err(dev, "Cannot get ice instance from %s\n",
>>>> -			dev_name(&pdev->dev));
>>>> +	base = devm_platform_ioremap_resource(pdev, 0);
>>>> +	if (IS_ERR(base)) {
>>>> +		dev_warn(&pdev->dev, "ICE registers not found\n");
>>>>  		platform_device_put(pdev);
>>>> -		return ERR_PTR(-EPROBE_DEFER);
>>>> +		return base;
>>>>  	}
>>>>  
>>>> -	link = device_link_add(dev, &pdev->dev, DL_FLAG_AUTOREMOVE_SUPPLIER);
>>>> -	if (!link) {
>>>> -		dev_err(&pdev->dev,
>>>> -			"Failed to create device link to consumer %s\n",
>>>> -			dev_name(dev));
>>>> +	ice = qcom_ice_create(&pdev->dev, base);
>>>> +	if (IS_ERR(ice)) {
>>>>  		platform_device_put(pdev);
>>>> -		ice = ERR_PTR(-EINVAL);
>>>> +		return ice_handle;
>>>>  	}
>>>>  
>>>> -	return ice;
>>>> +	ice_handle = ice;
>>>> +	kref_init(&ice_handle->refcount);
>>>> +
>>>> +	return ice_handle;
>>>>  }
>>>>  
>>>> -static void qcom_ice_put(const struct qcom_ice *ice)
>>>> +static void qcom_ice_put(struct kref *kref)
>>>>  {
>>>> -	struct platform_device *pdev = to_platform_device(ice->dev);
>>>> -
>>>> -	if (!platform_get_resource_byname(pdev, IORESOURCE_MEM, "ice"))
>>>> -		platform_device_put(pdev);
>>>> +	platform_device_put(to_platform_device(ice_handle->dev));
>>>> +	ice_handle = NULL;
>>>>  }
>>>>  
>>>>  static void devm_of_qcom_ice_put(struct device *dev, void *res)
>>>>  {
>>>> -	qcom_ice_put(*(struct qcom_ice **)res);
>>>> +	const struct qcom_ice *ice = *(struct qcom_ice **)res;
>>>> +	struct platform_device *pdev = to_platform_device(ice->dev);
>>>> +
>>>> +	if (!platform_get_resource_byname(pdev, IORESOURCE_MEM, "ice"))
>>>> +		kref_put(&ice_handle->refcount, qcom_ice_put);
>>>>  }
>>>>  
>>>>  /**
>>>> @@ -713,42 +730,3 @@ struct qcom_ice *devm_of_qcom_ice_get(struct device *dev)
>>>>  	return ice;
>>>>  }
>>>>  EXPORT_SYMBOL_GPL(devm_of_qcom_ice_get);
>>>> -
>>>> -static int qcom_ice_probe(struct platform_device *pdev)
>>>> -{
>>>> -	struct qcom_ice *engine;
>>>> -	void __iomem *base;
>>>> -
>>>> -	base = devm_platform_ioremap_resource(pdev, 0);
>>>> -	if (IS_ERR(base)) {
>>>> -		dev_warn(&pdev->dev, "ICE registers not found\n");
>>>> -		return PTR_ERR(base);
>>>> -	}
>>>> -
>>>> -	engine = qcom_ice_create(&pdev->dev, base);
>>>> -	if (IS_ERR(engine))
>>>> -		return PTR_ERR(engine);
>>>> -
>>>> -	platform_set_drvdata(pdev, engine);
>>>> -
>>>> -	return 0;
>>>> -}
>>>> -
>>>> -static const struct of_device_id qcom_ice_of_match_table[] = {
>>>> -	{ .compatible = "qcom,inline-crypto-engine" },
>>>> -	{ },
>>>> -};
>>>> -MODULE_DEVICE_TABLE(of, qcom_ice_of_match_table);
>>>> -
>>>> -static struct platform_driver qcom_ice_driver = {
>>>> -	.probe	= qcom_ice_probe,
>>>> -	.driver = {
>>>> -		.name = "qcom-ice",
>>>> -		.of_match_table = qcom_ice_of_match_table,
>>>> -	},
>>>> -};
>>>> -
>>>> -module_platform_driver(qcom_ice_driver);
>>>> -
>>>> -MODULE_DESCRIPTION("Qualcomm Inline Crypto Engine driver");
>>>> -MODULE_LICENSE("GPL");
>>>> -- 
>>>> 2.51.0
>>>>
>>>
>> Regards
>> Neeraj
> 
Regards,
Neeraj