[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

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.