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

[Michael Scott]

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@xxxxxxxxxxxxxxxx>
>
> 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