[PATCH v3] phy: qcom-qusb2: Fix NULL pointer dereference on early suspend

[Dmitry Baryshkov]

On Mon, Dec 15, 2025 at 09:49:21AM +0100, Loic Poulain wrote:
> Hi Dmitry,
> 
> On Fri, Dec 12, 2025 at 8:30 PM Dmitry Baryshkov
> <dmitry.baryshkov at oss.qualcomm.com> wrote:
> >
> > On Thu, Dec 11, 2025 at 04:35:36PM +0100, Loic Poulain wrote:
> > > Enabling runtime PM before attaching the QPHY 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.
> > > This causes a sporadic crash during boot:
> > >
> > > ```
> > > Unable to handle kernel NULL pointer dereference at virtual address 00000000000000a1
> > > [...]
> > > CPU: 0 UID: 0 PID: 11 Comm: kworker/0:1 Not tainted 6.16.7+ #116 PREEMPT
> > > Workqueue: pm pm_runtime_work
> > > pstate: 20000005 (nzCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> > > pc : qusb2_phy_runtime_suspend+0x14/0x1e0 [phy_qcom_qusb2]
> > > lr : pm_generic_runtime_suspend+0x2c/0x44
> > > [...]
> > > ```
> > >
> > > Attach the QPHY instance as driver data before enabling runtime PM to
> > > prevent NULL pointer dereference in runtime PM callbacks.
> > >
> > > Reorder pm_runtime_enable() and pm_runtime_forbid() to prevent a
> > > short window where an unnecessary runtime suspend can occur.
> > >
> > > Use the devres-managed version to ensure PM runtime is symmetrically
> > > disabled during driver removal for proper cleanup.
> > >
> > > Fixes: 891a96f65ac3 ("phy: qcom-qusb2: Add support for runtime PM")
> > > Signed-off-by: Loic Poulain <loic.poulain at oss.qualcomm.com>
> > > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov at oss.qualcomm.com>
> > > ---
> > >  v3: The phy-core expects parent's runtime-pm to be setup before creating
> > >      the phy, so move back runtime-pm to its initial location and
> > >      relocate dev_set_drvdata() instead.
> > >
> > >  v2: Move runtime-pm enabling after dev_set_drvdata
> > >      use devm_pm_runtime_enable() to fix unbalanced enabling on removal
> > >      reword commit message
> > >
> > >  drivers/phy/qualcomm/phy-qcom-qusb2.c | 14 ++++++--------
> > >  1 file changed, 6 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/phy/qualcomm/phy-qcom-qusb2.c b/drivers/phy/qualcomm/phy-qcom-qusb2.c
> > > index b5514a32ff8f..6386d834b0e4 100644
> > > --- a/drivers/phy/qualcomm/phy-qcom-qusb2.c
> > > +++ b/drivers/phy/qualcomm/phy-qcom-qusb2.c
> > > @@ -1093,29 +1093,27 @@ static int qusb2_phy_probe(struct platform_device *pdev)
> > >               or->hsdisc_trim.override = true;
> > >       }
> > >
> > > -     pm_runtime_set_active(dev);
> > > -     pm_runtime_enable(dev);
> > > +     dev_set_drvdata(dev, qphy);
> > > +
> > >       /*
> > > -      * Prevent runtime pm from being ON by default. Users can enable
> > > -      * it using power/control in sysfs.
> > > +      * Enable runtime PM support, but forbid it by default.
> > > +      * Users can allow it again via the power/control attribute in sysfs.
> > >        */
> > > +     pm_runtime_set_active(dev);
> > >       pm_runtime_forbid(dev);
> >
> > I think, this should be called after enabling runtime PM. Please correct
> > me if I'm wrong.
> 
> enable/disable and forbid/allow appear to be orthogonal, the purpose
> of inverting them here is to prevent an unnecessary suspend/resume
> cycle from occurring between enable and forbid. Also, this sequence
> has been tested and works correctly.

I see that in most of the places pm_runtime_forbid() is being called
after pm_runtime_enable() rather than before. It makes sense since
forbid is defined as: "Resume @dev if already suspended and block
runtime suspend". It's not clear what _is_ the behaviour if runtime PM
is disabled and will be enabled later.

> 
> >
> > > +     devm_pm_runtime_enable(dev);
> >
> > Missing error handling
> 
> I wasn’t certain whether we should abort on this non-fatal error,
> since the driver can operate without PM-runtime enabled. However, if
> this error occurs, it likely indicates a more significant underlying
> memory issue.

Yes. Nevertheless, in general errors should be handled rather than being
ignored.

> 
> 
> >
> > >
> > >       generic_phy = devm_phy_create(dev, NULL, &qusb2_phy_gen_ops);
> > >       if (IS_ERR(generic_phy)) {
> > >               ret = PTR_ERR(generic_phy);
> > >               dev_err(dev, "failed to create phy, %d\n", ret);
> > > -             pm_runtime_disable(dev);
> > >               return ret;
> > >       }
> > >       qphy->phy = generic_phy;
> > >
> > > -     dev_set_drvdata(dev, qphy);
> > >       phy_set_drvdata(generic_phy, qphy);
> > >
> > >       phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
> > > -     if (IS_ERR(phy_provider))
> > > -             pm_runtime_disable(dev);
> > >
> > >       return PTR_ERR_OR_ZERO(phy_provider);
> > >  }
> > > --
> > > 2.34.1
> > >
> >
> > --
> > With best wishes
> > Dmitry

-- 
With best wishes
Dmitry