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

Manivannan Sadhasivam
2026-02-13T02:17:46Z

On Thu, Feb 12, 2026 at 05:02:53PM -0800, Eric Biggers wrote:
> 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.
>

Oops... I should return 'ice' here, not '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;
> > }
>
> Elsewhere ice_handle is protected by ice_mutex, but this seems to modify
> it without holding the mutex.
>

I'll add it.

> 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?
>

No, they'll stick around because the platform device itself won't be removed.
of_qcom_ice_get() increments the refcount due to of_find_device_by_node() and we
keep the refcount as we expect the platform device to be available until all the
consumers call devm_of_qcom_ice_put(). Then we'll finally drop our own refcount.

But I get your concern that we are not freeing the ioremap.

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

Ack.

- Mani

--
மணிவண்ணன் சதாசிவம்