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

[Manivannan Sadhasivam]

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

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