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

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

[Oleg Keri]

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

[PATCH 1/2] phy: qcom: qmp-combo: do not decrement init_count on a forced init failure

[Oleg Keri]

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

[PATCH 2/2] phy: qcom: qmp-combo: check qmp_combo_com_init() 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 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

Re: [PATCH 2/2] phy: qcom: qmp-combo: check qmp_combo_com_init() 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] 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

Re: [PATCH 1/2] phy: qcom: qmp-combo: do not decrement init_count on a forced init failure

[sashiko-bot]

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

Re: [PATCH 1/2] phy: qcom: qmp-combo: do not decrement init_count on a forced init failure

[Oleg Keri]

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