Re: [PATCH] USB: hub: call ACPI _PRR reset during port power-cycle on enumeration failure

[AceLan Kao]

Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> 於 2026年7月3日週五 下午5:43寫道：
>
> On Thu, Mar 26, 2026 at 09:17:08AM +0800, Chia-Lin Kao (AceLan) wrote:
> > Some USB-connected devices (e.g. MT7925 Bluetooth on Dell laptops) expose
> > their hardware reset line via an ACPI Power Resource for Reset (_PRR)
> > rather than relying solely on VBUS cycling for recovery.  When the reset
> > GPIO gets stuck low the device stops responding on USB; a VBUS power-cycle
> > alone cannot recover it because the chip remains in reset regardless of
> > VBUS state.
> >
> > Add usb_acpi_port_prr_reset() in usb-acpi.c that, given a hub device and
> > one-based port number, looks up the port's ACPI companion handle, evaluates
> > _PRR to obtain the power-resource reference, and then calls _RST on that
> > reference to toggle the reset line.  The function is a no-op if the port
> > has no ACPI handle or no _PRR method, so it is safe to call unconditionally
> > for every port.
> >
> > Wire it into hub_port_connect() during the mid-retry VBUS power-cycle
> > (at (PORT_INIT_TRIES-1)/2 iterations), calling usb_acpi_port_prr_reset()
> > *after* VBUS goes off and *before* VBUS comes back on.  The ordering is
> > critical: on the tested hardware the ACPI _RST method (MBTR._RST) drives
> > BT_RST low for 200 ms then high again.  If _RST is called after VBUS is
> > already restored the GPIO pulse races with device enumeration starting on
> > the live bus; the device begins asserting USB signals while still held in
> > reset and enumeration fails.  Performing the reset while the port is
> > de-powered ensures the GPIO pulse completes fully before the device is
> > given power and time to initialise.
> >
> > After VBUS is restored, add an msleep(100) conditional on _PRR._RST having
> > succeeded.  USB 2.0 spec §7.1.7.3 (Fig. 7-29) mandates a minimum of 100 ms
> > between VBUS power-on and the first reset signalling for power settling.
> > On root hubs, hub_power_on_good_delay() returns bPwrOn2PwrGood * 2 with
> > no minimum floor; on the tested xHCI root hub bPwrOn2PwrGood = 10, yielding
> > only 20 ms — well below the spec minimum.  (External hubs already enforce
> > a 100 ms minimum via hub_power_on_good_delay().)  When _PRR._RST has been
> > exercised the device must also complete its full power-on sequence (GPIO
> > de-assertion, internal oscillator start, firmware load) before USB
> > enumeration begins.  The 100 ms sleep enforces the spec minimum and gives
> > the device adequate settling time.
> >
> > Tested on a Dell laptop with MT7925 Bluetooth (idVendor=0489,
> > idProduct=e139) whose BT_RST GPIO was stuck low.  With this fix the
> > device recovers autonomously at boot without requiring a G3
> > (mechanical power-off) cycle.  The relevant dmesg sequence:
> >
> >   [    1.448491] usb 3-10: new high-speed USB device number 4 using xhci_hcd
> >   [    6.813942] usb 3-10: device descriptor read/64, error -110
> >   [   22.685978] usb 3-10: device descriptor read/64, error -110
> >   [   22.901715] usb 3-10: new high-speed USB device number 5 using xhci_hcd
> >   [   28.317963] usb 3-10: device descriptor read/64, error -110
> >   [   44.189949] usb 3-10: device descriptor read/64, error -110
> >   [   44.294065] usb usb3-port10: attempt power cycle
> >   [   44.872709] usb 3-10: new high-speed USB device number 6 using xhci_hcd
> >   [   44.888293] usb 3-10: New USB device found, idVendor=0489, idProduct=e139, bcdDevice= 1.00
> >   [   44.888318] usb 3-10: Manufacturer: MediaTek Inc.
> >
> > Signed-off-by: Chia-Lin Kao (AceLan) <acelan.kao@xxxxxxxxxxxxx>
> > ---
> >  drivers/usb/core/hub.c      | 14 ++++++++
> >  drivers/usb/core/usb-acpi.c | 68 +++++++++++++++++++++++++++++++++++++
> >  drivers/usb/core/usb.h      |  3 ++
> >  3 files changed, 85 insertions(+)
> >
> > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> > index 24960ba9caa91..1740e96f73cc6 100644
> > --- a/drivers/usb/core/hub.c
> > +++ b/drivers/usb/core/hub.c
> > @@ -5603,11 +5603,25 @@ static void hub_port_connect(struct usb_hub *hub, int port1, u16 portstatus,
> >
> >               /* When halfway through our retry count, power-cycle the port */
> >               if (i == (PORT_INIT_TRIES - 1) / 2) {
> > +                     int prr_reset;
> > +
> >                       dev_info(&port_dev->dev, "attempt power cycle\n");
> >                       usb_hub_set_port_power(hdev, hub, port1, false);
> >                       msleep(2 * hub_power_on_good_delay(hub));
> > +                     prr_reset = usb_acpi_port_prr_reset(hdev, port1);
> >                       usb_hub_set_port_power(hdev, hub, port1, true);
> >                       msleep(hub_power_on_good_delay(hub));
> > +                     /*
> > +                      * USB 2.0 spec §7.1.7.3 requires at least 100 ms
> > +                      * between VBUS power-on and the first reset for power
> > +                      * settling.  hub_power_on_good_delay() on an xHCI root
> > +                      * hub returns bPwrOn2PwrGood * 2 with no minimum floor,
> > +                      * which can be as little as 20 ms.  When _PRR _RST was
> > +                      * also exercised the device must complete its power-on
> > +                      * sequence before enumeration; enforce the spec minimum.
> > +                      */
> > +                     if (prr_reset == 0)
> > +                             msleep(100);
> >               }
> >       }
> >       if (hub->hdev->parent ||
> > diff --git a/drivers/usb/core/usb-acpi.c b/drivers/usb/core/usb-acpi.c
> > index 489dbdc96f94a..ee62e3fd8e3a1 100644
> > --- a/drivers/usb/core/usb-acpi.c
> > +++ b/drivers/usb/core/usb-acpi.c
> > @@ -142,6 +142,74 @@ int usb_acpi_set_power_state(struct usb_device *hdev, int index, bool enable)
> >  }
> >  EXPORT_SYMBOL_GPL(usb_acpi_set_power_state);
> >
> > +/**
> > + * usb_acpi_port_prr_reset - issue an ACPI _PRR reset on a hub port
> > + * @hdev: USB device belonging to the usb hub
> > + * @port1: port number (one-based)
> > + *
> > + * Some devices expose their hardware reset line via an ACPI Power Resource for
> > + * Reset (_PRR).  When such a device fails to enumerate (e.g. because the reset
> > + * GPIO is stuck low), the USB power-cycle alone is not enough; the firmware
> > + * reset path must also be exercised.
> > + *
> > + * This function evaluates _PRR on the port's ACPI companion to obtain the
> > + * power-resource reference and then calls _RST on that resource to toggle the
> > + * reset line.  It is intended to be called alongside the mid-retry VBUS
> > + * power-cycle already performed by hub_port_connect().
> > + *
> > + * Returns 0 on success, -ENODEV if the port has no ACPI handle or no _PRR
> > + * method, or a negative error code on failure.
> > + */
> > +int usb_acpi_port_prr_reset(struct usb_device *hdev, int port1)
> > +{
> > +     acpi_handle port_handle;
> > +     struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> > +     union acpi_object *pkg, *ref;
> > +     acpi_status status;
> > +     int ret = 0;
> > +
> > +     port_handle = usb_get_hub_port_acpi_handle(hdev, port1);
> > +     if (!port_handle)
> > +             return -ENODEV;
> > +
> > +     if (!acpi_has_method(port_handle, "_PRR"))
> > +             return -ENODEV;
> > +
> > +     status = acpi_evaluate_object(port_handle, "_PRR", NULL, &buffer);
> > +     if (ACPI_FAILURE(status)) {
> > +             dev_dbg(&hdev->dev, "port%d: _PRR evaluation failed: %s\n",
> > +                     port1, acpi_format_exception(status));
> > +             return -ENODEV;
> > +     }
> > +
> > +     pkg = buffer.pointer;
> > +     if (!pkg || pkg->type != ACPI_TYPE_PACKAGE || pkg->package.count < 1) {
> > +             dev_dbg(&hdev->dev, "port%d: _PRR returned unexpected object\n",
> > +                     port1);
> > +             ret = -EINVAL;
> > +             goto out;
> > +     }
> > +
> > +     ref = &pkg->package.elements[0];
> > +     if (ref->type != ACPI_TYPE_LOCAL_REFERENCE || !ref->reference.handle) {
> > +             dev_dbg(&hdev->dev, "port%d: _PRR element is not a reference\n",
> > +                     port1);
> > +             ret = -EINVAL;
> > +             goto out;
> > +     }
> > +
> > +     status = acpi_evaluate_object(ref->reference.handle, "_RST", NULL, NULL);
> > +     if (ACPI_FAILURE(status)) {
> > +             dev_dbg(&hdev->dev, "port%d: _RST evaluation failed: %s\n",
> > +                     port1, acpi_format_exception(status));
> > +             ret = -EIO;
> > +     }
> > +
> > +out:
> > +     kfree(buffer.pointer);
> > +     return ret;
> > +}
> > +
> >  /**
> >   * usb_acpi_add_usb4_devlink - add device link to USB4 Host Interface for tunneled USB3 devices
> >   *
> > diff --git a/drivers/usb/core/usb.h b/drivers/usb/core/usb.h
> > index a9b37aeb515be..4d3dc3bd881b2 100644
> > --- a/drivers/usb/core/usb.h
> > +++ b/drivers/usb/core/usb.h
> > @@ -211,7 +211,10 @@ extern int usb_acpi_register(void);
> >  extern void usb_acpi_unregister(void);
> >  extern acpi_handle usb_get_hub_port_acpi_handle(struct usb_device *hdev,
> >       int port1);
> > +extern int usb_acpi_port_prr_reset(struct usb_device *hdev, int port1);
> >  #else
> >  static inline int usb_acpi_register(void) { return 0; };
> >  static inline void usb_acpi_unregister(void) { };
> > +static inline int usb_acpi_port_prr_reset(struct usb_device *hdev,
> > +                                       int port1) { return -ENODEV; }
> >  #endif
> > --
> > 2.53.0
> >
> >
>
> Please see the review comments at:
>         https://sashiko.dev/#/patchset/20260326011708.1128840-1-acelan.kao@xxxxxxxxxxxxx

Thanks Greg.

Went through the sashiko review. The single finding claims
usb_acpi_port_prr_reset() should iterate every element of the _PRR
package and call _RST on each. That's based on a misreading of _PRR.

Per ACPI 6.4 §7.3.26 (_PRR, Power Resource for Reset):

  "This object evaluates to a single reference to a power resource. ...
   Return Value: A single element Package containing a Reference to the
   power reset resource."

_PRR is a fixed single-element package, not a variable-length list.
ACPICA encodes this in acpredef.h as ACPI_PTYPE1_FIXED with 1 reference
(as opposed to _PR0/_PR1/_PR2/_PR3, which are ACPI_PTYPE1_VAR reference
lists -- I suspect that's the source of the confusion). The "reset must
affect all ACPI-described devices that reference it" wording in the _RST
description refers to the devices sharing that one power resource, not to
multiple resources in the package.

The two existing in-tree _PRR/_RST users take elements[0] and call _RST
on it exactly as this patch does:

  - drivers/bluetooth/btintel.c: btintel_acpi_reset_method()
  - drivers/net/.../iwlwifi/pcie/gen1_2/trans.c: iwl_trans_pcie_call_reset()

Both are in fact stricter -- they reject package.count != 1, whereas I
accept count >= 1 and use the first element. Happy to tighten my check
from "< 1" to "!= 1" to match that precedent and make the single-element
invariant explicit, but there's no functional issue here.

Thanks,
AceLan