Re: [PATCH] phy: qcom: qmp-combo: hold a runtime PM reference 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] Ignored return value of qmp_combo_com_init() leads to unclocked register access crashes and init_count state corruption.
--

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

    phy: qcom: qmp-combo: hold a runtime PM reference in the typec callbacks
    
    qmp_combo_typec_switch_set() and qmp_combo_typec_mux_set() tear the common
    block down and bring it straight back up, calling qmp_combo_com_exit() and
    qmp_combo_com_init() with force=true. Hold a runtime PM reference across the
    teardown and bringup so releasing the clocks cannot re-enter the driver's
    own suspend callback.

> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> index c39ced168d035..c08a22681e0e2 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
[ ... ]
> @@ -4786,6 +4787,8 @@ static int qmp_combo_typec_switch_set(struct typec_switch_dev *sw,
>  	qmp->orientation = orientation;
>  
>  	if (qmp->init_count) {
> +		guard(pm_runtime_active)(qmp->dev);
> +
>  		if (qmp->usb_init_count)
>  			qmp_combo_usb_power_off(qmp->usb_phy);
>  		qmp_combo_com_exit(qmp, true);

[Severity: High]
This isn't a regression introduced by this patch, but I noticed the return
value of qmp_combo_com_init() is ignored just below this diff in
drivers/phy/qualcomm/phy-qcom-qmp-combo.c:

qmp_combo_typec_switch_set() {
    ...
		qmp_combo_com_init(qmp, true);
		if (qmp->usb_init_count)
			qmp_combo_usb_power_on(qmp->usb_phy);
    ...
}

If qmp_combo_com_init() fails to enable clocks (e.g., clk_bulk_prepare_enable
fails), it leaves the clocks disabled and returns an error code.

Because this return value is ignored, the code proceeds to access hardware
registers via qmp_combo_usb_power_on().

Can accessing mapped peripheral registers while their clocks are disabled
cause a System Error (SError) exception on ARM/ARM64, resulting in a kernel
panic?

Additionally, if qmp_combo_com_init() fails when force=true, it can corrupt
the initialization state:

qmp_combo_com_init() {
    ...
	if (!force && qmp->init_count++)
		return 0;
    ...
err_decrement_count:
	qmp->init_count--;

	return ret;
}

The force=true flag causes the function to skip the corresponding increment
of init_count.

Does the error path need to avoid decrementing init_count when force=true?

> @@ -4853,6 +4856,8 @@ static int qmp_combo_typec_mux_set(struct typec_mux_dev *mux, struct typec_mux_s
>  	qmp->qmpphy_mode = new_mode;
>  
>  	if (qmp->init_count) {
> +		guard(pm_runtime_active)(qmp->dev);
> +
>  		if (qmp->usb_init_count)
>  			qmp_combo_usb_power_off(qmp->usb_phy);
>  

[Severity: High]
This is a pre-existing issue, but the return value of qmp_combo_com_init()
is similarly ignored in qmp_combo_typec_mux_set() in
drivers/phy/qualcomm/phy-qcom-qmp-combo.c:

qmp_combo_typec_mux_set() {
    ...
		qmp_combo_com_init(qmp, true);

		if (new_mode == QMPPHY_MODE_DP_ONLY) {
    ...
}

If the clock initialization fails here, does this also lead to subsequent
hardware accesses without active clocks?

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

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