pci: ensure valid link status bits for downstream ports

Commit Message

Sebastian Ott Nov. 11, 2024, 12:37 p.m. UTC

PCI hotplug for downstream endpoints on arm fails because Linux'
PCIe hotplug driver doesn't like the QEMU provided LNKSTA:

  pcieport 0000:08:01.0: pciehp: Slot(2): Card present
  pcieport 0000:08:01.0: pciehp: Slot(2): Link Up
  pcieport 0000:08:01.0: pciehp: Slot(2): Cannot train link: status 0x2000

There's 2 cases where LNKSTA isn't setup properly:
* the downstream device has no express capability
* max link width of the bridge is 0

Fix these by making the LNKSTA modifications independent of each other.

Signed-off-by: Sebastian Ott <[e-mail]>
---
 hw/pci/pcie.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

Comments

Sebastian Ott Nov. 11, 2024, 12:40 p.m. UTC | #1

On Mon, 11 Nov 2024, Sebastian Ott wrote:
> PCI hotplug for downstream endpoints on arm fails because Linux'
> PCIe hotplug driver doesn't like the QEMU provided LNKSTA:
>
>  pcieport 0000:08:01.0: pciehp: Slot(2): Card present
>  pcieport 0000:08:01.0: pciehp: Slot(2): Link Up
>  pcieport 0000:08:01.0: pciehp: Slot(2): Cannot train link: status 0x2000
>
> There's 2 cases where LNKSTA isn't setup properly:
> * the downstream device has no express capability

I stumbled over this while debugging - is a pci device attached
to a pcie downstream even a valid usecase?

> * max link width of the bridge is 0

MLW for the downstream is initialized with defaults but gets overwritten
later because speed and width properties are 0. Dunno if that's the issue
we should address?

Sebastian

Patch

diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index 0b455c8654..f714f4fb7c 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -1109,20 +1109,20 @@  void pcie_sync_bridge_lnk(PCIDevice *bridge_dev)
         lnksta = target->config_read(target,
                                      target->exp.exp_cap + PCI_EXP_LNKSTA,
                                      sizeof(lnksta));
-
-        if ((lnksta & PCI_EXP_LNKSTA_NLW) > (lnkcap & PCI_EXP_LNKCAP_MLW)) {
-            lnksta &= ~PCI_EXP_LNKSTA_NLW;
-            lnksta |= lnkcap & PCI_EXP_LNKCAP_MLW;
-        } else if (!(lnksta & PCI_EXP_LNKSTA_NLW)) {
-            lnksta |= QEMU_PCI_EXP_LNKSTA_NLW(QEMU_PCI_EXP_LNK_X1);
-        }
-
-        if ((lnksta & PCI_EXP_LNKSTA_CLS) > (lnkcap & PCI_EXP_LNKCAP_SLS)) {
-            lnksta &= ~PCI_EXP_LNKSTA_CLS;
-            lnksta |= lnkcap & PCI_EXP_LNKCAP_SLS;
-        } else if (!(lnksta & PCI_EXP_LNKSTA_CLS)) {
-            lnksta |= QEMU_PCI_EXP_LNKSTA_CLS(QEMU_PCI_EXP_LNK_2_5GT);
-        }
+    }
+    if ((lnksta & PCI_EXP_LNKSTA_NLW) > (lnkcap & PCI_EXP_LNKCAP_MLW)) {
+        lnksta &= ~PCI_EXP_LNKSTA_NLW;
+        lnksta |= lnkcap & PCI_EXP_LNKCAP_MLW;
+    }
+    if (!(lnksta & PCI_EXP_LNKSTA_NLW)) {
+        lnksta |= QEMU_PCI_EXP_LNKSTA_NLW(QEMU_PCI_EXP_LNK_X1);
+    }
+    if ((lnksta & PCI_EXP_LNKSTA_CLS) > (lnkcap & PCI_EXP_LNKCAP_SLS)) {
+        lnksta &= ~PCI_EXP_LNKSTA_CLS;
+        lnksta |= lnkcap & PCI_EXP_LNKCAP_SLS;
+    }
+    if (!(lnksta & PCI_EXP_LNKSTA_CLS)) {
+        lnksta |= QEMU_PCI_EXP_LNKSTA_CLS(QEMU_PCI_EXP_LNK_2_5GT);
     }
 
     pci_word_test_and_clear_mask(exp_cap + PCI_EXP_LNKSTA,