Re: [PATCH 2/4] phy: qcom: qmp-combo: track whether the cached typec_mux mode was committed to hardware

[sashiko-bot@kernel.org]

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

New issues:
- [High] The patch is a complete logical no-op and fails to alter the hardware programming flow in the described failure scenario.
- [Medium] The patch unconditionally marks `qmpphy_mode_committed = true` in `qmp_combo_typec_mux_set()`, ignoring hardware reprogramming failures.

Pre-existing issues:
- [High] `qmp_combo_com_init()` incorrectly decrements `init_count` on failure when `force == true`, corrupting reference counts.
- [High] TOCTOU race condition in `qmp_combo_usb_exit()` causes `usb_init_count` integer underflow.
- [High] Delayed PHY mode switches are permanently dropped if the DP PHY is actively powered on.
--

commit 6892dbad473d7036463b8a81f539f3ea5bbf55d4
Author: Michael Scott <mike.scott@oss.qualcomm.com>
Subject: phy: qcom: qmp-combo: track whether the cached typec_mux mode was committed to hardware

This patch attempts to fix an issue where the typec_mux_set function
bails out early and fails to reprogram hardware if called before
phy_init. It adds a committed flag to track if the cached mode matches
the actual hardware state.

> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> index 4057583c0cfeb..9b6da783ce607 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> @@ -3747,6 +3748,9 @@ static int qmp_combo_com_init(struct qmp_combo *qmp, bool force)
>  	qphy_setbits(qmp->pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL],
>  			SW_PWRDN);
>  
> +	/* com_init() just programmed registers from qmp->qmpphy_mode. */
> +	qmp->qmpphy_mode_committed = true;
> +
>  	return 0;
>  
>  err_assert_reset:
>  	reset_control_bulk_assert(cfg->num_resets, qmp->resets);
>  err_disable_regulators:
>  	regulator_bulk_disable(cfg->num_vregs, qmp->vregs);
>  err_decrement_count:
>  	qmp->init_count--;
>  
>  	return ret;
>  }

[Severity: High]
This is a pre-existing issue, but does this error path corrupt the reference
count when called with force == true?

If qmp_combo_com_init() is called with force = true, the short-circuit
evaluation bypasses incrementing qmp->init_count:

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

However, if a subsequent operation like regulator_bulk_enable() fails, the
error path jumps to err_decrement_count and unconditionally decrements it.

Could this underflow init_count to -1, permanently breaking the initialization
state machine since subsequent checks like `if (qmp->init_count++)` will see
truthy values?

[ ... ]

> @@ -4507,9 +4511,22 @@ static int qmp_combo_typec_mux_set(struct typec_mux_dev *mux, struct typec_mux_s
>  		new_mode = QMPPHY_MODE_USB3_ONLY;
>  	}
>  
> +	/*
> +	 * Fast-path bail only when the cached mode is also known to be
> +	 * committed to hardware. The cache may be ahead of the hardware
> +	 * if a typec_mux_set arrived while the PHY had not yet been
> +	 * initialised (init_count == 0); in that case the cache update
> +	 * below was the only thing that ran, and we still need to drive
> +	 * the registers when the PHY does come up.
> +	 */
>  	if (new_mode == qmp->qmpphy_mode) {
> -		dev_dbg(qmp->dev, "typec_mux_set: same qmpphy mode, bail out\n");
> -		return 0;
> +		if (qmp->qmpphy_mode_committed) {
> +			dev_dbg(qmp->dev,
> +				"typec_mux_set: same qmpphy mode (committed), bail out\n");
> +			return 0;
> +		}
> +		dev_dbg(qmp->dev,
> +			"typec_mux_set: same qmpphy mode but uncommitted; reprogramming\n");
>  	}
>  
>  	if (qmp->qmpphy_mode != QMPPHY_MODE_USB3_ONLY && qmp->dp_powered_on) {
>  		dev_err(qmp->dev, "DP PHY is still in use, delaying switch\n");
>  		return 0;
>  	}

[Severity: High]
This is a pre-existing issue, but does this early return permanently drop
delayed PHY mode switches?

If a switch to QMPPHY_MODE_USB3_ONLY is requested but the DP PHY is actively
powered on, this branch bails out early, logs a message, and returns 0.

Because it returns success without updating qmp->qmpphy_mode or scheduling a
delayed work to apply the switch later, the Type-C framework assumes the mode
was successfully applied.

Does this cause a permanent desynchronization between software state and
hardware, keeping the DP PHY powered when it should be disabled?

> @@ -4521,6 +4538,7 @@ static int qmp_combo_typec_mux_set(struct typec_mux_dev *mux, struct typec_mux_s
>  		qmp->qmpphy_mode, new_mode);
>  
>  	qmp->qmpphy_mode = new_mode;
> +	qmp->qmpphy_mode_committed = false;
>  
>  	if (qmp->init_count) {

[Severity: High]
Is this patch effectively a logical no-op in the failure scenario it
describes?

If typec_mux_set() is called before phy_init() (when init_count is 0), the
early bail-out is bypassed because qmpphy_mode_committed is false. However,
the code then reaches the `if (qmp->init_count)` check here, which evaluates
to false, causing the function to return without touching the hardware anyway.

When phy_init() eventually executes, qmp_combo_com_init() correctly syncs
hardware to the cached mode and unconditionally sets
qmpphy_mode_committed = true.

Subsequent calls to typec_mux_set() with the same mode will see
qmpphy_mode_committed == true and bail out early, exactly as they did before
this patch. Does this mean the hardware state remains unchanged from the
pre-patch behavior?

>  		if (qmp->usb_init_count)
[ ... ]
>  			if (qmp->dp_init_count)
>  				cfg->dp_aux_init(qmp);
>  		}
> +
> +		/* Reprogram complete; cache now reflects hardware. */
> +		qmp->qmpphy_mode_committed = true;
>  	}

[Severity: Medium]
Is it safe to unconditionally mark the hardware state as committed here?

The code calls qmp_combo_com_init(qmp, true) earlier in this block to apply
hardware changes, but ignores its return value. If qmp_combo_com_init() fails,
the hardware state remains unconfigured, but this flag is blindly set to true.

Would this prevent any subsequent attempts to correct the hardware state,
since future calls will trigger the early bail-out on
qmp->qmpphy_mode_committed?


[Severity: High]
This isn't a bug introduced by this patch, but is there a time-of-check to
time-of-use race condition in qmp_combo_usb_exit() that can underflow
usb_init_count?

If qmp->usb_init_count is checked outside the phy_mutex in
qmp_combo_usb_exit(), a concurrent Type-C mode switch could occur:

Thread 1: qmp_combo_usb_exit()
    if (qmp->usb_init_count == 0)
        return;
    // Preempted before acquiring phy_mutex

Thread 2: qmp_combo_typec_mux_set()
    mutex_lock(&qmp->phy_mutex);
    // Switches to QMPPHY_MODE_DP_ONLY
    qmp->usb_init_count--; // Now 0
    mutex_unlock(&qmp->phy_mutex);

Thread 1 resumes:
    mutex_lock(&qmp->phy_mutex);
    ...
    qmp->usb_init_count--; // Underflows to UINT_MAX

Could this permanently break subsequent power management logic and allow
unclocked hardware accesses?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260521003615.1260844-1-mike.scott@oss.qualcomm.com?part=2