Re: [PATCH v2 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 4 potential issue(s) to consider:

New issues:
- [High] The patch fails to resolve the unprogrammed hardware state because the reprogramming sequence is still completely bypassed when `init_count == 0`.

Pre-existing issues:
- [High] The error handling path in `qmp_combo_com_init()` incorrectly decrements `init_count` when called with `force == true`.
- [High] Type-C MUX set operations modify `usb_init_count` independently of `init_count`, leading to resource leaks.
- [High] Lockless access to `qmp->usb_init_count` in `qmp_combo_usb_exit()` causes a Time-Of-Check to Time-Of-Use race condition.
--

commit c4bd0ad144568b18cf6ccfdc50c09f8d088db9c4
Author: Michael Scott <mike.scott@oss.qualcomm.com>

    phy: qcom: qmp-combo: track whether the cached typec_mux mode was committed to hardware

    This commit introduces a qmpphy_mode_committed flag to track whether the
    cached typec_mux mode has been applied to hardware. It aims to prevent an
    early bail-out in qmp_combo_typec_mux_set() when the PHY has not yet been
    initialized and the hardware state is out of sync with the cache.

> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> index 0db200292642e..e28bc1cc7a78b 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
[ ... ]
> @@ -3754,6 +3755,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_disable_clocks:
>  	clk_bulk_disable_unprepare(qmp->num_clks, qmp->clks);
>  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--;

[Severity: High]
This is a pre-existing issue, but does this error path permanently break the
PHY's power management lifecycle?

When qmp_combo_com_init() is called with force == true, the init_count is not
incremented at the start of the function.  However, if a hardware
initialization step fails and execution jumps to err_decrement_count, it will
decrement qmp->init_count, leading to an integer underflow.

>  	return ret;
>  }
[ ... ]
> @@ -4509,9 +4513,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) {
> @@ -4523,6 +4540,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]
Does this actually fix the unprogrammed hardware state?

When init_count == 0, the early bailout is bypassed, but the hardware
programming block inside this if (qmp->init_count) condition is also skipped.

Once phy_init() is called, it executes qmp_combo_com_init(), which sets
qmpphy_mode_committed = true.  Would subsequent calls to typec_mux_set() then
see committed == true and hit the early bailout just as they did before this
patch?

>  		if (qmp->usb_init_count)

[Severity: High]
This isn't a bug introduced by this patch, but can the interaction between
usb_init_count and init_count lead to a resource leak?

When transitioning to DP-only mode in qmp_combo_typec_mux_set(),
qmp->usb_init_count is decremented while using qmp_combo_com_exit(qmp, true),
which bypasses decrementing the master qmp->init_count.

If usb_init_count drops to 0, would a subsequent phy_exit() by the USB consumer
return early in qmp_combo_usb_exit() and skip calling
qmp_combo_com_exit(qmp, false)? This would leave the USB consumer's original
contribution to init_count unaccounted for, keeping regulators and clocks
enabled forever.

[Severity: High]
This is a pre-existing issue, but does lockless access to qmp->usb_init_count
in qmp_combo_usb_exit() cause a Time-Of-Check to Time-Of-Use race condition?

If qmp_combo_usb_exit() checks usb_init_count without holding qmp->phy_mutex,
could it drop to 0 concurrently via a MUX switch to DP-only in
qmp_combo_typec_mux_set()?  When qmp_combo_usb_exit() subsequently acquires the
lock and continues, it would decrement usb_init_count causing an underflow to
UINT_MAX, allowing future exits to bypass the zero check and trigger unbalanced
teardowns.

> @@ -4551,6 +4569,9 @@ static int qmp_combo_typec_mux_set(struct typec_mux_dev *mux, struct typec_mux_s
>  			if (qmp->dp_init_count)
>  				cfg->dp_aux_init(qmp);
>  		}
> +
> +		/* Reprogram complete; cache now reflects hardware. */
> +		qmp->qmpphy_mode_committed = true;
>  	}
>  
>  	return 0;

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

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