Re: [PATCH 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 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