[PATCH 1/2] phy: qcom: qmp-combo: do not decrement init_count on a forced init failure

[sashiko-bot at kernel.org]

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

New issues:
- [High] Failed forced initialization desynchronizes the driver state from the hardware state, leading to unbalanced clock and regulator disables.

Pre-existing issues:
- [High] Manual manipulation of `usb_init_count` in the Type-C mux callback causes state desynchronization and integer underflow.
- [High] Double clk_disable_unprepare() on pipe_clk during USB power-on failure.
- [High] Type-C switch and mux callbacks ignore the return value of qmp_combo_usb_power_on(), causing clock desynchronization on PHY initialization timeout.
--

commit d04edd9a71708dccd20d740d1f23dda7649b7982
Author: Oleg Keri <okerixx at gmail.com>

phy: qcom: qmp-combo: do not decrement init_count on a forced init failure

This commit updates qmp_combo_com_init() to avoid decrementing init_count
when a forced initialization fails. This is intended to prevent init_count
from going negative after a failed forced initialization drops a reference
it never took.

> 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
> @@ -4113,7 +4113,8 @@ static int qmp_combo_com_init(struct qmp_combo *qmp, bool force)
>  err_disable_regulators:
>  	regulator_bulk_disable(cfg->num_vregs, qmp->vregs);
>  err_decrement_count:
> -	qmp->init_count--;
> +	if (!force)
> +		qmp->init_count--;

[Severity: High]
When a forced init fails here, doesn't skipping the decrement leave
init_count at 1 while the hardware is fully disabled by the preceding
error rollbacks?

If the normal consumer later cleans up via qmp_combo_com_exit(qmp, false),
init_count will drop to 0. Would this cause the driver to blindly execute
clk_bulk_disable_unprepare() and regulator_bulk_disable() on resources
that were already disabled by the error path?

>  
>  	return ret;
>  }

[Severity: High]
This is a pre-existing issue, but does manual manipulation of usb_init_count
in qmp_combo_typec_mux_set() cause state desynchronization?

In qmp_combo_typec_mux_set():

    if (new_mode == QMPPHY_MODE_DP_ONLY) {
        if (qmp->usb_init_count)
            qmp->usb_init_count--;
    }

If the USB core later unbinds or suspends, it invokes qmp_combo_usb_exit()
which unconditionally executes:

    qmp->usb_init_count--;

Could this cause an integer underflow, leading subsequent Type-C switches
to evaluate if (qmp->usb_init_count) as true and incorrectly access
powered-down PHY registers?

[Severity: High]
This is a pre-existing issue, but is there a double clk_disable_unprepare()
on pipe_clk during a USB power-on failure?

If qmp_combo_usb_power_on() fails (e.g. due to PHY timeout), its error path
calls:

err_disable_pipe_clk:
    clk_disable_unprepare(qmp->pipe_clk);

Since the caller qmp_combo_usb_init() handles this error by invoking
qmp_combo_com_exit(qmp, false), qmp_combo_com_exit() will symmetrically
clean up by calling clk_disable_unprepare(qmp->pipe_clk) again. Does this
cause an underflow since qmp_combo_usb_power_on() cleans up a clock
it didn't originally allocate?

[Severity: High]
This is a pre-existing issue, but do the Type-C switch and mux callbacks
safely handle initialization failures?

In qmp_combo_typec_switch_set():

    if (qmp->usb_init_count)
        qmp_combo_usb_power_on(qmp->usb_phy);

And similarly in qmp_combo_typec_mux_set():

    qmp_combo_usb_power_on(qmp->usb_phy);
    if (!qmp->usb_init_count)
        qmp->usb_init_count++;

Since the return value of qmp_combo_usb_power_on() is ignored, if it fails
and its error path disables pipe_clk, these callbacks proceed and track
usb_init_count as successfully enabled. When a subsequent exit event occurs,
will qmp_combo_com_exit() incorrectly disable pipe_clk again?

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