Re: [PATCH] soc: qcom: ice: Avoid probe deferring for un-supported ICE feature

[Sumit Garg]

On Fri, Jan 30, 2026 at 01:36:28PM +0100, Konrad Dybcio wrote:
> On 1/30/26 1:29 PM, Sumit Garg wrote:
> > On Fri, Jan 30, 2026 at 12:55:53PM +0100, Konrad Dybcio wrote:
> >> On 1/30/26 12:36 PM, Sumit Garg wrote:
> >>> On Fri, Jan 30, 2026 at 10:59:18AM +0100, Konrad Dybcio wrote:
> >>>> On 1/30/26 10:52 AM, Sumit Garg wrote:
> >>>>> On Fri, Jan 30, 2026 at 10:34:26AM +0100, Konrad Dybcio wrote:
> >>>>>> On 1/30/26 10:11 AM, Sumit Garg wrote:
> >>>>>>> From: Sumit Garg <sumit.garg@xxxxxxxxxxxxxxxx>
> >>
> >> [...]
> >>
> >>>>> Since qcom,ufs depends on qcom,ice via a phandle, so isn't the probe
> >>>>> orderering automatically taken care off? Or that isn't the case here?
> >>>>
> >>>> No, that's guaranteed by devlink only with certain properties.
> >>>
> >>> Okay I see. The other alternate solution I can come up is following to
> >>> keep the deferred probing intact. Let me know your thoughts on this:
> >>>
> >>> diff --git a/drivers/soc/qcom/ice.c b/drivers/soc/qcom/ice.c
> >>> index ab9586b8caf5..76bf9f94fbaf 100644
> >>> --- a/drivers/soc/qcom/ice.c
> >>> +++ b/drivers/soc/qcom/ice.c
> >>> @@ -559,7 +559,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(-ENODEV);
> >>>         }
> >>>
> >>>         engine = devm_kzalloc(dev, sizeof(*engine), GFP_KERNEL);
> >>> @@ -648,11 +648,14 @@ static struct qcom_ice *of_qcom_ice_get(struct device *dev)
> >>>         }
> >>>
> >>>         ice = platform_get_drvdata(pdev);
> >>> -       if (!ice) {
> >>> +       if (IS_ERR_OR_NULL(ice)) {
> >>>                 dev_err(dev, "Cannot get ice instance from %s\n",
> >>>                         dev_name(&pdev->dev));
> >>>                 platform_device_put(pdev);
> >>> -               return NULL;
> >>> +               if (PTR_ERR(ice) == -ENODEV)
> >>> +                       return NULL;
> >>> +               else
> >>> +                       return ERR_PTR(-EPROBE_DEFER);
> >>>         }
> >>>
> >>>         link = device_link_add(dev, &pdev->dev, DL_FLAG_AUTOREMOVE_SUPPLIER);
> >>> @@ -726,7 +729,7 @@ static int qcom_ice_probe(struct platform_device *pdev)
> >>>         }
> >>>
> >>>         engine = qcom_ice_create(&pdev->dev, base);
> >>> -       if (IS_ERR(engine))
> >>> +       if (IS_ERR(engine) && PTR_ERR(engine) != -ENODEV)
> >>>                 return PTR_ERR(engine);
> >>>
> >>>         platform_set_drvdata(pdev, engine);
> >>
> >> This looks more robust. Although the UFS and MMC drivers today check
> >> for EOPNOTSUPP, so perhaps throwing that would be even better
> > 
> > Sure, I can use that error code instead.
> > 
> >>
> >>>
> >>>> In this case though, I think it could make sense to add it to the
> >>>> "suppliers" list in drivers/of/property.c.
> >>>>
> >>>> I don't know if vendors adding their custom properties there is a
> >>>> pattern that +Rob will be happy about though..
> >>>
> >>> Not sure if that's a shorter path as I would like to see fix for this
> >>> issue backported as well.
> >>>
> >>> Aside, not sure how much stable ICE feature itself is as I got following
> >>> crash with QLI boot firmware on Kodiak:
> >>>
> >>> [    5.172970] SError Interrupt on CPU6, code 0x00000000be000000 -- SError
> >>> [    5.172986] CPU: 6 UID: 0 PID: 241 Comm: (udev-worker) Tainted: G   M                6.19.0-rc5-next-20260115-gc1a0fee87a05 #9 PREEMPT 
> >>> [    5.172996] Tainted: [M]=MACHINE_CHECK
> >>> [    5.172999] Hardware name: Qualcomm Technologies, Inc. Robotics RB3gen2 (DT)
> >>> [    5.173003] pstate: 80400005 (Nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> >>> [    5.173010] pc : qcom_ice_create.part.0+0x6c/0x24c [qcom_ice]
> >>> [    5.173024] lr : qcom_ice_create.part.0+0xe4/0x24c [qcom_ice]
> >>
> >> Could you please decode the pc value with ./scripts/faddr2line?
> >> My compiler produces different output
> >>
> >> ./scripts/faddr2line vmlinux(or path to .ko) <symbol_name>
> >>
> > 
> > This points at:
> > 
> > static bool qcom_ice_check_supported(struct qcom_ice *ice)
> > {
> > -->   u32 regval = qcom_ice_readl(ice, QCOM_ICE_REG_VERSION);
> > 
> > $ ./scripts/faddr2line ./drivers/soc/qcom/qcom_ice.ko qcom_ice_create.part.0+0x6c/0x24c
> > qcom_ice_create.part.0+0x6c/0x24c:
> > readl at /home/sumit/build/upstream/linux/./include/asm-generic/io.h:232 (discriminator 1)
> > (inlined by) qcom_ice_check_supported at /home/sumit/build/upstream/linux/drivers/soc/qcom/ice.c:118 (discriminator 1)
> > (inlined by) qcom_ice_create at /home/sumit/build/upstream/linux/drivers/soc/qcom/ice.c:587 (discriminator 1)
> > 
> > To me it looks like an issue related to access control policy. Note that
> > it's the Gunyah based stack only where this issue is seen.
> 
> Please first check if that's not just a lack of power, because apparently
> we never guaranteed that would be present, see:
> 
> https://lore.kernel.org/linux-arm-msm/20260123-qcom_ice_power_and_clk_vote-v1-0-e9059776f85c@xxxxxxxxxxxxxxxx/
> 
> https://lore.kernel.org/linux-arm-msm/20260123-enable-ufs-ice-clock-scaling-v3-0-d0d8532abd98@xxxxxxxxxxxxxxxx/

Thanks for these references. TBH, if there are some known issues with
ICE then it shouldn't be enabled in the common defconfig. BTW, it looks
like Mani is already looking into proper fix for this issue as per
offline communication.

-Sumit