* [PATCH 0/2] phy: qcom: qmp-combo: fix forced com_init() error handling
@ 2026-09-09 15:14 Oleg Keri
2026-09-09 15:14 ` [PATCH 1/2] phy: qcom: qmp-combo: do not decrement init_count on a forced init failure Oleg Keri
2026-09-09 15:14 ` [PATCH 2/2] phy: qcom: qmp-combo: check qmp_combo_com_init() in the typec callbacks Oleg Keri
0 siblings, 2 replies; 6+ messages in thread
From: Oleg Keri @ 2026-09-09 15:14 UTC (permalink / raw)
To: Vinod Koul, Neil Armstrong, Manivannan Sadhasivam,
Bjorn Andersson, Johan Hovold
Cc: linux-arm-msm, linux-phy, linux-kernel, Michael Scott
Two pre-existing problems in the typec_switch and typec_mux callbacks,
both on the force=true path that tears the common block down and brings it
back up. Found while looking at that code for
https://lore.kernel.org/all/20260909140345.4239-1-okerixx@gmail.com/
which is an unrelated fix to the same two functions and does not depend on
these; they can be applied in either order.
Patch 1 stops a forced qmp_combo_com_init() failure from dropping an
init_count reference it never took, which drives the counter negative and
makes qmp_combo_com_exit() return early for the rest of the boot.
Patch 2 stops both callbacks from carrying on to write PHY registers after
the bringup has failed and left the clocks disabled.
Neither is easy to trigger deliberately - both need qmp_combo_com_init() to
fail - so they are found by inspection rather than reproduced. The tree
they were written against is exercised daily on a Lenovo Yoga Slim 7x Gen 11
(Snapdragon X2 Elite), where the typec callbacks run on every USB-C
orientation and altmode change.
Oleg Keri (2):
phy: qcom: qmp-combo: do not decrement init_count on a forced init
failure
phy: qcom: qmp-combo: check qmp_combo_com_init() in the typec
callbacks
drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 17 +++++++++++++----
1 file changed, 13 insertions(+), 4 deletions(-)
--
2.55.0
base-commit: df2908090cda368b01ff43709f51890076c56157
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH 1/2] phy: qcom: qmp-combo: do not decrement init_count on a forced init failure 2026-09-09 15:14 [PATCH 0/2] phy: qcom: qmp-combo: fix forced com_init() error handling Oleg Keri @ 2026-09-09 15:14 ` Oleg Keri 2026-09-09 15:28 ` sashiko-bot 2026-09-09 15:14 ` [PATCH 2/2] phy: qcom: qmp-combo: check qmp_combo_com_init() in the typec callbacks Oleg Keri 1 sibling, 1 reply; 6+ messages in thread From: Oleg Keri @ 2026-09-09 15:14 UTC (permalink / raw) To: Vinod Koul, Neil Armstrong, Manivannan Sadhasivam, Bjorn Andersson, Johan Hovold Cc: linux-arm-msm, linux-phy, linux-kernel, Michael Scott qmp_combo_com_init() only takes a reference when it is not forced: if (!force && qmp->init_count++) return 0; With force set, && short-circuits on !force and init_count++ is never evaluated. The error path decrements unconditionally, so a forced init that fails drops a reference it never took and init_count goes negative. init_count is a plain int, so the damage persists for the rest of the boot. qmp_combo_com_exit() then 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; the runtime PM callbacks only bail on exactly zero, so they keep touching hardware that may already be off. Both callers that pass force are the typec_switch and typec_mux callbacks, which tear the common block down and bring it back up on an orientation or altmode change. Only decrement the count when it was actually taken. 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 | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c index bf4d29fe1719..7d740ed0ce16 100644 --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c @@ -4253,7 +4253,8 @@ static int qmp_combo_com_init(struct qmp_combo *qmp, bool force) err_disable_regulators: regulator_bulk_disable(cfg->num_vregs, qmp->vregs); err_decrement_count: - qmp->init_count--; + if (!force) + qmp->init_count--; return ret; } -- 2.55.0 -- linux-phy mailing list linux-phy@lists.infradead.org https://lists.infradead.org/mailman/listinfo/linux-phy ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] phy: qcom: qmp-combo: do not decrement init_count on a forced init failure 2026-09-09 15:14 ` [PATCH 1/2] phy: qcom: qmp-combo: do not decrement init_count on a forced init failure Oleg Keri @ 2026-09-09 15:28 ` sashiko-bot 2026-09-09 15:38 ` Oleg Keri 0 siblings, 1 reply; 6+ messages in thread From: sashiko-bot @ 2026-09-09 15:28 UTC (permalink / raw) To: Oleg Keri; +Cc: olteanv, linux-phy, neil.armstrong, vkoul Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: New issues: - [High] Failed forced initialization desynchronizes the driver state from the hardware state, leading to unbalanced clock and regulator disables. Pre-existing issues: - [High] Manual manipulation of `usb_init_count` in the Type-C mux callback causes state desynchronization and integer underflow. - [High] Double clk_disable_unprepare() on pipe_clk during USB power-on failure. - [High] Type-C switch and mux callbacks ignore the return value of qmp_combo_usb_power_on(), causing clock desynchronization on PHY initialization timeout. -- commit d04edd9a71708dccd20d740d1f23dda7649b7982 Author: Oleg Keri <okerixx@gmail.com> phy: qcom: qmp-combo: do not decrement init_count on a forced init failure This commit updates qmp_combo_com_init() to avoid decrementing init_count when a forced initialization fails. This is intended to prevent init_count from going negative after a failed forced initialization drops a reference it never took. > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c > --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c > +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c > @@ -4113,7 +4113,8 @@ static int qmp_combo_com_init(struct qmp_combo *qmp, bool force) > err_disable_regulators: > regulator_bulk_disable(cfg->num_vregs, qmp->vregs); > err_decrement_count: > - qmp->init_count--; > + if (!force) > + qmp->init_count--; [Severity: High] When a forced init fails here, doesn't skipping the decrement leave init_count at 1 while the hardware is fully disabled by the preceding error rollbacks? If the normal consumer later cleans up via qmp_combo_com_exit(qmp, false), init_count will drop to 0. Would this cause the driver to blindly execute clk_bulk_disable_unprepare() and regulator_bulk_disable() on resources that were already disabled by the error path? > > return ret; > } [Severity: High] This is a pre-existing issue, but does manual manipulation of usb_init_count in qmp_combo_typec_mux_set() cause state desynchronization? In qmp_combo_typec_mux_set(): if (new_mode == QMPPHY_MODE_DP_ONLY) { if (qmp->usb_init_count) qmp->usb_init_count--; } If the USB core later unbinds or suspends, it invokes qmp_combo_usb_exit() which unconditionally executes: qmp->usb_init_count--; Could this cause an integer underflow, leading subsequent Type-C switches to evaluate if (qmp->usb_init_count) as true and incorrectly access powered-down PHY registers? [Severity: High] This is a pre-existing issue, but is there a double clk_disable_unprepare() on pipe_clk during a USB power-on failure? If qmp_combo_usb_power_on() fails (e.g. due to PHY timeout), its error path calls: err_disable_pipe_clk: clk_disable_unprepare(qmp->pipe_clk); Since the caller qmp_combo_usb_init() handles this error by invoking qmp_combo_com_exit(qmp, false), qmp_combo_com_exit() will symmetrically clean up by calling clk_disable_unprepare(qmp->pipe_clk) again. Does this cause an underflow since qmp_combo_usb_power_on() cleans up a clock it didn't originally allocate? [Severity: High] This is a pre-existing issue, but do the Type-C switch and mux callbacks safely handle initialization failures? In qmp_combo_typec_switch_set(): if (qmp->usb_init_count) qmp_combo_usb_power_on(qmp->usb_phy); And similarly in qmp_combo_typec_mux_set(): qmp_combo_usb_power_on(qmp->usb_phy); if (!qmp->usb_init_count) qmp->usb_init_count++; Since the return value of qmp_combo_usb_power_on() is ignored, if it fails and its error path disables pipe_clk, these callbacks proceed and track usb_init_count as successfully enabled. When a subsequent exit event occurs, will qmp_combo_com_exit() incorrectly disable pipe_clk again? -- Sashiko AI review · https://sashiko.dev/#/patchset/20260909151443.10276-1-okerixx@gmail.com?part=1 -- linux-phy mailing list linux-phy@lists.infradead.org https://lists.infradead.org/mailman/listinfo/linux-phy ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] phy: qcom: qmp-combo: do not decrement init_count on a forced init failure 2026-09-09 15:28 ` sashiko-bot @ 2026-09-09 15:38 ` Oleg Keri 0 siblings, 0 replies; 6+ messages in thread From: Oleg Keri @ 2026-09-09 15:38 UTC (permalink / raw) To: Vinod Koul, Neil Armstrong, Manivannan Sadhasivam, Johan Hovold, Bjorn Andersson Cc: linux-phy, linux-arm-msm, linux-kernel, Michael Scott You are right, and this patch should be dropped. Please do not apply it. The diagnosis in the commit message holds - a forced qmp_combo_com_init() never takes the reference, because && short-circuits on !force, so the unconditional decrement in the error path drops one that was never taken. The remedy does not. Tracing a consumer that holds phy_init(), so init_count is 1 and the block is up, through a typec callback that does com_exit(force) then a com_init(force) which fails: before this patch init_count 0, hardware down. a later com_exit(false) does --init_count -> -1, which is non-zero, so it returns early: no double disable, but the reference is lost and the count stays negative. with this patch init_count 1, hardware down. a later com_exit(false) does --init_count -> 0 and proceeds to the full teardown, so clk_disable_unprepare(), clk_bulk_disable_unprepare() and regulator_bulk_disable() all run against resources the error path already released. So it swaps a silently negative counter for a real unbalanced disable, which is worse than what it replaces. Exactly what you asked. The underlying problem is that init_count is being used for two different things - how many consumers hold a reference, and whether the common block is currently powered - and after a failed forced re-init those two diverge. No adjustment to the single counter can express that, so I would rather send nothing here than send another half fix. I will come back to it with the hardware state tracked separately, once I can convince myself the teardown paths are balanced in every case. Patch 2/2 does not touch the counter at all - it only stops both callbacks carrying on to write PHY registers after the bringup has already failed and left the clocks disabled - so it stands on its own and can still be applied. The runtime PM patch it was a follow-up to is likewise independent: https://lore.kernel.org/all/20260909140345.4239-1-okerixx@gmail.com/ pw-bot: changes-requested -- linux-phy mailing list linux-phy@lists.infradead.org https://lists.infradead.org/mailman/listinfo/linux-phy ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] phy: qcom: qmp-combo: check qmp_combo_com_init() in the typec callbacks 2026-09-09 15:14 [PATCH 0/2] phy: qcom: qmp-combo: fix forced com_init() error handling Oleg Keri 2026-09-09 15:14 ` [PATCH 1/2] phy: qcom: qmp-combo: do not decrement init_count on a forced init failure Oleg Keri @ 2026-09-09 15:14 ` Oleg Keri 2026-09-09 15:25 ` sashiko-bot 1 sibling, 1 reply; 6+ messages in thread From: Oleg Keri @ 2026-09-09 15:14 UTC (permalink / raw) To: Vinod Koul, Neil Armstrong, Manivannan Sadhasivam, Bjorn Andersson, Johan Hovold Cc: linux-arm-msm, linux-phy, linux-kernel, Michael Scott qmp_combo_typec_switch_set() and qmp_combo_typec_mux_set() tear the common block down and bring it straight back up on an orientation or altmode change, but they discard the result of the bringup: qmp_combo_com_exit(qmp, true); 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 - a regulator, a reset or clk_bulk_prepare_enable() - it unwinds what it had brought up and returns an error, leaving the clocks disabled. Both callbacks then carry on and qmp_combo_usb_power_on(), and dp_aux_init() after it, write PHY registers with no clock running. Propagate the failure instead. 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 | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c index 7d740ed0ce16..44b6f29926dc 100644 --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c @@ -4924,6 +4924,7 @@ static int qmp_combo_typec_switch_set(struct typec_switch_dev *sw, { struct qmp_combo *qmp = typec_switch_get_drvdata(sw); const struct qmp_phy_cfg *cfg = qmp->cfg; + int ret = 0; if (orientation == qmp->orientation || orientation == TYPEC_ORIENTATION_NONE) return 0; @@ -4938,15 +4939,19 @@ static int qmp_combo_typec_switch_set(struct typec_switch_dev *sw, qmp_combo_usb_power_off(qmp->usb_phy); qmp_combo_com_exit(qmp, true); - qmp_combo_com_init(qmp, true); + ret = qmp_combo_com_init(qmp, true); + if (ret) + goto out; + if (qmp->usb_init_count) qmp_combo_usb_power_on(qmp->usb_phy); if (qmp->dp_init_count) cfg->dp_aux_init(qmp); } +out: mutex_unlock(&qmp->phy_mutex); - return 0; + return ret; } static int qmp_combo_typec_mux_set(struct typec_mux_dev *mux, struct typec_mux_state *state) @@ -4955,6 +4960,7 @@ static int qmp_combo_typec_mux_set(struct typec_mux_dev *mux, struct typec_mux_s const struct qmp_phy_cfg *cfg = qmp->cfg; enum qmpphy_mode new_mode; unsigned int svid; + int ret; guard(mutex)(&qmp->phy_mutex); @@ -5012,7 +5018,9 @@ static int qmp_combo_typec_mux_set(struct typec_mux_dev *mux, struct typec_mux_s qmp_combo_com_exit(qmp, true); /* Now everything's powered down, power up the right PHYs */ - qmp_combo_com_init(qmp, true); + ret = qmp_combo_com_init(qmp, true); + if (ret) + return ret; if (new_mode == QMPPHY_MODE_DP_ONLY) { if (qmp->usb_init_count) -- 2.55.0 -- linux-phy mailing list linux-phy@lists.infradead.org https://lists.infradead.org/mailman/listinfo/linux-phy ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] phy: qcom: qmp-combo: check qmp_combo_com_init() in the typec callbacks 2026-09-09 15:14 ` [PATCH 2/2] phy: qcom: qmp-combo: check qmp_combo_com_init() in the typec callbacks Oleg Keri @ 2026-09-09 15:25 ` sashiko-bot 0 siblings, 0 replies; 6+ messages in thread From: sashiko-bot @ 2026-09-09 15:25 UTC (permalink / raw) To: Oleg Keri; +Cc: neil.armstrong, vkoul, linux-phy, olteanv Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: Pre-existing issues: - [High] Unclocked register access causing a kernel panic during PHY teardown after a Type-C switch failure. -- commit cb054e3d77c744e573bfa78e02cb5e307f60efa6 Author: Oleg Keri <okerixx@gmail.com> phy: qcom: qmp-combo: check qmp_combo_com_init() in the typec callbacks This commit updates the Type-C callbacks in the Qualcomm QMP combo PHY driver to propagate errors from qmp_combo_com_init(). Instead of ignoring a bringup failure, it bails out early to avoid writing to PHY registers without a clock running. > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c > --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c > +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c [ ... ] > @@ -4791,15 +4792,19 @@ static int qmp_combo_typec_switch_set(struct typec_switch_dev *sw, > qmp_combo_usb_power_off(qmp->usb_phy); > qmp_combo_com_exit(qmp, true); > > - qmp_combo_com_init(qmp, true); > + ret = qmp_combo_com_init(qmp, true); > + if (ret) > + goto out; [Severity: High] This is a pre-existing issue, but does this early bailout path leave the logical state out of sync with the hardware? If qmp_combo_com_init() fails, the hardware remains unclocked, but the generic PHY framework appears unaware of this failure. > + > if (qmp->usb_init_count) > qmp_combo_usb_power_on(qmp->usb_phy); [ ... ] > @@ -4863,7 +4869,9 @@ static int qmp_combo_typec_mux_set(struct typec_mux_dev *mux, struct typec_mux_s > qmp_combo_com_exit(qmp, true); > > /* Now everything's powered down, power up the right PHYs */ > - qmp_combo_com_init(qmp, true); > + ret = qmp_combo_com_init(qmp, true); > + if (ret) > + return ret; [Severity: High] This is also a pre-existing issue, but does this early exit similarly leave the PHY powered down while software counters indicate it is still active? If a consumer later tears down the PHY by calling phy_exit(), the teardown path will call qmp_combo_usb_power_off(). Since the clocks were left disabled during the early return here, qmp_combo_usb_power_off() will perform an unconditional register access without clocks: qmp_combo_usb_power_off() { ... qphy_setbits(qmp->pcs, cfg->regs[QPHY_SW_RESET], SW_RESET); ... } Could this trigger a Synchronous External Abort (SEA) kernel panic on Qualcomm platforms, since the early exit strategy leaves the PHY framework thinking the PHY is still active? > > if (new_mode == QMPPHY_MODE_DP_ONLY) { > if (qmp->usb_init_count) -- Sashiko AI review · https://sashiko.dev/#/patchset/20260909151443.10276-1-okerixx@gmail.com?part=2 -- linux-phy mailing list linux-phy@lists.infradead.org https://lists.infradead.org/mailman/listinfo/linux-phy ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2026-09-09 15:38 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-09-09 15:14 [PATCH 0/2] phy: qcom: qmp-combo: fix forced com_init() error handling Oleg Keri 2026-09-09 15:14 ` [PATCH 1/2] phy: qcom: qmp-combo: do not decrement init_count on a forced init failure Oleg Keri 2026-09-09 15:28 ` sashiko-bot 2026-09-09 15:38 ` Oleg Keri 2026-09-09 15:14 ` [PATCH 2/2] phy: qcom: qmp-combo: check qmp_combo_com_init() in the typec callbacks Oleg Keri 2026-09-09 15:25 ` sashiko-bot
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.