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

[Michael Scott <mike.scott@oss.qualcomm.com>]

On 5/21/26 4:58 AM, Bryan O'Donoghue wrote:
> 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@oss.qualcomm.com>
>
> Something like this requires a Fixes: tag
Thanks!  Noted.
>
>> ---
>>   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.

Yep, there are a few issues with this patch (and this looks like an 
extra check I accidentally left in).  I'm dropping this from the 
patchset and I'll revisit at a later date.

Apologies for the noise.

>
>>       if (ret)
>> -- 
>> 2.53.0
>>
>