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

[Loic Poulain]

On Fri, Dec 5, 2025 at 10:30 PM Bjorn Andersson <andersson at kernel.org> wrote:
>
> On Fri, Dec 05, 2025 at 05:04:37PM +0100, Loic Poulain wrote:
> > Reorder runtime PM setup to ensure pm_runtime_forbid() is applied before
> > enabling runtime PM.
>
> Your commit message is pretty good, but in the future please consider
> following the documentation [1] and really start your commit message by
> establishing the problem. The "most significant bit" of your change
> request isn't "let's reorder something", it's something along the line
> of "enable before forbid can result in autosuspend, which results in
> NULL pointer dereference".
>
> [1] https://docs.kernel.org/process/submitting-patches.html#describe-your-changes
>
> > This avoids possible early autosuspend and prevents
> > qusb2_phy_runtime_suspend() from being called before driver data is
> > initialized, which could otherwise lead to a NULL pointer dereference:
> >
> > ```
> > 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
>
> If you pipe the log through "./scripts/decode_stacktrace.sh vmlinux . ."
> we get these decoded.
>
> But that said, I'm pretty sure that what fails is the attempt to pull
> cfg out of NULL on the second line?

Not exactly. The actual NULL dereference happens on line 7:
    if (!qphy->phy_initialized) {
I assume the second line is optimized and the compiler defers the load
until cfg is actually used.

This is what I got when debugging this:
/scripts/faddr2line drivers/phy/qualcomm/phy-qcom-qusb2.ko
qusb2_phy_runtime_suspend+0x14
qusb2_phy_runtime_suspend+0x14/0x1e0:
qusb2_phy_runtime_suspend at drivers/phy/qualcomm/phy-qcom-qusb2.c:639

>
> > [...]
> > ```
> >
> > Fixes: 891a96f65ac3 ("phy: qcom-qusb2: Add support for runtime PM")
> > Signed-off-by: Loic Poulain <loic.poulain at oss.qualcomm.com>
> > ---
> >  drivers/phy/qualcomm/phy-qcom-qusb2.c | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/phy/qualcomm/phy-qcom-qusb2.c b/drivers/phy/qualcomm/phy-qcom-qusb2.c
> > index b5514a32ff8f..97bc3755a390 100644
> > --- a/drivers/phy/qualcomm/phy-qcom-qusb2.c
> > +++ b/drivers/phy/qualcomm/phy-qcom-qusb2.c
> > @@ -1093,13 +1093,14 @@ static int qusb2_phy_probe(struct platform_device *pdev)
> >               or->hsdisc_trim.override = true;
> >       }
> >
> > -     pm_runtime_set_active(dev);
> > -     pm_runtime_enable(dev);
> > +
>
> Doesn't this end up being two blank lines?

Hmm, yes good catch.

>
> >       /*
> > -      * 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.
>
> I think this warrants adding "TODO: Removing the pm_runtime_forbid()
> might allow autosuspend to invoke qusb2_phy_runtime_suspend() to
> dereference the NULL drvdata" here.
>
>
> But, if we know what the problem is, perhaps we should just fix it right
> away?
>
> Looks like the fix for the NULL pointer dereference is to move the
> dev_set_drvdata() and phy_set_drvdata() above thus hunk.
>
> And the "problem statement" stating why we're doing this change (which
> still makes sense) is that "a wasteful autosuspend might happen between
> pm_runtime_enable() and the pm_runtime_forbid()".

That's correct. My initial lazy approach was a bit simplistic, just
swapping pm_runtime_enable() and pm_runtime_forbid() to indirectly
prevents the NULL dereference by avoiding runtime suspend, but
ideally, we should apply both, swap the calls to prevent unnecessary
suspend, and reorder them with dev_set_drvdata() to restore proper
initialization logic. => Will do this in v2.

Note that Once this change is accepted, I plan a series to replicate
the fix in other PHY drivers that exhibit the same issue (e.g.,
phy-qcom-qmp-usbc, phy-qcom-qmp-combo, etc.).

Thanks,
Loic