Re: [PATCH] PCI/PM: Move ASUS EHCI workaround out of generic code

[Rafael J. Wysocki]

On Thu, Sep 11, 2025 at 3:12 PM Lukas Wunner <lukas@xxxxxxxxx> wrote:
>
> In 2012, commit dbf0e4c7257f ("PCI: EHCI: fix crash during suspend on ASUS
> computers") amended pci_pm_suspend_noirq() to work around a BIOS issue by
> clearing the Command register if the suspended device is a USB EHCI host
> controller.
>
> Commit 0b68c8e2c3af ("PCI: EHCI: Fix crash during hibernation on ASUS
> computers") subsequently amended pci_pm_poweroff_noirq() to do the same.
>
> Two years later, commit 7d2a01b87f16 ("PCI: Add pci_fixup_suspend_late
> quirk pass") introduced the ability to execute arbitrary quirks
> specifically in pci_pm_suspend_noirq() and pci_pm_poweroff_noirq().
>
> This allows moving the ASUS workaround out of generic code and into a
> proper quirk to improve maintainability and readability.  Constrain to x86
> since the ASUS BIOS doesn't seem to have been used on other arches.
>
> lspci output of affected EHCI host controllers reveals that the only bits
> set in the Command register are Memory Space Enable and Bus Master Enable:
>   https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=658778
>
> The latter is cleared by:
>   hcd_pci_suspend()
>     suspend_common()
>       pci_disable_device()
>
> pci_disable_device() does not clear I/O and Memory Space Enable, although
> its name suggests otherwise.  The kernel has never disabled these bits
> once they're enabled.  Doing so would avoid the need for the quirk, but it
> is unclear what will break if this fundamental behavior is changed.
>
> Signed-off-by: Lukas Wunner <lukas@xxxxxxxxx>

Acked-by: Rafael J. Wysocki (Intel) <rafael@xxxxxxxxxx>

> ---
>  arch/x86/pci/fixup.c     | 19 +++++++++++++++++++
>  drivers/pci/pci-driver.c | 19 -------------------
>  2 files changed, 19 insertions(+), 19 deletions(-)
>
> diff --git a/arch/x86/pci/fixup.c b/arch/x86/pci/fixup.c
> index e7e71490bd25..c34ff72434f2 100644
> --- a/arch/x86/pci/fixup.c
> +++ b/arch/x86/pci/fixup.c
> @@ -1041,3 +1041,22 @@ static void quirk_tuxeo_rp_d3(struct pci_dev *pdev)
>  }
>  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_AMD, 0x1502, quirk_tuxeo_rp_d3);
>  #endif /* CONFIG_SUSPEND */
> +
> +#ifdef CONFIG_PM_SLEEP
> +/*
> + * Some BIOSes from ASUS have a bug: If a USB EHCI host controller's Command
> + * register is not 0 on suspend, the BIOS assumes that the controller has not
> + * been quiesced and tries to turn it off.  If the controller is already in D3,
> + * this can hang or cause memory corruption.
> + *
> + * Since the value of the Command register does not matter once the device has
> + * been suspended, it can safely be set to 0.
> + */
> +static void quirk_clear_command_reg(struct pci_dev *pdev)
> +{
> +       pci_write_config_word(pdev, PCI_COMMAND, 0);
> +}
> +DECLARE_PCI_FIXUP_CLASS_SUSPEND_LATE(PCI_ANY_ID, PCI_ANY_ID,
> +                                    PCI_CLASS_SERIAL_USB_EHCI, 0,
> +                                    quirk_clear_command_reg);
> +#endif /* CONFIG_PM_SLEEP */
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index 63665240ae87..e1089dfeb419 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -914,18 +914,6 @@ static int pci_pm_suspend_noirq(struct device *dev)
>
>         pci_pm_set_unknown_state(pci_dev);
>
> -       /*
> -        * Some BIOSes from ASUS have a bug: If a USB EHCI host controller's
> -        * PCI COMMAND register isn't 0, the BIOS assumes that the controller
> -        * hasn't been quiesced and tries to turn it off.  If the controller
> -        * is already in D3, this can hang or cause memory corruption.
> -        *
> -        * Since the value of the COMMAND register doesn't matter once the
> -        * device has been suspended, we can safely set it to 0 here.
> -        */
> -       if (pci_dev->class == PCI_CLASS_SERIAL_USB_EHCI)
> -               pci_write_config_word(pci_dev, PCI_COMMAND, 0);
> -
>  Fixup:
>         pci_fixup_device(pci_fixup_suspend_late, pci_dev);
>
> @@ -1205,13 +1193,6 @@ static int pci_pm_poweroff_noirq(struct device *dev)
>         if (!pci_dev->state_saved && !pci_has_subordinate(pci_dev))
>                 pci_prepare_to_sleep(pci_dev);
>
> -       /*
> -        * The reason for doing this here is the same as for the analogous code
> -        * in pci_pm_suspend_noirq().
> -        */
> -       if (pci_dev->class == PCI_CLASS_SERIAL_USB_EHCI)
> -               pci_write_config_word(pci_dev, PCI_COMMAND, 0);
> -
>         pci_fixup_device(pci_fixup_suspend_late, pci_dev);
>
>         return 0;
> --
> 2.51.0
>