| Message ID | 20241111123756.18393-1-sebott@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | pci: ensure valid link status bits for downstream ports | expand |
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
On 2024/11/11 20:37, 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 > * max link width of the bridge is 0 > > Fix these by making the LNKSTA modifications independent of each other. > > Signed-off-by: Sebastian Ott<sebott@redhat.com> > --- > hw/pci/pcie.c | 28 ++++++++++++++-------------- > 1 file changed, 14 insertions(+), 14 deletions(-) > > 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, |[PATCH] pci: ensure valid link status bits for downstream ports Test on aarch64 host(v9.2.0-rc0-1-gbde0d70333) PCI hotplug for downstream endpoints on arm succeed The test results are as expected. | |Tested-by: Zhenyu Zhang <zhenyzha@redhat.com> |
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 > * max link width of the bridge is 0 > > Fix these by making the LNKSTA modifications independent of each other. > > Signed-off-by: Sebastian Ott <sebott@redhat.com> Friendly ping. This fixes PCI hotplug to a downstream port on ARM. Thanks, Sebastian
On Mon, 11 Nov 2024 13:37:56 +0100 Sebastian Ott <sebott@redhat.com> 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 > * max link width of the bridge is 0 > > Fix these by making the LNKSTA modifications independent of each other. > > Signed-off-by: Sebastian Ott <sebott@redhat.com> > --- > hw/pci/pcie.c | 28 ++++++++++++++-------------- > 1 file changed, 14 insertions(+), 14 deletions(-) > > 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, Only half of these seem valid to me. How can we ever hit the conditions where the status fields exceed the capability fields in the case where we've set the status field from the capability field? It seems like we'd only want to move the sanity checks added in 88c869198aa63 outside of the branch. Thanks, Alex
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,
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 <sebott@redhat.com> --- hw/pci/pcie.c | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-)