In the suspend / resume callbacks, qmp->phy could be NULL because PHY is created after the PM ops are enabled, which lead to the NULL pointer deference.
Internally issue is reported on qcom-qmp-usb driver. Since the fix is applicable to legacy and usbc drivers, incoporated the fixes for those driver as well.
qcom-qmp-usb-legacy and qcom-qmp-usbc drivers are splitted out from qcom-qmp-usb driver in v6.6 and v6.9 respectively. So splitted the changes into 3, for ease of backporting.
Signed-off-by: Kathiravan Thirumoorthy kathiravan.thirumoorthy@oss.qualcomm.com --- Poovendhan Selvaraj (3): phy: qcom-qmp-usb: fix NULL pointer dereference in PM callbacks phy: qcom-qmp-usb-legacy: fix NULL pointer dereference in PM callbacks phy: qcom-qmp-usbc: fix NULL pointer dereference in PM callbacks
drivers/phy/qualcomm/phy-qcom-qmp-usb-legacy.c | 4 ++-- drivers/phy/qualcomm/phy-qcom-qmp-usb.c | 4 ++-- drivers/phy/qualcomm/phy-qcom-qmp-usbc.c | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) --- base-commit: 0f4c93f7eb861acab537dbe94441817a270537bf change-id: 20250825-qmp-null-deref-on-pm-fd98a91c775b
Best regards,
From: Poovendhan Selvaraj quic_poovendh@quicinc.com
The pm ops are enabled before qmp phy create which causes a NULL pointer dereference when accessing qmp->phy->init_count in the qmp_usb_runtime_suspend.
So if qmp->phy is NULL, bail out early in suspend / resume callbacks to avoid the NULL pointer dereference in qmp_usb_runtime_suspend and qmp_usb_runtime_resume.
Below is the stacktrace for reference:
[<818381a0>] (qmp_usb_runtime_suspend [phy_qcom_qmp_usb]) from [<4051d1d8>] (__rpm_callback+0x3c/0x110) [<4051d1d8>] (__rpm_callback) from [<4051d2fc>] (rpm_callback+0x50/0x54) [<4051d2fc>] (rpm_callback) from [<4051d940>] (rpm_suspend+0x23c/0x428) [<4051d940>] (rpm_suspend) from [<4051e808>] (pm_runtime_work+0x74/0x8c) [<4051e808>] (pm_runtime_work) from [<401311f4>] (process_scheduled_works+0x1d0/0x2c8) [<401311f4>] (process_scheduled_works) from [<40131d48>] (worker_thread+0x260/0x2e4) [<40131d48>] (worker_thread) from [<40138970>] (kthread+0x118/0x12c) [<40138970>] (kthread) from [<4010013c>] (ret_from_fork+0x14/0x38)
Cc: stable@vger.kernel.org # v6.0 Fixes: 65753f38f530 ("phy: qcom-qmp-usb: drop multi-PHY support") Signed-off-by: Poovendhan Selvaraj quic_poovendh@quicinc.com Signed-off-by: Kathiravan Thirumoorthy kathiravan.thirumoorthy@oss.qualcomm.com --- drivers/phy/qualcomm/phy-qcom-qmp-usb.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-usb.c b/drivers/phy/qualcomm/phy-qcom-qmp-usb.c index ed646a7e705ba3259708775ed5fedbbbada13735..cd04e8f22a0fe81b086b308d02713222aa95cae3 100644 --- a/drivers/phy/qualcomm/phy-qcom-qmp-usb.c +++ b/drivers/phy/qualcomm/phy-qcom-qmp-usb.c @@ -1940,7 +1940,7 @@ static int __maybe_unused qmp_usb_runtime_suspend(struct device *dev)
dev_vdbg(dev, "Suspending QMP phy, mode:%d\n", qmp->mode);
- if (!qmp->phy->init_count) { + if (!qmp->phy || !qmp->phy->init_count) { dev_vdbg(dev, "PHY not initialized, bailing out\n"); return 0; } @@ -1960,7 +1960,7 @@ static int __maybe_unused qmp_usb_runtime_resume(struct device *dev)
dev_vdbg(dev, "Resuming QMP phy, mode:%d\n", qmp->mode);
- if (!qmp->phy->init_count) { + if (!qmp->phy || !qmp->phy->init_count) { dev_vdbg(dev, "PHY not initialized, bailing out\n"); return 0; }
On Mon, Aug 25, 2025 at 05:22:02PM +0530, Kathiravan Thirumoorthy wrote:
From: Poovendhan Selvaraj quic_poovendh@quicinc.com
The pm ops are enabled before qmp phy create which causes a NULL pointer dereference when accessing qmp->phy->init_count in the qmp_usb_runtime_suspend.
How does that happen? Do we end up in the error path inbetween the devm_pm_runtime_enable()? Or does it happen by some other means?
This would be quite useful information for others to know if they hit the same or just a similar problem.
So if qmp->phy is NULL, bail out early in suspend / resume callbacks to avoid the NULL pointer dereference in qmp_usb_runtime_suspend and qmp_usb_runtime_resume.
Below is the stacktrace for reference:
[<818381a0>] (qmp_usb_runtime_suspend [phy_qcom_qmp_usb]) from [<4051d1d8>] (__rpm_callback+0x3c/0x110) [<4051d1d8>] (__rpm_callback) from [<4051d2fc>] (rpm_callback+0x50/0x54) [<4051d2fc>] (rpm_callback) from [<4051d940>] (rpm_suspend+0x23c/0x428) [<4051d940>] (rpm_suspend) from [<4051e808>] (pm_runtime_work+0x74/0x8c) [<4051e808>] (pm_runtime_work) from [<401311f4>] (process_scheduled_works+0x1d0/0x2c8) [<401311f4>] (process_scheduled_works) from [<40131d48>] (worker_thread+0x260/0x2e4) [<40131d48>] (worker_thread) from [<40138970>] (kthread+0x118/0x12c) [<40138970>] (kthread) from [<4010013c>] (ret_from_fork+0x14/0x38)
Cc: stable@vger.kernel.org # v6.0 Fixes: 65753f38f530 ("phy: qcom-qmp-usb: drop multi-PHY support")
Has this been a reproducible issue for last 3 years? I think the fixes makes sense in that it introduced the indirection, but when did the issue actually show up?
Regards, Bjorn
Signed-off-by: Poovendhan Selvaraj quic_poovendh@quicinc.com Signed-off-by: Kathiravan Thirumoorthy kathiravan.thirumoorthy@oss.qualcomm.com
drivers/phy/qualcomm/phy-qcom-qmp-usb.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-usb.c b/drivers/phy/qualcomm/phy-qcom-qmp-usb.c index ed646a7e705ba3259708775ed5fedbbbada13735..cd04e8f22a0fe81b086b308d02713222aa95cae3 100644 --- a/drivers/phy/qualcomm/phy-qcom-qmp-usb.c +++ b/drivers/phy/qualcomm/phy-qcom-qmp-usb.c @@ -1940,7 +1940,7 @@ static int __maybe_unused qmp_usb_runtime_suspend(struct device *dev) dev_vdbg(dev, "Suspending QMP phy, mode:%d\n", qmp->mode);
- if (!qmp->phy->init_count) {
- if (!qmp->phy || !qmp->phy->init_count) { dev_vdbg(dev, "PHY not initialized, bailing out\n"); return 0; }
@@ -1960,7 +1960,7 @@ static int __maybe_unused qmp_usb_runtime_resume(struct device *dev) dev_vdbg(dev, "Resuming QMP phy, mode:%d\n", qmp->mode);
- if (!qmp->phy->init_count) {
- if (!qmp->phy || !qmp->phy->init_count) { dev_vdbg(dev, "PHY not initialized, bailing out\n"); return 0; }
-- 2.34.1
On 8/26/2025 8:20 AM, Bjorn Andersson wrote:
On Mon, Aug 25, 2025 at 05:22:02PM +0530, Kathiravan Thirumoorthy wrote:
From: Poovendhan Selvaraj quic_poovendh@quicinc.com
The pm ops are enabled before qmp phy create which causes a NULL pointer dereference when accessing qmp->phy->init_count in the qmp_usb_runtime_suspend.
How does that happen? Do we end up in the error path inbetween the devm_pm_runtime_enable()? Or does it happen by some other means?
qmp_usb_probe() is scheduled out per the below stack (collected from the RAM dump),
Stack trace of the kmodloader process: [<0x408def88>] __schedule+0x348/0x55c [<0x408df1f8>] schedule+0x5c/0x98 [<0x4052c318>] rpm_resume+0x150/0x404 [<0x4052d4e4>] pm_runtime_forbid+0x54/0x60 [<0x629c47f0>] qmp_usb_probe+0x3c4/0x5d0 [phy_qcom_qmp_usb.ko]
From the above snippet, we can see that the phy-create has not happened yet as the probe is still in pm_runtime_forbid() and qmp->phy is NULL. Meanwhile, qmp_usb_runtime_suspend() is called, causing the NULL pointer de-reference issue. Since the issue is not easily reproducible, we are not able to find out who/why the suspend was called.
This would be quite useful information for others to know if they hit the same or just a similar problem.
So if qmp->phy is NULL, bail out early in suspend / resume callbacks to avoid the NULL pointer dereference in qmp_usb_runtime_suspend and qmp_usb_runtime_resume.
Below is the stacktrace for reference:
[<818381a0>] (qmp_usb_runtime_suspend [phy_qcom_qmp_usb]) from [<4051d1d8>] (__rpm_callback+0x3c/0x110) [<4051d1d8>] (__rpm_callback) from [<4051d2fc>] (rpm_callback+0x50/0x54) [<4051d2fc>] (rpm_callback) from [<4051d940>] (rpm_suspend+0x23c/0x428) [<4051d940>] (rpm_suspend) from [<4051e808>] (pm_runtime_work+0x74/0x8c) [<4051e808>] (pm_runtime_work) from [<401311f4>] (process_scheduled_works+0x1d0/0x2c8) [<401311f4>] (process_scheduled_works) from [<40131d48>] (worker_thread+0x260/0x2e4) [<40131d48>] (worker_thread) from [<40138970>] (kthread+0x118/0x12c) [<40138970>] (kthread) from [<4010013c>] (ret_from_fork+0x14/0x38)
Cc: stable@vger.kernel.org # v6.0 Fixes: 65753f38f530 ("phy: qcom-qmp-usb: drop multi-PHY support")
Has this been a reproducible issue for last 3 years? I think the fixes makes sense in that it introduced the indirection, but when did the issue actually show up?
After migrating the QSDK Linux from 5.4 to 6.6, we are started seeing this issue randomly. We didn't had a chance to test in the other kernel versions.
Regards, Bjorn
Signed-off-by: Poovendhan Selvaraj quic_poovendh@quicinc.com Signed-off-by: Kathiravan Thirumoorthy kathiravan.thirumoorthy@oss.qualcomm.com
drivers/phy/qualcomm/phy-qcom-qmp-usb.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-usb.c b/drivers/phy/qualcomm/phy-qcom-qmp-usb.c index ed646a7e705ba3259708775ed5fedbbbada13735..cd04e8f22a0fe81b086b308d02713222aa95cae3 100644 --- a/drivers/phy/qualcomm/phy-qcom-qmp-usb.c +++ b/drivers/phy/qualcomm/phy-qcom-qmp-usb.c @@ -1940,7 +1940,7 @@ static int __maybe_unused qmp_usb_runtime_suspend(struct device *dev) dev_vdbg(dev, "Suspending QMP phy, mode:%d\n", qmp->mode);
- if (!qmp->phy->init_count) {
- if (!qmp->phy || !qmp->phy->init_count) { dev_vdbg(dev, "PHY not initialized, bailing out\n"); return 0; }
@@ -1960,7 +1960,7 @@ static int __maybe_unused qmp_usb_runtime_resume(struct device *dev) dev_vdbg(dev, "Resuming QMP phy, mode:%d\n", qmp->mode);
- if (!qmp->phy->init_count) {
- if (!qmp->phy || !qmp->phy->init_count) { dev_vdbg(dev, "PHY not initialized, bailing out\n"); return 0; }
-- 2.34.1
On 25-08-25, 17:22, Kathiravan Thirumoorthy wrote:
From: Poovendhan Selvaraj quic_poovendh@quicinc.com
The pm ops are enabled before qmp phy create which causes a NULL pointer dereference when accessing qmp->phy->init_count in the qmp_usb_runtime_suspend.
So if qmp->phy is NULL, bail out early in suspend / resume callbacks to avoid the NULL pointer dereference in qmp_usb_runtime_suspend and qmp_usb_runtime_resume.
That is a band-aid. we should enable pm only when ready... Why not do that instead?
Below is the stacktrace for reference:
[<818381a0>] (qmp_usb_runtime_suspend [phy_qcom_qmp_usb]) from [<4051d1d8>] (__rpm_callback+0x3c/0x110) [<4051d1d8>] (__rpm_callback) from [<4051d2fc>] (rpm_callback+0x50/0x54) [<4051d2fc>] (rpm_callback) from [<4051d940>] (rpm_suspend+0x23c/0x428) [<4051d940>] (rpm_suspend) from [<4051e808>] (pm_runtime_work+0x74/0x8c) [<4051e808>] (pm_runtime_work) from [<401311f4>] (process_scheduled_works+0x1d0/0x2c8) [<401311f4>] (process_scheduled_works) from [<40131d48>] (worker_thread+0x260/0x2e4) [<40131d48>] (worker_thread) from [<40138970>] (kthread+0x118/0x12c) [<40138970>] (kthread) from [<4010013c>] (ret_from_fork+0x14/0x38)
Cc: stable@vger.kernel.org # v6.0 Fixes: 65753f38f530 ("phy: qcom-qmp-usb: drop multi-PHY support") Signed-off-by: Poovendhan Selvaraj quic_poovendh@quicinc.com Signed-off-by: Kathiravan Thirumoorthy kathiravan.thirumoorthy@oss.qualcomm.com
drivers/phy/qualcomm/phy-qcom-qmp-usb.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-usb.c b/drivers/phy/qualcomm/phy-qcom-qmp-usb.c index ed646a7e705ba3259708775ed5fedbbbada13735..cd04e8f22a0fe81b086b308d02713222aa95cae3 100644 --- a/drivers/phy/qualcomm/phy-qcom-qmp-usb.c +++ b/drivers/phy/qualcomm/phy-qcom-qmp-usb.c @@ -1940,7 +1940,7 @@ static int __maybe_unused qmp_usb_runtime_suspend(struct device *dev) dev_vdbg(dev, "Suspending QMP phy, mode:%d\n", qmp->mode);
- if (!qmp->phy->init_count) {
- if (!qmp->phy || !qmp->phy->init_count) { dev_vdbg(dev, "PHY not initialized, bailing out\n"); return 0; }
@@ -1960,7 +1960,7 @@ static int __maybe_unused qmp_usb_runtime_resume(struct device *dev) dev_vdbg(dev, "Resuming QMP phy, mode:%d\n", qmp->mode);
- if (!qmp->phy->init_count) {
- if (!qmp->phy || !qmp->phy->init_count) { dev_vdbg(dev, "PHY not initialized, bailing out\n"); return 0; }
-- 2.34.1
On 9/1/2025 7:24 PM, Vinod Koul wrote:
On 25-08-25, 17:22, Kathiravan Thirumoorthy wrote:
From: Poovendhan Selvarajquic_poovendh@quicinc.com
The pm ops are enabled before qmp phy create which causes a NULL pointer dereference when accessing qmp->phy->init_count in the qmp_usb_runtime_suspend.
So if qmp->phy is NULL, bail out early in suspend / resume callbacks to avoid the NULL pointer dereference in qmp_usb_runtime_suspend and qmp_usb_runtime_resume.
That is a band-aid. we should enable pm only when ready... Why not do that instead?
Thanks Vinod. Sorry for the delay, I'm just back from my vacation.
Sure, let me check why the PM is getting enabled before its ready.
Thanks,
Kathiravan T.
From: Poovendhan Selvaraj quic_poovendh@quicinc.com
The pm ops are enabled before qmp phy create which causes a NULL pointer dereference when accessing qmp->phy->init_count in the qmp_usb_runtime_suspend.
So if qmp->phy is NULL, bail out early in suspend / resume callbacks to avoid the NULL pointer dereference in qmp_usb_runtime_suspend and qmp_usb_runtime_resume.
Cc: stable@vger.kernel.org # v6.6 Fixes: e464a3180a43 ("phy: qcom-qmp-usb: split off the legacy USB+dp_com support") Signed-off-by: Poovendhan Selvaraj quic_poovendh@quicinc.com Signed-off-by: Kathiravan Thirumoorthy kathiravan.thirumoorthy@oss.qualcomm.com --- drivers/phy/qualcomm/phy-qcom-qmp-usb-legacy.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-usb-legacy.c b/drivers/phy/qualcomm/phy-qcom-qmp-usb-legacy.c index 8bf951b0490cfd811635df8940de1b789e21b46c..ef28e59ffd58a12d6d416a553a3a478e9691b8c5 100644 --- a/drivers/phy/qualcomm/phy-qcom-qmp-usb-legacy.c +++ b/drivers/phy/qualcomm/phy-qcom-qmp-usb-legacy.c @@ -988,7 +988,7 @@ static int __maybe_unused qmp_usb_legacy_runtime_suspend(struct device *dev)
dev_vdbg(dev, "Suspending QMP phy, mode:%d\n", qmp->mode);
- if (!qmp->phy->init_count) { + if (!qmp->phy || !qmp->phy->init_count) { dev_vdbg(dev, "PHY not initialized, bailing out\n"); return 0; } @@ -1009,7 +1009,7 @@ static int __maybe_unused qmp_usb_legacy_runtime_resume(struct device *dev)
dev_vdbg(dev, "Resuming QMP phy, mode:%d\n", qmp->mode);
- if (!qmp->phy->init_count) { + if (!qmp->phy || !qmp->phy->init_count) { dev_vdbg(dev, "PHY not initialized, bailing out\n"); return 0; }
From: Poovendhan Selvaraj quic_poovendh@quicinc.com
The pm ops are enabled before qmp phy create which causes a NULL pointer dereference when accessing qmp->phy->init_count in the qmp_usb_runtime_suspend.
So if qmp->phy is NULL, bail out early in suspend / resume callbacks to avoid the NULL pointer dereference in qmp_usb_runtime_suspend and qmp_usb_runtime_resume.
Cc: stable@vger.kernel.org # v6.9 Fixes: 19281571a4d5 ("phy: qcom: qmp-usb: split USB-C PHY driver") Signed-off-by: Poovendhan Selvaraj quic_poovendh@quicinc.com Signed-off-by: Kathiravan Thirumoorthy kathiravan.thirumoorthy@oss.qualcomm.com --- drivers/phy/qualcomm/phy-qcom-qmp-usbc.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-usbc.c b/drivers/phy/qualcomm/phy-qcom-qmp-usbc.c index 5e7fcb26744a4401c3076960df9c0dcbec7fdef7..640f6520f7c1cd78f9e79843a0778c1bee790f64 100644 --- a/drivers/phy/qualcomm/phy-qcom-qmp-usbc.c +++ b/drivers/phy/qualcomm/phy-qcom-qmp-usbc.c @@ -690,7 +690,7 @@ static int __maybe_unused qmp_usbc_runtime_suspend(struct device *dev)
dev_vdbg(dev, "Suspending QMP phy, mode:%d\n", qmp->mode);
- if (!qmp->phy->init_count) { + if (!qmp->phy || !qmp->phy->init_count) { dev_vdbg(dev, "PHY not initialized, bailing out\n"); return 0; } @@ -710,7 +710,7 @@ static int __maybe_unused qmp_usbc_runtime_resume(struct device *dev)
dev_vdbg(dev, "Resuming QMP phy, mode:%d\n", qmp->mode);
- if (!qmp->phy->init_count) { + if (!qmp->phy || !qmp->phy->init_count) { dev_vdbg(dev, "PHY not initialized, bailing out\n"); return 0; }
linux-stable-mirror@lists.linaro.org
-
Bjorn Andersson -
Kathiravan Thirumoorthy -
Vinod Koul