[PATCH] phy: qcom: qmp-combo: hold a runtime PM reference in the typec callbacks

[PATCH] phy: qcom: qmp-combo: hold a runtime PM reference in the typec callbacks

[Oleg Keri]

qmp_combo_typec_switch_set() and qmp_combo_typec_mux_set() tear the common
block down and bring it straight back up, calling qmp_combo_com_exit() and
qmp_combo_com_init() with force=true.  Both release and re-acquire the PHY
clocks.

Unlike the PHY operations, which the PHY core always invokes with a
runtime PM reference held - phy_pm_runtime_get_sync() in phy_init(),
phy_exit() and phy_power_on() - these two are typec_switch and typec_mux
callbacks and hold no such reference.  Releasing the clocks is then enough
to drop the device's last reference, so clk_core_unprepare() runs
pm_runtime_idle() and re-enters the driver through
qmp_combo_runtime_suspend(), which disables the very clocks that are being
torn down:

  gcc_usb3_prim_phy_pipe_clk already disabled
  WARNING: drivers/clk/clk.c:1259 at clk_core_disable+0x298/0x300
  Workqueue: events_freezable pmic_glink_altmode_worker
   clk_disable
   qmp_combo_runtime_suspend
   pm_generic_runtime_suspend
   genpd_runtime_suspend
   rpm_suspend
   rpm_idle
   __pm_runtime_idle
   clk_core_unprepare
   clk_core_unprepare
   clk_core_unprepare

qmp_combo_runtime_suspend() only checks init_count, which is still
non-zero at that point, so it proceeds and the clock enable and prepare
counts underflow.

Runtime PM is forbidden at probe, so this only becomes reachable once
userspace opts in through power/control - which is exactly what the
comment above pm_runtime_forbid() invites it to do.

Hold a runtime PM reference across the teardown and bringup so releasing
the clocks cannot re-enter the driver's own suspend callback.

Fixes: 2851117f8f42 ("phy: qcom-qmp-combo: Introduce orientation switching")
Signed-off-by: Oleg Keri <okerixx@gmail.com>
---
 drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
index d77fe94b312c..bf4d29fe1719 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
@@ -16,6 +16,7 @@
 #include <linux/of_graph.h>
 #include <linux/phy/phy.h>
 #include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
 #include <linux/regulator/consumer.h>
 #include <linux/reset.h>
 #include <linux/slab.h>
