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

Eric Biggers
2026-02-13T01:02:55Z

On Tue, Feb 10, 2026 at 12:26:50PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> drivers/soc/qcom/ice.c | 118 ++++++++++++++++++-------------------------------
> 1 file changed, 44 insertions(+), 74 deletions(-)

I don't yet know enough to be confident that this is the correct fix,
but there are a few things I noticed that look like bugs:

> +static DEFINE_MUTEX(ice_mutex);
> +struct qcom_ice *ice_handle;

ice_handle is used only in this file, so it should be static

> @@ -643,41 +645,42 @@ static struct qcom_ice *of_qcom_ice_get(struct device *dev)
[...]
> + ice = qcom_ice_create(&pdev->dev, base);
> + if (IS_ERR(ice)) {
> platform_device_put(pdev);
> - ice = ERR_PTR(-EINVAL);
> + return ice_handle;
> }

This error path returns NULL, where this patch seems to have been
intended to remove NULL as a possible return value.

> -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;
> }

Elsewhere ice_handle is protected by ice_mutex, but this seems to modify
it without holding the mutex.

I'm also wondering what happens if all consumer devices are removed.
platform_device_put() gets executed on the ICE platform_device for each
one, but does that actually drop the last reference and cause the
resources allocated with devm_*() to be freed? On do they stick around
until/unless the ICE device is actually removed as well?

> 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);
> }

Above probably should use the ice local variable, not ice_handle.

- Eric