Re: [PATCH v4 9/9] phy: qualcomm: qmp-combo: Fix error propagation in qmp_combo_dp_power_on()

[sashiko-bot@kernel.org]

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

New issues:
- [High] Propagating the error from `cfg->configure_dp_phy()` bypasses the framework's cleanup, leaving the DP PHY hardware partially powered on indefinitely.

Pre-existing issues:
- [High] The identical bug (ignoring `configure_dp_phy()` error) exists in the sibling driver `phy-qcom-qmp-usbc.c` and was missed by this patch.
- [High] Ignored failure of `qmp_combo_com_init()` in Type-C multiplexing logic leads to unclocked register access and kernel panic.
--

commit cba00203ff9ff2a7508310091ad676a9c5ba878e
Author: Nabige Aala <nabige.aala@oss.qualcomm.com>

phy: qualcomm: qmp-combo: Fix error propagation in qmp_combo_dp_power_on()

This commit modifies qmp_combo_dp_power_on() to capture and propagate
errors from the configure_dp_phy() callback instead of silently ignoring
them. On failure, it also skips setting the dp_powered_on flag and
returns the error to the caller.

> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> index bb1c5df52d095..d77fe94b312cb 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c

[ ... ]

> @@ -4328,13 +4329,17 @@ static int qmp_combo_dp_power_on(struct phy *phy)
>  	cfg->configure_dp_tx(qmp);
>  
>  	/* Configure link rate, swing, etc. */
> -	cfg->configure_dp_phy(qmp);
> +	ret = cfg->configure_dp_phy(qmp);
> +	if (ret) {
> +		dev_err(qmp->dev, "DP PHY configuration failed: %d\n", ret);
> +		goto out_unlock;

[Severity: High]
Does propagating the error directly to the caller leave the DP PHY hardware
partially powered on indefinitely?

Earlier in this flow, the DP PHY block is brought out of power-down by
setting the active bits in QSERDES_DP_PHY_PD_CTL. Because the PHY core
framework treats a failure here as a failed power-on, it will never invoke
qmp_combo_dp_power_off() to restore the power-down state.

Could this error path assert DP_PHY_PD_CTL_PSR_PWRDN before returning to
prevent a power leak and undefined hardware state?

> +	}
>  
>  	qmp->dp_powered_on = true;
>  
> +out_unlock:
>  	mutex_unlock(&qmp->phy_mutex);
> -
> -	return 0;
> +	return ret;
>  }

[Severity: High]
This is a pre-existing issue, but does the identical bug exist in the
sibling driver?

Looking at qmp_usbc_dp_power_on() in drivers/phy/qualcomm/phy-qcom-qmp-usbc.c,
it shares the same initialization flow but unconditionally returns 0:

    /* Configure link rate, swing, etc. */
    cfg->configure_dp_phy(qmp);

    mutex_unlock(&qmp->phy_mutex);

    return 0;

If configure_dp_phy() fails, the framework is falsely informed that the PHY
is successfully powered on, which will cause the display subsystem to
silently fail during link training. Should a similar fix be applied there
for consistency?

[Severity: High]
This is a pre-existing issue, but does the Type-C multiplexing logic ignore
initialization failures?

In qmp_combo_typec_switch_set(), the code calls qmp_combo_com_init() to
enable PHY clocks and regulators:

        qmp_combo_com_exit(qmp, true);

        qmp_combo_com_init(qmp, true);
        if (qmp->usb_init_count)
            qmp_combo_usb_power_on(qmp->usb_phy);

If enabling clocks or regulators fails, the code proceeds to call
qmp_combo_usb_power_on() and cfg->dp_aux_init() which perform MMIO writes.

Can accessing unclocked or unpowered peripheral registers on these platforms
trigger a synchronous external abort and kernel panic?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260908-glymur-phy-v3-v4-0-ff22e5150538@oss.qualcomm.com?part=9

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