@@ -4930,6 +4931,8 @@ static int qmp_combo_typec_switch_set(struct typec_switch_dev *sw,
 	qmp->orientation = orientation;
 
 	if (qmp->init_count) {
+		guard(pm_runtime_active)(qmp->dev);
+
 		if (qmp->usb_init_count)
 			qmp_combo_usb_power_off(qmp->usb_phy);
 		qmp_combo_com_exit(qmp, true);
@@ -4997,6 +5000,8 @@ static int qmp_combo_typec_mux_set(struct typec_mux_dev *mux, struct typec_mux_s
 	qmp->qmpphy_mode = new_mode;
 
 	if (qmp->init_count) {
+		guard(pm_runtime_active)(qmp->dev);
+
 		if (qmp->usb_init_count)
 			qmp_combo_usb_power_off(qmp->usb_phy);
 
-- 
2.55.0


base-commit: df2908090cda368b01ff43709f51890076c56157

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH] phy: qcom: qmp-combo: hold a runtime PM reference in the typec callbacks

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Ignored return value of qmp_combo_com_init() leads to unclocked register access crashes and init_count state corruption.
--

commit e725afa70b650e2a1f27710fa3b4e0c5a5568682
Author: Oleg Keri <okerixx@gmail.com>

    phy: qcom: qmp-combo: hold a runtime PM reference in the typec callbacks
    
    qmp_combo_typec_switch_set() and qmp_combo_typec_mux_set() tear the common
    block down and bring it straight back up, calling qmp_combo_com_exit() and
    qmp_combo_com_init() with force=true. Hold a runtime PM reference across the
    teardown and bringup so releasing the clocks cannot re-enter the driver's
    own suspend callback.

> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> index c39ced168d035..c08a22681e0e2 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
[ ... ]
> @@ -4786,6 +4787,8 @@ static int qmp_combo_typec_switch_set(struct typec_switch_dev *sw,
>  	qmp->orientation = orientation;
>  
>  	if (qmp->init_count) {
> +		guard(pm_runtime_active)(qmp->dev);
> +
>  		if (qmp->usb_init_count)
>  			qmp_combo_usb_power_off(qmp->usb_phy);
>  		qmp_combo_com_exit(qmp, true);

[Severity: High]
This isn't a regression introduced by this patch, but I noticed the return
value of qmp_combo_com_init() is ignored just below this diff in
drivers/phy/qualcomm/phy-qcom-qmp-combo.c:

qmp_combo_typec_switch_set() {
    ...
		qmp_combo_com_init(qmp, true);
		if (qmp->usb_init_count)
			qmp_combo_usb_power_on(qmp->usb_phy);
    ...
}

If qmp_combo_com_init() fails to enable clocks (e.g., clk_bulk_prepare_enable
fails), it leaves the clocks disabled and returns an error code.

Because this return value is ignored, the code proceeds to access hardware
registers via qmp_combo_usb_power_on().

Can accessing mapped peripheral registers while their clocks are disabled
cause a System Error (SError) exception on ARM/ARM64, resulting in a kernel
panic?

Additionally, if qmp_combo_com_init() fails when force=true, it can corrupt
the initialization state:

qmp_combo_com_init() {
    ...
	if (!force && qmp->init_count++)
		return 0;
    ...
err_decrement_count:
	qmp->init_count--;

	return ret;
}

The force=true flag causes the function to skip the corresponding increment
of init_count.

Does the error path need to avoid decrementing init_count when force=true?

> @@ -4853,6 +4856,8 @@ static int qmp_combo_typec_mux_set(struct typec_mux_dev *mux, struct typec_mux_s
>  	qmp->qmpphy_mode = new_mode;
>  
>  	if (qmp->init_count) {
> +		guard(pm_runtime_active)(qmp->dev);
> +
>  		if (qmp->usb_init_count)
>  			qmp_combo_usb_power_off(qmp->usb_phy);
>  

[Severity: High]
This is a pre-existing issue, but the return value of qmp_combo_com_init()
is similarly ignored in qmp_combo_typec_mux_set() in
drivers/phy/qualcomm/phy-qcom-qmp-combo.c:

qmp_combo_typec_mux_set() {
    ...
		qmp_combo_com_init(qmp, true);

		if (new_mode == QMPPHY_MODE_DP_ONLY) {
    ...
}

If the clock initialization fails here, does this also lead to subsequent
hardware accesses without active clocks?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909140345.4239-1-okerixx@gmail.com?part=1

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH] phy: qcom: qmp-combo: hold a runtime PM reference in the typec callbacks

[Oleg Keri]

Both findings are real, and I have posted a follow-up series for them:

  [PATCH 0/2] phy: qcom: qmp-combo: fix forced com_init() error handling

To answer the two questions directly.

Yes, the error path has to skip the decrement when force is set.  The
reference is only taken in

	if (!force && qmp->init_count++)
		return 0;

and && short-circuits on !force, so with force set init_count++ is never
evaluated, while err_decrement_count decrements unconditionally.  A forced
init that fails therefore drops a reference it never took.  init_count is a
plain int, so it goes negative rather than wrapping, and the damage lasts
for the rest of the boot: qmp_combo_com_exit() sees a non-zero value in

	if (!force && --qmp->init_count)
		return 0;

and returns early every time, so the clocks, resets and regulators are
never released, while the runtime PM callbacks only bail on exactly zero
and keep touching hardware that may already be off.

And yes, on arm64 a read or write to a peripheral whose clock is gated is
not a benign no-op - it typically raises an imprecise external abort, which
arrives as an SError.  Whether that reaches the kernel or is taken by
firmware is platform dependent; either way it is not something to walk into
after an init failure has already unwound the clocks.

Both are pre-existing, as you say, and neither depends on the patch you are
reviewing - that one only stops the teardown re-entering the driver's own
runtime suspend callback.  The three are independent and can be applied in
any order.

For the record, the two problems are reached only when
qmp_combo_com_init() itself fails, so I have not been able to trigger them
deliberately; they are found by inspection.

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy