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

[Dmitry Baryshkov]

On Tue, Dec 16, 2025 at 10:36:54AM +0100, Loic Poulain wrote:
> Hi Dmitry,
> 
> On Mon, Dec 15, 2025 at 9:16 PM Dmitry Baryshkov
> <dmitry.baryshkov at oss.qualcomm.com> wrote:
> >
> > 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.
> 
> Yes, this pattern has been used/replicated in several places, and
> while it is logically valid, it doesn’t necessarily make it a good
> practice. In fact, it leaves room for opportunistic runtime suspend to
> occur, whereas the usual expectation is to completely block runtime
> suspend (as initially expected in this phy driver).
> 
> An example of the 'right' pattern can be found in the PCI core: the
> pci_pm_init() function calls pm_runtime_set_active() and
> pm_runtime_forbid() before invoking pm_runtime_enable().
> 
> > 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.
> 
> I’m not entirely convinced that it implies a specific order.
> Another common pattern seen in drivers is the following:
> ```
> pm_runtime_get_noresume(dev);
> pm_runtime_set_active(dev);
> pm_runtime_enable(dev);
> ```
> 
> The above is typically used to prevent the device from entering
> suspend during initialization. pm_runtime_get_noresume() behaves
> somewhat like pm_runtime_forbid(), but get_noresume() is usually
> balanced by a corresponding pm_runtime_put_*() at the end of
> initialization, whereas pm_runtime_forbid() blocks runtime suspend
> until it is explicitly reverted with pm_runtime_allow() (usually from
> userside via sysfs).
> 
> Also  pm_runtime_enable() operates on a depth counter, so calling it
> at a specific point does not necessarily guarantee its activation, I
> think this reduces any strictness of ordering expectations compared to
> pm_runtime_forbid().

Ack.

> 
> >
> > >
> > > >
> > > > > +     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.
> 
> Ack.
> 
> >
> > >
> > >
> > > >
> > > > >
> > > > >       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
> 
> Regards,
> Loic

-- 
With best wishes
Dmitry