linux-kernel - Re: [PATCH] PCI: pciehp: Fix system hang on resume after hot-unplug during suspend

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAFv23QkwxmT7qrnbfEpJNN+mnevNAor6Dk7efvYNOdjR9tGyrw@mail.gmail.com>
Date: Mon, 30 Sep 2024 09:31:53 +0800
From: AceLan Kao <acelan.kao@...onical.com>
To: Lukas Wunner <lukas@...ner.de>
Cc: Bjorn Helgaas <bhelgaas@...gle.com>, Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>, 
	linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] PCI: pciehp: Fix system hang on resume after hot-unplug
 during suspend

Lukas Wunner <lukas@...ner.de> 於 2024年9月28日 週六 下午8:51寫道：
>
> On Fri, Sep 27, 2024 at 11:28:54AM +0200, Lukas Wunner wrote:
> > I realize now that commit 9d573d19547b ("PCI: pciehp: Detect device
> > replacement during system sleep") is a little overzealous because it
> > not only reacts to *replaced* devices but also to *unplugged* devices:
> > If the device was unplugged, reading the vendor and device ID returns
> > 0xffff, which is different from the cached value, so the device is
> > assumed to have been replaced even though it's actually been unplugged.
> >
> > The device replacement check runs in the ->resume_noirq phase.  Later on
> > in the ->resume phase, pciehp_resume() calls pciehp_check_presence() to
> > check for unplugged devices.  Commit 9d573d19547b inadvertantly reacts
> > before pciehp_check_presence() gets a chance to react.  So that's something
> > that we should probably change.
>
> FWIW, below is a (compile-tested only) patch which modifies
> pciehp_device_replaced() to return false if the device was
> *unplugged* during system sleep.  It continues to return
> true if it was *replaced* during system sleep.
>
> This might avoid the issue you're seeing, though it would
> be good if you could also try Keith's deadlock prevention
> patch (without any other patch) to determine if the deadlock
> is the actual root cause (as I suspect).
>
> Thanks!
>
> -- >8 --
>
> diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c
> index ff458e6..174832b 100644
> --- a/drivers/pci/hotplug/pciehp_core.c
> +++ b/drivers/pci/hotplug/pciehp_core.c
> @@ -287,24 +287,32 @@ static int pciehp_suspend(struct pcie_device *dev)
>  static bool pciehp_device_replaced(struct controller *ctrl)
>  {
>         struct pci_dev *pdev __free(pci_dev_put);
> +       u64 dsn;
>         u32 reg;
>
>         pdev = pci_get_slot(ctrl->pcie->port->subordinate, PCI_DEVFN(0, 0));
>         if (!pdev)
> +               return false;
> +
> +       if (pci_read_config_dword(pdev, PCI_VENDOR_ID, &reg) == 0 &&
> +           !PCI_POSSIBLE_ERROR(reg) &&
> +           reg != (pdev->vendor | (pdev->device << 16)))
>                 return true;
>
> -       if (pci_read_config_dword(pdev, PCI_VENDOR_ID, &reg) ||
> -           reg != (pdev->vendor | (pdev->device << 16)) ||
> -           pci_read_config_dword(pdev, PCI_CLASS_REVISION, &reg) ||
> +       if (pci_read_config_dword(pdev, PCI_CLASS_REVISION, &reg) == 0 &&
> +           !PCI_POSSIBLE_ERROR(reg) &&
>             reg != (pdev->revision | (pdev->class << 8)))
>                 return true;
>
>         if (pdev->hdr_type == PCI_HEADER_TYPE_NORMAL &&
> -           (pci_read_config_dword(pdev, PCI_SUBSYSTEM_VENDOR_ID, &reg) ||
> -            reg != (pdev->subsystem_vendor | (pdev->subsystem_device << 16))))
> +           pci_read_config_dword(pdev, PCI_SUBSYSTEM_VENDOR_ID, &reg) == 0 &&
> +           !PCI_POSSIBLE_ERROR(reg) &&
> +           reg != (pdev->subsystem_vendor | (pdev->subsystem_device << 16)))
>                 return true;
>
> -       if (pci_get_dsn(pdev) != ctrl->dsn)
> +       dsn = pci_get_dsn(pdev);
> +       if (!PCI_POSSIBLE_ERROR(dsn) &&
> +           dsn != ctrl->dsn)
>                 return true;
In my case, the pciehp_device_replaced() returns true from this final check.
And these are the values I got
dsn = 0x00000000, ctrl->dsn = 0x7800AA00
dsn = 0x00000000, ctrl->dsn = 0x21B7D000

Did some other test
TBT HDD -> TBT dock -> laptop
   suspend
TBT HDD -> laptop(replace TBT dock with the TBT HDD)
   resume
Got the same result as above, looks like it didn't detect the TBT dock
has been replaced by TBT HDD.

In the origin call trace, unplug TBT dock or replace it with TBT HDD,
it returns true by the below check
        if (pci_read_config_dword(pdev, PCI_VENDOR_ID, &reg) ||
           reg != (pdev->vendor | (pdev->device << 16)) ||
           pci_read_config_dword(pdev, PCI_CLASS_REVISION, &reg) ||
           reg != (pdev->revision | (pdev->class << 8)))
               return true;

>
>         return false;