Re: [PATCH v2 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] Deferred kernel panic in phy_power_off on com_init failure
- [High] PHY remains permanently disabled on transient com_init failure
--

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

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

This commit propagates the return value of qmp_combo_com_init() in
qmp_combo_typec_switch_set() and qmp_combo_reconfigure_phy(). It aims
to prevent writing to PHY registers with no clock running when the
common block initialization fails.

> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> index dad7c5cfcf0a6..50790a49f4cf4 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
[ ... ]
> @@ -4471,7 +4473,9 @@ static int qmp_combo_reconfigure_phy(struct qmp_combo *qmp, enum qmpphy_mode new
>  	qmp->qmpphy_mode = new_mode;
>  
>  	/* 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]
Could returning early here leave the PHY permanently disabled if a transient
error occurs?

Because qmp->qmpphy_mode is updated to new_mode just before this check, if
com_init fails and returns early, future type-c events for the same mode
will hit the cached mode check and skip hardware bringup entirely.

Additionally, could this early return cause a deferred kernel panic?

If com_init fails, the PHY clocks remain disabled but the PHY consumers (like
USB or DP subsystems) maintain their internal logical power state. When the
consumer later shuts down or suspends, it calls phy_power_off(), which
executes qmp_combo_usb_power_off() or qmp_combo_dp_power_off().

For example, qmp_combo_usb_power_off() unconditionally accesses registers:

qmp_combo_usb_power_off()
	qphy_setbits(pcs, cfg->regs[QPHY_SW_RESET], SW_RESET);

Would this unclocked register access cause a synchronous external abort?

>  
>  	if ((qmp->usb_init_count || qmp->usb4_init_count) &&
>  	    new_mode != QMPPHY_MODE_DP_ONLY)
[ ... ]
> @@ -5042,7 +5047,10 @@ 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)
> +			return ret;
> +

[Severity: High]
Similar to the issue above, could returning early here cause a deferred
kernel panic when phy_power_off() is later called by a consumer?

Also, earlier in this function (before this quoted context), qmp->orientation
is updated:

	if (orientation == qmp->orientation || orientation == TYPEC_ORIENTATION_NONE)
		return 0;

	guard(mutex)(&qmp->phy_mutex);
	qmp->orientation = orientation;

	if (qmp->init_count) {
		...
		ret = qmp_combo_com_init(qmp, true);
		if (ret)
			return ret;

If we return early here due to a transient error, will future orientation
switch events hit the cached orientation check and skip the hardware bringup,
leaving the PHY silently broken for that orientation?

>  		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/20260916125619.4587-1-okerixx@gmail.com?part=2

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