Re: [PATCH v3 5/5] phy: qcom: snps-femto-v2: Fix possible NULL-deref on early runtime suspend

[Vladimir Oltean <olteanv@gmail.com>]

Hello Loic,

On Fri, Feb 13, 2026 at 04:04:43PM +0100, Loic Poulain wrote:
> On Fri, Feb 13, 2026 at 11:45 AM Johan Hovold <johan@kernel.org> wrote:
> >
> > On Fri, Feb 13, 2026 at 10:45:32AM +0100, Loic Poulain wrote:
> > > On Fri, Feb 13, 2026 at 10:07 AM Johan Hovold <johan@kernel.org> wrote:
> > > >
> > > > On Thu, Feb 05, 2026 at 05:02:40PM +0100, Loic Poulain wrote:
> > > > > Enabling runtime PM before attaching the hsphy instance as driver data
> > > > > can lead to a NULL pointer dereference in runtime PM callbacks that
> > > > > expect valid driver data. There is a small window where the suspend
> > > > > callback may run after PM runtime enabling and before runtime forbid.
> > > >
> > > > So here too, the commit should reflect that this cannot really happen in
> > > > practice.
> > >
> > > This happened  in practice in the qcom‑qusb2 PHY driver, with the same
> > > code flow.
> > > Bug: https://github.com/qualcomm-linux/qcom-deb-images/issues/208
> > > Patch: https://lore.kernel.org/linux-arm-msm/20251219085640.114473-1-loic.poulain@oss.qualcomm.com/
> >
> > Thanks for the link.
> >
> > > I know it may sound unlikely, but this crash has been reported
> > > several times during boot‑stress testing. I haven’t investigated
> > > deeply enough to determine whether it’s caused by an unfortunate
> > > preemption window or a racing CPU.
> >
> > But I'm literally asking for *what* would trigger the suspend in that
> > initial window between enable() and forbid() cause I don't see it.
> 
> To be honest, I had not initially looked into the exact cause of the
> suspend trigger until now, but here is what is happening.
> 
> The PHY is a supplier of the USB device. A USB device cannot be probed
> until all its suppliers are ready. As long as the PHY is not ready, the
> device core keeps retrying the probe, which fails with -EPROBE_DEFER.
> 
> At some point the PHY probe finally runs, but the device core may still be
> attempting to probe the USB device concurrently.
> 
> Inside __driver_probe_device(), we have:
> 
>     ret = really_probe(dev, drv);
>     pm_request_idle(dev);
> 
>     if (dev->parent)
>         pm_runtime_put(dev->parent);
> 
>     pm_runtime_put_suppliers(dev);
>     return ret;
> 
> This means that whenever a USB probe attempt completes, whether with an
> error or not, its suppliers are released via pm_runtime_put_suppliers().
> Releasing suppliers may in turn trigger a runtime suspend.
> 
> In our case, since the PHY is a supplier of the USB device, the USB core
> keeps 'looping' in __driver_probe_device() returning -EPROBE_DEFER until
> the PHY becomes ready. As a result, pm_runtime_put_suppliers() may run
> concurrently with the PHY's probe function. If this happens after
> runtime PM has been enabled for the PHY, but before the driver has
> forbidden suspend or taken a PM reference, the PHY may end up being
> runtime-suspended 'unexpectedly'.

Please resend this patch with the commit message including this
explanation (note that your code snippet from __driver_probe_device() is
missing a relevant call to pm_runtime_get_suppliers()).

Also, please separate the devres change to its own patch. It is fixing a
different logical issue (missing pm_runtime_disable() causes device with
unbound driver to have non-zero dev->power.disable_depth; should warn on
driver re-probe).

Another comment upon reviewing this driver's runtime PM use (although
this is at most something that may result in a patch for "next"):

This driver uses hsphy->phy_initialized to make sure qcom_snps_hsphy_suspend()
isn't called unless qcom_snps_hsphy_init() was called.

Don't we achieve the same behaviour by replacing "hsphy->phy_initialized = true"
with pm_runtime_get_sync(dev) and "hsphy->phy_initialized = false" with
pm_runtime_put(dev)?

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy