[PATCH v3] phy: qcom-qusb2: Fix NULL pointer dereference on early suspend
Loic Poulain
loic.poulain at oss.qualcomm.com
Tue Dec 16 01:36:54 PST 2025
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().
>
> >
> > >
> > > > + 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
More information about the linux-phy
mailing list