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

[Bjorn Andersson]

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?

> [...]
> ```
> 
> 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?

>  	/*
> -	 * 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()".

Regards,
Bjorn

>  	 */
> +	pm_runtime_set_active(dev);
>  	pm_runtime_forbid(dev);
> +	pm_runtime_enable(dev);
>  
>  	generic_phy = devm_phy_create(dev, NULL, &qusb2_phy_gen_ops);
>  	if (IS_ERR(generic_phy)) {
> -- 
> 2.34.1
> 
>