Re: [PATCH v3 2/2] phy: qcom: qmp-combo: check qmp_combo_com_init() in the typec callbacks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] phy: qmp-combo: unclocked MMIO access on teardown or retry after init failure
- [Medium] phy: qmp-combo: double disable of clocks and regulators on retry
--

commit baac7a5df2e9b48e6beb5448e1fa906647b06cb5
Author: Oleg Keri <okerixx@gmail.com>

    phy: qcom: qmp-combo: check qmp_combo_com_init() in the typec callbacks

This commit adds error checking to qmp_combo_com_init() in the Type-C and
USB4 callbacks to prevent proceeding when PHY initialization fails. By
returning early, it avoids accessing PHY registers with disabled clocks
in the immediate call path.

> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> index dad7c5cfcf0a6..65c30f3232d1b 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
[ ... ]
> @@ -5042,7 +5050,12 @@ 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) {
> +			qmp->orientation = TYPEC_ORIENTATION_NONE;
> +			return ret;
> +		}
> +

[Severity: High]
Does returning here introduce a regression by abandoning the hardware in an
unclocked state while leaving the logical initialization counters active?

If qmp_combo_com_init() fails, it unwinds and leaves clocks disabled.
However, because the logical initialization counters are still active, a
subsequent Type-C event (retry) will call qmp_combo_usb_power_off() before
re-enabling clocks:

qmp_combo_typec_switch_set() {
    ...
    if (qmp->usb_init_count || qmp->usb4_init_count)
        qmp_combo_usb_power_off(qmp->usb_phy);
    ...
}

This leads to qmp_combo_usb_power_off() performing an MMIO write without
clocks running:

qmp_combo_usb_power_off() {
    ...
    /* PHY reset */
    qphy_setbits(pcs, cfg->regs[QPHY_SW_RESET], SW_RESET);
    ...
}

Will this cause a synchronous external abort (kernel panic) during teardown
or on the next retry?

[Severity: Medium]
Additionally, if we return early on failure, will the subsequent retry cause
a regression by double disabling clocks and regulators?

On the next Type-C event, if both USB and DP were active, the event bypasses
the fast-path check and calls qmp_combo_com_exit(qmp, true) again.

Since force is true, this unconditionally calls clk_bulk_disable_unprepare()
and regulator_bulk_disable() in qmp_combo_com_exit() on resources that were
already disabled by the error path in qmp_combo_com_init(). Could this
underflow the enable counts and corrupt resource tracking?

>  		if (qmp->usb_init_count || qmp->usb4_init_count)
>  			qmp_combo_usb_power_on(qmp->usb_phy);
>  		if (qmp->dp_init_count)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260916133814.3977-1-okerixx@gmail.com?part=2

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