On Thu, Nov 10, 2016 at 10:55 AM, Bjorn Helgaas <bhelgaas@xxxxxxxxxx> wrote: > Len Brown reported that resume on a Dell XPS11 laptop takes longer than it > should. The delay is caused by pciehp scanning for a device below a Root > Port that has nothing connected to it. > > At boot-time, the 00:1c.0 Root Port's PCI Express Capabilities Register > (PCIe spec r3.0, sec 7.8.2) advertises a slot (PCI_EXP_FLAGS_SLOT is set), > so pciehp claims the port. > > At resume-time, PCI_EXP_FLAGS_SLOT is clear, so the Root Port no longer > advertises a slot. But pciehp doesn't notice that change, and it reads > Slot Status to see if anything changed. Slot Status says a device is > present (Ports not connected to slots are required to report "Card Present" > per sec 7.8.11), so pciehp tries to bring up the link and scan for the > device, which accounts for the delay. > > Per sec 7.8.2, the PCIe Capabilities Register is all read-only, so I > think the fact that it changes between boot- and resume-time is a > firmware defect. > > Work around this by re-reading the Capabilites at resume-time and updating > the cached copy in pci_dev->pcie_flags_reg. Then stop using pciehp on the > port if it no longer advertises a slot. > > Reported-and-tested-by: Len Brown <lenb@xxxxxxxxxx> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=99751 > Signed-off-by: Bjorn Helgaas <bhelgaas@xxxxxxxxxx> I wish we could handle such firmware bugs using quirks (pci_fixup_resume_early), but it looks difficult for this one :-( Reviewed-by: Rajat Jain <rajatja@xxxxxxxxxx> > --- > drivers/pci/hotplug/pciehp_core.c | 10 ++++++++++ > drivers/pci/pci-driver.c | 13 +++++++++++++ > 2 files changed, 23 insertions(+) > > diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c > index 7d32fa33..f5461cb 100644 > --- a/drivers/pci/hotplug/pciehp_core.c > +++ b/drivers/pci/hotplug/pciehp_core.c > @@ -278,6 +278,9 @@ static void pciehp_remove(struct pcie_device *dev) > { > struct controller *ctrl = get_service_data(dev); > > + if (!ctrl) > + return; > + > cleanup_slot(ctrl); > pciehp_release_ctrl(ctrl); > } > @@ -296,6 +299,13 @@ static int pciehp_resume(struct pcie_device *dev) > > ctrl = get_service_data(dev); > > + if (!(pcie_caps_reg(dev->port) & PCI_EXP_FLAGS_SLOT)) { > + dev_info(&dev->port->dev, "No longer supports hotplug\n"); May be add a comment here, that this is only to work around a firmware bug? > + pciehp_remove(dev); > + set_service_data(dev, NULL); > + return 0; > + } > + > /* reinitialize the chipset's event detection logic */ > pcie_enable_notification(ctrl); > > diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c > index 1ccce1c..fe8e964 100644 > --- a/drivers/pci/pci-driver.c > +++ b/drivers/pci/pci-driver.c > @@ -505,7 +505,20 @@ static int pci_restore_standard_config(struct pci_dev *pci_dev) > > static void pci_pm_default_resume_early(struct pci_dev *pci_dev) > { > + u16 flags; > + > pci_power_up(pci_dev); > + > + if (pci_dev->pcie_cap) { may consider using accessor functions here (pci_is_pcie() / pcie_caps_reg() etc). > + pci_read_config_word(pci_dev, pci_dev->pcie_cap + PCI_EXP_FLAGS, > + &flags); > + if (pci_dev->pcie_flags_reg != flags) { > + dev_info(&pci_dev->dev, "PCIe Capabilities was %#06x, is %#06x after resume (possible firmware defect)\n", > + pci_dev->pcie_flags_reg, flags); > + pci_dev->pcie_flags_reg = flags; > + } > + } > + > pci_restore_state(pci_dev); > pci_fixup_device(pci_fixup_resume_early, pci_dev); > } > > -- > To unsubscribe from this list: send the line "unsubscribe linux-pci" in > the body of a message to majordomo@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
