Re: [PATCH v2 1/4] phy: qcom: qmp-combo: skip USB power_off/exit after device teardown

[Bryan O'Donoghue]

On 21/05/2026 02:09, Michael Scott wrote:
> qmp_combo_usb_power_off() is reachable from an external consumer
> (notably dwc3 via phy_exit() during driver unbind) after this device's
> backing resources have already been released along a separate teardown
> chain. The dereference of qmp->pcs (whose ioremap mapping has been
> freed by devm cleanup) then takes a level-3 translation fault and
> oopses.
>
> Easily reproducible during testing of USB-C role-switch enablement on
> Dell Latitude 7455 (X1E80100), by writing "none" to a USB-C DWC3's
> usb_role_switch role attribute, e.g.
>
>    echo none > /sys/class/usb_role/a800000.usb-role-switch/role
>
> which triggers the chain:
>
>    Unable to handle kernel paging request at virtual address ffff8000876c5400
>    pc : qmp_combo_usb_power_off.isra.0+0x58/0x470 [phy_qcom_qmp_combo]
>    Call trace:
>      qmp_combo_usb_power_off+0x58/0x470 [phy_qcom_qmp_combo]
>      qmp_combo_usb_exit+0x38/0x90 [phy_qcom_qmp_combo]
>      phy_exit
>      dwc3_phy_exit [dwc3]
>      dwc3_core_remove [dwc3]
>      dwc3_remove [dwc3]
>      platform_remove
>      device_release_driver_internal
>      device_driver_detach
>      unbind_store
>      sysfs_kf_write
>      vfs_write
>      ksys_write
>      __arm64_sys_write
>      el0_svc
>
> Two WARNs precede the oops from the same teardown chain, confirming
> the resource ordering:
>
>    WARNING: drivers/clk/clk.c:4494 at clk_nodrv_disable_unprepare+0x8/0x18
>    WARNING: drivers/regulator/core.c:2657 at _regulator_put+0x84/0x98
>
> i.e. the pipe clock provider has been unregistered and the regulators
> released before qmp_combo_usb_power_off() runs.
>
> The proper long-term fix is a teardown-ordering rework so the QMP
> PHY's backing resources outlive any consumer that may still call its
> phy_ops. Pending that, guard the power_off/exit paths with the
> existing usb_init_count balance so re-entry after teardown does not
> oops. usb_init_count tracks the balance of usb_power_on/off; if it
> is zero we have either never powered on or have already powered off,
> and there is nothing to do.
>
> The same guard is added to qmp_combo_usb_exit() since it is the entry
> point used by external consumers via phy_exit().
>
> Signed-off-by: Michael Scott <mike.scott@xxxxxxxxxxxxxxxx>

Something like this requires a Fixes: tag

> ---
>   drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 22 ++++++++++++++++++++++
>   1 file changed, 22 insertions(+)
>
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> index cdcfad2e86b1..0db200292642 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> @@ -3926,6 +3926,17 @@ static int qmp_combo_usb_power_off(struct phy *phy)
>   	struct qmp_combo *qmp = phy_get_drvdata(phy);
>   	const struct qmp_phy_cfg *cfg = qmp->cfg;
>   
> +	/*
> +	 * Reachable as ->exit from external consumers (notably dwc3) after
> +	 * this device's backing resources have already been released along
> +	 * a teardown chain. Refuse to touch registers in that case.
> +	 */
> +	if (!qmp->usb_init_count) {
> +		dev_dbg(qmp->dev, "%s: PHY not powered on, skipping\n",
> +			__func__);
> +		return 0;
> +	}
> +
>   	/* PHY reset */
>   	qphy_setbits(qmp->pcs, cfg->regs[QPHY_SW_RESET], SW_RESET);
>   
> @@ -3968,6 +3979,17 @@ static int qmp_combo_usb_exit(struct phy *phy)
>   	struct qmp_combo *qmp = phy_get_drvdata(phy);
>   	int ret;
>   
> +	/*
> +	 * See qmp_combo_usb_power_off(): an external consumer may call
> +	 * phy_exit() after the QMP device's resources have been torn
> +	 * down. usb_init_count tracks usb_init/usb_exit balance.
> +	 */
> +	if (!qmp->usb_init_count) {
> +		dev_dbg(qmp->dev, "%s: PHY not initialised, skipping\n",
> +			__func__);
> +		return 0;
> +	}
> +
>   	mutex_lock(&qmp->phy_mutex);
>   	ret = qmp_combo_usb_power_off(phy);

This can't be right - you check usb_init_count before the mutex and then 
again inside the mutex @ qmp_combo_usb_power_off();

It seems like an error to even get to this function with !usb_init_count 
also check if that is a signed or an unsigned value as usb_init_count = 
-1 will evaluate true.

>   	if (ret)
> --
> 2.53.0