Re: [PATCH v3 1/4] soc: qcom: ice: Fix race between qcom_ice_probe() and of_qcom_ice_get()

[Manivannan Sadhasivam]

On Mon, Mar 02, 2026 at 03:41:54PM +0530, Neeraj Soni wrote:
> 
> 
> On 2/23/2026 1:32 PM, Manivannan Sadhasivam via B4 Relay wrote:
> > From: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxxxxxxxx>
> > 
> > The current platform driver design causes probe ordering races with
> > consumers (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 consumers non-functional even when ICE should
> > be gracefully disabled. devm_of_qcom_ice_get() doesn't 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 consumer drivers
> > as 'qcom,ice' is not considered as a DT 'supplier'. So the consumer drivers
> > have no idea of when the ICE driver is going to probe.
> > 
> > To address these issues, introduce a global ice_handle to store the valid
> > ICE handle pointer, and set during successful ICE driver probe. On probe
> > failure, set it to an error pointer and propagate the error from
> > of_qcom_ice_get().
> > 
> > Additionally, add a global ice_mutex to synchronize qcom_ice_probe() and
> > of_qcom_ice_get().
> > 
> > Note that this change only fixes the standalone ICE DT node bindings and
> > not the ones with 'ice' range embedded in the consumer nodes, where there
> > is no issue.
> > 
> > Cc: <stable@xxxxxxxxxxxxxxx> # 6.4
> > Fixes: 2afbf43a4aec ("soc: qcom: Make the Qualcomm UFS/SDCC ICE a dedicated driver")
> > Reported-by: Sumit Garg <sumit.garg@xxxxxxxxxxxxxxxx>
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxxxxxxxx>
> > ---
> >  drivers/soc/qcom/ice.c | 44 +++++++++++++++++++++++++++-----------------
> >  1 file changed, 27 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/soc/qcom/ice.c b/drivers/soc/qcom/ice.c
> > index b203bc685cad..3c3c189e24f9 100644
> > --- a/drivers/soc/qcom/ice.c
> > +++ b/drivers/soc/qcom/ice.c
> > @@ -113,6 +113,9 @@ struct qcom_ice {
> >  	u8 hwkm_version;
> >  };
> >  
> > +static DEFINE_MUTEX(ice_mutex);
> > +static 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);
> > @@ -608,7 +611,6 @@ static struct qcom_ice *qcom_ice_create(struct device *dev,
> >  static struct qcom_ice *of_qcom_ice_get(struct device *dev)
> >  {
> >  	struct platform_device *pdev = to_platform_device(dev);
> > -	struct qcom_ice *ice;
> >  	struct resource *res;
> >  	void __iomem *base;
> >  	struct device_link *link;
> > @@ -631,6 +633,22 @@ static struct qcom_ice *of_qcom_ice_get(struct device *dev)
> >  		return qcom_ice_create(&pdev->dev, base);
> >  	}
> >  
> > +	guard(mutex)(&ice_mutex);
> > +
> > +	/*
> > +	 * If ice_handle is NULL, then it means the ICE driver is not probed
> > +	 * yet. So return -EPROBE_DEFER to let the client try later.
> > +	 */
> > +	if (!ice_handle)
> > +		return ERR_PTR(-EPROBE_DEFER);
> > +
> > +	/*
> > +	 * If ice_handle has error code, then it means the ICE driver has probe
> > +	 * failed. So return the handle for the client to digest it.
> > +	 */
> > +	if (IS_ERR(ice_handle))
> > +		return ice_handle;
> > +
> >  	/*
> >  	 * If the consumer node does not provider an 'ice' reg range
> >  	 * (legacy DT binding), then it must at least provide a phandle
> > @@ -647,24 +665,16 @@ static struct qcom_ice *of_qcom_ice_get(struct device *dev)
> >  		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));
> > -		platform_device_put(pdev);
> > -		return ERR_PTR(-EPROBE_DEFER);
> > -	}
> > -
> >  	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));
> >  		platform_device_put(pdev);
> > -		ice = ERR_PTR(-EINVAL);
> > +		return ERR_PTR(-EINVAL);
> >  	}
> >  
> > -	return ice;
> > +	return ice_handle;
> >  }
> >  
> >  static void qcom_ice_put(const struct qcom_ice *ice)
> > @@ -716,20 +726,20 @@ EXPORT_SYMBOL_GPL(devm_of_qcom_ice_get);
> >  
> >  static int qcom_ice_probe(struct platform_device *pdev)
> >  {
> > -	struct qcom_ice *engine;
> >  	void __iomem *base;
> >  
> > +	guard(mutex)(&ice_mutex);
> > +
> >  	base = devm_platform_ioremap_resource(pdev, 0);
> >  	if (IS_ERR(base)) {
> >  		dev_warn(&pdev->dev, "ICE registers not found\n");
> > +		ice_handle = base;
> >  		return PTR_ERR(base);
> >  	}
> >  
> > -	engine = qcom_ice_create(&pdev->dev, base);
> > -	if (IS_ERR(engine))
> > -		return PTR_ERR(engine);
> > -
> > -	platform_set_drvdata(pdev, engine);
> 
> This allows the driver to set the data per ICE device instance which allows
> the addition of multiple ICE platform devices. For example this patch:
> https://lore.kernel.org/all/20260217052526.2335759-1-neeraj.soni@xxxxxxxxxxxxxxxx/
> utilizes this capability. I think it doesen't harm to keep this support. 
> Moreover, the issue which your patch intends to address do not need this to be removed.
> 

Hmm, ok. I figured out that if we store the err ptr in drvdata, we can fix the
race and also allow multiple ice instances in a SoC. So sent v4 incorporating
this change.

- Mani

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