1. Patchew
  2. linux
  3. [PATCH v4 0/5] soc: qcom: ice: Fix race between qcom_ice_probe() and of_qcom_ice_get()
  4. View patch

[PATCH v4 3/5] soc: qcom: ice: Return proper error codes from devm_of_qcom_ice_get() instead of NULL

Manivannan Sadhasivam via B4 Relay posted 5 patches 1 month, 1 week ago
There is a newer version of this series
[PATCH v4 3/5] soc: qcom: ice: Return proper error codes from devm_of_qcom_ice_get() instead of NULL
Posted by Manivannan Sadhasivam via B4 Relay 1 month, 1 week ago 
From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>

devm_of_qcom_ice_get() currently returns NULL if ICE SCM is not available
or "qcom,ice" property is not found in DT. But this confuses the clients
since NULL doesn't convey the reason for failure. So return proper error
codes instead of NULL.

Reported-by: Sumit Garg <sumit.garg@oss.qualcomm.com>
Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
---
 drivers/soc/qcom/ice.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/soc/qcom/ice.c b/drivers/soc/qcom/ice.c
index 833d23dc7b06..d1efc676b63c 100644
--- a/drivers/soc/qcom/ice.c
+++ b/drivers/soc/qcom/ice.c
@@ -561,7 +561,7 @@ static struct qcom_ice *qcom_ice_create(struct device *dev,
 
 	if (!qcom_scm_ice_available()) {
 		dev_warn(dev, "ICE SCM interface not found\n");
-		return NULL;
+		return ERR_PTR(-EOPNOTSUPP);
 	}
 
 	engine = devm_kzalloc(dev, sizeof(*engine), GFP_KERNEL);
@@ -643,7 +643,7 @@ static struct qcom_ice *of_qcom_ice_get(struct device *dev)
 	struct device_node *node __free(device_node) = of_parse_phandle(dev->of_node,
 									"qcom,ice", 0);
 	if (!node)
-		return NULL;
+		return ERR_PTR(-ENODEV);
 
 	pdev = of_find_device_by_node(node);
 	if (!pdev) {
@@ -696,8 +696,7 @@ static void devm_of_qcom_ice_put(struct device *dev, void *res)
  * phandle via 'qcom,ice' property to an ICE DT, the ICE instance will already
  * be created and so this function will return that instead.
  *
- * Return: ICE pointer on success, NULL if there is no ICE data provided by the
- * consumer or ERR_PTR() on error.
+ * Return: ICE pointer on success, ERR_PTR() on error.
  */
 struct qcom_ice *devm_of_qcom_ice_get(struct device *dev)
 {
@@ -708,7 +707,7 @@ struct qcom_ice *devm_of_qcom_ice_get(struct device *dev)
 		return ERR_PTR(-ENOMEM);
 
 	ice = of_qcom_ice_get(dev);
-	if (!IS_ERR_OR_NULL(ice)) {
+	if (!IS_ERR(ice)) {
 		*dr = ice;
 		devres_add(dev, dr);
 	} else {

-- 
2.51.0
Re: [PATCH v4 3/5] soc: qcom: ice: Return proper error codes from devm_of_qcom_ice_get() instead of NULL
Posted by Sumit Garg 1 month ago 
Hey Mani,

On Mon, Mar 2, 2026 at 6:30 PM Manivannan Sadhasivam via B4 Relay
<devnull+manivannan.sadhasivam.oss.qualcomm.com@kernel.org> wrote:
>
> From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
>
> devm_of_qcom_ice_get() currently returns NULL if ICE SCM is not available
> or "qcom,ice" property is not found in DT. But this confuses the clients
> since NULL doesn't convey the reason for failure. So return proper error
> codes instead of NULL.
>
> Reported-by: Sumit Garg <sumit.garg@oss.qualcomm.com>
> Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> ---
>  drivers/soc/qcom/ice.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/soc/qcom/ice.c b/drivers/soc/qcom/ice.c
> index 833d23dc7b06..d1efc676b63c 100644
> --- a/drivers/soc/qcom/ice.c
> +++ b/drivers/soc/qcom/ice.c
> @@ -561,7 +561,7 @@ static struct qcom_ice *qcom_ice_create(struct device *dev,
>
>         if (!qcom_scm_ice_available()) {
>                 dev_warn(dev, "ICE SCM interface not found\n");
> -               return NULL;
> +               return ERR_PTR(-EOPNOTSUPP);
>         }

With this patch-set on top of v7.0-rc2, I still see UFS probe failing
when ICE isn't supported with OP-TEE as follows:

[    5.401558] qcom-ice 1d88000.crypto: ICE SCM interface not found
[    5.419482] qcom-ice 1d88000.crypto: probe with driver qcom-ice
failed with error -95
<snip>
[   18.662977] ufshcd-qcom 1d84000.ufshc: freq-table-hz property not specified
[   18.670193] ufshcd-qcom 1d84000.ufshc: ufshcd_populate_vreg: Unable
to find vdd-hba-supply regulator, assuming enabled
[   18.737665] platform 1d84000.ufshc: deferred probe pending:
ufshcd-qcom: ufshcd_pltfrm_init() failed
[   18.747141] platform 3370000.codec: deferred probe pending:
platform: wait for supplier /soc@0/pinctrl@33c0000/dmic23-data-state

Maybe it's the "qcom-ice" driver failure leading to this deferred
probe problem again.

-Sumit

>
>         engine = devm_kzalloc(dev, sizeof(*engine), GFP_KERNEL);
> @@ -643,7 +643,7 @@ static struct qcom_ice *of_qcom_ice_get(struct device *dev)
>         struct device_node *node __free(device_node) = of_parse_phandle(dev->of_node,
>                                                                         "qcom,ice", 0);
>         if (!node)
> -               return NULL;
> +               return ERR_PTR(-ENODEV);
>
>         pdev = of_find_device_by_node(node);
>         if (!pdev) {
> @@ -696,8 +696,7 @@ static void devm_of_qcom_ice_put(struct device *dev, void *res)
>   * phandle via 'qcom,ice' property to an ICE DT, the ICE instance will already
>   * be created and so this function will return that instead.
>   *
> - * Return: ICE pointer on success, NULL if there is no ICE data provided by the
> - * consumer or ERR_PTR() on error.
> + * Return: ICE pointer on success, ERR_PTR() on error.
>   */
>  struct qcom_ice *devm_of_qcom_ice_get(struct device *dev)
>  {
> @@ -708,7 +707,7 @@ struct qcom_ice *devm_of_qcom_ice_get(struct device *dev)
>                 return ERR_PTR(-ENOMEM);
>
>         ice = of_qcom_ice_get(dev);
> -       if (!IS_ERR_OR_NULL(ice)) {
> +       if (!IS_ERR(ice)) {
>                 *dr = ice;
>                 devres_add(dev, dr);
>         } else {
>
> --
> 2.51.0
>
>
Re: [PATCH v4 3/5] soc: qcom: ice: Return proper error codes from devm_of_qcom_ice_get() instead of NULL
Posted by Sumit Garg 1 month ago 
On Fri, Mar 6, 2026 at 2:17 PM Sumit Garg <sumit.garg@oss.qualcomm.com> wrote:
>
> Hey Mani,
>
> On Mon, Mar 2, 2026 at 6:30 PM Manivannan Sadhasivam via B4 Relay
> <devnull+manivannan.sadhasivam.oss.qualcomm.com@kernel.org> wrote:
> >
> > From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> >
> > devm_of_qcom_ice_get() currently returns NULL if ICE SCM is not available
> > or "qcom,ice" property is not found in DT. But this confuses the clients
> > since NULL doesn't convey the reason for failure. So return proper error
> > codes instead of NULL.
> >
> > Reported-by: Sumit Garg <sumit.garg@oss.qualcomm.com>
> > Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> > ---
> >  drivers/soc/qcom/ice.c | 9 ++++-----
> >  1 file changed, 4 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/soc/qcom/ice.c b/drivers/soc/qcom/ice.c
> > index 833d23dc7b06..d1efc676b63c 100644
> > --- a/drivers/soc/qcom/ice.c
> > +++ b/drivers/soc/qcom/ice.c
> > @@ -561,7 +561,7 @@ static struct qcom_ice *qcom_ice_create(struct device *dev,
> >
> >         if (!qcom_scm_ice_available()) {
> >                 dev_warn(dev, "ICE SCM interface not found\n");
> > -               return NULL;
> > +               return ERR_PTR(-EOPNOTSUPP);
> >         }
>
> With this patch-set on top of v7.0-rc2, I still see UFS probe failing
> when ICE isn't supported with OP-TEE as follows:
>
> [    5.401558] qcom-ice 1d88000.crypto: ICE SCM interface not found
> [    5.419482] qcom-ice 1d88000.crypto: probe with driver qcom-ice
> failed with error -95
> <snip>
> [   18.662977] ufshcd-qcom 1d84000.ufshc: freq-table-hz property not specified
> [   18.670193] ufshcd-qcom 1d84000.ufshc: ufshcd_populate_vreg: Unable
> to find vdd-hba-supply regulator, assuming enabled
> [   18.737665] platform 1d84000.ufshc: deferred probe pending:
> ufshcd-qcom: ufshcd_pltfrm_init() failed
> [   18.747141] platform 3370000.codec: deferred probe pending:
> platform: wait for supplier /soc@0/pinctrl@33c0000/dmic23-data-state
>
> Maybe it's the "qcom-ice" driver failure leading to this deferred
> probe problem again.
>

Following diff on top of your patchset allows the UFS driver to probe
successfully without ICE support. I suppose just setting the drvdata
should be sufficient.

diff --git a/drivers/soc/qcom/ice.c b/drivers/soc/qcom/ice.c
index d1efc676b63c..a86980647097 100644
--- a/drivers/soc/qcom/ice.c
+++ b/drivers/soc/qcom/ice.c
@@ -734,12 +734,6 @@ static int qcom_ice_probe(struct platform_device *pdev)
        }

        engine = qcom_ice_create(&pdev->dev, base);
-       if (IS_ERR(engine)) {
-               /* Store the error pointer for devm_of_qcom_ice_get() */
-               platform_set_drvdata(pdev, engine);
-               return PTR_ERR(engine);
-       }
-
        platform_set_drvdata(pdev, engine);

-Sumit

>
> >
> >         engine = devm_kzalloc(dev, sizeof(*engine), GFP_KERNEL);
> > @@ -643,7 +643,7 @@ static struct qcom_ice *of_qcom_ice_get(struct device *dev)
> >         struct device_node *node __free(device_node) = of_parse_phandle(dev->of_node,
> >                                                                         "qcom,ice", 0);
> >         if (!node)
> > -               return NULL;
> > +               return ERR_PTR(-ENODEV);
> >
> >         pdev = of_find_device_by_node(node);
> >         if (!pdev) {
> > @@ -696,8 +696,7 @@ static void devm_of_qcom_ice_put(struct device *dev, void *res)
> >   * phandle via 'qcom,ice' property to an ICE DT, the ICE instance will already
> >   * be created and so this function will return that instead.
> >   *
> > - * Return: ICE pointer on success, NULL if there is no ICE data provided by the
> > - * consumer or ERR_PTR() on error.
> > + * Return: ICE pointer on success, ERR_PTR() on error.
> >   */
> >  struct qcom_ice *devm_of_qcom_ice_get(struct device *dev)
> >  {
> > @@ -708,7 +707,7 @@ struct qcom_ice *devm_of_qcom_ice_get(struct device *dev)
> >                 return ERR_PTR(-ENOMEM);
> >
> >         ice = of_qcom_ice_get(dev);
> > -       if (!IS_ERR_OR_NULL(ice)) {
> > +       if (!IS_ERR(ice)) {
> >                 *dr = ice;
> >                 devres_add(dev, dr);
> >         } else {
> >
> > --
> > 2.51.0
> >
> >
Re: [PATCH v4 3/5] soc: qcom: ice: Return proper error codes from devm_of_qcom_ice_get() instead of NULL
Posted by Manivannan Sadhasivam 1 month ago 
On Fri, Mar 06, 2026 at 02:30:02PM +0530, Sumit Garg wrote:
> On Fri, Mar 6, 2026 at 2:17 PM Sumit Garg <sumit.garg@oss.qualcomm.com> wrote:
> >
> > Hey Mani,
> >
> > On Mon, Mar 2, 2026 at 6:30 PM Manivannan Sadhasivam via B4 Relay
> > <devnull+manivannan.sadhasivam.oss.qualcomm.com@kernel.org> wrote:
> > >
> > > From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> > >
> > > devm_of_qcom_ice_get() currently returns NULL if ICE SCM is not available
> > > or "qcom,ice" property is not found in DT. But this confuses the clients
> > > since NULL doesn't convey the reason for failure. So return proper error
> > > codes instead of NULL.
> > >
> > > Reported-by: Sumit Garg <sumit.garg@oss.qualcomm.com>
> > > Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> > > ---
> > >  drivers/soc/qcom/ice.c | 9 ++++-----
> > >  1 file changed, 4 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/soc/qcom/ice.c b/drivers/soc/qcom/ice.c
> > > index 833d23dc7b06..d1efc676b63c 100644
> > > --- a/drivers/soc/qcom/ice.c
> > > +++ b/drivers/soc/qcom/ice.c
> > > @@ -561,7 +561,7 @@ static struct qcom_ice *qcom_ice_create(struct device *dev,
> > >
> > >         if (!qcom_scm_ice_available()) {
> > >                 dev_warn(dev, "ICE SCM interface not found\n");
> > > -               return NULL;
> > > +               return ERR_PTR(-EOPNOTSUPP);
> > >         }
> >
> > With this patch-set on top of v7.0-rc2, I still see UFS probe failing
> > when ICE isn't supported with OP-TEE as follows:
> >
> > [    5.401558] qcom-ice 1d88000.crypto: ICE SCM interface not found
> > [    5.419482] qcom-ice 1d88000.crypto: probe with driver qcom-ice
> > failed with error -95
> > <snip>
> > [   18.662977] ufshcd-qcom 1d84000.ufshc: freq-table-hz property not specified
> > [   18.670193] ufshcd-qcom 1d84000.ufshc: ufshcd_populate_vreg: Unable
> > to find vdd-hba-supply regulator, assuming enabled
> > [   18.737665] platform 1d84000.ufshc: deferred probe pending:
> > ufshcd-qcom: ufshcd_pltfrm_init() failed
> > [   18.747141] platform 3370000.codec: deferred probe pending:
> > platform: wait for supplier /soc@0/pinctrl@33c0000/dmic23-data-state
> >
> > Maybe it's the "qcom-ice" driver failure leading to this deferred
> > probe problem again.
> >

Urgh... I completely forgot that the driver core removes the drvdata during
probe error >.<

> 
> Following diff on top of your patchset allows the UFS driver to probe
> successfully without ICE support. I suppose just setting the drvdata
> should be sufficient.
> 
> diff --git a/drivers/soc/qcom/ice.c b/drivers/soc/qcom/ice.c
> index d1efc676b63c..a86980647097 100644
> --- a/drivers/soc/qcom/ice.c
> +++ b/drivers/soc/qcom/ice.c
> @@ -734,12 +734,6 @@ static int qcom_ice_probe(struct platform_device *pdev)
>         }
> 
>         engine = qcom_ice_create(&pdev->dev, base);
> -       if (IS_ERR(engine)) {
> -               /* Store the error pointer for devm_of_qcom_ice_get() */
> -               platform_set_drvdata(pdev, engine);
> -               return PTR_ERR(engine);
> -       }
> -

No, this will indicate probe success which we do not want and is not safe all
the time. Like what if qcom_ice_create() returned -EPROBE_DEFER.

Let me just store the ice_handle in a global xarray with key based on node
phandle instead of drvdata. This will ensure that the pointer stays till the
driver is loaded.

- Mani

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

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.